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

[Christophe Leroy <christophe.leroy@csgroup.eu>]

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