* [PATCH] Revert "dma: use dma/cpu conversions correctly in dma_map/unmap_single" @ 2023-05-31 10:23 Sascha Hauer 2023-05-31 10:32 ` Ahmad Fatoum 0 siblings, 1 reply; 4+ messages in thread From: Sascha Hauer @ 2023-05-31 10:23 UTC (permalink / raw) To: Barebox List; +Cc: Denis Orlov 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Revert "dma: use dma/cpu conversions correctly in dma_map/unmap_single" 2023-05-31 10:23 [PATCH] Revert "dma: use dma/cpu conversions correctly in dma_map/unmap_single" Sascha Hauer @ 2023-05-31 10:32 ` Ahmad Fatoum 2023-05-31 13:02 ` Denis Orlov 0 siblings, 1 reply; 4+ messages in thread From: Ahmad Fatoum @ 2023-05-31 10:32 UTC (permalink / raw) To: Denis Orlov; +Cc: Barebox List 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 | ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Revert "dma: use dma/cpu conversions correctly in dma_map/unmap_single" 2023-05-31 10:32 ` Ahmad Fatoum @ 2023-05-31 13:02 ` Denis Orlov 2023-05-31 13:10 ` Ahmad Fatoum 0 siblings, 1 reply; 4+ messages in thread From: Denis Orlov @ 2023-05-31 13:02 UTC (permalink / raw) To: Ahmad Fatoum; +Cc: Barebox List 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 | > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Revert "dma: use dma/cpu conversions correctly in dma_map/unmap_single" 2023-05-31 13:02 ` Denis Orlov @ 2023-05-31 13:10 ` Ahmad Fatoum 0 siblings, 0 replies; 4+ messages in thread From: Ahmad Fatoum @ 2023-05-31 13:10 UTC (permalink / raw) To: Denis Orlov; +Cc: Barebox List 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 | ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-05-31 13:11 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-05-31 10:23 [PATCH] Revert "dma: use dma/cpu conversions correctly in dma_map/unmap_single" Sascha Hauer 2023-05-31 10:32 ` Ahmad Fatoum 2023-05-31 13:02 ` Denis Orlov 2023-05-31 13:10 ` Ahmad Fatoum
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox