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 |
