usb: io_edgeport: eliminate get_string()

PATCH In review Dan Carpenter 5 participants 13 messages Jan 25, 2010 – Jan 26, 2010

Subsystems (3)

USB Kernel janitors USB serial

lore

Share & tools

lore

Dan Carpenter

author

2010-01-25 11:53 UTC #0 lore raw

PATCH 1/1 usb: io_edgeport: eliminate get_string()

Johan Hovold points out that get_string() is basically just a re-implimentation of usb_string(). It is also buggy. It does DMA on the stack and it doesn't handle negative returns from usb_get_descriptor(). Plus unicode_to_ascii() is a rubbish function and moving to usb_string() avoids using it.

Let's eliminate get_string() entirely.

Reported-by Johan Hovold <[email protected]>

Signed-off-by Dan Carpenter <[email protected]>

Cc [email protected]

drivers/usb/serial/io_edgeport.c +4 ~~-38~~

@@ -364,42 +364,6 @@ static void update_edgeport_E2PROM(struc
 	release_firmware(fw);
 }
 
-
-/************************************************************************
- *									*
- *  Get string descriptor from device					*
- *									*
- ************************************************************************/
-static int get_string(struct usb_device *dev, int Id, char *string, int buflen)
-{
-	struct usb_string_descriptor StringDesc;
-	struct usb_string_descriptor *pStringDesc;
-
-	dbg("%s - USB String ID = %d", __func__, Id);
-
-	if (!usb_get_descriptor(dev, USB_DT_STRING, Id,
-					&StringDesc, sizeof(StringDesc)))
-		return 0;
-
-	pStringDesc = kmalloc(StringDesc.bLength, GFP_KERNEL);
-	if (!pStringDesc)
-		return 0;
-
-	if (!usb_get_descriptor(dev, USB_DT_STRING, Id,
-					pStringDesc, StringDesc.bLength)) {
-		kfree(pStringDesc);
-		return 0;
-	}
-
-	unicode_to_ascii(string, buflen,
-				pStringDesc->wData, pStringDesc->bLength/2);
-
-	kfree(pStringDesc);
-	dbg("%s - USB String %s", __func__, string);
-	return strlen(string);
-}
-
-
 #if 0
 /************************************************************************
  *

@@ -2997,10 +2961,12 @@ static int edge_startup(struct usb_seria
 	usb_set_serial_data(serial, edge_serial);
 
 	/* get the name for the device from the device */
