* [PATCH] net: ethernet: fs-enet: remove casting dma_alloc_coherent @ 2020-12-11 8:52 Xu Wang 2020-12-11 15:22 ` Christophe Leroy 0 siblings, 1 reply; 6+ messages in thread From: Xu Wang @ 2020-12-11 8:52 UTC (permalink / raw) To: pantelis.antoniou, davem, kuba, linuxppc-dev; +Cc: linux-kernel Remove casting the values returned by dma_alloc_coherent. Signed-off-by: Xu Wang <vulab@iscas.ac.cn> --- drivers/net/ethernet/freescale/fs_enet/mac-fec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ethernet/freescale/fs_enet/mac-fec.c b/drivers/net/ethernet/freescale/fs_enet/mac-fec.c index 99fe2c210d0f..3ae345676e50 100644 --- a/drivers/net/ethernet/freescale/fs_enet/mac-fec.c +++ b/drivers/net/ethernet/freescale/fs_enet/mac-fec.c @@ -131,7 +131,7 @@ static int allocate_bd(struct net_device *dev) struct fs_enet_private *fep = netdev_priv(dev); const struct fs_platform_info *fpi = fep->fpi; - fep->ring_base = (void __force __iomem *)dma_alloc_coherent(fep->dev, + fep->ring_base = dma_alloc_coherent(fep->dev, (fpi->tx_ring + fpi->rx_ring) * sizeof(cbd_t), &fep->ring_mem_addr, GFP_KERNEL); -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] net: ethernet: fs-enet: remove casting dma_alloc_coherent 2020-12-11 8:52 [PATCH] net: ethernet: fs-enet: remove casting dma_alloc_coherent Xu Wang @ 2020-12-11 15:22 ` Christophe Leroy 2020-12-11 16:07 ` David Laight 0 siblings, 1 reply; 6+ messages in thread From: Christophe Leroy @ 2020-12-11 15:22 UTC (permalink / raw) To: Xu Wang, pantelis.antoniou, davem, kuba, linuxppc-dev; +Cc: linux-kernel Le 11/12/2020 à 09:52, Xu Wang a écrit : > Remove casting the values returned by dma_alloc_coherent. Can you explain more in the commit log ? As far as I can see, dma_alloc_coherent() doesn't return __iomem, and ring_base member is __iomem Christophe > > Signed-off-by: Xu Wang <vulab@iscas.ac.cn> > --- > drivers/net/ethernet/freescale/fs_enet/mac-fec.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/freescale/fs_enet/mac-fec.c b/drivers/net/ethernet/freescale/fs_enet/mac-fec.c > index 99fe2c210d0f..3ae345676e50 100644 > --- a/drivers/net/ethernet/freescale/fs_enet/mac-fec.c > +++ b/drivers/net/ethernet/freescale/fs_enet/mac-fec.c > @@ -131,7 +131,7 @@ static int allocate_bd(struct net_device *dev) > struct fs_enet_private *fep = netdev_priv(dev); > const struct fs_platform_info *fpi = fep->fpi; > > - fep->ring_base = (void __force __iomem *)dma_alloc_coherent(fep->dev, > + fep->ring_base = dma_alloc_coherent(fep->dev, > (fpi->tx_ring + fpi->rx_ring) * > sizeof(cbd_t), &fep->ring_mem_addr, > GFP_KERNEL); > ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] net: ethernet: fs-enet: remove casting dma_alloc_coherent 2020-12-11 15:22 ` Christophe Leroy @ 2020-12-11 16:07 ` David Laight 2020-12-11 16:42 ` Christophe Leroy 0 siblings, 1 reply; 6+ messages in thread From: David Laight @ 2020-12-11 16:07 UTC (permalink / raw) To: 'Christophe Leroy', Xu Wang, pantelis.antoniou@gmail.com, davem@davemloft.net, kuba@kernel.org, linuxppc-dev@lists.ozlabs.org Cc: linux-kernel@vger.kernel.org From: Christophe Leroy > Sent: 11 December 2020 15:22 > > Le 11/12/2020 à 09:52, Xu Wang a écrit : > > Remove casting the values returned by dma_alloc_coherent. > > Can you explain more in the commit log ? > > As far as I can see, dma_alloc_coherent() doesn't return __iomem, and ring_base member is __iomem Which is probably wrong - that is the kernel address of kernel memory. So it shouldn't have the __iomem marker. I wonder what else is wrong.... David > > Christophe > > > > > Signed-off-by: Xu Wang <vulab@iscas.ac.cn> > > --- > > drivers/net/ethernet/freescale/fs_enet/mac-fec.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/freescale/fs_enet/mac-fec.c > b/drivers/net/ethernet/freescale/fs_enet/mac-fec.c > > index 99fe2c210d0f..3ae345676e50 100644 > > --- a/drivers/net/ethernet/freescale/fs_enet/mac-fec.c > > +++ b/drivers/net/ethernet/freescale/fs_enet/mac-fec.c > > @@ -131,7 +131,7 @@ static int allocate_bd(struct net_device *dev) > > struct fs_enet_private *fep = netdev_priv(dev); > > const struct fs_platform_info *fpi = fep->fpi; > > > > - fep->ring_base = (void __force __iomem *)dma_alloc_coherent(fep->dev, > > + fep->ring_base = dma_alloc_coherent(fep->dev, > > (fpi->tx_ring + fpi->rx_ring) * > > sizeof(cbd_t), &fep->ring_mem_addr, > > GFP_KERNEL); > > - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: ethernet: fs-enet: remove casting dma_alloc_coherent 2020-12-11 16:07 ` David Laight @ 2020-12-11 16:42 ` Christophe Leroy 2020-12-11 16:55 ` David Laight 0 siblings, 1 reply; 6+ messages in thread From: Christophe Leroy @ 2020-12-11 16:42 UTC (permalink / raw) To: David Laight, Xu Wang, pantelis.antoniou@gmail.com, davem@davemloft.net, kuba@kernel.org, linuxppc-dev@lists.ozlabs.org Cc: linux-kernel@vger.kernel.org Le 11/12/2020 à 17:07, David Laight a écrit : > From: Christophe Leroy >> Sent: 11 December 2020 15:22 >> >> Le 11/12/2020 à 09:52, Xu Wang a écrit : >>> Remove casting the values returned by dma_alloc_coherent. >> >> Can you explain more in the commit log ? >> >> As far as I can see, dma_alloc_coherent() doesn't return __iomem, and ring_base member is __iomem > > Which is probably wrong - that is the kernel address of kernel memory. > So it shouldn't have the __iomem marker. That's where the buffer descriptors are, the driver accesses to the content of the buffer descriptors using the IO accessors in_be16()/out_be16(). Is it not correct ? Christophe > > I wonder what else is wrong.... > > David > >> >> Christophe >> >>> >>> Signed-off-by: Xu Wang <vulab@iscas.ac.cn> >>> --- >>> drivers/net/ethernet/freescale/fs_enet/mac-fec.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/net/ethernet/freescale/fs_enet/mac-fec.c >> b/drivers/net/ethernet/freescale/fs_enet/mac-fec.c >>> index 99fe2c210d0f..3ae345676e50 100644 >>> --- a/drivers/net/ethernet/freescale/fs_enet/mac-fec.c >>> +++ b/drivers/net/ethernet/freescale/fs_enet/mac-fec.c >>> @@ -131,7 +131,7 @@ static int allocate_bd(struct net_device *dev) >>> struct fs_enet_private *fep = netdev_priv(dev); >>> const struct fs_platform_info *fpi = fep->fpi; >>> >>> - fep->ring_base = (void __force __iomem *)dma_alloc_coherent(fep->dev, >>> + fep->ring_base = dma_alloc_coherent(fep->dev, >>> (fpi->tx_ring + fpi->rx_ring) * >>> sizeof(cbd_t), &fep->ring_mem_addr, >>> GFP_KERNEL); >>> > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) > ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] net: ethernet: fs-enet: remove casting dma_alloc_coherent 2020-12-11 16:42 ` Christophe Leroy @ 2020-12-11 16:55 ` David Laight 2021-03-16 18:10 ` Christophe Leroy 0 siblings, 1 reply; 6+ messages in thread From: David Laight @ 2020-12-11 16:55 UTC (permalink / raw) To: 'Christophe Leroy', Xu Wang, pantelis.antoniou@gmail.com, davem@davemloft.net, kuba@kernel.org, linuxppc-dev@lists.ozlabs.org Cc: linux-kernel@vger.kernel.org From: Christophe Leroy > Sent: 11 December 2020 16:43 > > Le 11/12/2020 à 17:07, David Laight a écrit : > > From: Christophe Leroy > >> Sent: 11 December 2020 15:22 > >> > >> Le 11/12/2020 à 09:52, Xu Wang a écrit : > >>> Remove casting the values returned by dma_alloc_coherent. > >> > >> Can you explain more in the commit log ? > >> > >> As far as I can see, dma_alloc_coherent() doesn't return __iomem, and ring_base member is __iomem > > > > Which is probably wrong - that is the kernel address of kernel memory. > > So it shouldn't have the __iomem marker. > > That's where the buffer descriptors are, the driver accesses to the content of the buffer > descriptors using the IO accessors in_be16()/out_be16(). Is it not correct ? I've just been looking at the crap in there. My understanding is that IO accessors are for IO devices (eg addresses from io_remap() etc). Buffers allocated by dma_alloc_coherent() are normal kernel memory and don't need any accessors. Now you might need some barriers - mostly because an ethernet chip can typically read a ring entry without being prodded. IIRC there is a barrier in writel() to ensure the dma master will 'see' all memory writes done before the IO write that kicks it into doing some processing. The fact that the driver contains so many __iomem casts (eg in tx_restart) is an indication that something is badly awry. __iomem exists to check you are using the correct type of pointer. Any __iomem casts are dubious. David > > Christophe > > > > > I wonder what else is wrong.... > > > > David > > > >> > >> Christophe > >> > >>> > >>> Signed-off-by: Xu Wang <vulab@iscas.ac.cn> > >>> --- > >>> drivers/net/ethernet/freescale/fs_enet/mac-fec.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/net/ethernet/freescale/fs_enet/mac-fec.c > >> b/drivers/net/ethernet/freescale/fs_enet/mac-fec.c > >>> index 99fe2c210d0f..3ae345676e50 100644 > >>> --- a/drivers/net/ethernet/freescale/fs_enet/mac-fec.c > >>> +++ b/drivers/net/ethernet/freescale/fs_enet/mac-fec.c > >>> @@ -131,7 +131,7 @@ static int allocate_bd(struct net_device *dev) > >>> struct fs_enet_private *fep = netdev_priv(dev); > >>> const struct fs_platform_info *fpi = fep->fpi; > >>> > >>> - fep->ring_base = (void __force __iomem *)dma_alloc_coherent(fep->dev, > >>> + fep->ring_base = dma_alloc_coherent(fep->dev, > >>> (fpi->tx_ring + fpi->rx_ring) * > >>> sizeof(cbd_t), &fep->ring_mem_addr, > >>> GFP_KERNEL); > >>> > > > > - > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > > Registration No: 1397386 (Wales) > > - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: ethernet: fs-enet: remove casting dma_alloc_coherent 2020-12-11 16:55 ` David Laight @ 2021-03-16 18:10 ` Christophe Leroy 0 siblings, 0 replies; 6+ messages in thread From: Christophe Leroy @ 2021-03-16 18:10 UTC (permalink / raw) To: David Laight, Xu Wang, pantelis.antoniou@gmail.com, davem@davemloft.net, kuba@kernel.org, linuxppc-dev@lists.ozlabs.org Cc: linux-kernel@vger.kernel.org Le 11/12/2020 à 17:55, David Laight a écrit : > From: Christophe Leroy >> Sent: 11 December 2020 16:43 >> >> Le 11/12/2020 à 17:07, David Laight a écrit : >>> From: Christophe Leroy >>>> Sent: 11 December 2020 15:22 >>>> >>>> Le 11/12/2020 à 09:52, Xu Wang a écrit : >>>>> Remove casting the values returned by dma_alloc_coherent. >>>> >>>> Can you explain more in the commit log ? >>>> >>>> As far as I can see, dma_alloc_coherent() doesn't return __iomem, and ring_base member is __iomem >>> >>> Which is probably wrong - that is the kernel address of kernel memory. >>> So it shouldn't have the __iomem marker. >> >> That's where the buffer descriptors are, the driver accesses to the content of the buffer >> descriptors using the IO accessors in_be16()/out_be16(). Is it not correct ? > > I've just been looking at the crap in there. > My understanding is that IO accessors are for IO devices (eg addresses > from io_remap() etc). > > Buffers allocated by dma_alloc_coherent() are normal kernel memory > and don't need any accessors. > Now you might need some barriers - mostly because an ethernet chip > can typically read a ring entry without being prodded. > IIRC there is a barrier in writel() to ensure the dma master will > 'see' all memory writes done before the IO write that kicks it into > doing some processing. As far as I can see, writel() is using __iomem memory, see https://elixir.bootlin.com/linux/v5.12-rc2/source/include/asm-generic/io.h#L221 > > The fact that the driver contains so many __iomem casts (eg in > tx_restart) is an indication that something is badly awry. > __iomem exists to check you are using the correct type of pointer. > Any __iomem casts are dubious. I agree, but what else can we do to guarantee proper access to that memory ? Christophe ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-03-16 18:10 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-12-11 8:52 [PATCH] net: ethernet: fs-enet: remove casting dma_alloc_coherent Xu Wang 2020-12-11 15:22 ` Christophe Leroy 2020-12-11 16:07 ` David Laight 2020-12-11 16:42 ` Christophe Leroy 2020-12-11 16:55 ` David Laight 2021-03-16 18:10 ` Christophe Leroy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox