[PATCH] Revert "dma: use dma/cpu conversions correctly in dma_map/unmap_single"

[PATCH] Revert "dma: use dma/cpu conversions correctly in dma_map/unmap_single"

[Sascha Hauer]

This reverts commit d13d870986eeecc58d8652268557e4a159b9d4c4.

While the patch itself is correct, it at least breaks USB on the
Raspberry Pi 3b.

With this patch dma_sync_single_for_device() is passed the DMA address.
This is correct as even the prototype suggests that it should get a
dma_addr_t. Unfortunately this is not what the function implements and
also not what the users expect. Most if not all users simply pass a CPU
pointer casted to unsigned long. dma_sync_single_for_device() on ARM
then takes the DMA address as a CPU pointer and does cache maintenance
on it.

Before we can merge this patch again dma_sync_single_for_device() must
get a struct device * argument and (on ARM) the cpu_to_dma() conversion
must be reverted before doing cache maintenance.
---
 drivers/dma/map.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/map.c b/drivers/dma/map.c
index fea04c38a3..114c0f7db3 100644
--- a/drivers/dma/map.c
+++ b/drivers/dma/map.c
@@ -23,15 +23,17 @@ static inline void *dma_to_cpu(struct device *dev, dma_addr_t addr)
 dma_addr_t dma_map_single(struct device *dev, void *ptr, size_t size,
 			  enum dma_data_direction dir)
 {
-	dma_addr_t ret = cpu_to_dma(dev, ptr);
+	unsigned long addr = (unsigned long)ptr;
 
-	dma_sync_single_for_device(ret, size, dir);
+	dma_sync_single_for_device(addr, size, dir);
 
-	return ret;
+	return cpu_to_dma(dev, ptr);
 }
 
 void dma_unmap_single(struct device *dev, dma_addr_t dma_addr, size_t size,
 		      enum dma_data_direction dir)
 {
-	dma_sync_single_for_cpu(dma_addr, size, dir);
+	unsigned long addr = (unsigned long)dma_to_cpu(dev, dma_addr);
+
+	dma_sync_single_for_cpu(addr, size, dir);
 }
-- 
2.39.2

Re: [PATCH] Revert "dma: use dma/cpu conversions correctly in dma_map/unmap_single"

[Ahmad Fatoum]

On 31.05.23 12:23, Sascha Hauer wrote:
> This reverts commit d13d870986eeecc58d8652268557e4a159b9d4c4.
> 
> While the patch itself is correct, it at least breaks USB on the
> Raspberry Pi 3b.
> 
> With this patch dma_sync_single_for_device() is passed the DMA address.
> This is correct as even the prototype suggests that it should get a
> dma_addr_t. Unfortunately this is not what the function implements and
> also not what the users expect. Most if not all users simply pass a CPU
> pointer casted to unsigned long. dma_sync_single_for_device() on ARM
> then takes the DMA address as a CPU pointer and does cache maintenance
> on it.
> 
> Before we can merge this patch again dma_sync_single_for_device() must
> get a struct device * argument and (on ARM) the cpu_to_dma() conversion
> must be reverted before doing cache maintenance.

@Denis, could you give some background on your patch? I assume this was
for MIPS? Did this patch fix breakage for you? In what driver? Maybe
a follow-up patch that fixes your particular breakage while not breaking
ARM could be found until that wart is cleaned up for good.

Cheers,
Ahmad


> ---
>  drivers/dma/map.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/dma/map.c b/drivers/dma/map.c
> index fea04c38a3..114c0f7db3 100644
> --- a/drivers/dma/map.c
> +++ b/drivers/dma/map.c
> @@ -23,15 +23,17 @@ static inline void *dma_to_cpu(struct device *dev, dma_addr_t addr)
>  dma_addr_t dma_map_single(struct device *dev, void *ptr, size_t size,
>  			  enum dma_data_direction dir)
>  {
> -	dma_addr_t ret = cpu_to_dma(dev, ptr);
> +	unsigned long addr = (unsigned long)ptr;
>  
> -	dma_sync_single_for_device(ret, size, dir);
> +	dma_sync_single_for_device(addr, size, dir);
>  
> -	return ret;
> +	return cpu_to_dma(dev, ptr);
>  }
>  
>  void dma_unmap_single(struct device *dev, dma_addr_t dma_addr, size_t size,
>  		      enum dma_data_direction dir)
>  {
> -	dma_sync_single_for_cpu(dma_addr, size, dir);
> +	unsigned long addr = (unsigned long)dma_to_cpu(dev, dma_addr);
> +
> +	dma_sync_single_for_cpu(addr, size, dir);
>  }

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] Revert "dma: use dma/cpu conversions correctly in dma_map/unmap_single"

