[PATCH] dell_rbu: stop abusing the DMA API

[PATCH] dell_rbu: stop abusing the DMA API

[Christoph Hellwig]

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)
-- 
2.20.1

Re: [PATCH] dell_rbu: stop abusing the DMA API

[Stuart Hayes]

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>

Re: [PATCH] dell_rbu: stop abusing the DMA API

[Darren Hart]

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.

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH] dell_rbu: stop abusing the DMA API

[Darren Hart]

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.

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH] dell_rbu: stop abusing the DMA API

[Christoph Hellwig]

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.

Re: [PATCH] dell_rbu: stop abusing the DMA API

[Darren Hart]

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.


-- 
Darren Hart
VMware Open Source Technology Center