linux-usb:Define a new macro for USB storage match rules

PATCH In review fangxiaozhi 00110321 3 participants 7 messages Dec 18, 2012 – Jan 4, 2013

Subsystems (2)

USB USB mass storage driver

lore

Share & tools

lore

fangxiaozhi 00110321

author

2012-12-18 02:42 UTC #0 lore raw

PATCH 1/2 linux-usb:Define a new macro for USB storage match rules

From: fangxiaozhi <[email protected]>

1. Define a new macro for USB storage match rules:
matching with Vendor ID and interface descriptors.

Signed-off-by fangxiaozhi <[email protected]>

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

drivers/usb/storage/usb.c +12 -0

diff -uprN linux-3.7_bak/drivers/usb/storage/usb.c linux-3.7/drivers/usb/storage/usb.c
@@ -120,6 +120,17 @@ MODULE_PARM_DESC(quirks, "supplemental l
 	.useTransport = use_transport,	\
 }
 
+#define UNUSUAL_VENDOR_INTF(idVendor, cl, sc, pr, \
+		    vendor_name, product_name, use_protocol, use_transport, \
+		    init_function, Flags) \
+{ \
+	.vendorName = vendor_name,	\
+	.productName = product_name,	\
+	.useProtocol = use_protocol,	\
+	.useTransport = use_transport,	\
+	.initFunction = init_function,	\
+}
+
 static struct us_unusual_dev us_unusual_dev_list[] = {
 #	include "unusual_devs.h"
 	{ }		/* Terminating entry */

@@ -131,6 +142,7 @@ static struct us_unusual_dev for_dynamic
 #undef UNUSUAL_DEV
 #undef COMPLIANT_DEV
 #undef USUAL_DEV
+#undef UNUSUAL_VENDOR_INTF
 
 #ifdef CONFIG_LOCKDEP

drivers/usb/storage/usual-tables.c +14 -0

diff -uprN linux-3.7_bak/drivers/usb/storage/usual-tables.c linux-3.7/drivers/usb/storage/usual-tables.c
@@ -41,6 +41,19 @@
 #define USUAL_DEV(useProto, useTrans) \
 { USB_INTERFACE_INFO(USB_CLASS_MASS_STORAGE, useProto, useTrans) }
 
+/* Define the device is matched with Vendor ID and interface descriptors */
+#define UNUSUAL_VENDOR_INTF(id_vendor, cl, sc, pr, \
+			vendorName, productName, useProtocol, useTransport, \
+			initFunction, flags) \
+{ \
+	.match_flags = USB_DEVICE_ID_MATCH_INT_INFO \
+				| USB_DEVICE_ID_MATCH_VENDOR, \
+	.idVendor    = (id_vendor), \
+	.bInterfaceClass = (cl), \
+	.bInterfaceSubClass = (sc), \
+	.bInterfaceProtocol = (pr), \
+	.driver_info = (flags) }
+
 struct usb_device_id usb_storage_usb_ids[] = {
 #	include "unusual_devs.h"
 	{ }		/* Terminating entry */

@@ -50,6 +63,7 @@ MODULE_DEVICE_TABLE(usb, usb_storage_usb
 #undef UNUSUAL_DEV
 #undef COMPLIANT_DEV
 #undef USUAL_DEV
+#undef UNUSUAL_VENDOR_INTF
 
 /*
  * The table of devices to ignore

fangxiaozhi 00110321

author

2012-12-18 02:44 UTC #1 lore raw

PATCH 2/2 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.

Signed-off-by fangxiaozhi <[email protected]>

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

drivers/usb/storage/initializers.c +58 -2

diff -uprN linux-3.7_bak/drivers/usb/storage/initializers.c linux-3.7/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,59 @@ int usb_stor_huawei_e220_init(struct us_
 	US_DEBUGP("Huawei mode set result is %d\n", result);
 	return 0;
 }
+
+/* Find the supported Huawei USB 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;
+	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;
+}
+
+static int usb_stor_huawei_scsi_init(struct us_data *us)
+{
+	int result = 0;
+	int act_len = 0;
+	char rewind_cmd[] = {0x11, 0x06, 0x20, 0x00, 0x00, 0x01, 0x01, 0x00,
+			0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
+	struct bulk_cb_wrap *bcbw = (struct bulk_cb_wrap *) us->iobuf;
+	
+	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_CS_WRAP_LEN, &act_len);
+	US_DEBUGP("transfer actual length=%d, result=%d\n", act_len, result);
+	return result;
+}
+
+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.7_bak/drivers/usb/storage/initializers.h linux-3.7/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.7_bak/drivers/usb/storage/initializers.o and linux-3.7/drivers/usb/storage/initializers.o differ

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

337 lines not on this page · raw message

Sebastian Andrzej Siewior

2012-12-18 14:10 UTC #2 lore raw

Re: [PATCH 2/2]linux-usb:optimize to match the Huawei USB storage devices and support new switch command

Quoted 24 lines · On Tue, Dec 18, 2012 at 10:44:19AM +0800, fangxiaozhi 00110321 wrote:

diff -uprN linux-3.7_bak/drivers/usb/storage/initializers.c linux-3.7/drivers/usb/storage/initializers.c

drivers/usb/storage/initializers.c +18 -0

 	US_DEBUGP("Huawei mode set result is %d\n", result);
 	return 0;
 }
+
+/* Find the supported Huawei USB 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;
+	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;
+		}

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

Quoted 10 lines

drivers/usb/storage/initializers.c +10 -0

+	}
+	return 0;
+}
+
+static int usb_stor_huawei_scsi_init(struct us_data *us)
+{
+	int result = 0;
+	int act_len = 0;
+	char rewind_cmd[] = {0x11, 0x06, 0x20, 0x00, 0x00, 0x01, 0x01, 0x00,
+			0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};

Has this something to do with the SPACE command as defined in SSC-2? I don't see the code (0x6 here) to be defined. But then you name is rewind.

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

Quoted 7 lines

drivers/usb/storage/initializers.c +7 -0

+	struct bulk_cb_wrap *bcbw = (struct bulk_cb_wrap *) us->iobuf;
+	
+	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;

A memset() followed by an init of each member of the struct. Could please chose one side?

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

Quoted 5 lines

drivers/usb/storage/initializers.c +5 -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_CS_WRAP_LEN, &act_len);

I am a little confused here. Shouldn't this be bcbw aka us->iobuf and not &bcbw ?

And shouldn't you read something from the us->recv_bulk_pipe after that?

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

Quoted 4 lines

drivers/usb/storage/initializers.c +4 -0

+	US_DEBUGP("transfer actual length=%d, result=%d\n", act_len, result);
+	return result;
+}
+

Sebastian

Fangxiaozhi (Franko)

author

2012-12-19 03:14 UTC #3 lore raw

RE: [PATCH 2/2]linux-usb:optimize to match the Huawei USB storage devices and support new switch command

Dear Sebastian:
Please see the comments follows yours.

By the way, I found the kernel is updated to 3.7.1 today. So I have to update my patch based on 3.7.1, and resubmit it? Right?

Best Regards,
Franko Fang

Quoted 41 lines

-----Original Message-----
From: Sebastian Andrzej Siewior [mailto:[email protected]]
Sent: Tuesday, December 18, 2012 10:10 PM
To: Fangxiaozhi (Franko)
Cc: [email protected]; [email protected]; Xueguiying (Zihan); Linlei (Lei Lin); [email protected]; Yili (Neil); Wangyuhua (Roger, Credit); Huqiao; [email protected]
Subject: Re: [PATCH 2/2]linux-usb:optimize to match the Huawei USB storage devices and support new switch command
Quoted 1 line · On Tue, Dec 18, 2012 at 10:44:19AM +0800, fangxiaozhi 00110321 wrote:
diff -uprN linux-3.7_bak/drivers/usb/storage/initializers.c
linux-3.7/drivers/usb/storage/initializers.c
Quoted 1 line
--- linux-3.7_bak/drivers/usb/storage/initializers.c 2012-12-11
09:56:11.000000000 +0800
Quoted 1 line
+++ linux-3.7/drivers/usb/storage/initializers.c 2012-12-17
11:12:12.000000000 +0800
Quoted 24 lines
drivers/usb/storage/initializers.c +21 -0
 	US_DEBUGP("Huawei mode set result is %d\n", result);
 	return 0;
 }
+
+/* Find the supported Huawei USB 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;
+	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;
+}
+

Quoted 8 lines

drivers/usb/storage/initializers.c int usb_stor_huawei_e220_init(struct us_ · line 134
Quoted 5 lines
drivers/usb/storage/initializers.c +5 -0
+static int usb_stor_huawei_scsi_init(struct us_data *us) {
+	int result = 0;
+	int act_len = 0;
+	char rewind_cmd[] = {0x11, 0x06, 0x20, 0x00, 0x00, 0x01, 0x01, 0x00,
+			0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
Has this something to do with the SPACE command as defined in SSC-2? I don't see the code (0x6 here) to be defined. But then you name is rewind.

Yes, we redefine the SPACE based on our need, and named it "rewind"

Quoted 12 lines

drivers/usb/storage/initializers.c int usb_stor_huawei_e220_init(struct us_ · line 141
Quoted 8 lines
drivers/usb/storage/initializers.c +7 -0
+	struct bulk_cb_wrap *bcbw = (struct bulk_cb_wrap *) us->iobuf;
+
+	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;
A memset() followed by an init of each member of the struct. Could please chose one side?

Quoted 8 lines

drivers/usb/storage/initializers.c int usb_stor_huawei_e220_init(struct us_ · line 146
Quoted 5 lines
drivers/usb/storage/initializers.c +5 -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_CS_WRAP_LEN, &act_len);
I am a little confused here. Shouldn't this be bcbw aka us->iobuf and not &bcbw ?

Yes, you are right.

Quoted 3 lines

And shouldn't you read something from the us->recv_bulk_pipe after that?

Well, because our device will re-connect to switch the ports if it receives the command. So it is not necessary to read the response of the command.

Quoted 8 lines

Review
Quoted 2 lines
(quoted diff) +1 -0
+	US_DEBUGP("transfer actual length=%d, result=%d\n", act_len,
result);
drivers/usb/storage/initializers.c int usb_stor_huawei_e220_init(struct us_ · line 150
Quoted 3 lines
drivers/usb/storage/initializers.c +3 -0
+	return result;
+}
+
Sebastian

��{.n�+��+%��ݶ��w��{.n�+��{��G��{ay�ʇڙ�,j��f��h��z_�(�階�ݢj"��m��G��?��&��~��iO��z��v�^�m��?�I�

Sebastian Andrzej Siewior

2012-12-19 08:34 UTC #4 lore raw

Re: [PATCH 2/2]linux-usb:optimize to match the Huawei USB storage devices and support new switch command

Quoted 2 lines · Fangxiaozhi (Franko)

By the way, I found the kernel is updated to 3.7.1 today. So I have to update my patch based on 3.7.1, and resubmit it? Right?

You should rebase your patch on top of Greg's usb-next branch of his usb tree.
http://git.kernel.org/?p=linux/kernel/git/gregkh/usb.git;a=shortlog;h=refs/heads/usb-next

but I guess that there are hardly any changes in that area. The last change in drivers/usb/storage/initializers.* is yours "USB: usb-storage fails to attach to Huawei Datacard cdrom device".

If you call ./scripts/get_maintainer.pl on your patch you should learn that you miss
|Matthew Dharm <[email protected]> |[email protected]

Quoted 4 lines

Quoted 2 lines
And shouldn't you read something from the us->recv_bulk_pipe after that?
Well, because our device will re-connect to switch the ports if it receives the command. So it is not necessary to read the response of the command.

Hmm. I guess this for Matthew / Greg to decide, I don't insist on anything. Maybe a comment would be nice because now it looks, atleast to me, that something is missing.

Sebastian

Matthew Dharm

2012-12-19 15:41 UTC #5 lore raw

Re: [PATCH 2/2]linux-usb:optimize to match the Huawei USB storage devices and support new switch command

Quoted 9 lines · Sebastian Andrzej Siewior

Quoted 4 lines · Fangxiaozhi (Franko)
Quoted 2 lines
And shouldn't you read something from the us->recv_bulk_pipe after that?
Well, because our device will re-connect to switch the ports if it receives the command. So it is not necessary to read the response of the command.
Hmm. I guess this for Matthew / Greg to decide, I don't insist on anything. Maybe a comment would be nice because now it looks, atleast to me, that something is missing.

I think an unusual situation like that deserves a comment that explains that the device is about to disconnect / reconnect, so reading status is not necessary.

I am also concerned about the error of using &bcbw instead of bcbw. I doubt this code would have worked with that typo in place. How was this patch tested?

Also, the dongles_pid function is really just a different implementation of the unusual_devs.h table. I think that it is much easier for people to add new entries to the table, rather than edit your code, when new dongles are released. BUT, your code includes many more PIDs than the table did. Again, how was this tested for the new PIDs covered? At a minimum, some comment in dongles_pid is required to highlight this area of code for possible future expansion as new devices are released.

Matt

Signature · 2 lines

Matthew Dharm
Maintainer, USB Mass Storage driver for Linux

Fangxiaozhi (Franko)

author

2013-01-04 07:31 UTC #6 lore raw

RE: [PATCH 2/2]linux-usb:optimize to match the Huawei USB storage devices and support new switch command

Dear Matthew:

Quoted 17 lines

-----Original Message-----
From: Matthew Dharm [mailto:[email protected]]
Sent: Wednesday, December 19, 2012 11:41 PM
To: Sebastian Andrzej Siewior
Cc: Fangxiaozhi (Franko); [email protected]; [email protected]; Xueguiying (Zihan); Linlei (Lei Lin); [email protected]; Yili (Neil); Wangyuhua (Roger, Credit); Huqiao; [email protected]
Subject: Re: [PATCH 2/2]linux-usb:optimize to match the Huawei USB storage devices and support new switch command
Quoted 4 lines · Sebastian Andrzej Siewior
Quoted 3 lines · Fangxiaozhi (Franko)
Quoted 2 lines
And shouldn't you read something from the us->recv_bulk_pipe after that?
Well, because our device will re-connect to switch the ports if it
receives the command.

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
So it is not necessary to read the response of the command.
Hmm. I guess this for Matthew / Greg to decide, I don't insist on anything. Maybe a comment would be nice because now it looks, atleast to me, that something is missing.
I think an unusual situation like that deserves a comment that explains that the device is about to disconnect / reconnect, so reading status is not necessary.

You mean that we have to add some comment in the source code,
to explain why we don't read the response. Right?

Quoted 10 lines

I am also concerned about the error of using &bcbw instead of bcbw. I doubt this code would have worked with that typo in place. How was this patch tested?
Also, the dongles_pid function is really just a different implementation of the unusual_devs.h table. I think that it is much easier for people to add new entries to the table, rather than edit your code, when new dongles are released. BUT, your code includes many more PIDs than the table did. Again, how was this tested for the new PIDs covered?

In the dongles_pid function, we have check all the product IDs for our dongles, which is assigned for all of our Mobile Broadband products in our company. So the product ID of our new dongle in future, will also be included in this list. In our lab, we can configure our dongle firmware to support all of these product ID. We have test them(cover all the product ID), and this function works fine.

Quoted 3 lines

At a minimum, some comment in
dongles_pid is required to highlight this area of code for possible future expansion as new devices are released.

As far as I know, the product ID list in dongles_pid function includes all. We will not add any other product ID for our dongle. So we need not update the product ID list in dongles_pid function in future. However, I also will add the comment to highlight the area of code, as your advice did.

Quoted 6 lines

Matt
--
Matthew Dharm
Maintainer, USB Mass Storage driver for Linux

Best Regards,
Franko Fang ��{.n�+��+%��ݶ��w��{.n�+��{��G��{ay�ʇڙ�,j��f��h��z_�(�階�ݢj"��m��G��?��&��~��iO��z��v�^�m��?�I