[Denis Orlov]

Hi!

>
> On 31.05.23 12:23, Sascha Hauer wrote:
> > This reverts commit d13d870986eeecc58d8652268557e4a159b9d4c4.
> >
> > While the patch itself is correct, it at least breaks USB on the
> > Raspberry Pi 3b.
> >
> > With this patch dma_sync_single_for_device() is passed the DMA address.
> > This is correct as even the prototype suggests that it should get a
> > dma_addr_t. Unfortunately this is not what the function implements and
> > also not what the users expect. Most if not all users simply pass a CPU
> > pointer casted to unsigned long. dma_sync_single_for_device() on ARM
> > then takes the DMA address as a CPU pointer and does cache maintenance
> > on it.
> >
> > Before we can merge this patch again dma_sync_single_for_device() must
> > get a struct device * argument and (on ARM) the cpu_to_dma() conversion
> > must be reverted before doing cache maintenance.
>
> @Denis, could you give some background on your patch? I assume this was
> for MIPS? Did this patch fix breakage for you? In what driver? Maybe
> a follow-up patch that fixes your particular breakage while not breaking
> ARM could be found until that wart is cleaned up for good.

I'm okay with this patch being reverted, sorry for any inconvenience.
Will try to come up with a better one in the meantime.

It appears that MIPS was *always* somewhat broken in this regard.
Without this patch, we end up calling dma_sync_single_for_device()
with a virtual address, and dma_sync_single_for_cpu() with a physical
one. As on MIPS phys/virt addresses do not map 1:1 to each other, we
can't really do anything sensible on the MIPS side in this case. Either
map or unmap calls will be broken. On actual boards this will result in
address errors with any driver that does DMA mappings.

I originally sent an RFC with the whole streaming DMA interface rework,
but I was a bit hesitant if such changes are actually needed. It occured
to me that they are useful in theory, however, nothing in barebox seemed
to require it at the moment. So I just resorted to smaller changes that
were enough to fix MIPS, but I must have totally forgotten to check if
other archs are fine with those changes applied.

I don't like having to do cpu_to_dma() in common code just to call
dma_to_cpu() on the arch side. But it seems like we have to do it in the
most generic case.

>
> Cheers,
> Ahmad
>
>
> > ---
> >  drivers/dma/map.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/dma/map.c b/drivers/dma/map.c
> > index fea04c38a3..114c0f7db3 100644
> > --- a/drivers/dma/map.c
> > +++ b/drivers/dma/map.c
> > @@ -23,15 +23,17 @@ static inline void *dma_to_cpu(struct device *dev, dma_addr_t addr)
> >  dma_addr_t dma_map_single(struct device *dev, void *ptr, size_t size,
> >                         enum dma_data_direction dir)
> >  {
> > -     dma_addr_t ret = cpu_to_dma(dev, ptr);
> > +     unsigned long addr = (unsigned long)ptr;
> >
> > -     dma_sync_single_for_device(ret, size, dir);
> > +     dma_sync_single_for_device(addr, size, dir);
> >
> > -     return ret;
> > +     return cpu_to_dma(dev, ptr);
> >  }
> >
> >  void dma_unmap_single(struct device *dev, dma_addr_t dma_addr, size_t size,
> >                     enum dma_data_direction dir)
> >  {
> > -     dma_sync_single_for_cpu(dma_addr, size, dir);
> > +     unsigned long addr = (unsigned long)dma_to_cpu(dev, dma_addr);
> > +
> > +     dma_sync_single_for_cpu(addr, size, dir);
> >  }
>
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>

Re: [PATCH] Revert "dma: use dma/cpu conversions correctly in dma_map/unmap_single"

[Ahmad Fatoum]

