On Mon, 24 May 2010 02:36:24 +0200, Yann Cantin <yann.cantin@xxxxxxxxxxx> wrote:
Hi
Add a parameter to file_storage usb gadget module to be able to specify
the serial number of a device (for example, the serial number of an
emulated usb stick).
Things i'm not sure i've done well about :
- 12 chars limitation for CBI transport : I need more (24 chars) for sticks serial, so i've
modified the static serial declaration, and cut this string (\0 in serial[12]) if CBI is selected.
- i've keep the serial static, but now that it can be modified, perhaps it should be integrated
in the mod_data struct.
- DRIVER_SERIAL_NUM_NOT_SET : i need a way to know if the parameter have been passed, only find that.
- hexadecimal characters check : is there a better way ?
- The string copy handling between char array (serial) and char pointer (serial_parm). I'm not sure
it's rock-solid.
- I keep the dead (moved) code enclosed in /* */
Be smart, it's my first day :)
Is it just my mail client or aren't there any tabs in the patch? There should.
If you are using git you may find 'git format-patch' and 'git send-email' useful.
@@ -288,6 +290,8 @@ MODULE_LICENSE("Dual BSD/GPL");
#define DRIVER_VENDOR_ID 0x0525 // NetChip
#define DRIVER_PRODUCT_ID 0xa4a5 // Linux-USB File-backed Storage Gadget
+/* Serial number test string. */
+#define DRIVER_SERIAL_NUM_NOT_SET "Not set"
I don't see the need for this macro. Read further.
@@ -356,6 +360,7 @@ static struct {
char *transport_parm;
char *protocol_parm;
+ char *serial_parm;
unsigned short vendor;
unsigned short product;
unsigned short release;
@@ -369,6 +374,7 @@ static struct {
} mod_data = { // Default values
.transport_parm = "BBB",
.protocol_parm = "SCSI",
+ .serial_parm = DRIVER_SERIAL_NUM_NOT_SET,
Leave it as
+ .serial_parm = NULL,
@@ -996,8 +1005,10 @@ ep_desc(struct usb_gadget *g, struct usb
/* The CBI specification limits the serial string to 12 uppercase hexadecimal
* characters. */
+/* Others can handle more : Typically 12 uppercase hexadecimal *bytes* for
+ * usb sticks, ie 24 characters. */
Merge the two comments?
static char manufacturer[64];
-static char serial[13];
+static char serial[25];
/* Static strings, in UTF-8 (for simplicity we use only ASCII characters) */
static struct usb_string strings[] = {
@@ -3853,11 +3864,11 @@ static void /* __init_or_exit */ fsg_unb
set_gadget_data(gadget, NULL);
}
-
It's just me but I'd avoid removing newlines in patches. This just makes
the patch needlessly large.
static int __init check_parameters(struct fsg_dev *fsg)
{
int prot;
int gcnum;
+ int i;
/* Store the default values */
mod_data.transport_type = USB_PR_BULK;
@@ -3937,6 +3948,41 @@ static int __init check_parameters(struc
ERROR(fsg, "invalid buflen\n");
return -ETOOSMALL;
}
+
+ /* Serial string handling */
+ memset(&serial, 0, sizeof(serial));
+ if (strncmp(mod_data.serial_parm, DRIVER_SERIAL_NUM_NOT_SET, 10) == 0) {
+ /* Serial number not specified, make our own. */
+ /* On a real device, serial[] would be loaded from permanent
+ * storage. We just encode it from the driver version string.
+ */
+ for (i = 0; i < sizeof(serial) - 2; i += 2) {
+ unsigned char c = DRIVER_VERSION[i / 2];
+
+ if (!c)
+ break;
+ sprintf(&serial[i], "%02X", c);
+ }
+ } else {
+ int slength = min(sizeof(serial), strlen(mod_data.serial_parm));
+ for (i = 0; i < slength; i += 1) {
+ unsigned char c = mod_data.serial_parm[i];
+ if (!((c >= '0' && c <= '9') ||
+ (c >= 'A' && c <= 'F'))) {
+ ERROR(fsg, "invalid serial string: %s\n", mod_data.serial_parm);
+ return -EINVAL;
+ }
+ sprintf(&serial[i], "%c", c);
+ }
+ }
+
+ /* The CBI specification limits the serial string to 12 uppercase
+ * hexadecimal characters. */
+ if (mod_data.transport_type == USB_PR_CBI && strlen(serial) > 12) {
+ WARNING(fsg, "Cutting the serial string to 12 characters (See CBI spec).\n");
+ serial[12] = '\0';
+ }
+
#endif /* CONFIG_USB_FILE_STORAGE_TEST */
return 0;
How about something along the lines of:
if (mod_data.serial_parm) {
const char *ch;
unsigned len = 0;
for (ch = mod_data.serial_parm; *ch; ++ch)
if (++len == sizeof(serial) ||
((*ch < '0' || *ch > '9') && (*ch < 'A' || *ch > 'F'))) {
WARNING(fsg, "invalid serial string: %s; failing back to default\n", mod_data.serial_parm);
goto fill_serial;
}
memcpy(serial, mod_data.serial_parm, len + 1);
} else {
fill_serial:
int i;
/* Serial number not specified, make our own. */
/* On a real device, serial[] would be loaded from permanent
* storage. We just encode it from the driver version string.
*/
for (i = 0; i < 12; i += 2) {
unsigned char c = DRIVER_VERSION[i / 2];
if (!c)
break;
sprintf(&serial[i], "%02X", c);
}
}
Not tested.
Note that you do not need DRIVER_SERIAL_NUM_NOT_SET. Just checking if
mod_data.serial_parm is NULL is enough.
@@ -4110,8 +4156,10 @@ static int __init fsg_bind(struct usb_ga
init_utsname()->sysname, init_utsname()->release,
gadget->name);
+ /* Serial string handling moved to __init check_parameters */
/* On a real device, serial[] would be loaded from permanent
* storage. We just encode it from the driver version string. */
+ /*
for (i = 0; i < sizeof(serial) - 2; i += 2) {
unsigned char c = DRIVER_VERSION[i / 2];
@@ -4119,6 +4167,7 @@ static int __init fsg_bind(struct usb_ga
break;
sprintf(&serial[i], "%02X", c);
}
+ */
Remove the code all together. No need to keep it.
Or move the new serial initialisation code here instead of the
other place.
--
Best regards, _ _
| Humble Liege of Serenely Enlightened Majesty of o' \,=./ `o
| Computer Science, Michał "mina86" Nazarewicz (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--
--
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
