On 22 Aug 2009, George Spelvin wrote:
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -337,72 +337,93 @@ static const u8 ss_rh_config_descriptor[] = {
>
> /*-------------------------------------------------------------------------*/
>
> -/*
> - * helper routine for returning string descriptors in UTF-16LE
> - * input can actually be ISO-8859-1; ASCII is its 7-bit subset
> +/**
> + * ascii2desc() - Helper routine for producing UTF-16LE string descriptors
> + * @s: Null-terminated ASCII (actually ISO-8859-1) string
> + * @utf: Buffer for USB string descritpro (header + UTF-16LE)
> + * @utfmax: Length (in bytes; may be odd) of utf buffer.
> + *
> + * The return value is the number of bytes filled in: 2 + 2*strlen(s) or
> + * utfmax, whichever is less.
> */
> -static unsigned ascii2utf(char *s, u8 *utf, int utfmax)
> +static unsigned
> +ascii2desc(char const *s, u8 *utf, unsigned utfmax)
> {
> - unsigned retval;
> + u8 * const utf0 = utf;
> + unsigned slen = strlen(s);
When you use slen below, you multiply it by 2. Might as well do that
multiplication once, here.
> +
> + if (slen > 126)
> + slen = 126; /* Limit so 2 + 2*slen <= 255 */
Use the min() macro.
>
> - for (retval = 0; *s && utfmax > 1; utfmax -= 2, retval += 2) {
> + if (!utfmax--)
> + goto end;
> + *utf++ = 2 + 2*slen; /* Descriptor total length */
> +
> + if (!utfmax--)
> + goto end;
> + *utf++ = 3; /* Descriptor type 3 (string) */
Although this was copied from the original code, you might as well
clean it up. USB_DT_STRING is more informative and doesn't need a
comment.
> +
> + if (utfmax > 2*slen)
> + utfmax = 2*slen;
> +
> + while (utfmax--) {
> *utf++ = *s++;
> + if (!utfmax--)
> + break;
> *utf++ = 0;
> }
> - if (utfmax > 0) {
> - *utf = *s;
> - ++retval;
> - }
> - return retval;
> +end:
> + return (unsigned)(utf - utf0);
> }
Is this really a cleanup? You have added a bunch of code without
making anything significantly better, apart from that one erroneous
test.
>
> -/*
> - * rh_string - provides manufacturer, product and serial strings for root hub
> - * @id: the string ID number (1: serial number, 2: product, 3: vendor)
> +/**
> + * rh_string() - provides string descriptors for root hub
> + * @id: the string ID number (0: langids, 1: serial #, 2: product, 3: vendor)
> * @hcd: the host controller for this root hub
> - * @data: return packet in UTF-16 LE
> - * @len: length of the return packet
> + * @data: buffer for output packet
> + * @len: length of the provided buffer
> *
> * Produces either a manufacturer, product or serial number string for the
> * virtual root hub device.
> + * Returns the number of bytes filled in: the length of the descriptor or
> + * of the provided buffer, whichever is less.
> */
> -static unsigned rh_string(int id, struct usb_hcd *hcd, u8 *data, unsigned len)
> +static unsigned
> +rh_string(int id, struct usb_hcd const *hcd, u8 *data, unsigned len)
> {
> - char buf [100];
> + char buf[100];
> + char const *s;
> + static char const langids[4] = {4, 3, 0x09, 0x04};
>
> // language ids
> - if (id == 0) {
> - buf[0] = 4; buf[1] = 3; /* 4 bytes string data */
> - buf[2] = 0x09; buf[3] = 0x04; /* MSFT-speak for "en-us" */
> - len = min_t(unsigned, len, 4);
> - memcpy (data, buf, len);
> + switch (id) {
> + case 0:
> + /* Array of LANGID codes (0x0409 is MSFT-speak for "en-us") */
> + /* See http://www.usb.org/developers/docs/USB_LANGIDs.pdf */
> + if (len > 4)
> + len = 4;
Use min().
> + memcpy(data, langids, len);
> return len;
> -
> - // serial number
> - } else if (id == 1) {
> - strlcpy (buf, hcd->self.bus_name, sizeof buf);
> -
> - // product description
> - } else if (id == 2) {
> - strlcpy (buf, hcd->product_desc, sizeof buf);
> -
> - // id 3 == vendor description
> - } else if (id == 3) {
> + case 1:
> + /* Serial number */
> + s = hcd->self.bus_name;
> + break;
> + case 2:
> + /* Product name */
> + s = hcd->product_desc;
> + break;
> + case 3:
> + /* Manufacturer */
> snprintf (buf, sizeof buf, "%s %s %s", init_utsname()->sysname,
> init_utsname()->release, hcd->driver->description);
> - }
> -
> - switch (len) { /* All cases fall through */
> + s = buf;
> + break;
> default:
> - len = 2 + ascii2utf (buf, data + 2, len - 2);
> - case 2:
> - data [1] = 3; /* type == string */
> - case 1:
> - data [0] = 2 * (strlen (buf) + 1);
> - case 0:
> - ; /* Compiler wants a statement here */
> + /* Can't happen; caller guarantees it */
> + return 0;
> }
> - return len;
> +
> + return ascii2desc(s, data, len);
> }
This part looks good.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