Hello Denis,

On 31.05.23 15:02, Denis Orlov wrote:
> Hi!
> 
>>
>> On 31.05.23 12:23, Sascha Hauer wrote:
>>> This reverts commit d13d870986eeecc58d8652268557e4a159b9d4c4.
>>>
>>> While the patch itself is correct, it at least breaks USB on the
>>> Raspberry Pi 3b.
>>>
>>> With this patch dma_sync_single_for_device() is passed the DMA address.
>>> This is correct as even the prototype suggests that it should get a
>>> dma_addr_t. Unfortunately this is not what the function implements and
>>> also not what the users expect. Most if not all users simply pass a CPU
>>> pointer casted to unsigned long. dma_sync_single_for_device() on ARM
>>> then takes the DMA address as a CPU pointer and does cache maintenance
>>> on it.
>>>
>>> Before we can merge this patch again dma_sync_single_for_device() must
>>> get a struct device * argument and (on ARM) the cpu_to_dma() conversion
>>> must be reverted before doing cache maintenance.
>>
>> @Denis, could you give some background on your patch? I assume this was
>> for MIPS? Did this patch fix breakage for you? In what driver? Maybe
>> a follow-up patch that fixes your particular breakage while not breaking
>> ARM could be found until that wart is cleaned up for good.
> 
> I'm okay with this patch being reverted, sorry for any inconvenience.
> Will try to come up with a better one in the meantime.
> 
> It appears that MIPS was *always* somewhat broken in this regard.
> Without this patch, we end up calling dma_sync_single_for_device()
> with a virtual address, and dma_sync_single_for_cpu() with a physical
> one. As on MIPS phys/virt addresses do not map 1:1 to each other, we
> can't really do anything sensible on the MIPS side in this case. Either
> map or unmap calls will be broken. On actual boards this will result in
> address errors with any driver that does DMA mappings.
> 
> I originally sent an RFC with the whole streaming DMA interface rework,
> but I was a bit hesitant if such changes are actually needed.

I had forgotten about that one. Approach looks fine IMO. Feel free to rebase
and resend. I can give this a test on a number of ARM boards I have in the
remote lab here. 

> It occured
> to me that they are useful in theory, however, nothing in barebox seemed
> to require it at the moment. So I just resorted to smaller changes that
> were enough to fix MIPS, but I must have totally forgotten to check if
> other archs are fine with those changes applied.
> 
> I don't like having to do cpu_to_dma() in common code just to call
> dma_to_cpu() on the arch side. But it seems like we have to do it in the
> most generic case.

Ye, it sounds wrong, but it feels right?!

Cheers,
Ahmad

> 
>>
>> Cheers,
>> Ahmad
>>
>>
>>> ---
>>>  drivers/dma/map.c | 10 ++++++----
>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/dma/map.c b/drivers/dma/map.c
>>> index fea04c38a3..114c0f7db3 100644
>>> --- a/drivers/dma/map.c
>>> +++ b/drivers/dma/map.c
>>> @@ -23,15 +23,17 @@ static inline void *dma_to_cpu(struct device *dev, dma_addr_t addr)
>>>  dma_addr_t dma_map_single(struct device *dev, void *ptr, size_t size,
>>>                         enum dma_data_direction dir)
>>>  {
>>> -     dma_addr_t ret = cpu_to_dma(dev, ptr);
>>> +     unsigned long addr = (unsigned long)ptr;
>>>
>>> -     dma_sync_single_for_device(ret, size, dir);
>>> +     dma_sync_single_for_device(addr, size, dir);
>>>
>>> -     return ret;
>>> +     return cpu_to_dma(dev, ptr);
>>>  }
>>>
>>>  void dma_unmap_single(struct device *dev, dma_addr_t dma_addr, size_t size,
>>>                     enum dma_data_direction dir)
>>>  {
>>> -     dma_sync_single_for_cpu(dma_addr, size, dir);
>>> +     unsigned long addr = (unsigned long)dma_to_cpu(dev, dma_addr);
>>> +
>>> +     dma_sync_single_for_cpu(addr, size, dir);
>>>  }
>>
>> --
>> Pengutronix e.K.                           |                             |
>> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
>> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
>> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>>
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |