| Message ID | 20190129073409.7247-1-hch@lst.de (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Darren Hart |
| Headers | show |
| Series | dell_rbu: stop abusing the DMA API | expand |
On 1/29/2019 1:34 AM, Christoph Hellwig wrote: > For some odd reason dell_rbu actually seems to want the physical and > not a bus address for the allocated buffer. Lets assume that actually > is correct given that it is BIOS-related and that is a good source > of insanity. In that case we should not use dma_alloc_coherent with > a NULL device to allocate memory, but use GFP_DMA32 to stay under > the 32-bit BIOS limit. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > drivers/platform/x86/dell_rbu.c | 52 ++++++++++----------------------- > 1 file changed, 15 insertions(+), 37 deletions(-) > > diff --git a/drivers/platform/x86/dell_rbu.c b/drivers/platform/x86/dell_rbu.c > index ccefa84f7305..fba7d96c1714 100644 > --- a/drivers/platform/x86/dell_rbu.c > +++ b/drivers/platform/x86/dell_rbu.c > @@ -59,7 +59,6 @@ static struct _rbu_data { > unsigned long image_update_buffer_size; > unsigned long bios_image_size; > int image_update_ordernum; > - int dma_alloc; > spinlock_t lock; > unsigned long packet_read_count; > unsigned long num_packets; > @@ -89,7 +88,6 @@ static struct packet_data packet_data_head; > > static struct platform_device *rbu_device; > static int context; > -static dma_addr_t dell_rbu_dmaaddr; > > static void init_packet_head(void) > { > @@ -380,12 +378,8 @@ static void img_update_free(void) > */ > memset(rbu_data.image_update_buffer, 0, > rbu_data.image_update_buffer_size); > - if (rbu_data.dma_alloc == 1) > - dma_free_coherent(NULL, rbu_data.bios_image_size, > - rbu_data.image_update_buffer, dell_rbu_dmaaddr); > - else > - free_pages((unsigned long) rbu_data.image_update_buffer, > - rbu_data.image_update_ordernum); > + free_pages((unsigned long) rbu_data.image_update_buffer, > + rbu_data.image_update_ordernum); > > /* > * Re-initialize the rbu_data variables after a free > @@ -394,7 +388,6 @@ static void img_update_free(void) > rbu_data.image_update_buffer = NULL; > rbu_data.image_update_buffer_size = 0; > rbu_data.bios_image_size = 0; > - rbu_data.dma_alloc = 0; > } > > /* > @@ -410,10 +403,8 @@ static void img_update_free(void) > static int img_update_realloc(unsigned long size) > { > unsigned char *image_update_buffer = NULL; > - unsigned long rc; > unsigned long img_buf_phys_addr; > int ordernum; > - int dma_alloc = 0; > > /* > * check if the buffer of sufficient size has been > @@ -444,36 +435,23 @@ static int img_update_realloc(unsigned long size) > > ordernum = get_order(size); > image_update_buffer = > - (unsigned char *) __get_free_pages(GFP_KERNEL, ordernum); > - > - img_buf_phys_addr = > - (unsigned long) virt_to_phys(image_update_buffer); > - > - if (img_buf_phys_addr > BIOS_SCAN_LIMIT) { > - free_pages((unsigned long) image_update_buffer, ordernum); > - ordernum = -1; > - image_update_buffer = dma_alloc_coherent(NULL, size, > - &dell_rbu_dmaaddr, GFP_KERNEL); > - dma_alloc = 1; > - } > - > - spin_lock(&rbu_data.lock); > - > - if (image_update_buffer != NULL) { > - rbu_data.image_update_buffer = image_update_buffer; > - rbu_data.image_update_buffer_size = size; > - rbu_data.bios_image_size = > - rbu_data.image_update_buffer_size; > - rbu_data.image_update_ordernum = ordernum; > - rbu_data.dma_alloc = dma_alloc; > - rc = 0; > - } else { > + (unsigned char *)__get_free_pages(GFP_DMA32, ordernum); > + if (!image_update_buffer) { > pr_debug("Not enough memory for image update:" > "size = %ld\n", size); > - rc = -ENOMEM; > + return -ENOMEM; > } > > - return rc; > + img_buf_phys_addr = (unsigned long)virt_to_phys(image_update_buffer); > + if (WARN_ON_ONCE(img_buf_phys_addr > BIOS_SCAN_LIMIT)) > + return -EINVAL; /* can't happen per defintion */ > + > + spin_lock(&rbu_data.lock); > + rbu_data.image_update_buffer = image_update_buffer; > + rbu_data.image_update_buffer_size = size; > + rbu_data.bios_image_size = rbu_data.image_update_buffer_size; > + rbu_data.image_update_ordernum = ordernum; > + return 0; > } > > static ssize_t read_packet_data(char *buffer, loff_t pos, size_t count) > Acked-by: Stuart Hayes <stuart.w.hayes@gmail.com>
On Tue, Jan 29, 2019 at 08:34:09AM +0100, Christoph Hellwig wrote: > For some odd reason dell_rbu actually seems to want the physical and > not a bus address for the allocated buffer. Lets assume that actually > is correct given that it is BIOS-related and that is a good source > of insanity. In that case we should not use dma_alloc_coherent with > a NULL device to allocate memory, but use GFP_DMA32 to stay under > the 32-bit BIOS limit. + Mario re bios related physical address - is Christoph's assumption correct? Christoph, did you observe a failure? If so, we should probably also tag for stable.
On Fri, Feb 01, 2019 at 12:28:46PM -0600, Stuart Hayes wrote: > > On 1/29/2019 1:34 AM, Christoph Hellwig wrote: > > For some odd reason dell_rbu actually seems to want the physical and > > not a bus address for the allocated buffer. Lets assume that actually > > is correct given that it is BIOS-related and that is a good source > > of insanity. In that case we should not use dma_alloc_coherent with > > a NULL device to allocate memory, but use GFP_DMA32 to stay under > > the 32-bit BIOS limit. > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > --- ... > > Acked-by: Stuart Hayes <stuart.w.hayes@gmail.com> > Ah, prematurely pulled in Mario - thanks Stuart.
On Fri, Feb 01, 2019 at 03:15:59PM -0800, Darren Hart wrote: > On Tue, Jan 29, 2019 at 08:34:09AM +0100, Christoph Hellwig wrote: > > For some odd reason dell_rbu actually seems to want the physical and > > not a bus address for the allocated buffer. Lets assume that actually > > is correct given that it is BIOS-related and that is a good source > > of insanity. In that case we should not use dma_alloc_coherent with > > a NULL device to allocate memory, but use GFP_DMA32 to stay under > > the 32-bit BIOS limit. > > + Mario re bios related physical address - is Christoph's assumption > correct? > > Christoph, did you observe a failure? If so, we should probably also > tag for stable. No, I've been auditing for DMA API (ab-)users that don't pass a struct device. Generally the fix was to just pass a struct device that is easily available. But dell_rbu doesn't actually seem to be a "device" in the traditional sense, and the way it uses the DMA API is really, really odd - it first does a virt_to_phys on memory allocated from the page allocator (so works with physical addresses in that case) and the retries with a dma_alloc_coherent with a NULL argument, which in no way is guaranteed to you give you something else, although for the current x86 implementation will give you the equivalent of a GFP_DMA32 page allocator allocation plus virt_to_phys.
On Sat, Feb 02, 2019 at 06:16:59PM +0100, Christoph Hellwig wrote: > On Fri, Feb 01, 2019 at 03:15:59PM -0800, Darren Hart wrote: > > On Tue, Jan 29, 2019 at 08:34:09AM +0100, Christoph Hellwig wrote: > > > For some odd reason dell_rbu actually seems to want the physical and > > > not a bus address for the allocated buffer. Lets assume that actually > > > is correct given that it is BIOS-related and that is a good source > > > of insanity. In that case we should not use dma_alloc_coherent with > > > a NULL device to allocate memory, but use GFP_DMA32 to stay under > > > the 32-bit BIOS limit. > > > > + Mario re bios related physical address - is Christoph's assumption > > correct? > > > > Christoph, did you observe a failure? If so, we should probably also > > tag for stable. > > No, I've been auditing for DMA API (ab-)users that don't pass a > struct device. Generally the fix was to just pass a struct device > that is easily available. But dell_rbu doesn't actually seem to > be a "device" in the traditional sense, and the way it uses the > DMA API is really, really odd - it first does a virt_to_phys on > memory allocated from the page allocator (so works with physical > addresses in that case) and the retries with a dma_alloc_coherent > with a NULL argument, which in no way is guaranteed to you give > you something else, although for the current x86 implementation > will give you the equivalent of a GFP_DMA32 page allocator allocation > plus virt_to_phys. > Thanks Christoph, merged to for-next.
diff --git a/drivers/platform/x86/dell_rbu.c b/drivers/platform/x86/dell_rbu.c index ccefa84f7305..fba7d96c1714 100644 --- a/drivers/platform/x86/dell_rbu.c +++ b/drivers/platform/x86/dell_rbu.c @@ -59,7 +59,6 @@ static struct _rbu_data { unsigned long image_update_buffer_size; unsigned long bios_image_size; int image_update_ordernum; - int dma_alloc; spinlock_t lock; unsigned long packet_read_count; unsigned long num_packets; @@ -89,7 +88,6 @@ static struct packet_data packet_data_head; static struct platform_device *rbu_device; static int context; -static dma_addr_t dell_rbu_dmaaddr; static void init_packet_head(void) { @@ -380,12 +378,8 @@ static void img_update_free(void) */ memset(rbu_data.image_update_buffer, 0, rbu_data.image_update_buffer_size); - if (rbu_data.dma_alloc == 1) - dma_free_coherent(NULL, rbu_data.bios_image_size, - rbu_data.image_update_buffer, dell_rbu_dmaaddr); - else - free_pages((unsigned long) rbu_data.image_update_buffer, - rbu_data.image_update_ordernum); + free_pages((unsigned long) rbu_data.image_update_buffer, + rbu_data.image_update_ordernum); /* * Re-initialize the rbu_data variables after a free @@ -394,7 +388,6 @@ static void img_update_free(void) rbu_data.image_update_buffer = NULL; rbu_data.image_update_buffer_size = 0; rbu_data.bios_image_size = 0; - rbu_data.dma_alloc = 0; } /* @@ -410,10 +403,8 @@ static void img_update_free(void) static int img_update_realloc(unsigned long size) { unsigned char *image_update_buffer = NULL; - unsigned long rc; unsigned long img_buf_phys_addr; int ordernum; - int dma_alloc = 0; /* * check if the buffer of sufficient size has been @@ -444,36 +435,23 @@ static int img_update_realloc(unsigned long size) ordernum = get_order(size); image_update_buffer = - (unsigned char *) __get_free_pages(GFP_KERNEL, ordernum); - - img_buf_phys_addr = - (unsigned long) virt_to_phys(image_update_buffer); - - if (img_buf_phys_addr > BIOS_SCAN_LIMIT) { - free_pages((unsigned long) image_update_buffer, ordernum); - ordernum = -1; - image_update_buffer = dma_alloc_coherent(NULL, size, - &dell_rbu_dmaaddr, GFP_KERNEL); - dma_alloc = 1; - } - - spin_lock(&rbu_data.lock); - - if (image_update_buffer != NULL) { - rbu_data.image_update_buffer = image_update_buffer; - rbu_data.image_update_buffer_size = size; - rbu_data.bios_image_size = - rbu_data.image_update_buffer_size; - rbu_data.image_update_ordernum = ordernum; - rbu_data.dma_alloc = dma_alloc; - rc = 0; - } else { + (unsigned char *)__get_free_pages(GFP_DMA32, ordernum); + if (!image_update_buffer) { pr_debug("Not enough memory for image update:" "size = %ld\n", size); - rc = -ENOMEM; + return -ENOMEM; } - return rc; + img_buf_phys_addr = (unsigned long)virt_to_phys(image_update_buffer); + if (WARN_ON_ONCE(img_buf_phys_addr > BIOS_SCAN_LIMIT)) + return -EINVAL; /* can't happen per defintion */ + + spin_lock(&rbu_data.lock); + rbu_data.image_update_buffer = image_update_buffer; + rbu_data.image_update_buffer_size = size; + rbu_data.bios_image_size = rbu_data.image_update_buffer_size; + rbu_data.image_update_ordernum = ordernum; + return 0; } static ssize_t read_packet_data(char *buffer, loff_t pos, size_t count)
For some odd reason dell_rbu actually seems to want the physical and not a bus address for the allocated buffer. Lets assume that actually is correct given that it is BIOS-related and that is a good source of insanity. In that case we should not use dma_alloc_coherent with a NULL device to allocate memory, but use GFP_DMA32 to stay under the 32-bit BIOS limit. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/platform/x86/dell_rbu.c | 52 ++++++++++----------------------- 1 file changed, 15 insertions(+), 37 deletions(-)