Re: v3.8 regression: Huawei mode switching fails (was Re: [PATCH 2/2]linux-usb:optimize to match the Huawei USB storage devices and support new switch command)
On Tue, Mar 05, 2013 at 01:23:33PM +0100, Bjørn Mork wrote: > As you are well aware of, we parse descriptors as needed on Linux and > can just as easy support the Windows look'n'feel if required. There is > absolutely no need to create special Linux descriptors to adapt to > existing drivers. We'll rather fix the drivers than using a mode which > isn't tested by the Windows users. I totally agree with this, and this is the way the rest of Linux works. There should not be any difference in the hardware descriptors between operating systems. If Linux isn't working properly for some hardware, we will fix it, do not work around Linux issues (perceived or real) in hardware. Bjørn, thanks for catching this kernel change, I'll revert it and push the changes to the stable kernel releases. It was my fault to not notice this bug, my apologies. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: v3.8 regression: Huawei mode switching fails (was Re: [PATCH 2/2]linux-usb:optimize to match the Huawei USB storage devices and support new switch command)
Dear Oliver:
> -Original Message-
> From: Oliver Neukum [mailto:oneu...@suse.de]
> Sent: Tuesday, March 05, 2013 4:49 PM
> To: Fangxiaozhi (Franko)
> Cc: Bjørn Mork; linux-...@vger.kernel.org; linux-kernel@vger.kernel.org;
> Xueguiying (Zihan); Linlei (Lei Lin); g...@kroah.com; Yili (Neil); Wangyuhua
> (Roger, Credit); Huqiao (C); ba...@ti.com; mdharm-...@one-eyed-alien.net;
> sebast...@breakpoint.cc
> Subject: Re: v3.8 regression: Huawei mode switching fails (was Re: [PATCH
> 2/2]linux-usb:optimize to match the Huawei USB storage devices and support
> new switch command)
>
> On Tuesday 05 March 2013 02:28:07 Fangxiaozhi wrote:
>
> Hi,
>
> > 1. As far as I know, usb_modeswitch is now only integrated in the PC
> > Linux.
> It isn't integrated to other system with linux kernel, such as Android, Chrome
> OS, etc. On these system, how can we switch the device?
>
> Then those systems will have to add modeswitch.
> If we are to do mode switching in kernel space, there has to be an additional
> technical benefit over the existing and well working solution in user space.
-The vendor doesn't want to add the modeswitch on their system. We try to
push them to do it, but they don't agree.
>
> > 2. usb_modeswitch often sends the Windows switching command to
> switch Huawei device, but not sends Linux switching command. This maybe
> causes some unexpected problem.
>
> You have two commands? What is the difference? Anyway, if usb_modeswitch
> has issues, these problems need to be fixed.
--Sorry, it is one command, but with different values in it for different
system, such as:
1. For Windows:
{0x11, 0x06, 0x00, 0x00, 0x00, 0x01, 0x01, 0x00,
- 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
2. For Mac OS:
{0x11, 0x06, 0x10, 0x00, 0x00, 0x01, 0x01, 0x00,
- 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
3. For Linux:
{0x11, 0x06, 0x20, 0x00, 0x00, 0x01, 0x01, 0x00,
- 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
Sometimes, modeswitch will send the command for Windows to device on
Linux system, but not the command for Linux.
>
> Independent from that, there is a problem with user space mode switching.
> Namely it doesn't work with reset_resume(). Thus we cannot do sane S4 with
> the user space solution.
> However, the problem cannot be solved by merely by adding the switch to the
> storage driver.
-For this issue, can you offer more details? And we can try to solve it
together.
>
> Regards
> Oliver
Best Regards,
Franko Fang
Re: v3.8 regression: Huawei mode switching fails (was Re: [PATCH 2/2]linux-usb:optimize to match the Huawei USB storage devices and support new switch command)
On Tuesday 05 March 2013 02:28:07 Fangxiaozhi wrote: Hi, > 1. As far as I know, usb_modeswitch is now only integrated in the PC > Linux. It isn't integrated to other system with linux kernel, such as > Android, Chrome OS, etc. On these system, how can we switch the device? Then those systems will have to add modeswitch. If we are to do mode switching in kernel space, there has to be an additional technical benefit over the existing and well working solution in user space. > 2. usb_modeswitch often sends the Windows switching command to switch > Huawei device, but not sends Linux switching command. This maybe causes some > unexpected problem. You have two commands? What is the difference? Anyway, if usb_modeswitch has issues, these problems need to be fixed. Independent from that, there is a problem with user space mode switching. Namely it doesn't work with reset_resume(). Thus we cannot do sane S4 with the user space solution. However, the problem cannot be solved by merely by adding the switch to the storage driver. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: v3.8 regression: Huawei mode switching fails (was Re: [PATCH 2/2]linux-usb:optimize to match the Huawei USB storage devices and support new switch command)
On Tuesday 05 March 2013 02:28:07 Fangxiaozhi wrote: Hi, 1. As far as I know, usb_modeswitch is now only integrated in the PC Linux. It isn't integrated to other system with linux kernel, such as Android, Chrome OS, etc. On these system, how can we switch the device? Then those systems will have to add modeswitch. If we are to do mode switching in kernel space, there has to be an additional technical benefit over the existing and well working solution in user space. 2. usb_modeswitch often sends the Windows switching command to switch Huawei device, but not sends Linux switching command. This maybe causes some unexpected problem. You have two commands? What is the difference? Anyway, if usb_modeswitch has issues, these problems need to be fixed. Independent from that, there is a problem with user space mode switching. Namely it doesn't work with reset_resume(). Thus we cannot do sane S4 with the user space solution. However, the problem cannot be solved by merely by adding the switch to the storage driver. Regards Oliver -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: v3.8 regression: Huawei mode switching fails (was Re: [PATCH 2/2]linux-usb:optimize to match the Huawei USB storage devices and support new switch command)
Dear Oliver:
-Original Message-
From: Oliver Neukum [mailto:oneu...@suse.de]
Sent: Tuesday, March 05, 2013 4:49 PM
To: Fangxiaozhi (Franko)
Cc: Bjørn Mork; linux-...@vger.kernel.org; linux-kernel@vger.kernel.org;
Xueguiying (Zihan); Linlei (Lei Lin); g...@kroah.com; Yili (Neil); Wangyuhua
(Roger, Credit); Huqiao (C); ba...@ti.com; mdharm-...@one-eyed-alien.net;
sebast...@breakpoint.cc
Subject: Re: v3.8 regression: Huawei mode switching fails (was Re: [PATCH
2/2]linux-usb:optimize to match the Huawei USB storage devices and support
new switch command)
On Tuesday 05 March 2013 02:28:07 Fangxiaozhi wrote:
Hi,
1. As far as I know, usb_modeswitch is now only integrated in the PC
Linux.
It isn't integrated to other system with linux kernel, such as Android, Chrome
OS, etc. On these system, how can we switch the device?
Then those systems will have to add modeswitch.
If we are to do mode switching in kernel space, there has to be an additional
technical benefit over the existing and well working solution in user space.
-The vendor doesn't want to add the modeswitch on their system. We try to
push them to do it, but they don't agree.
2. usb_modeswitch often sends the Windows switching command to
switch Huawei device, but not sends Linux switching command. This maybe
causes some unexpected problem.
You have two commands? What is the difference? Anyway, if usb_modeswitch
has issues, these problems need to be fixed.
--Sorry, it is one command, but with different values in it for different
system, such as:
1. For Windows:
{0x11, 0x06, 0x00, 0x00, 0x00, 0x01, 0x01, 0x00,
- 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
2. For Mac OS:
{0x11, 0x06, 0x10, 0x00, 0x00, 0x01, 0x01, 0x00,
- 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
3. For Linux:
{0x11, 0x06, 0x20, 0x00, 0x00, 0x01, 0x01, 0x00,
- 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
Sometimes, modeswitch will send the command for Windows to device on
Linux system, but not the command for Linux.
Independent from that, there is a problem with user space mode switching.
Namely it doesn't work with reset_resume(). Thus we cannot do sane S4 with
the user space solution.
However, the problem cannot be solved by merely by adding the switch to the
storage driver.
-For this issue, can you offer more details? And we can try to solve it
together.
Regards
Oliver
Best Regards,
Franko Fang
Re: v3.8 regression: Huawei mode switching fails (was Re: [PATCH 2/2]linux-usb:optimize to match the Huawei USB storage devices and support new switch command)
On Tue, Mar 05, 2013 at 01:23:33PM +0100, Bjørn Mork wrote: As you are well aware of, we parse descriptors as needed on Linux and can just as easy support the Windows look'n'feel if required. There is absolutely no need to create special Linux descriptors to adapt to existing drivers. We'll rather fix the drivers than using a mode which isn't tested by the Windows users. I totally agree with this, and this is the way the rest of Linux works. There should not be any difference in the hardware descriptors between operating systems. If Linux isn't working properly for some hardware, we will fix it, do not work around Linux issues (perceived or real) in hardware. Bjørn, thanks for catching this kernel change, I'll revert it and push the changes to the stable kernel releases. It was my fault to not notice this bug, my apologies. greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: v3.8 regression: Huawei mode switching fails (was Re: [PATCH 2/2]linux-usb:optimize to match the Huawei USB storage devices and support new switch command)
Dear Mork:
Thank you very much for your test.
> -Original Message-
> From: Bjørn Mork [mailto:bj...@mork.no]
> Sent: Monday, March 04, 2013 7:41 PM
> To: Fangxiaozhi (Franko)
> Cc: linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; Xueguiying
> (Zihan); Linlei (Lei Lin); g...@kroah.com; Yili (Neil); Wangyuhua (Roger,
> Credit);
> Huqiao (C); ba...@ti.com; mdharm-...@one-eyed-alien.net;
> sebast...@breakpoint.cc
> Subject: v3.8 regression: Huawei mode switching fails (was Re: [PATCH
> 2/2]linux-usb:optimize to match the Huawei USB storage devices and support
> new switch command)
>
> Hello Franko,
>
> This patch causes a number of regressions for both the Huawei devices I have
> available for testing. One of them is completely unusable in v3.8 (unable to
> switch to modem mode) unless the usb-storage driver is disabled.
--Which device, can you tell me more information?
--Which model? And what's its firmware version?
>
> I realize that some devices are historically handled by the usb-storage
> driver,
> but userspace modeswitching is the rule for new devices AFAIK.
> I see absolutely no valid argument for adding new devices. And there is
> absolutely no excuse for adding devices which have been handled by
> userspace switching for years! The patch does not just "optimize"
> matching. It adds a large number of devices which were previously handled
> just fine by usb_modeswitch. This is guaranteed to confuse users with existing
> configurations, and users looking up any of the existing documentation
> pointing to usb_modeswitch.
--Yes. But there is some problems for this:
1. As far as I know, usb_modeswitch is now only integrated in the PC
Linux. It isn't integrated to other system with linux kernel, such as Android,
Chrome OS, etc. On these system, how can we switch the device?
2. usb_modeswitch often sends the Windows switching command to switch
Huawei device, but not sends Linux switching command. This maybe causes some
unexpected problem.
>
> There is no need to repeat the discussion of kernel vs userspace switching. I
> will only note that userspace switching:
> - has been selected as the rule for new devices,
> - allow the user to temporarily disable switching e.g. for mounting and
>inspecting the usb-storage exported data,
> - allow the user/system to apply device specific switching quirks
>
>
> The v3.7->v3.8 regressions I observe are:
>
> 1) Temporarily disabling mode switching and mounting the CD image is now
> impossible. Mode switching can only be disabled by blacklisting
> usb-storage, which of course prevents usb-storage from working
>
> Prior to v3.8, modeswitching could easily be disabled by userspace
> configuration. The change breaks existing configurations.
>
> 2) Switching to non-default modes is now impossible. The E392 (initially
> appearing as 12d1:1505) Windows drivers will use a different command than
> the one used by Linux, causing the device to select a different configuration
> in Linux and Windows. This forces Linux and Windows to see the device
> differently.
>
> Prior to v3.8, different modeswitching commands could be configured per
> device. The change breaks existing configurations.
>
> 3) Switching fails for some devices. The E367 (initially appearing as
> 12d1:1446) does not switch when it receives the command sent by
> usb-storage. But the command disable further switching, preventing
> userspace switching from working as well.
>
> Prior to v3.8, the device would switch fine using a default usb_modeswitch
> configuration. The change breaks existing configurations.
>
> I do note that there is a slight difference between the known working
> usb_modeswitch command:
>
>
> 55534243123456780011062001
>
> and the command sent by usb-storage:
>
> char rewind_cmd[] = {0x11, 0x06, 0x20, 0x00, 0x00, 0x01, 0x01, 0x00,
> 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
>
> I assume the cause of the failure is either this difference or some timing
> issue.
>
> Anyway, I believe this patch must be reverted. It disables currently used,
> well
> documented, and extremely useful, userspace funtionaliy for all devices
> implicitly added by the conversion from device matching to vendor matching.
>
>
>
> Bjørn
Best Regards,
Franko Fang
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
v3.8 regression: Huawei mode switching fails (was Re: [PATCH 2/2]linux-usb:optimize to match the Huawei USB storage devices and support new switch command)
Hello Franko,
This patch causes a number of regressions for both the Huawei devices I
have available for testing. One of them is completely unusable in v3.8
(unable to switch to modem mode) unless the usb-storage driver is
disabled.
I realize that some devices are historically handled by the usb-storage
driver, but userspace modeswitching is the rule for new devices AFAIK.
I see absolutely no valid argument for adding new devices. And there is
absolutely no excuse for adding devices which have been handled by
userspace switching for years! The patch does not just "optimize"
matching. It adds a large number of devices which were previously
handled just fine by usb_modeswitch. This is guaranteed to confuse users
with existing configurations, and users looking up any of the existing
documentation pointing to usb_modeswitch.
There is no need to repeat the discussion of kernel vs userspace
switching. I will only note that userspace switching:
- has been selected as the rule for new devices,
- allow the user to temporarily disable switching e.g. for mounting and
inspecting the usb-storage exported data,
- allow the user/system to apply device specific switching quirks
The v3.7->v3.8 regressions I observe are:
1) Temporarily disabling mode switching and mounting the CD image is now
impossible. Mode switching can only be disabled by blacklisting
usb-storage, which of course prevents usb-storage from working
Prior to v3.8, modeswitching could easily be disabled by userspace
configuration. The change breaks existing configurations.
2) Switching to non-default modes is now impossible. The E392
(initially appearing as 12d1:1505) Windows drivers will use a different
command than the one used by Linux, causing the device to select a
different configuration in Linux and Windows. This forces Linux and
Windows to see the device differently.
Prior to v3.8, different modeswitching commands could be configured
per device. The change breaks existing configurations.
3) Switching fails for some devices. The E367 (initially appearing as
12d1:1446) does not switch when it receives the command sent by
usb-storage. But the command disable further switching, preventing
userspace switching from working as well.
Prior to v3.8, the device would switch fine using a default
usb_modeswitch configuration. The change breaks existing
configurations.
I do note that there is a slight difference between the known working
usb_modeswitch command:
55534243123456780011062001
and the command sent by usb-storage:
char rewind_cmd[] = {0x11, 0x06, 0x20, 0x00, 0x00, 0x01, 0x01, 0x00,
0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
I assume the cause of the failure is either this difference or some
timing issue.
Anyway, I believe this patch must be reverted. It disables currently
used, well documented, and extremely useful, userspace funtionaliy for
all devices implicitly added by the conversion from device matching to
vendor matching.
Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
v3.8 regression: Huawei mode switching fails (was Re: [PATCH 2/2]linux-usb:optimize to match the Huawei USB storage devices and support new switch command)
Hello Franko,
This patch causes a number of regressions for both the Huawei devices I
have available for testing. One of them is completely unusable in v3.8
(unable to switch to modem mode) unless the usb-storage driver is
disabled.
I realize that some devices are historically handled by the usb-storage
driver, but userspace modeswitching is the rule for new devices AFAIK.
I see absolutely no valid argument for adding new devices. And there is
absolutely no excuse for adding devices which have been handled by
userspace switching for years! The patch does not just optimize
matching. It adds a large number of devices which were previously
handled just fine by usb_modeswitch. This is guaranteed to confuse users
with existing configurations, and users looking up any of the existing
documentation pointing to usb_modeswitch.
There is no need to repeat the discussion of kernel vs userspace
switching. I will only note that userspace switching:
- has been selected as the rule for new devices,
- allow the user to temporarily disable switching e.g. for mounting and
inspecting the usb-storage exported data,
- allow the user/system to apply device specific switching quirks
The v3.7-v3.8 regressions I observe are:
1) Temporarily disabling mode switching and mounting the CD image is now
impossible. Mode switching can only be disabled by blacklisting
usb-storage, which of course prevents usb-storage from working
Prior to v3.8, modeswitching could easily be disabled by userspace
configuration. The change breaks existing configurations.
2) Switching to non-default modes is now impossible. The E392
(initially appearing as 12d1:1505) Windows drivers will use a different
command than the one used by Linux, causing the device to select a
different configuration in Linux and Windows. This forces Linux and
Windows to see the device differently.
Prior to v3.8, different modeswitching commands could be configured
per device. The change breaks existing configurations.
3) Switching fails for some devices. The E367 (initially appearing as
12d1:1446) does not switch when it receives the command sent by
usb-storage. But the command disable further switching, preventing
userspace switching from working as well.
Prior to v3.8, the device would switch fine using a default
usb_modeswitch configuration. The change breaks existing
configurations.
I do note that there is a slight difference between the known working
usb_modeswitch command:
55534243123456780011062001
and the command sent by usb-storage:
char rewind_cmd[] = {0x11, 0x06, 0x20, 0x00, 0x00, 0x01, 0x01, 0x00,
0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
I assume the cause of the failure is either this difference or some
timing issue.
Anyway, I believe this patch must be reverted. It disables currently
used, well documented, and extremely useful, userspace funtionaliy for
all devices implicitly added by the conversion from device matching to
vendor matching.
Bjørn
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: v3.8 regression: Huawei mode switching fails (was Re: [PATCH 2/2]linux-usb:optimize to match the Huawei USB storage devices and support new switch command)
Dear Mork:
Thank you very much for your test.
-Original Message-
From: Bjørn Mork [mailto:bj...@mork.no]
Sent: Monday, March 04, 2013 7:41 PM
To: Fangxiaozhi (Franko)
Cc: linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; Xueguiying
(Zihan); Linlei (Lei Lin); g...@kroah.com; Yili (Neil); Wangyuhua (Roger,
Credit);
Huqiao (C); ba...@ti.com; mdharm-...@one-eyed-alien.net;
sebast...@breakpoint.cc
Subject: v3.8 regression: Huawei mode switching fails (was Re: [PATCH
2/2]linux-usb:optimize to match the Huawei USB storage devices and support
new switch command)
Hello Franko,
This patch causes a number of regressions for both the Huawei devices I have
available for testing. One of them is completely unusable in v3.8 (unable to
switch to modem mode) unless the usb-storage driver is disabled.
--Which device, can you tell me more information?
--Which model? And what's its firmware version?
I realize that some devices are historically handled by the usb-storage
driver,
but userspace modeswitching is the rule for new devices AFAIK.
I see absolutely no valid argument for adding new devices. And there is
absolutely no excuse for adding devices which have been handled by
userspace switching for years! The patch does not just optimize
matching. It adds a large number of devices which were previously handled
just fine by usb_modeswitch. This is guaranteed to confuse users with existing
configurations, and users looking up any of the existing documentation
pointing to usb_modeswitch.
--Yes. But there is some problems for this:
1. As far as I know, usb_modeswitch is now only integrated in the PC
Linux. It isn't integrated to other system with linux kernel, such as Android,
Chrome OS, etc. On these system, how can we switch the device?
2. usb_modeswitch often sends the Windows switching command to switch
Huawei device, but not sends Linux switching command. This maybe causes some
unexpected problem.
There is no need to repeat the discussion of kernel vs userspace switching. I
will only note that userspace switching:
- has been selected as the rule for new devices,
- allow the user to temporarily disable switching e.g. for mounting and
inspecting the usb-storage exported data,
- allow the user/system to apply device specific switching quirks
The v3.7-v3.8 regressions I observe are:
1) Temporarily disabling mode switching and mounting the CD image is now
impossible. Mode switching can only be disabled by blacklisting
usb-storage, which of course prevents usb-storage from working
Prior to v3.8, modeswitching could easily be disabled by userspace
configuration. The change breaks existing configurations.
2) Switching to non-default modes is now impossible. The E392 (initially
appearing as 12d1:1505) Windows drivers will use a different command than
the one used by Linux, causing the device to select a different configuration
in Linux and Windows. This forces Linux and Windows to see the device
differently.
Prior to v3.8, different modeswitching commands could be configured per
device. The change breaks existing configurations.
3) Switching fails for some devices. The E367 (initially appearing as
12d1:1446) does not switch when it receives the command sent by
usb-storage. But the command disable further switching, preventing
userspace switching from working as well.
Prior to v3.8, the device would switch fine using a default usb_modeswitch
configuration. The change breaks existing configurations.
I do note that there is a slight difference between the known working
usb_modeswitch command:
55534243123456780011062001
and the command sent by usb-storage:
char rewind_cmd[] = {0x11, 0x06, 0x20, 0x00, 0x00, 0x01, 0x01, 0x00,
0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
I assume the cause of the failure is either this difference or some timing
issue.
Anyway, I believe this patch must be reverted. It disables currently used,
well
documented, and extremely useful, userspace funtionaliy for all devices
implicitly added by the conversion from device matching to vendor matching.
Bjørn
Best Regards,
Franko Fang
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf��^jǫy�m��@A�a���
0��h���i
Re: [PATCH 2/2]linux-usb:optimize to match the Huawei USB storage devices and support new switch command
On Fri, Jan 25, 2013 at 10:46:13AM +0800, fangxiaozhi 00110321 wrote:
> diff -uprN linux-3.8-rc4_orig/drivers/usb/storage/initializers.c
> linux-3.8-rc4/drivers/usb/storage/initializers.c
> --- linux-3.8-rc4_orig/drivers/usb/storage/initializers.c 2013-01-22
> 14:12:42.594238726 +0800
> +++ linux-3.8-rc4/drivers/usb/storage/initializers.c 2013-01-22
> 14:28:21.512294889 +0800
> @@ -104,3 +104,75 @@ 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.
> + */
> +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));
First you memset() 16 bytes of CDB to 0
> + memcpy(bcbw->CDB, rewind_cmd, sizeof(rewind_cmd));
And now you copy 16 bytes from rewind_cmd to CDB. So the memset is not really
required, is it?
> +
> + result = usb_stor_bulk_transfer_buf(us, us->send_bulk_pipe, bcbw,
> + US_BULK_CB_WRAP_LEN, _len);
> + US_DEBUGP("transfer actual length=%d, result=%d\n", act_len, result);
> + return result;
> +}
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2]linux-usb:optimize to match the Huawei USB storage devices and support new switch command
On Fri, Jan 25, 2013 at 10:46:13AM +0800, fangxiaozhi 00110321 wrote:
diff -uprN linux-3.8-rc4_orig/drivers/usb/storage/initializers.c
linux-3.8-rc4/drivers/usb/storage/initializers.c
--- linux-3.8-rc4_orig/drivers/usb/storage/initializers.c 2013-01-22
14:12:42.594238726 +0800
+++ linux-3.8-rc4/drivers/usb/storage/initializers.c 2013-01-22
14:28:21.512294889 +0800
@@ -104,3 +104,75 @@ 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.
+ */
+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));
First you memset() 16 bytes of CDB to 0
+ memcpy(bcbw-CDB, rewind_cmd, sizeof(rewind_cmd));
And now you copy 16 bytes from rewind_cmd to CDB. So the memset is not really
required, is it?
+
+ 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;
+}
Sebastian
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2]linux-usb:optimize to match the Huawei USB storage devices and support new switch command
Dear Greg: > -Original Message- > From: Greg KH [mailto:g...@kroah.com] > Sent: Saturday, January 12, 2013 8:22 AM > To: Fangxiaozhi (Franko) > Cc: linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; Xueguiying > (Zihan); > Linlei (Lei Lin); Yili (Neil); Wangyuhua (Roger, Credit); Huqiao (C); > ba...@ti.com; > mdharm-...@one-eyed-alien.net; sebast...@breakpoint.cc > 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: > > From: fangxiaozhi > > > > 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. > > I don't see a 1/2 patch in this series, yet I see two different 1/1 patches. --Yes, this email includes the entire patch. --I mask the "2/2" to distinguish it from the previous email, which mask 1/1. > > Can you resend all of your outstanding patchs, in the correct order, with the > correct numbering, so I know what order to apply them in? > -This email is the whole patch, so can I resend it again? > thanks, > > greg k-h Best Regards, Franko Fang
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, _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 = >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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2]linux-usb:optimize to match the Huawei USB storage devices and support new switch command
Dear Greg: -Original Message- From: Greg KH [mailto:g...@kroah.com] Sent: Saturday, January 12, 2013 8:22 AM To: Fangxiaozhi (Franko) Cc: linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; Xueguiying (Zihan); Linlei (Lei Lin); Yili (Neil); Wangyuhua (Roger, Credit); Huqiao (C); ba...@ti.com; mdharm-...@one-eyed-alien.net; sebast...@breakpoint.cc 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: From: fangxiaozhi huana...@huawei.com 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. I don't see a 1/2 patch in this series, yet I see two different 1/1 patches. --Yes, this email includes the entire patch. --I mask the 2/2 to distinguish it from the previous email, which mask 1/1. Can you resend all of your outstanding patchs, in the correct order, with the correct numbering, so I know what order to apply them in? -This email is the whole patch, so can I resend it again? thanks, greg k-h Best Regards, Franko Fang
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: > From: fangxiaozhi > > 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. I don't see a 1/2 patch in this series, yet I see two different 1/1 patches. Can you resend all of your outstanding patchs, in the correct order, with the correct numbering, so I know what order to apply them in? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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: From: fangxiaozhi huana...@huawei.com 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. I don't see a 1/2 patch in this series, yet I see two different 1/1 patches. Can you resend all of your outstanding patchs, in the correct order, with the correct numbering, so I know what order to apply them in? thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2]linux-usb:optimize to match the Huawei USB storage devices and support new switch command
Dear Matthew: > -Original Message- > From: Matthew Dharm [mailto:mdharm-...@one-eyed-alien.net] > Sent: Wednesday, December 19, 2012 11:41 PM > To: Sebastian Andrzej Siewior > Cc: Fangxiaozhi (Franko); linux-...@vger.kernel.org; > linux-kernel@vger.kernel.org; Xueguiying (Zihan); Linlei (Lei Lin); > g...@kroah.com; Yili (Neil); Wangyuhua (Roger, Credit); Huqiao; ba...@ti.com > Subject: Re: [PATCH 2/2]linux-usb:optimize to match the Huawei USB storage > devices and support new switch command > > On Wed, Dec 19, 2012 at 12:34 AM, Sebastian Andrzej Siewior > wrote: > > On Wed, Dec 19, 2012 at 03:13:32AM +, Fangxiaozhi (Franko) wrote: > >> > 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. You mean that we have to add some comment in the source code, to explain why we don't read the response. Right? > > I am also concerned about the error of using 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. >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. > > Matt > > -- > Matthew Dharm > Maintainer, USB Mass Storage driver for Linux Best Regards, Franko Fang
RE: [PATCH 2/2]linux-usb:optimize to match the Huawei USB storage devices and support new switch command
Dear Matthew: -Original Message- From: Matthew Dharm [mailto:mdharm-...@one-eyed-alien.net] Sent: Wednesday, December 19, 2012 11:41 PM To: Sebastian Andrzej Siewior Cc: Fangxiaozhi (Franko); linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; Xueguiying (Zihan); Linlei (Lei Lin); g...@kroah.com; Yili (Neil); Wangyuhua (Roger, Credit); Huqiao; ba...@ti.com Subject: Re: [PATCH 2/2]linux-usb:optimize to match the Huawei USB storage devices and support new switch command On Wed, Dec 19, 2012 at 12:34 AM, Sebastian Andrzej Siewior sebast...@breakpoint.cc wrote: On Wed, Dec 19, 2012 at 03:13:32AM +, Fangxiaozhi (Franko) wrote: 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. You mean that we have to add some comment in the source code, to explain why we don't read the response. Right? 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. 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. Matt -- Matthew Dharm Maintainer, USB Mass Storage driver for Linux Best Regards, Franko Fang
Re: [PATCH 2/2]linux-usb:optimize to match the Huawei USB storage devices and support new switch command
On Wed, Dec 19, 2012 at 12:34 AM, Sebastian Andrzej Siewior wrote: > On Wed, Dec 19, 2012 at 03:13:32AM +, Fangxiaozhi (Franko) wrote: >> > 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 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 -- Matthew Dharm Maintainer, USB Mass Storage driver for Linux -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2]linux-usb:optimize to match the Huawei USB storage devices and support new switch command
On Wed, Dec 19, 2012 at 03:13:32AM +, Fangxiaozhi (Franko) wrote: > 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 |usb-stor...@lists.one-eyed-alien.net > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2]linux-usb:optimize to match the Huawei USB storage devices and support new switch command
On Wed, Dec 19, 2012 at 03:13:32AM +, Fangxiaozhi (Franko) wrote: 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 mdharm-...@one-eyed-alien.net |usb-stor...@lists.one-eyed-alien.net 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2]linux-usb:optimize to match the Huawei USB storage devices and support new switch command
On Wed, Dec 19, 2012 at 12:34 AM, Sebastian Andrzej Siewior sebast...@breakpoint.cc wrote: On Wed, Dec 19, 2012 at 03:13:32AM +, Fangxiaozhi (Franko) wrote: 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 -- Matthew Dharm Maintainer, USB Mass Storage driver for Linux -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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
> -Original Message-
> From: Sebastian Andrzej Siewior [mailto:sebast...@breakpoint.cc]
> Sent: Tuesday, December 18, 2012 10:10 PM
> To: Fangxiaozhi (Franko)
> Cc: linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; Xueguiying
> (Zihan); Linlei (Lei Lin); g...@kroah.com; Yili (Neil); Wangyuhua (Roger,
> Credit); Huqiao; ba...@ti.com
> Subject: Re: [PATCH 2/2]linux-usb:optimize to match the Huawei USB
> storage devices and support new switch command
>
> 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
> > --- linux-3.7_bak/drivers/usb/storage/initializers.c2012-12-11
> 09:56:11.0 +0800
> > +++ linux-3.7/drivers/usb/storage/initializers.c2012-12-17
> 11:12:12.0 +0800
> > 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 = >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};
>
> 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"
>
> > + 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?
>
> > + bcbw->Length = sizeof(rewind_cmd);
> > + memcpy(bcbw->CDB, rewind_cmd, sizeof(rewind_cmd));
> > +
> > + result = usb_stor_bulk_transfer_buf(us, us->send_bulk_pipe, ,
> > + US_BULK_CS_WRAP_LEN, _len);
>
> I am a little confused here. Shouldn't this be bcbw aka us->iobuf and not
> ?
Yes, you are right.
>
> 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.
>
> > + US_DEBUGP("transfer actual length=%d, result=%d\n", act_len,
> result);
> > + return result;
> > +}
> > +
>
> Sebastian
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
Re: [PATCH 2/2]linux-usb:optimize to match the Huawei USB storage devices and support new switch command
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
> --- linux-3.7_bak/drivers/usb/storage/initializers.c 2012-12-11
> 09:56:11.0 +0800
> +++ linux-3.7/drivers/usb/storage/initializers.c 2012-12-17
> 11:12:12.0 +0800
> 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 = >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};
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.
> + 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?
> + bcbw->Length = sizeof(rewind_cmd);
> + memcpy(bcbw->CDB, rewind_cmd, sizeof(rewind_cmd));
> +
> + result = usb_stor_bulk_transfer_buf(us, us->send_bulk_pipe, ,
> + US_BULK_CS_WRAP_LEN, _len);
I am a little confused here. Shouldn't this be bcbw aka us->iobuf and not
?
And shouldn't you read something from the us->recv_bulk_pipe after that?
> + US_DEBUGP("transfer actual length=%d, result=%d\n", act_len, result);
> + return result;
> +}
> +
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2]linux-usb:optimize to match the Huawei USB storage devices and support new switch command
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
--- linux-3.7_bak/drivers/usb/storage/initializers.c 2012-12-11
09:56:11.0 +0800
+++ linux-3.7/drivers/usb/storage/initializers.c 2012-12-17
11:12:12.0 +0800
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};
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.
+ 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?
+ 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?
+ US_DEBUGP(transfer actual length=%d, result=%d\n, act_len, result);
+ return result;
+}
+
Sebastian
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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
-Original Message-
From: Sebastian Andrzej Siewior [mailto:sebast...@breakpoint.cc]
Sent: Tuesday, December 18, 2012 10:10 PM
To: Fangxiaozhi (Franko)
Cc: linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; Xueguiying
(Zihan); Linlei (Lei Lin); g...@kroah.com; Yili (Neil); Wangyuhua (Roger,
Credit); Huqiao; ba...@ti.com
Subject: Re: [PATCH 2/2]linux-usb:optimize to match the Huawei USB
storage devices and support new switch command
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
--- linux-3.7_bak/drivers/usb/storage/initializers.c2012-12-11
09:56:11.0 +0800
+++ linux-3.7/drivers/usb/storage/initializers.c2012-12-17
11:12:12.0 +0800
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};
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
+ 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?
+ 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.
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.
+ US_DEBUGP(transfer actual length=%d, result=%d\n, act_len,
result);
+ return result;
+}
+
Sebastian
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf��^jǫy�m��@A�a���
0��h���i
