Re: [PATCH 0/6] get rid of on-stack dma buffers
From: James Bottomley Date: Tue, 22 Mar 2011 08:35:04 -0500 > The API will round up so that the correct region covers the API. > However, if you have other structures packed into the space (as very > often happens on stack), you get cache line interference in the CPU if > they get accessed: The act of accessing an adjacent object pulls in > cache above your object and destroys DMA coherence. This is the > principle reason why DMA to stack is a bad idea. Another major real reason we can't DMA on-stack stuff is because the stack is mapped virtually on some platforms. And that is the original reason the restriction was put in place. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/6] get rid of on-stack dma buffers
2011/3/22 Roedel, Joerg : > On Mon, Mar 21, 2011 at 05:03:15PM -0400, Florian Mickler wrote: >> I guess (not verified), that the dma api takes sufficient precautions >> to abort the dma transfer if a timeout happens. So freeing _should_ >> not be an issue. (At least, I would expect big fat warnings everywhere >> if that were the case) > > Freeing is very well an issue. All you can expect from the DMA-API is to > give you a valid DMA handle for your device. But it can not prevent that > a device uses this handle after you returned it. You need to make sure > yourself that any pending DMA is canceled before calling kfree(). sorry, I meant usb_control_msg above when I said 'dma api'... as that is the function these drivers use to do the dma. Regards, Flo -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/6] get rid of on-stack dma buffers
2011/3/22 James Bottomley : > On Mon, 2011-03-21 at 22:03 +0100, Florian Mickler wrote: >> On Mon, 21 Mar 2011 15:26:43 -0400 >> Andy Walls wrote: >> >> > Florian Mickler wrote: >> >> To be blunt, I'm not shure I fully understand the requirements myself. >> But as far as I grasped it, the main problem is that we need memory >> which the processor can see as soon as the device has scribbled upon >> it. (think caches and the like) >> >> Somewhere down the line, the buffer to usb_control_msg get's to be >> a parameter to dma_map_single which is described as part of >> the DMA API in Documentation/DMA-API.txt >> >> The main point I filter out from that is that the memory has to begin >> exactly at a cache line boundary... > > The API will round up so that the correct region covers the API. > However, if you have other structures packed into the space (as very > often happens on stack), you get cache line interference in the CPU if > they get accessed: The act of accessing an adjacent object pulls in > cache above your object and destroys DMA coherence. This is the > principle reason why DMA to stack is a bad idea. Thanks, this was the missing piece of information to make sense of why it's bad for stack memory to be part of this. > >> I guess (not verified), that the dma api takes sufficient precautions >> to abort the dma transfer if a timeout happens. So freeing _should_ >> not be an issue. (At least, I would expect big fat warnings everywhere >> if that were the case) I did mean s/dma api/usb_control_msg/ in the above paragraph. As that is the ''dma api'' these drivers are using... sorry for the confusion there... > > No, it doesn't take any precautions like this. the DMA API is just > mapping (possibly via an IOMMU). If the transfer times out, that's done > in the DMA engine of the card, and must be cleaned up by the driver and > unmapped. ok. > The general rule though is never DMA to stack. On some processors, the > way stack is allocated can actually make this not work. > > James thanks, Flo p.s.: hope this message get's through to the list... I am on the road at the moment, so I'm not shure that there won't be any html in it again :( -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/6] get rid of on-stack dma buffers
On Mon, 2011-03-21 at 22:03 +0100, Florian Mickler wrote: > On Mon, 21 Mar 2011 15:26:43 -0400 > Andy Walls wrote: > > > Florian Mickler wrote: > > > > >Hi all! > > > > > >These patches get rid of on-stack dma buffers for some of the dvb-usb > > >drivers. > > >I do not own the hardware, so these are only compile tested. I would > > >appreciate testing and review. > > >They were previously sent to the list, but some error on my side > > >prevented (some of?) them from beeing delivered to all parties (the > > >lists). > > > > > >These changes are motivated by > > >https://bugzilla.kernel.org/show_bug.cgi?id=15977 . > > > > > >The patches which got tested already were submitted to Mauro (and > > >lkml/linux-media) yesterday seperately. Those fix this same issue for > > >ec168, > > >ce6230, au6610 and lmedm04. > > > > > >A fix for vp702x has been submitted seperately for review on the list. > > >I have > > >similiar fixes like the vp702x-fix for dib0700 (overlooked some > > >on-stack > > >buffers in there in my original submission as well) and gp8psk, but I > > >am > > >holding them back 'till I got time to recheck those and getting some > > >feedback > > >on vp702x. > > > > > >Please review and test. > > > > > >Regards, > > >Flo > > > > > >Florian Mickler (6): > > > [media] a800: get rid of on-stack dma buffers > > > [media v2] vp7045: get rid of on-stack dma buffers > > > [media] friio: get rid of on-stack dma buffers > > > [media] dw2102: get rid of on-stack dma buffer > > > [media] m920x: get rid of on-stack dma buffers > > > [media] opera1: get rid of on-stack dma buffer > > > > > > drivers/media/dvb/dvb-usb/a800.c | 17 ++--- > > > drivers/media/dvb/dvb-usb/dw2102.c | 10 ++- > > > drivers/media/dvb/dvb-usb/friio.c | 23 ++--- > > > drivers/media/dvb/dvb-usb/m920x.c | 33 > > > drivers/media/dvb/dvb-usb/opera1.c | 31 +++ > > >drivers/media/dvb/dvb-usb/vp7045.c | 47 > > >++-- > > > 6 files changed, 116 insertions(+), 45 deletions(-) > > > > > >-- > > >1.7.4.1 > > > > > >-- > > >To unsubscribe from this list: send the line "unsubscribe linux-media" > > >in > > >the body of a message to majord...@vger.kernel.org > > >More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > > Florian, > > > > For all of these, what happens when the USB call times out and you kfree() > > the buffer? Can the USB DMA actually complete after this kfree(), possibly > > corrupting space that has been reallocated off the heap, since the kfree()? > > > > This is the scenario for which I assume allocating off the stack is bad. > > > > Do these changes simply make corruption less noticable since heap gets > > corrupted vs stack? > > > > Regards, > > Andy > > To be blunt, I'm not shure I fully understand the requirements myself. > But as far as I grasped it, the main problem is that we need memory > which the processor can see as soon as the device has scribbled upon > it. (think caches and the like) > > Somewhere down the line, the buffer to usb_control_msg get's to be > a parameter to dma_map_single which is described as part of > the DMA API in Documentation/DMA-API.txt > > The main point I filter out from that is that the memory has to begin > exactly at a cache line boundary... The API will round up so that the correct region covers the API. However, if you have other structures packed into the space (as very often happens on stack), you get cache line interference in the CPU if they get accessed: The act of accessing an adjacent object pulls in cache above your object and destroys DMA coherence. This is the principle reason why DMA to stack is a bad idea. > I guess (not verified), that the dma api takes sufficient precautions > to abort the dma transfer if a timeout happens. So freeing _should_ > not be an issue. (At least, I would expect big fat warnings everywhere > if that were the case) No, it doesn't take any precautions like this. the DMA API is just mapping (possibly via an IOMMU). If the transfer times out, that's done in the DMA engine of the card, and must be cleaned up by the driver and unmapped. The general rule though is never DMA to stack. On some processors, the way stack is allocated can actually make this not work. James -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/6] get rid of on-stack dma buffers
On Tue, Mar 22, 2011 at 11:59:32AM +0100, Jiri Kosina wrote: > On Mon, 21 Mar 2011, Florian Mickler wrote: > > > To be blunt, I'm not shure I fully understand the requirements myself. > > But as far as I grasped it, the main problem is that we need memory > > which the processor can see as soon as the device has scribbled upon it. > > (think caches and the like) > > > > Somewhere down the line, the buffer to usb_control_msg get's to be a > > parameter to dma_map_single which is described as part of the DMA API in > > Documentation/DMA-API.txt > > > > The main point I filter out from that is that the memory has to begin > > exactly at a cache line boundary... > > > > I guess (not verified), that the dma api takes sufficient precautions to > > abort the dma transfer if a timeout happens. So freeing _should_ not be > > an issue. (At least, I would expect big fat warnings everywhere if that > > were the case) > > > > I cc'd some people that hopefully will correct me if I'm wrong... > > It all boils down to making sure that you don't free the memory which is > used as target of DMA transfer. > > This is very likely to hit you when DMA memory region is on stack, but > blindly just converting this to kmalloc()/kfree() isn't any better if you > don't make sure that all the DMA transfers have been finished and device > will not be making any more to that particular memory region. I think it is important that one cache line is not shared between a DMA buffer and other objects, especially on architectures where cache coherency is managed in software (ARM, MIPS etc.). If you use kmalloc() for the DMA buffer that is guaranteed. (E.g. DMA API will do writeback/invalidate before the DMA starts, but if the CPU accesses a variable which is next to the buffer while DMA is still pending then the whole cacheline is read back into the cache. If the CPU is then trying to read the DMA buffer after the DMA finished it will see stale data from the cache. You lose.) It depends on the device doing DMA if it needs special alignment. If you meet its alignment requirements, and wait for the end of the DMA before returning, and make sure the buffer does not share cache lines with neighbouring objects on the stack, then you can use DMA buffers on stack. In practice it's simpler and much less error prone to use kmalloc(). Regarding usb_control_msg(), since it is a synchronous API which waits for the end of the transfer I'm relatively sure there is no DMA pending when it returns, even if it aborts with timeout or any other error code. Johannes -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/6] get rid of on-stack dma buffers
Am Dienstag, 22. März 2011, 14:08:17 schrieb Florian Mickler: > Am 22.03.2011 12:10 schrieb "Roedel, Joerg" : > > > > On Mon, Mar 21, 2011 at 05:03:15PM -0400, Florian Mickler wrote: > > > I guess (not verified), that the dma api takes sufficient precautions > > > to abort the dma transfer if a timeout happens. So freeing _should_ > > > not be an issue. (At least, I would expect big fat warnings everywhere > > > if that were the case) > > > > Freeing is very well an issue. All you can expect from the DMA-API is to > > give you a valid DMA handle for your device. But it can not prevent that > > a device uses this handle after you returned it. You need to make sure > > yourself that any pending DMA is canceled before calling kfree(). > > Does usb_control_msg do this? It waits for completion but takes also a > timeout parameter. I will recheck this once I'm home. It uses usb_start_wait_urb() which upon a timeout kills the URB. The buffer is unused after usb_control_msg() returns. HTH Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/6] get rid of on-stack dma buffers
On Mon, 21 Mar 2011, Florian Mickler wrote: > To be blunt, I'm not shure I fully understand the requirements myself. > But as far as I grasped it, the main problem is that we need memory > which the processor can see as soon as the device has scribbled upon it. > (think caches and the like) > > Somewhere down the line, the buffer to usb_control_msg get's to be a > parameter to dma_map_single which is described as part of the DMA API in > Documentation/DMA-API.txt > > The main point I filter out from that is that the memory has to begin > exactly at a cache line boundary... > > I guess (not verified), that the dma api takes sufficient precautions to > abort the dma transfer if a timeout happens. So freeing _should_ not be > an issue. (At least, I would expect big fat warnings everywhere if that > were the case) > > I cc'd some people that hopefully will correct me if I'm wrong... It all boils down to making sure that you don't free the memory which is used as target of DMA transfer. This is very likely to hit you when DMA memory region is on stack, but blindly just converting this to kmalloc()/kfree() isn't any better if you don't make sure that all the DMA transfers have been finished and device will not be making any more to that particular memory region. -- Jiri Kosina SUSE Labs, Novell Inc. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/6] get rid of on-stack dma buffers
On Mon, Mar 21, 2011 at 05:03:15PM -0400, Florian Mickler wrote: > I guess (not verified), that the dma api takes sufficient precautions > to abort the dma transfer if a timeout happens. So freeing _should_ > not be an issue. (At least, I would expect big fat warnings everywhere > if that were the case) Freeing is very well an issue. All you can expect from the DMA-API is to give you a valid DMA handle for your device. But it can not prevent that a device uses this handle after you returned it. You need to make sure yourself that any pending DMA is canceled before calling kfree(). Joerg -- AMD Operating System Research Center Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach General Managers: Alberto Bozzo, Andrew Bowd Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/6] get rid of on-stack dma buffers
On Mon, 21 Mar 2011 15:26:43 -0400 Andy Walls wrote: > Florian Mickler wrote: > > >Hi all! > > > >These patches get rid of on-stack dma buffers for some of the dvb-usb > >drivers. > >I do not own the hardware, so these are only compile tested. I would > >appreciate testing and review. > >They were previously sent to the list, but some error on my side > >prevented (some of?) them from beeing delivered to all parties (the > >lists). > > > >These changes are motivated by > >https://bugzilla.kernel.org/show_bug.cgi?id=15977 . > > > >The patches which got tested already were submitted to Mauro (and > >lkml/linux-media) yesterday seperately. Those fix this same issue for > >ec168, > >ce6230, au6610 and lmedm04. > > > >A fix for vp702x has been submitted seperately for review on the list. > >I have > >similiar fixes like the vp702x-fix for dib0700 (overlooked some > >on-stack > >buffers in there in my original submission as well) and gp8psk, but I > >am > >holding them back 'till I got time to recheck those and getting some > >feedback > >on vp702x. > > > >Please review and test. > > > >Regards, > >Flo > > > >Florian Mickler (6): > > [media] a800: get rid of on-stack dma buffers > > [media v2] vp7045: get rid of on-stack dma buffers > > [media] friio: get rid of on-stack dma buffers > > [media] dw2102: get rid of on-stack dma buffer > > [media] m920x: get rid of on-stack dma buffers > > [media] opera1: get rid of on-stack dma buffer > > > > drivers/media/dvb/dvb-usb/a800.c | 17 ++--- > > drivers/media/dvb/dvb-usb/dw2102.c | 10 ++- > > drivers/media/dvb/dvb-usb/friio.c | 23 ++--- > > drivers/media/dvb/dvb-usb/m920x.c | 33 > > drivers/media/dvb/dvb-usb/opera1.c | 31 +++ > >drivers/media/dvb/dvb-usb/vp7045.c | 47 > >++-- > > 6 files changed, 116 insertions(+), 45 deletions(-) > > > >-- > >1.7.4.1 > > > >-- > >To unsubscribe from this list: send the line "unsubscribe linux-media" > >in > >the body of a message to majord...@vger.kernel.org > >More majordomo info at http://vger.kernel.org/majordomo-info.html > > Florian, > > For all of these, what happens when the USB call times out and you kfree() > the buffer? Can the USB DMA actually complete after this kfree(), possibly > corrupting space that has been reallocated off the heap, since the kfree()? > > This is the scenario for which I assume allocating off the stack is bad. > > Do these changes simply make corruption less noticable since heap gets > corrupted vs stack? > > Regards, > Andy To be blunt, I'm not shure I fully understand the requirements myself. But as far as I grasped it, the main problem is that we need memory which the processor can see as soon as the device has scribbled upon it. (think caches and the like) Somewhere down the line, the buffer to usb_control_msg get's to be a parameter to dma_map_single which is described as part of the DMA API in Documentation/DMA-API.txt The main point I filter out from that is that the memory has to begin exactly at a cache line boundary... I guess (not verified), that the dma api takes sufficient precautions to abort the dma transfer if a timeout happens. So freeing _should_ not be an issue. (At least, I would expect big fat warnings everywhere if that were the case) I cc'd some people that hopefully will correct me if I'm wrong... regards, Flo -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/6] get rid of on-stack dma buffers
Florian Mickler wrote: >Hi all! > >These patches get rid of on-stack dma buffers for some of the dvb-usb >drivers. >I do not own the hardware, so these are only compile tested. I would >appreciate testing and review. >They were previously sent to the list, but some error on my side >prevented (some of?) them from beeing delivered to all parties (the >lists). > >These changes are motivated by >https://bugzilla.kernel.org/show_bug.cgi?id=15977 . > >The patches which got tested already were submitted to Mauro (and >lkml/linux-media) yesterday seperately. Those fix this same issue for >ec168, >ce6230, au6610 and lmedm04. > >A fix for vp702x has been submitted seperately for review on the list. >I have >similiar fixes like the vp702x-fix for dib0700 (overlooked some >on-stack >buffers in there in my original submission as well) and gp8psk, but I >am >holding them back 'till I got time to recheck those and getting some >feedback >on vp702x. > >Please review and test. > >Regards, >Flo > >Florian Mickler (6): > [media] a800: get rid of on-stack dma buffers > [media v2] vp7045: get rid of on-stack dma buffers > [media] friio: get rid of on-stack dma buffers > [media] dw2102: get rid of on-stack dma buffer > [media] m920x: get rid of on-stack dma buffers > [media] opera1: get rid of on-stack dma buffer > > drivers/media/dvb/dvb-usb/a800.c | 17 ++--- > drivers/media/dvb/dvb-usb/dw2102.c | 10 ++- > drivers/media/dvb/dvb-usb/friio.c | 23 ++--- > drivers/media/dvb/dvb-usb/m920x.c | 33 > drivers/media/dvb/dvb-usb/opera1.c | 31 +++ >drivers/media/dvb/dvb-usb/vp7045.c | 47 >++-- > 6 files changed, 116 insertions(+), 45 deletions(-) > >-- >1.7.4.1 > >-- >To unsubscribe from this list: send the line "unsubscribe linux-media" >in >the body of a message to majord...@vger.kernel.org >More majordomo info at http://vger.kernel.org/majordomo-info.html Florian, For all of these, what happens when the USB call times out and you kfree() the buffer? Can the USB DMA actually complete after this kfree(), possibly corrupting space that has been reallocated off the heap, since the kfree()? This is the scenario for which I assume allocating off the stack is bad. Do these changes simply make corruption less noticable since heap gets corrupted vs stack? Regards, Andy -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
