RE: [PATCH] net: ethernet: fs-enet: remove casting dma_alloc_coherent

[David Laight <David.Laight@ACULAB.COM>]

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)