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

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

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 |