-	i = get_string(dev, dev->descriptor.iManufacturer,
+	i = usb_string(dev, dev->descriptor.iManufacturer,
 	    &edge_serial->name[0], MAX_NAME_LEN+1);
+	if (i < 0)
+		i = 0;
 	edge_serial->name[i++] = ' ';
-	get_string(dev, dev->descriptor.iProduct,
+	usb_string(dev, dev->descriptor.iProduct,
 	    &edge_serial->name[i], MAX_NAME_LEN+2 - i);
 
 	dev_info(&serial->dev->dev, "%s detected\n", edge_serial->name);

Johan Hovold

2010-01-25 16:57 UTC #1 lore raw

Quoted 10 lines · Dan Carpenter

Johan Hovold points out that get_string() is basically just a re-implimentation of usb_string(). It is also buggy. It does DMA on the stack and it doesn't handle negative returns from usb_get_descriptor(). Plus unicode_to_ascii() is a rubbish function and moving to usb_string() avoids using it.
Let's eliminate get_string() entirely.
Reported-by Johan Hovold <[email protected]>
Signed-off-by Dan Carpenter <[email protected]>
Cc [email protected]

Acked.

How do we generally deal with patches with multiple authors? This patch is a logical and obvious extension of the patch I submitted yesterday, which in turn was a fix (implemented from scratch) for two bugs pointed out by Dan.

This particular change is trivial of course, but are there any general guidelines for such cases?

Thanks,
Johan

Greg KH

2010-01-25 20:40 UTC #2 lore raw

In reply to Dan Carpenter

Quoted 6 lines · Dan Carpenter

Johan Hovold points out that get_string() is basically just a re-implimentation of usb_string(). It is also buggy. It does DMA on the stack and it doesn't handle negative returns from usb_get_descriptor(). Plus unicode_to_ascii() is a rubbish function and moving to usb_string() avoids using it.
Let's eliminate get_string() entirely.

Hm, for some reason I couldn't use usb_string() when I originally wrote this code (way back in 1999). I can't recall what it is, but have you tested this with an edgeport device to verify that it doesn't break anything?

thanks,

greg k-h

Greg KH

2010-01-25 20:41 UTC #3 lore raw

In reply to Dan Carpenter

Quoted 14 lines · Johan Hovold

Quoted 10 lines · Dan Carpenter
Johan Hovold points out that get_string() is basically just a re-implimentation of usb_string(). It is also buggy. It does DMA on the stack and it doesn't handle negative returns from usb_get_descriptor(). Plus unicode_to_ascii() is a rubbish function and moving to usb_string() avoids using it.
Let's eliminate get_string() entirely.
Reported-by Johan Hovold <[email protected]>
Signed-off-by Dan Carpenter <[email protected]>
Cc [email protected]
Acked.

Quoted 7 lines

How do we generally deal with patches with multiple authors? This patch is a logical and obvious extension of the patch I submitted yesterday, which in turn was a fix (implemented from scratch) for two bugs pointed out by Dan.
This particular change is trivial of course, but are there any general guidelines for such cases?

No, not really. If you can think of something that would work with git as-is, I'm sure that there would be people interested.

thanks,

greg k-h

Greg KH

2010-01-25 20:41 UTC #4 lore raw

In reply to Dan Carpenter

Quoted 10 lines · Dan Carpenter

Johan Hovold points out that get_string() is basically just a re-implimentation of usb_string(). It is also buggy. It does DMA on the stack and it doesn't handle negative returns from usb_get_descriptor(). Plus unicode_to_ascii() is a rubbish function and moving to usb_string() avoids using it.
Let's eliminate get_string() entirely.
Reported-by Johan Hovold <[email protected]>
Signed-off-by Dan Carpenter <[email protected]>
Cc [email protected]

Why stable? Does this fix an existing bug somewhere?

thanks,

greg k-h

Johan Hovold

Acked-by on v1 1/1

2010-01-25 20:54 UTC #5 lore raw

In reply to Dan Carpenter

Quoted 6 lines · Johan Hovold

Quoted 5 lines · Dan Carpenter
Johan Hovold points out that get_string() is basically just a re-implimentation of usb_string(). It is also buggy. It does DMA on the stack and it doesn't handle negative returns from usb_get_descriptor(). Plus unicode_to_ascii() is a rubbish function and moving to usb_string() avoids using it.

Quoted 7 lines

Quoted 5 lines
Let's eliminate get_string() entirely.
Reported-by Johan Hovold <[email protected]>
Signed-off-by Dan Carpenter <[email protected]>
Cc [email protected]
Acked.

Should qualify that ack:

I don't think this patch should go to stable as it does not fix anything severe. Worst case is that in the unlikely event that usb_control_msg fails _and_ you have debugging enabled one may end up with 64 non-sense characters in you log. There's no risk for any memory corruption AFAICT.

The patch do however change the way communication is made at probe time quite a bit (asking for language codes available, resends, first tries to fetch strings using the maximum length then falls back to getting the length first, etc.).

So except for the stable bit:

Acked-by Johan Hovold <[email protected]>

Oliver Neukum

2010-01-25 21:47 UTC #6 lore raw

In reply to Dan Carpenter

Am Montag, 25. Januar 2010 21:54:16 schrieb Johan Hovold:

Quoted 5 lines

I don't think this patch should go to stable as it does not fix anything severe. Worst case is that in the unlikely event that usb_control_msg fails and you have debugging enabled one may end up with 64 non-sense characters in you log. There's no risk for any memory corruption AFAICT.

It does fix DMA on the stack. Are you sure this is always harmless on non-x86?

Regards
Oliver

Johan Hovold

2010-01-25 22:13 UTC #7 lore raw

In reply to Dan Carpenter

Quoted 9 lines · Oliver Neukum

Am Montag, 25. Januar 2010 21:54:16 schrieb Johan Hovold:
Quoted 5 lines
I don't think this patch should go to stable as it does not fix anything severe. Worst case is that in the unlikely event that usb_control_msg fails and you have debugging enabled one may end up with 64 non-sense characters in you log. There's no risk for any memory corruption AFAICT.
It does fix DMA on the stack. Are you sure this is always harmless on non-x86?

Good point. But if that is considered critical enough then Dan's original patch should go to stable instead of this one. And so should the other 10+ DMA-to-stack patches in Greg's tree?

/Johan

Greg KH

2010-01-26 04:27 UTC #8 lore raw

In reply to Dan Carpenter

Quoted 8 lines · Johan Hovold

Quoted 7 lines · Oliver Neukum
Am Montag, 25. Januar 2010 21:54:16 schrieb Johan Hovold:
Quoted 5 lines
I don't think this patch should go to stable as it does not fix anything severe. Worst case is that in the unlikely event that usb_control_msg fails and you have debugging enabled one may end up with 64 non-sense characters in you log. There's no risk for any memory corruption AFAICT.

Quoted 6 lines

Quoted 2 lines
It does fix DMA on the stack. Are you sure this is always harmless on non-x86?
Good point. But if that is considered critical enough then Dan's original patch should go to stable instead of this one. And so should the other 10+ DMA-to-stack patches in Greg's tree?

Given the age of this bug (1999) and the fact that the devices are no longer made, I don't think this is -stable worthy.

thanks,

greg k-h

Oliver Neukum

2010-01-26 06:50 UTC #9 lore raw

In reply to Dan Carpenter

Am Dienstag, 26. Januar 2010 05:27:31 schrieb Greg KH:

Quoted 6 lines

Quoted 3 lines
Good point. But if that is considered critical enough then Dan's original patch should go to stable instead of this one. And so should the other 10+ DMA-to-stack patches in Greg's tree?
Given the age of this bug (1999) and the fact that the devices are no longer made, I don't think this is -stable worthy.

That's an odd form of reasoning. As the patch touches just this driver it makes no difference if you don't have the hardware. If you have the hardware on an architecture where it matters, this is no longer true.

Regards
Oliver

Dan Carpenter

author

2010-01-26 09:15 UTC #10 lore raw

In reply to Dan Carpenter

Quoted 6 lines · Greg KH

Quoted 5 lines · Dan Carpenter
Johan Hovold points out that get_string() is basically just a re-implimentation of usb_string(). It is also buggy. It does DMA on the stack and it doesn't handle negative returns from usb_get_descriptor(). Plus unicode_to_ascii() is a rubbish function and moving to usb_string() avoids using it.

Quoted 8 lines

Quoted 5 lines
Let's eliminate get_string() entirely.
Reported-by Johan Hovold <[email protected]>
Signed-off-by Dan Carpenter <[email protected]>
Cc [email protected]
Why stable? Does this fix an existing bug somewhere?

No sorry. My script added that automatically and I meant forgot to remove it. This should go through -next I think.

I will be more careful in the future.

regards,
dan carpenter

Quoted 3 lines

thanks,
greg k-h

Dan Carpenter

author

2010-01-26 09:21 UTC #11 lore raw

In reply to Dan Carpenter

Quoted 8 lines · Oliver Neukum

Am Dienstag, 26. Januar 2010 05:27:31 schrieb Greg KH:
Quoted 6 lines
Quoted 3 lines
Good point. But if that is considered critical enough then Dan's original patch should go to stable instead of this one. And so should the other 10+ DMA-to-stack patches in Greg's tree?
Given the age of this bug (1999) and the fact that the devices are no longer made, I don't think this is -stable worthy.

Quoted 7 lines

That's an odd form of reasoning. As the patch touches just this driver it makes no difference if you don't have the hardware. If you have the hardware on an architecture where it matters, this is no longer true.
Regards
Oliver

There are 89 files that do dma on the stack in the -rc5 kernel. No one ever complains about it, so it can't be that serious.

regards,
dan carpenter

Johan Hovold

2010-01-26 09:46 UTC #12 lore raw

In reply to Dan Carpenter

Quoted 9 lines · Dan Carpenter

Quoted 8 lines · Oliver Neukum
Am Dienstag, 26. Januar 2010 05:27:31 schrieb Greg KH:
Quoted 6 lines
Quoted 3 lines
Good point. But if that is considered critical enough then Dan's original patch should go to stable instead of this one. And so should the other 10+ DMA-to-stack patches in Greg's tree?
Given the age of this bug (1999) and the fact that the devices are no longer made, I don't think this is -stable worthy.

Quoted 7 lines

Quoted 5 lines
That's an odd form of reasoning. As the patch touches just this driver it makes no difference if you don't have the hardware. If you have the hardware on an architecture where it matters, this is no longer true.
There are 89 files that do dma on the stack in the -rc5 kernel. No one ever complains about it, so it can't be that serious.

DMA on stack can actually cause memory corruption on cache-incoherent architectures as Oliver implies. However unlikely it may be, perhaps this should be given higher priority to fix?

Thanks,
Johan