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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130113220545.GB4876@breakpoint.cc>
Date:	Sun, 13 Jan 2013 23:05:45 +0100
From:	Sebastian Andrzej Siewior <sebastian@...akpoint.cc>
To:	fangxiaozhi 00110321 <fangxiaozhi@...wei.com>
Cc:	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
	zihan@...wei.com, Lin.Lei@...wei.com, greg@...ah.com,
	neil.yi@...wei.com, wangyuhua@...wei.com, huqiao36@...wei.com,
	balbi@...com, mdharm-usb@...-eyed-alien.net
Subject: Re: [PATCH 2/2]linux-usb:optimize to match the Huawei USB storage
 devices and support new switch command

On Fri, Jan 11, 2013 at 05:57:44PM +0800, fangxiaozhi 00110321 wrote:
> diff -uprN linux-3.8-rc3_orig/drivers/usb/storage/initializers.c linux-3.8-rc3/drivers/usb/storage/initializers.c
> --- linux-3.8-rc3_orig/drivers/usb/storage/initializers.c	2013-01-11 17:53:19.757842845 +0800
> +++ linux-3.8-rc3/drivers/usb/storage/initializers.c	2013-01-11 17:55:04.137841849 +0800
> @@ -104,3 +104,71 @@ int usb_stor_huawei_e220_init(struct us_
>  	US_DEBUGP("Huawei mode set result is %d\n", result);
>  	return 0;
>  }
> +
> +/* It 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.
> + * So it is  unnecessary to read its response. */
This multiline comment is wrong. See Documentation/CodingStyle Chapter 8:
Commenting

> +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};
> +	
> +	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);
> +	memset(bcbw->CDB, 0, sizeof(bcbw->CDB));
> +	memcpy(bcbw->CDB, rewind_cmd, sizeof(rewind_cmd));

You asked me about this, and just replied.

> +	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;
> +}
> +
> +/* It tries 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.*/

multiline comment again.

> +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. */

Multiline comment + means fits in the first line, doesn't it?

> +	if (idesc && idesc->bInterfaceNumber == 0) {
if you do

	if (!idesc || idesc-> != 0)
		return 0;

then you lose one ident level.

> +		if ((idProduct == 0x1001)
> +			|| (idProduct == 0x1003)
> +			|| (idProduct == 0x1004)
> +			|| (idProduct >= 0x1401 && idProduct <= 0x1500)
> +			|| (idProduct >= 0x1505 && idProduct <= 0x1600)
> +			|| (idProduct >= 0x1c02 && idProduct <= 0x2202)) {
> +			return 1;

what about the switch case here?

> +		}
> +	}
> +	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;
> +}
> Binary files linux-3.8-rc3_orig/drivers/usb/storage/initializers.o and linux-3.8-rc3/drivers/usb/storage/initializers.o differ
^^^

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/