* [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, davem, kuba, linuxppc-dev
Cc: linux-kernel
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, davem, kuba, linuxppc-dev
Cc: linux-kernel
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, davem, kuba, linuxppc-dev
Cc: linux-kernel
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, davem, kuba, linuxppc-dev
Cc: linux-kernel
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:11 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.