Skip to contentSkip to conversation

linux-usb:optimize to match the Huawei USB storage devices and support new switch command

PATCH In review fangxiaozhi 00110321 3 participants 3 messages Jan 5, 2013 – Jan 8, 2013

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 linux-usb:optimize to match the Huawei USB storage devices and support new switch command

From: fangxiaozhi <[email protected]>

1. Optimize the match rules with new macro for Huawei USB storage devices, to avoid to load USB storage driver for the modem interface with Huawei devices.
2. Add to support new switch command for new Huawei USB dongles.

--------------------------------------------------------------------

drivers/usb/storage/initializers.c +74 -2
diff -uprN linux-3.8-rc2_orig/drivers/usb/storage/initializers.c linux-3.8-rc2/drivers/usb/storage/initializers.c
@@ -92,8 +92,8 @@ int usb_stor_ucr61s2b_init(struct us_dat
 	return 0;
 }
 
-/* This places the HUAWEI E220 devices in multi-port mode */
-int usb_stor_huawei_e220_init(struct us_data *us)
+/* This places the HUAWEI usb dongles in multi-port mode */
+static int usb_stor_huawei_feature_init(struct us_data *us)
 {
 	int result;
 
@@ -104,3 +104,75 @@ int usb_stor_huawei_e220_init(struct us_
 	US_DEBUGP("Huawei mode set result is %d\n", result);
 	return 0;
 }
+
+/* This function will send
+ * a scsi switch command called rewind' to huawei dongle.
+ * When the dongle receives this command at the first time,
+ * it will reboot immediately,
+ * after rebooted, it will ignore this command and do nothing,
+ * if it receives this command again.
+ * So it is  unnecessary to read its response. */
+static int usb_stor_huawei_scsi_init(struct us_data *us)
+{
+	int result = 0;
+	int act_len = 0;
+	struct bulk_cb_wrap *bcbw = (struct bulk_cb_wrap *) us->iobuf;
+	char rewind_cmd[] = {0x11, 0x06, 0x20, 0x00, 0x00, 0x01, 0x01, 0x00,
+			0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
+	
+	memset(bcbw, 0, sizeof(struct bulk_cb_wrap));
+	bcbw->Signature = cpu_to_le32(US_BULK_CB_SIGN);
+	bcbw->Tag = 0;
+	bcbw->DataTransferLength = 0;
+	bcbw->Flags = bcbw->Lun = 0;
+	bcbw->Length = sizeof(rewind_cmd);
+	memcpy(bcbw->CDB, rewind_cmd, sizeof(rewind_cmd));
+
+	result = usb_stor_bulk_transfer_buf(us, us->send_bulk_pipe, bcbw,
+					US_BULK_CB_WRAP_LEN, &act_len);
+	US_DEBUGP("transfer actual length=%d, result=%d\n", act_len, result);
+	return result;
+}
+
+/* usb_stor_huawei_dongles_pid: try to find the supported Huawei USB dongles
+ * In Huawei, they assign the following product IDs
+ * for all of their mobile broadband dongles,
+ * including the new dongles in the future.
+ * So if the product ID is not included in this list,
+ * it means it is not Huawei's mobile broadband dongles.
+ */
+static int usb_stor_huawei_dongles_pid(struct us_data *us)
+{
+	struct usb_interface_descriptor *idesc;
+	int idProduct;
+	
+	idesc = &us->pusb_intf->cur_altsetting->desc;
+	idProduct = us->pusb_dev->descriptor.idProduct;
+	/* The first port is CDROM,
+	 * means the dongle in the single port mode,
+	 * and a switch command is required to be sent. */
+	if (idesc && idesc->bInterfaceNumber == 0) {
+		if ((idProduct == 0x1001)
+			|| (idProduct == 0x1003)
+			|| (idProduct == 0x1004)
+			|| (idProduct >= 0x1401 && idProduct < 0x1501)
+			|| (idProduct > 0x1504 && idProduct <= 0x1600)
+			|| (idProduct >= 0x1c02 && idProduct <= 0x2202)) {
+			return 1;
+		}
+	}
+	return 0;
+}
+
+int usb_stor_huawei_init(struct us_data *us)
+{
+	int result = 0;
+	
+	if (usb_stor_huawei_dongles_pid(us)) {
+		if (us->pusb_dev->descriptor.idProduct >= 0x1446)
+			result = usb_stor_huawei_scsi_init(us);
+		else
+			result = usb_stor_huawei_feature_init(us);
+	}
+	return result;
+}
drivers/usb/storage/initializers.h +2 -2
diff -uprN linux-3.8-rc2_orig/drivers/usb/storage/initializers.h linux-3.8-rc2/drivers/usb/storage/initializers.h
@@ -46,5 +46,5 @@ int usb_stor_euscsi_init(struct us_data 
  * flash reader */
 int usb_stor_ucr61s2b_init(struct us_data *us);
 
-/* This places the HUAWEI E220 devices in multi-port mode */
-int usb_stor_huawei_e220_init(struct us_data *us);
+/* This places the HUAWEI usb dongles in multi-port mode */
+int usb_stor_huawei_init(struct us_data *us);

Binary files linux-3.8-rc2_orig/drivers/usb/storage/initializers.o and linux-3.8-rc2/drivers/usb/storage/initializers.o differ

drivers/usb/storage/unusual_devs.h +2 -327

337 lines not on this page · raw message

#1 lore raw
Quoted 25 lines · On Sat, Jan 05, 2013 at 10:57:42AM +0800, fangxiaozhi 00110321 wrote:

From: fangxiaozhi <[email protected]>

1. Optimize the match rules with new macro for Huawei USB storage devices, to avoid to load USB storage driver for the modem interface with Huawei devices.
2. Add to support new switch command for new Huawei USB dongles.

--------------------------------------------------------------------
diff -uprN linux-3.8-rc2_orig/drivers/usb/storage/initializers.c linux-3.8-rc2/drivers/usb/storage/initializers.c

drivers/usb/storage/initializers.c +2 -2
@@ -92,8 +92,8 @@ int usb_stor_ucr61s2b_init(struct us_dat
 	return 0;
 }
 
-/* This places the HUAWEI E220 devices in multi-port mode */
-int usb_stor_huawei_e220_init(struct us_data *us)
+/* This places the HUAWEI usb dongles in multi-port mode */
+static int usb_stor_huawei_feature_init(struct us_data *us)
 {
 	int result;
 
@@ -104,3 +104,75 @@ int usb_stor_huawei_e220_init(struct us_
 	US_DEBUGP("Huawei mode set result is %d\n", result);

drivers/usb/storage/initializers.c int usb_stor_huawei_e220_init(struct us_ · line 114

Quoted 10 lines
drivers/usb/storage/initializers.c +8 -0
 	return 0;
 }
+
+/* This function will send
+ * a scsi switch command called rewind' to huawei dongle.
+ * When the dongle receives this command at the first time,
+ * it will reboot immediately,
+ * after rebooted, it will ignore this command and do nothing,
+ * if it receives this command again.
+ * So it is  unnecessary to read its response. */

This is not how a proper multi line comment looks like. The line break in the middle of the sentence does not look good IMHO.

Quoted 4 lines
drivers/usb/storage/initializers.c +4 -0
+static int usb_stor_huawei_scsi_init(struct us_data *us)
+{
+	int result = 0;
+	int act_len = 0;

drivers/usb/storage/initializers.c int usb_stor_huawei_e220_init(struct us_ · line 128

Quoted 10 lines
drivers/usb/storage/initializers.c +10 -0
+	struct bulk_cb_wrap *bcbw = (struct bulk_cb_wrap *) us->iobuf;
+	char rewind_cmd[] = {0x11, 0x06, 0x20, 0x00, 0x00, 0x01, 0x01, 0x00,
+			0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
+	
+	memset(bcbw, 0, sizeof(struct bulk_cb_wrap));
+	bcbw->Signature = cpu_to_le32(US_BULK_CB_SIGN);
+	bcbw->Tag = 0;
+	bcbw->DataTransferLength = 0;
+	bcbw->Flags = bcbw->Lun = 0;
+	bcbw->Length = sizeof(rewind_cmd);

I asked earlier and I ask again: why memset to zero followed by init to zero. Could we stick to one thing?

drivers/usb/storage/initializers.c int usb_stor_huawei_e220_init(struct us_ · line 132

Quoted 4 lines
drivers/usb/storage/initializers.c +4 -0
+	memcpy(bcbw->CDB, rewind_cmd, sizeof(rewind_cmd));
+
+	result = usb_stor_bulk_transfer_buf(us, us->send_bulk_pipe, bcbw,
+					US_BULK_CB_WRAP_LEN, &act_len);

This looks like it could work. Was it really tested before sending this time? :P

drivers/usb/storage/initializers.c int usb_stor_huawei_e220_init(struct us_ · line 143

Quoted 11 lines
drivers/usb/storage/initializers.c +11 -0
+	US_DEBUGP("transfer actual length=%d, result=%d\n", act_len, result);
+	return result;
+}
+
+/* usb_stor_huawei_dongles_pid: try to find the supported Huawei USB dongles
+ * In Huawei, they assign the following product IDs
+ * for all of their mobile broadband dongles,
+ * including the new dongles in the future.
+ * So if the product ID is not included in this list,
+ * it means it is not Huawei's mobile broadband dongles.
+ */

Not a proper multiple line comment. Kernel doc format is different btw. and is described in Documentation/kernel-doc-nano-HOWTO.txt

Quoted 6 lines
drivers/usb/storage/initializers.c +6 -0
+static int usb_stor_huawei_dongles_pid(struct us_data *us)
+{
+	struct usb_interface_descriptor *idesc;
+	int idProduct;
+	
+	idesc = &us->pusb_intf->cur_altsetting->desc;

drivers/usb/storage/initializers.c int usb_stor_huawei_e220_init(struct us_ · line 159

Quoted 10 lines
drivers/usb/storage/initializers.c +10 -0
+	idProduct = us->pusb_dev->descriptor.idProduct;
+	/* The first port is CDROM,
+	 * means the dongle in the single port mode,
+	 * and a switch command is required to be sent. */
+	if (idesc && idesc->bInterfaceNumber == 0) {
+		if ((idProduct == 0x1001)
+			|| (idProduct == 0x1003)
+			|| (idProduct == 0x1004)
+			|| (idProduct >= 0x1401 && idProduct < 0x1501)
+			|| (idProduct > 0x1504 && idProduct <= 0x1600)

why not >= 1505 and <= 1500 instead of the < and > operators? It would look better. Do you exclude them on purpose or by accident?
On a second look, why not do this instead:

	switch (idProduct)
	case 0x1001:
	case 0x1401 .. 0x1500
		return 1;
	default:
		return 0;

This reads way way beter.

Quoted 9 lines
drivers/usb/storage/initializers.c +9 -0
+			|| (idProduct >= 0x1c02 && idProduct <= 0x2202)) {
+			return 1;
+		}
+	}
+	return 0;
+}
+
+int usb_stor_huawei_init(struct us_data *us)
+{

drivers/usb/storage/initializers.c int usb_stor_huawei_e220_init(struct us_ · line 178

Quoted 10 lines
drivers/usb/storage/initializers.c +10 -0
+	int result = 0;
+	
+	if (usb_stor_huawei_dongles_pid(us)) {
+		if (us->pusb_dev->descriptor.idProduct >= 0x1446)
+			result = usb_stor_huawei_scsi_init(us);
+		else
+			result = usb_stor_huawei_feature_init(us);
+	}
+	return result;
+}

Sebastian

#2 lore raw
Quoted 9 lines · On Tuesday 08 January 2013 11:57:30 Sebastian Andrzej Siewior wrote:
Quoted 9 lines
drivers/usb/storage/initializers.c +9 -0
+static int usb_stor_huawei_scsi_init(struct us_data *us)
+{
+     int result = 0;
+     int act_len = 0;
+     struct bulk_cb_wrap *bcbw = (struct bulk_cb_wrap *) us->iobuf;
+     char rewind_cmd[] = {0x11, 0x06, 0x20, 0x00, 0x00, 0x01, 0x01, 0x00,
+                     0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
+     
+     memset(bcbw, 0, sizeof(struct bulk_cb_wrap));
Quoted 8 lines

drivers/usb/storage/initializers.c int usb_stor_huawei_e220_init(struct us_ · line 128

Quoted 5 lines
drivers/usb/storage/initializers.c +5 -0
+     bcbw->Signature = cpu_to_le32(US_BULK_CB_SIGN);
+     bcbw->Tag = 0;
+     bcbw->DataTransferLength = 0;
+     bcbw->Flags = bcbw->Lun = 0;
+     bcbw->Length = sizeof(rewind_cmd);

I asked earlier and I ask again: why memset to zero followed by init to zero. Could we stick to one thing?

We shouldn't. The compiler will do the right thing. This is for the human reader. You tell the reader that you want.

a) a clean slate to start with
b) you issue a command with certain parameters specified. That these parameters are zero is beside the point. The point is telling the reader which parameters are important here.

Regards
Oliver

PS: What about endianness of bcbw->Length?