Message ID | 20200819065555.1802761-23-hch@lst.de (mailing list archive) |
---|---|
State | Not Applicable, archived |
Headers |
Received: from vger.kernel.org ([23.128.96.18]) by www.linuxtv.org with esmtp (Exim 4.92) (envelope-from <linux-media-owner@vger.kernel.org>) id 1k8I2x-008e3n-Tx; Wed, 19 Aug 2020 06:58:08 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726920AbgHSHDZ (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Wed, 19 Aug 2020 03:03:25 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:36816 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726875AbgHSG5h (ORCPT <rfc822;linux-media@vger.kernel.org>); Wed, 19 Aug 2020 02:57:37 -0400 Received: from casper.infradead.org (casper.infradead.org [IPv6:2001:8b0:10b:1236::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 8B700C061389; Tue, 18 Aug 2020 23:57:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=Content-Transfer-Encoding:MIME-Version: References:In-Reply-To:Message-Id:Date:Subject:Cc:To:From:Sender:Reply-To: Content-Type:Content-ID:Content-Description; bh=RFTX12/6vaWoRhhjSqpC5wgk9pFG99OSZ6G21Gj4zUk=; b=mO0h3/QJQ4IMJcw7idTzxUEyfy FuPqv+qEDbTDPKK7FEenUd7ionlcjYL5GNBSYPlMCFwiQJm5+2rN19erxYXKVTBf390ZI8EWPbL0a ab7QtB66NQzyOAGNH+ofzZrE9kkefR1unxsPvJ5aJmbuHVe11Py2x1Qkb3NICIBaF7SLnVjd2jf1j WogQQTRrOF5qGuiMHBe3WOb95aMfkixb9aeUOuDYPDLOtubIROb53k2qV4Kd5JURqGiFRezaphnme D+80I4GP/SkvHK3cjj0XcV7p+d8ZZ2xWBwT2HC7Kew3oBm8FRsyQUXwUGkUv2IaqYMxBQPG/p6CP2 xIzfAfMg==; Received: from [2001:4bb8:198:f3b2:86b6:2277:f429:37a1] (helo=localhost) by casper.infradead.org with esmtpsa (Exim 4.92.3 #3 (Red Hat Linux)) id 1k8I1e-0008V1-Rd; Wed, 19 Aug 2020 06:56:47 +0000 From: Christoph Hellwig <hch@lst.de> To: Mauro Carvalho Chehab <mchehab@kernel.org>, Thomas Bogendoerfer <tsbogend@alpha.franken.de>, "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>, Joonyoung Shim <jy0922.shim@samsung.com>, Seung-Woo Kim <sw0312.kim@samsung.com>, Kyungmin Park <kyungmin.park@samsung.com>, Ben Skeggs <bskeggs@redhat.com>, Pawel Osciak <pawel@osciak.com>, Marek Szyprowski <m.szyprowski@samsung.com>, Matt Porter <mporter@kernel.crashing.org>, iommu@lists.linux-foundation.org Cc: Tom Lendacky <thomas.lendacky@amd.com>, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-media@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-ia64@vger.kernel.org, linux-mips@vger.kernel.org, linux-parisc@vger.kernel.org, linux-samsung-soc@vger.kernel.org, nouveau@lists.freedesktop.org, netdev@vger.kernel.org, linux-nvme@lists.infradead.org, linux-scsi@vger.kernel.org, linux-mm@kvack.org, alsa-devel@alsa-project.org Subject: [PATCH 22/28] sgiseeq: convert from dma_cache_sync to dma_sync_single_for_device Date: Wed, 19 Aug 2020 08:55:49 +0200 Message-Id: <20200819065555.1802761-23-hch@lst.de> X-Mailer: git-send-email 2.28.0 In-Reply-To: <20200819065555.1802761-1-hch@lst.de> References: <20200819065555.1802761-1-hch@lst.de> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SRS-Rewrite: SMTP reverse-path rewritten from <hch@infradead.org> by casper.infradead.org. See http://www.infradead.org/rpr.html Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-LSpam-Score: -2.3 (--) X-LSpam-Report: No, score=-2.3 required=5.0 tests=BAYES_00=-1.9,DKIM_SIGNED=0.1,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1,T_DKIM_INVALID=0.01 autolearn=ham autolearn_force=no |
Series |
[01/28] mm: turn alloc_pages into an inline function
|
|
Commit Message
Christoph Hellwig
Aug. 19, 2020, 6:55 a.m. UTC
Use the proper modern API to transfer cache ownership for incoherent DMA.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/net/ethernet/seeq/sgiseeq.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
Comments
On Wed, Aug 19, 2020 at 08:55:49AM +0200, Christoph Hellwig wrote: > Use the proper modern API to transfer cache ownership for incoherent DMA. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > drivers/net/ethernet/seeq/sgiseeq.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/seeq/sgiseeq.c b/drivers/net/ethernet/seeq/sgiseeq.c > index 39599bbb5d45b6..f91dae16d69a19 100644 > --- a/drivers/net/ethernet/seeq/sgiseeq.c > +++ b/drivers/net/ethernet/seeq/sgiseeq.c > @@ -112,14 +112,18 @@ struct sgiseeq_private { > > static inline void dma_sync_desc_cpu(struct net_device *dev, void *addr) > { > - dma_cache_sync(dev->dev.parent, addr, sizeof(struct sgiseeq_rx_desc), > - DMA_FROM_DEVICE); > + struct sgiseeq_private *sp = netdev_priv(dev); > + > + dma_sync_single_for_cpu(dev->dev.parent, VIRT_TO_DMA(sp, addr), > + sizeof(struct sgiseeq_rx_desc), DMA_BIDIRECTIONAL); > } > > static inline void dma_sync_desc_dev(struct net_device *dev, void *addr) > { > - dma_cache_sync(dev->dev.parent, addr, sizeof(struct sgiseeq_rx_desc), > - DMA_TO_DEVICE); > + struct sgiseeq_private *sp = netdev_priv(dev); > + > + dma_sync_single_for_device(dev->dev.parent, VIRT_TO_DMA(sp, addr), > + sizeof(struct sgiseeq_rx_desc), DMA_BIDIRECTIONAL); > } this breaks ethernet on IP22 completely, but I haven't figured out why, yet. Thomas.
On Tue, Sep 01, 2020 at 05:22:09PM +0200, Thomas Bogendoerfer wrote: > On Wed, Aug 19, 2020 at 08:55:49AM +0200, Christoph Hellwig wrote: > > Use the proper modern API to transfer cache ownership for incoherent DMA. > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > --- > > drivers/net/ethernet/seeq/sgiseeq.c | 12 ++++++++---- > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/net/ethernet/seeq/sgiseeq.c b/drivers/net/ethernet/seeq/sgiseeq.c > > index 39599bbb5d45b6..f91dae16d69a19 100644 > > --- a/drivers/net/ethernet/seeq/sgiseeq.c > > +++ b/drivers/net/ethernet/seeq/sgiseeq.c > > @@ -112,14 +112,18 @@ struct sgiseeq_private { > > > > static inline void dma_sync_desc_cpu(struct net_device *dev, void *addr) > > { > > - dma_cache_sync(dev->dev.parent, addr, sizeof(struct sgiseeq_rx_desc), > > - DMA_FROM_DEVICE); > > + struct sgiseeq_private *sp = netdev_priv(dev); > > + > > + dma_sync_single_for_cpu(dev->dev.parent, VIRT_TO_DMA(sp, addr), > > + sizeof(struct sgiseeq_rx_desc), DMA_BIDIRECTIONAL); > > } > > > > static inline void dma_sync_desc_dev(struct net_device *dev, void *addr) > > { > > - dma_cache_sync(dev->dev.parent, addr, sizeof(struct sgiseeq_rx_desc), > > - DMA_TO_DEVICE); > > + struct sgiseeq_private *sp = netdev_priv(dev); > > + > > + dma_sync_single_for_device(dev->dev.parent, VIRT_TO_DMA(sp, addr), > > + sizeof(struct sgiseeq_rx_desc), DMA_BIDIRECTIONAL); > > } > > this breaks ethernet on IP22 completely, but I haven't figured out why, yet. the problem is that dma_sync_single_for_cpu() doesn't flush anything for IP22, because it only flushes for CPUs which do speculation. So either MIPS arch_sync_dma_for_cpu() should always flush or sgiseeq needs to use a different sync funktion, when it wants to re-read descriptors from memory. Thomas.
On Tue, Sep 01, 2020 at 07:12:41PM +0200, Thomas Bogendoerfer wrote: > On Tue, Sep 01, 2020 at 05:22:09PM +0200, Thomas Bogendoerfer wrote: > > On Wed, Aug 19, 2020 at 08:55:49AM +0200, Christoph Hellwig wrote: > > > Use the proper modern API to transfer cache ownership for incoherent DMA. > > > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > > --- > > > drivers/net/ethernet/seeq/sgiseeq.c | 12 ++++++++---- > > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/seeq/sgiseeq.c b/drivers/net/ethernet/seeq/sgiseeq.c > > > index 39599bbb5d45b6..f91dae16d69a19 100644 > > > --- a/drivers/net/ethernet/seeq/sgiseeq.c > > > +++ b/drivers/net/ethernet/seeq/sgiseeq.c > > > @@ -112,14 +112,18 @@ struct sgiseeq_private { > > > > > > static inline void dma_sync_desc_cpu(struct net_device *dev, void *addr) > > > { > > > - dma_cache_sync(dev->dev.parent, addr, sizeof(struct sgiseeq_rx_desc), > > > - DMA_FROM_DEVICE); > > > + struct sgiseeq_private *sp = netdev_priv(dev); > > > + > > > + dma_sync_single_for_cpu(dev->dev.parent, VIRT_TO_DMA(sp, addr), > > > + sizeof(struct sgiseeq_rx_desc), DMA_BIDIRECTIONAL); > > > } > > > > > > static inline void dma_sync_desc_dev(struct net_device *dev, void *addr) > > > { > > > - dma_cache_sync(dev->dev.parent, addr, sizeof(struct sgiseeq_rx_desc), > > > - DMA_TO_DEVICE); > > > + struct sgiseeq_private *sp = netdev_priv(dev); > > > + > > > + dma_sync_single_for_device(dev->dev.parent, VIRT_TO_DMA(sp, addr), > > > + sizeof(struct sgiseeq_rx_desc), DMA_BIDIRECTIONAL); > > > } > > > > this breaks ethernet on IP22 completely, but I haven't figured out why, yet. > > the problem is that dma_sync_single_for_cpu() doesn't flush anything > for IP22, because it only flushes for CPUs which do speculation. So > either MIPS arch_sync_dma_for_cpu() should always flush or sgiseeq > needs to use a different sync funktion, when it wants to re-read descriptors > from memory. Well, if IP22 doesn't speculate (which I'm pretty sure is the case), dma_sync_single_for_cpu should indeeed be a no-op. But then there also shouldn't be anything in the cache, as the previous dma_sync_single_for_device should have invalidated it. So it seems like we are missing one (or more) ownership transfers to the device. I'll try to look at the the ownership management in a little more detail tomorrow.
On Tue, Sep 01, 2020 at 07:16:27PM +0200, Christoph Hellwig wrote: > Well, if IP22 doesn't speculate (which I'm pretty sure is the case), > dma_sync_single_for_cpu should indeeed be a no-op. But then there > also shouldn't be anything in the cache, as the previous > dma_sync_single_for_device should have invalidated it. So it seems like > we are missing one (or more) ownership transfers to the device. I'll > try to look at the the ownership management in a little more detail > tomorrow. this is the problem: /* Always check for received packets. */ sgiseeq_rx(dev, sp, hregs, sregs); so the driver will look at the rx descriptor on every interrupt, so we cache the rx descriptor on the first interrupt and if there was $no rx packet, we will only see it, if cache line gets flushed for some other reason. kick_tx() does a busy loop checking tx descriptors, with just sync_desc_cpu... Thomas.
On Tue, Sep 01, 2020 at 07:38:10PM +0200, Thomas Bogendoerfer wrote: > On Tue, Sep 01, 2020 at 07:16:27PM +0200, Christoph Hellwig wrote: > > Well, if IP22 doesn't speculate (which I'm pretty sure is the case), > > dma_sync_single_for_cpu should indeeed be a no-op. But then there > > also shouldn't be anything in the cache, as the previous > > dma_sync_single_for_device should have invalidated it. So it seems like > > we are missing one (or more) ownership transfers to the device. I'll > > try to look at the the ownership management in a little more detail > > tomorrow. > > this is the problem: > > /* Always check for received packets. */ > sgiseeq_rx(dev, sp, hregs, sregs); > > so the driver will look at the rx descriptor on every interrupt, so > we cache the rx descriptor on the first interrupt and if there was > $no rx packet, we will only see it, if cache line gets flushed for > some other reason. kick_tx() does a busy loop checking tx descriptors, > with just sync_desc_cpu... the patch below fixes the problem. Thomas. diff --git a/drivers/net/ethernet/seeq/sgiseeq.c b/drivers/net/ethernet/seeq/sgiseeq.c index 8507ff242014..876e3700a0e4 100644 --- a/drivers/net/ethernet/seeq/sgiseeq.c +++ b/drivers/net/ethernet/seeq/sgiseeq.c @@ -112,14 +112,18 @@ struct sgiseeq_private { static inline void dma_sync_desc_cpu(struct net_device *dev, void *addr) { - dma_cache_sync(dev->dev.parent, addr, sizeof(struct sgiseeq_rx_desc), - DMA_FROM_DEVICE); + struct sgiseeq_private *sp = netdev_priv(dev); + + dma_sync_single_for_device(dev->dev.parent, VIRT_TO_DMA(sp, addr), + sizeof(struct sgiseeq_rx_desc), DMA_FROM_DEVICE); } static inline void dma_sync_desc_dev(struct net_device *dev, void *addr) { - dma_cache_sync(dev->dev.parent, addr, sizeof(struct sgiseeq_rx_desc), - DMA_TO_DEVICE); + struct sgiseeq_private *sp = netdev_priv(dev); + + dma_sync_single_for_device(dev->dev.parent, VIRT_TO_DMA(sp, addr), + sizeof(struct sgiseeq_rx_desc), DMA_TO_DEVICE); }
On Wed, Sep 02, 2020 at 11:38:09PM +0200, Thomas Bogendoerfer wrote: > the patch below fixes the problem. But is very wrong unfortunately. > static inline void dma_sync_desc_cpu(struct net_device *dev, void *addr) > { > - dma_cache_sync(dev->dev.parent, addr, sizeof(struct sgiseeq_rx_desc), > - DMA_FROM_DEVICE); > + struct sgiseeq_private *sp = netdev_priv(dev); > + > + dma_sync_single_for_device(dev->dev.parent, VIRT_TO_DMA(sp, addr), > + sizeof(struct sgiseeq_rx_desc), DMA_FROM_DEVICE); > } > > static inline void dma_sync_desc_dev(struct net_device *dev, void *addr) > { > - dma_cache_sync(dev->dev.parent, addr, sizeof(struct sgiseeq_rx_desc), > - DMA_TO_DEVICE); > + struct sgiseeq_private *sp = netdev_priv(dev); > + > + dma_sync_single_for_device(dev->dev.parent, VIRT_TO_DMA(sp, addr), > + sizeof(struct sgiseeq_rx_desc), DMA_TO_DEVICE); This is not how the DMA API works. You can only call dma_sync_single_for_{device,cpu} with the direction that the memory was mapped. It then transfer ownership to the device or the cpu, and the ownership of the memory is a fundamental concept that allows for reasoning about the caching interaction. > } > > -- > Crap can work. Given enough thrust pigs will fly, but it's not necessarily a > good idea. [ RFC1925, 2.3 ] ---end quoted text---
On Tue, Sep 01, 2020 at 07:38:10PM +0200, Thomas Bogendoerfer wrote: > this is the problem: > > /* Always check for received packets. */ > sgiseeq_rx(dev, sp, hregs, sregs); > > so the driver will look at the rx descriptor on every interrupt, so > we cache the rx descriptor on the first interrupt and if there was > $no rx packet, we will only see it, if cache line gets flushed for > some other reason. That means a transfer back to device ownership is missing after a (negative) check. > kick_tx() does a busy loop checking tx descriptors, > with just sync_desc_cpu... > > Thomas. > > -- > Crap can work. Given enough thrust pigs will fly, but it's not necessarily a > good idea. [ RFC1925, 2.3 ] ---end quoted text---
On Thu, Sep 03, 2020 at 10:43:02AM +0200, Christoph Hellwig wrote: > On Tue, Sep 01, 2020 at 07:38:10PM +0200, Thomas Bogendoerfer wrote: > > this is the problem: > > > > /* Always check for received packets. */ > > sgiseeq_rx(dev, sp, hregs, sregs); > > > > so the driver will look at the rx descriptor on every interrupt, so > > we cache the rx descriptor on the first interrupt and if there was > > $no rx packet, we will only see it, if cache line gets flushed for > > some other reason. > > That means a transfer back to device ownership is missing after a > (negative) check. E.g. something like this for the particular problem, although there might be other hiding elsewhere: diff --git a/drivers/net/ethernet/seeq/sgiseeq.c b/drivers/net/ethernet/seeq/sgiseeq.c index 8507ff2420143a..a1c7be8a0d1e5d 100644 --- a/drivers/net/ethernet/seeq/sgiseeq.c +++ b/drivers/net/ethernet/seeq/sgiseeq.c @@ -403,6 +403,8 @@ static inline void sgiseeq_rx(struct net_device *dev, struct sgiseeq_private *sp rd = &sp->rx_desc[sp->rx_new]; dma_sync_desc_cpu(dev, rd); } + dma_sync_desc_dev(dev, rd); + dma_sync_desc_cpu(dev, &sp->rx_desc[orig_end]); sp->rx_desc[orig_end].rdma.cntinfo &= ~(HPCDMA_EOR); dma_sync_desc_dev(dev, &sp->rx_desc[orig_end]);
diff --git a/drivers/net/ethernet/seeq/sgiseeq.c b/drivers/net/ethernet/seeq/sgiseeq.c index 39599bbb5d45b6..f91dae16d69a19 100644 --- a/drivers/net/ethernet/seeq/sgiseeq.c +++ b/drivers/net/ethernet/seeq/sgiseeq.c @@ -112,14 +112,18 @@ struct sgiseeq_private { static inline void dma_sync_desc_cpu(struct net_device *dev, void *addr) { - dma_cache_sync(dev->dev.parent, addr, sizeof(struct sgiseeq_rx_desc), - DMA_FROM_DEVICE); + struct sgiseeq_private *sp = netdev_priv(dev); + + dma_sync_single_for_cpu(dev->dev.parent, VIRT_TO_DMA(sp, addr), + sizeof(struct sgiseeq_rx_desc), DMA_BIDIRECTIONAL); } static inline void dma_sync_desc_dev(struct net_device *dev, void *addr) { - dma_cache_sync(dev->dev.parent, addr, sizeof(struct sgiseeq_rx_desc), - DMA_TO_DEVICE); + struct sgiseeq_private *sp = netdev_priv(dev); + + dma_sync_single_for_device(dev->dev.parent, VIRT_TO_DMA(sp, addr), + sizeof(struct sgiseeq_rx_desc), DMA_BIDIRECTIONAL); } static inline void hpc3_eth_reset(struct hpc3_ethregs *hregs)