Thank you for the review!
>> + unsigned slen = strlen(s);
>
> When you use slen below, you multiply it by 2. Might as well do that
> multiplication once, here.
I expect the compiler to be able to manage that level of common
subexpression elimination. So it will make no difference to the
generated code. Do you think it makes the code clearer?
Ah! On second thought, I *will* do that.
What I can do is rename the variable:
desclen = 2+2*strlen(s);
if (desclen > 254)
desclen = 254; /* Shouldn't happen, but clamp just in case */
and move the comparison with utfmax up. (I may also rename "utfmax".)
>> + if (slen > 126)
>> + slen = 126; /* Limit so 2 + 2*slen <= 255 */
>
> Use the min() macro.
Um... why? I prefer the asymmetrical form when there is an exceptional
condition. I find it easier to follow the "normal flow" when it's a subset
of the lines, rather than having to pick lines apart.
I agree it would be shorter to write:
unsigned slen = min(strlen(s), 126);
and don't really object, I'm just curious.
>> - 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.
Definitely an improvement, and I already did it on my own, actually!
(I just had to find the right header file and preprocessor name.
I also did it to the langids[] array.)
>> +
>> + 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.
I think it makes the "continue until we run out of utfmax" logic
much easier to follow, because it's all done in one place with one
pattern ("if (!utfmax--)").
By moving the descriptor header into here, I've lost the simplification
in this function (it's back to about where it was), but removed a
different sort of "don't output more than len" logic (the "switch(len)")
from rh_string().
In general "continue until you run out of A or you run out of B" loops
are a bit messy if you don't consume equal numbers of each per loop
iteration.
By cacheing the initial value of the "utf" pointer and returning
"(utf - utf0)", I also reoved t
>
>>
>> -/*
>> - * 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
