usb: core: config.c: use buffer on stack rather then on heap

PATCH In review Michal Nazarewicz 3 participants 5 messages Apr 17, 2010 – Apr 30, 2010 USB

Share & tools

Michal Nazarewicz

author

2010-04-17 12:38 UTC #0 lore raw

PATCH 1/1 usb: core: config.c: use buffer on stack rather then on heap

usb_get_configuration() uses a temporary buffer allocated on heap to read USB configuration descriptor. The buffer is just nine bytes an so it is a waste to allocate it on heap where it can be allocated on stack with the rest of local variables. This simplifies the code and minimises memory usage.

Signed-off-by Michal Nazarewicz <[email protected]>

---
 drivers/usb/core/config.c |   16 ++++------------
 1 files changed, 4 insertions(+), 12 deletions(-)

drivers/usb/core/config.c +4 ~~-12~~

index 0d3af6a..78d3f87 100644
@@ -722,13 +722,12 @@ int usb_get_configuration(struct usb_device *dev)
 	int ncfg = dev->descriptor.bNumConfigurations;
 	int result = 0;
 	unsigned int cfgno, length;
-	unsigned char *buffer;
 	unsigned char *bigbuffer;
-	struct usb_config_descriptor *desc;
+	struct usb_config_descriptor desc;
 
 	cfgno = 0;
 	if (dev->authorized == 0)	/* Not really an error */
