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

[Christophe Leroy]

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