On Tue, 25 May 2010, Yann Cantin wrote:
> Hi,
>
> I've done my homework the best i can, so here it is.
>
> This patch add a serial number parameter to the g_file_storage
> module. There's validity checks against the string passed to comply
> with the specs.
>
> It's against a linux-next git clone.
>
> A remaining question about the serial number :
> The fallback serial may not be ok if DRIVER_VERSION isn't long enough, and it is
> in fact hardcoded.
Don't worry about this. It's not likely that DRIVER_VERSION will be
shorter than 6 characters. If you want to add a comment saying that
DRIVER_VERSION should be at least 6 characters long in order to
construct a valid serial number, go ahead.
> What if 2 devices are used, as they should have unique serial ?
That's not possible. The design of the gadget API doesn't permit more
than one gadget to be defined.
> Should it be randomized ?
No.
> I also note a kernel oops after inserting/removing the module several times (even
> with the vanilla code). Does it need to be investigated ?
Absolutely!
> @@ -274,7 +275,7 @@
>
> static char fsg_string_manufacturer[64];
> static const char fsg_string_product[] = DRIVER_DESC;
> -static char fsg_string_serial[13];
> +static char fsg_string_serial[25];
Why did you choose 24? The spec says only that the minimum length is
12 digits. Since this is an arbitrary choice, you should create a
#define'd symbol for it.
> @@ -3272,6 +3279,56 @@ static int __init check_parameters(struct fsg_dev *fsg)
> ERROR(fsg, "invalid buflen\n");
> return -ETOOSMALL;
> }
> +
> + /* Serial string handling.
> + * On a real device, fsg_string_serial[] would be loaded
> + * from permanent storage. */
> + memset(&fsg_string_serial, 0, sizeof fsg_string_serial);
The memset isn't necessary, since static storage locations are set to 0
when the driver is loaded.
> + if (mod_data.serial_parm) {
> + const char *ch;
> + unsigned len = 0;
> +
> + /* Sanity check :
> + * The CB[I] specification limits the serial string to
> + * 12 uppercase hexadecimal characters.
> + * BBB need *at least* 12 uppercase hexadecimal characters.
> + * (Usb sticks use 24 uppercase hexadecimal characters). */
Maybe some USB (not Usb!) sticks do, but I bet plenty of them don't.
> + len = strlen(mod_data.serial_parm);
> + if (len >= sizeof fsg_string_serial ||
> + (mod_data.transport_type == USB_PR_BULK && len < 12) ||
> + (mod_data.transport_type != USB_PR_BULK && len > 12)) {
> + WARNING(fsg,
> + "Invalid serial string length; "
> + "Failing back to default\n");
> + goto fill_serial;
> + }
> + len = 0;
> + for (ch = mod_data.serial_parm; *ch; ++ch) {
> + ++len;
Why do you count the characters like this? You already know the
correct value of len from the strlen() call above.
> + if ((*ch < '0' || *ch > '9') &&
> + (*ch < 'A' || *ch > 'F')) { /* not uppercase hex */
> + WARNING(fsg,
> + "Invalid serial string character: %c; "
> + "Failing back to default\n",
> + *ch);
> + goto fill_serial;
> + }
> + }
> + memcpy(fsg_string_serial, mod_data.serial_parm, len + 1);
> + } else {
> +fill_serial:
> + /* Serial number not specified or invalid, make our own.
> + * We just encode it from the driver version string,
> + * 12 characters long to comply both CB[I] and BBB spec. */
... comply with both ...
The rest looks okay.
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