-		goto out_not_authorized;
+		goto err;
 	result = -ENOMEM;
 	if (ncfg > USB_MAXCONFIG) {
 		dev_warn(ddev, "too many configurations: %d, "

@@ -751,17 +750,12 @@ int usb_get_configuration(struct usb_device *dev)
 	if (!dev->rawdescriptors)
 		goto err2;
 
-	buffer = kmalloc(USB_DT_CONFIG_SIZE, GFP_KERNEL);
-	if (!buffer)
-		goto err2;
-	desc = (struct usb_config_descriptor *)buffer;
-
 	result = 0;
 	for (; cfgno < ncfg; cfgno++) {
 		/* We grab just the first descriptor so we know how long
 		 * the whole configuration is */
 		result = usb_get_descriptor(dev, USB_DT_CONFIG, cfgno,
-		    buffer, USB_DT_CONFIG_SIZE);
+		    &desc, USB_DT_CONFIG_SIZE);
 		if (result < 0) {
 			dev_err(ddev, "unable to read config index %d "
 			    "descriptor/%s: %d\n", cfgno, "start", result);

@@ -775,7 +769,7 @@ int usb_get_configuration(struct usb_device *dev)
 			result = -EINVAL;
 			goto err;
 		}
-		length = max((int) le16_to_cpu(desc->wTotalLength),
+		length = max((int) le16_to_cpu(desc.wTotalLength),
 		    USB_DT_CONFIG_SIZE);
 
 		/* Now that we know the length, get the whole thing */

@@ -810,8 +804,6 @@ int usb_get_configuration(struct usb_device *dev)
 	result = 0;
 
 err:
-	kfree(buffer);
-out_not_authorized:
 	dev->descriptor.bNumConfigurations = cfgno;
 err2:
 	if (result == -ENOMEM)

Signature · 4 lines

Best regards,                                         _     _
 .o. | Liege of Serenly Enlightened Majesty of      o' \,=./ `o
 ..o | Computer Science,  Michal "mina86" Nazarewicz   (o o)
 ooo +--<mina86-tlen.pl>--<jid:mina86-jabber.org>--ooO--(_)--Ooo--

Alan Stern

2010-04-17 14:42 UTC #1 lore raw

Quoted 5 lines · Michal Nazarewicz

usb_get_configuration() uses a temporary buffer allocated on heap to read USB configuration descriptor. The buffer is just nine bytes an so it is a waste to allocate it on heap where it can be allocated on stack with the rest of local variables. This simplifies the code and minimises memory usage.

This is completely wrong. You are not allowed to do DMA to buffers on the stack; some architectures are not capable of handling it.

Alan Stern

Michal Nazarewicz

author

2010-04-17 15:13 UTC #2 lore raw

PATCH 1/1 usb: core: config.c: usb_get_configuration() simplified

usb_gat_configuratio() used two pointers to point to the same memory. Code simplified, by removing one of them.

Signed-off-by Michal Nazarewicz <[email protected]>

---
 drivers/usb/core/config.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

Quoted 6 lines · Michal Nazarewicz

Quoted 5 lines
usb_get_configuration() uses a temporary buffer allocated on heap to read USB configuration descriptor. The buffer is just nine bytes an so it is a waste to allocate it on heap where it can be allocated on stack with the rest of local variables. This simplifies the code and minimises memory usage.

Quoted 2 lines · Alan Stern

This is completely wrong. You are not allowed to do DMA to buffers on the stack; some architectures are not capable of handling it.

That makes sense; I haven't considered this thinking that copying nine bytes by CPU, rather then using DMA, is not a big issue.

Still, the change proposed by attached commit does not suffer from DMA issue and still simplify the code.

drivers/usb/core/config.c +4 -6

index 0d3af6a..4b23e7f 100644
@@ -722,7 +722,6 @@ int usb_get_configuration(struct usb_device *dev)
 	int ncfg = dev->descriptor.bNumConfigurations;
 	int result = 0;
 	unsigned int cfgno, length;
-	unsigned char *buffer;
 	unsigned char *bigbuffer;
 	struct usb_config_descriptor *desc;

@@ -751,17 +750,16 @@ int usb_get_configuration(struct usb_device *dev)
 	if (!dev->rawdescriptors)
 		goto err2;
 
-	buffer = kmalloc(USB_DT_CONFIG_SIZE, GFP_KERNEL);
-	if (!buffer)
+	desc = kmalloc(USB_DT_CONFIG_SIZE, GFP_KERNEL);
+	if (!desc)
 		goto err2;
-	desc = (struct usb_config_descriptor *)buffer;
 
 	result = 0;
 	for (; cfgno < ncfg; cfgno++) {
 		/* We grab just the first descriptor so we know how long
 		 * the whole configuration is */
 		result = usb_get_descriptor(dev, USB_DT_CONFIG, cfgno,
-		    buffer, USB_DT_CONFIG_SIZE);
+		    desc, USB_DT_CONFIG_SIZE);
 		if (result < 0) {
 			dev_err(ddev, "unable to read config index %d "
 			    "descriptor/%s: %d\n", cfgno, "start", result);

@@ -810,7 +808,7 @@ int usb_get_configuration(struct usb_device *dev)
 	result = 0;
 
 err:
-	kfree(buffer);
+	kfree(desc);
 out_not_authorized:
 	dev->descriptor.bNumConfigurations = cfgno;
 err2:

Signature · 4 lines

Best regards,                                         _     _
 .o. | Liege of Serenly Enlightened Majesty of      o' \,=./ `o
 ..o | Computer Science,  Michal "mina86" Nazarewicz   (o o)
 ooo +--<mina86-tlen.pl>--<jid:mina86-jabber.org>--ooO--(_)--Ooo--

Alan Stern

2010-04-17 16:13 UTC #3 lore raw

Re: [PATCH] usb: core: config.c: usb_get_configuration() simplified

Quoted 16 lines · Michal Nazarewicz

usb_gat_configuratio() used two pointers to point to the same memory. Code simplified, by removing one of them.
Signed-off-by Michal Nazarewicz <[email protected]>
---
 drivers/usb/core/config.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)
Quoted 6 lines · Michal Nazarewicz
Quoted 5 lines
usb_get_configuration() uses a temporary buffer allocated on heap to read USB configuration descriptor. The buffer is just nine bytes an so it is a waste to allocate it on heap where it can be allocated on stack with the rest of local variables. This simplifies the code and minimises memory usage.
Alan Stern <[email protected]> writes:

Quoted 8 lines

Quoted 2 lines
This is completely wrong. You are not allowed to do DMA to buffers on the stack; some architectures are not capable of handling it.
That makes sense; I haven't considered this thinking that copying nine bytes by CPU, rather then using DMA, is not a big issue.
Still, the change proposed by attached commit does not suffer from DMA issue and still simplify the code.

Yes, this is fine.

Alan Stern

[email protected]

Applied

2010-04-30 16:59 UTC #4 lore raw

patch usb-core-config.c-usb_get_configuration-simplified.patch added to gregkh-2.6 tree

In reply to Michal Nazarewicz

This is a note to let you know that I've just added the patch titled

Subject: USB: core: config.c: usb_get_configuration() simplified

to my gregkh-2.6 tree. Its filename is

usb-core-config.c-usb_get_configuration-simplified.patch

This tree can be found at
http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/

Quoted 1 line

From [email protected] Thu Apr 29 16:05:40 2010

From: Michal Nazarewicz <[email protected]>
Date: Sat, 17 Apr 2010 17:12:58 +0200
Subject: USB: core: config.c: usb_get_configuration() simplified
To: Alan Stern <[email protected]>
Cc: [email protected], <[email protected]>, Greg Kroah-Hartman <[email protected]>
Message-ID: <[email protected]>

usb_gat_configuratio() used two pointers to point to the same memory. Code simplified, by removing one of them.

Signed-off-by Michal Nazarewicz <[email protected]>

Cc Alan Stern <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 drivers/usb/core/config.c |   10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

drivers/usb/core/config.c +4 -6

@@ -735,7 +735,6 @@ int usb_get_configuration(struct usb_dev
 	int ncfg = dev->descriptor.bNumConfigurations;
 	int result = 0;
 	unsigned int cfgno, length;
-	unsigned char *buffer;
 	unsigned char *bigbuffer;
 	struct usb_config_descriptor *desc;

@@ -764,17 +763,16 @@ int usb_get_configuration(struct usb_dev
 	if (!dev->rawdescriptors)
 		goto err2;
 
-	buffer = kmalloc(USB_DT_CONFIG_SIZE, GFP_KERNEL);
-	if (!buffer)
+	desc = kmalloc(USB_DT_CONFIG_SIZE, GFP_KERNEL);
+	if (!desc)
 		goto err2;
-	desc = (struct usb_config_descriptor *)buffer;
 
 	result = 0;
 	for (; cfgno < ncfg; cfgno++) {
 		/* We grab just the first descriptor so we know how long
 		 * the whole configuration is */
 		result = usb_get_descriptor(dev, USB_DT_CONFIG, cfgno,
-		    buffer, USB_DT_CONFIG_SIZE);
+		    desc, USB_DT_CONFIG_SIZE);
 		if (result < 0) {
 			dev_err(ddev, "unable to read config index %d "
 			    "descriptor/%s: %d\n", cfgno, "start", result);

@@ -823,7 +821,7 @@ int usb_get_configuration(struct usb_dev
 	result = 0;
 
 err:
-	kfree(buffer);
+	kfree(desc);
 out_not_authorized:
 	dev->descriptor.bNumConfigurations = cfgno;
 err2: