Patchwork [1/1] Ubuntu: SAUCE: Workaround for SATA drive failure on Ubuntu installation

login
register
mail settings
Submitter Andy Whitcroft
Date Dec. 16, 2009, 11:57 a.m.
Message ID <20091216115756.GA23682@shadowen.org>
Download mbox | patch
Permalink /patch/41250/
State Accepted
Headers show

Comments

Andy Whitcroft - Dec. 16, 2009, 11:57 a.m.
On Wed, Dec 16, 2009 at 05:21:12PM +0800, Bryan Wu wrote:
> From: Dinh Nguyen <r00091@freescale.com>
> 
> BugLink: http://bugs.launchpad.net/bugs/431963
> 
> Original patch was from Dinh Nguyen. That one find the root cause
> of this SATA drive failure issue. The USB2SATA chip GL830 can not
> accept the ATA PASS THROUGH command.
> 
> This patch will skip the ATA PASS THROUGH command only for GL830
> USB device. So it will not effect other USB devices.
> 
> Signed-off-by: Dinh Nguyen <r00091@freescale.com>
> Signed-off-by: Bryan Wu <bryan.wu@canonical.com>
> ---
>  drivers/usb/storage/usb.c |   17 +++++++++++++++--
>  1 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
> index 8060b85..d538511 100644
> --- a/drivers/usb/storage/usb.c
> +++ b/drivers/usb/storage/usb.c
> @@ -329,8 +329,21 @@ static int usb_stor_control_thread(void * __us)
>  
>  		/* we've got a command, let's do it! */
>  		else {
> -			US_DEBUG(usb_stor_show_command(us->srb));

This changes ...

> -			us->proto_handler(us->srb, us);
> +#ifdef CONFIG_MACH_MX51_BABBAGE
> +			u16 vid =
> +				le16_to_cpu(us->pusb_dev->descriptor.idVendor);
> +			u16 pid =
> +				le16_to_cpu(us->pusb_dev->descriptor.idProduct);
> +#endif
> +			US_DEBUGP(usb_stor_show_command(us->srb));

... to this, which is wrong as the routine there is a void, so there is
nothing to print.  I think this will translate into a compile error when
CONFIG_USB_STORAGE_DEBUG is enabled.

It also seems wrong to be looking up and translating these ids for every
command when we only care for 0x85 commands.

> +#ifdef CONFIG_MACH_MX51_BABBAGE
> +			if ((us->srb->cmnd[0] == 0x85) &&
> +				(vid == 0x05e3) &&
> +				(pid == 0x0718))
> +				US_DEBUGP("Skip ATA PASS-THROUGH command\n");
> +			else
> +#endif
> +				us->proto_handler(us->srb, us);
>  		}
>  
>  		/* lock access to the state */

So I propose to apply the patch as below.

Comments?

-apw

commit 95860a73f4c1ebff91cbf03783027c15600361d7
Author: Dinh Nguyen <r00091@freescale.com>
Date:   Wed Dec 16 17:21:12 2009 +0800

    Ubuntu: SAUCE: Workaround for SATA drive failure on Ubuntu installation
    
    BugLink: http://bugs.launchpad.net/bugs/431963
    
    Original patch was from Dinh Nguyen. That one find the root cause
    of this SATA drive failure issue. The USB2SATA chip GL830 can not
    accept the ATA PASS THROUGH command.
    
    This patch will skip the ATA PASS THROUGH command only for GL830
    USB device. So it will not effect other USB devices.
    
    [apw@canonical.com: fixed debug and optimised.]
    Signed-off-by: Dinh Nguyen <r00091@freescale.com>
    Signed-off-by: Bryan Wu <bryan.wu@canonical.com>
    Signed-off-by: Andy Whitcroft <apw@canonical.com>
Stefan Bader - Dec. 16, 2009, 2:46 p.m.
Andy Whitcroft wrote:
> On Wed, Dec 16, 2009 at 05:21:12PM +0800, Bryan Wu wrote:
>> From: Dinh Nguyen <r00091@freescale.com>
>>
>> BugLink: http://bugs.launchpad.net/bugs/431963
>>
>> Original patch was from Dinh Nguyen. That one find the root cause
>> of this SATA drive failure issue. The USB2SATA chip GL830 can not
>> accept the ATA PASS THROUGH command.
>>
>> This patch will skip the ATA PASS THROUGH command only for GL830
>> USB device. So it will not effect other USB devices.
>>
>> Signed-off-by: Dinh Nguyen <r00091@freescale.com>
>> Signed-off-by: Bryan Wu <bryan.wu@canonical.com>
>> ---
>>  drivers/usb/storage/usb.c |   17 +++++++++++++++--
>>  1 files changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
>> index 8060b85..d538511 100644
>> --- a/drivers/usb/storage/usb.c
>> +++ b/drivers/usb/storage/usb.c
>> @@ -329,8 +329,21 @@ static int usb_stor_control_thread(void * __us)
>>  
>>  		/* we've got a command, let's do it! */
>>  		else {
>> -			US_DEBUG(usb_stor_show_command(us->srb));
> 
> This changes ...
> 
>> -			us->proto_handler(us->srb, us);
>> +#ifdef CONFIG_MACH_MX51_BABBAGE
>> +			u16 vid =
>> +				le16_to_cpu(us->pusb_dev->descriptor.idVendor);
>> +			u16 pid =
>> +				le16_to_cpu(us->pusb_dev->descriptor.idProduct);
>> +#endif
>> +			US_DEBUGP(usb_stor_show_command(us->srb));
> 
> ... to this, which is wrong as the routine there is a void, so there is
> nothing to print.  I think this will translate into a compile error when
> CONFIG_USB_STORAGE_DEBUG is enabled.
> 
> It also seems wrong to be looking up and translating these ids for every
> command when we only care for 0x85 commands.
> 
>> +#ifdef CONFIG_MACH_MX51_BABBAGE
>> +			if ((us->srb->cmnd[0] == 0x85) &&
>> +				(vid == 0x05e3) &&
>> +				(pid == 0x0718))
>> +				US_DEBUGP("Skip ATA PASS-THROUGH command\n");
>> +			else
>> +#endif
>> +				us->proto_handler(us->srb, us);
>>  		}
>>  
>>  		/* lock access to the state */
> 
> So I propose to apply the patch as below.
> 
> Comments?
> 
> -apw
> 
> commit 95860a73f4c1ebff91cbf03783027c15600361d7
> Author: Dinh Nguyen <r00091@freescale.com>
> Date:   Wed Dec 16 17:21:12 2009 +0800
> 
>     Ubuntu: SAUCE: Workaround for SATA drive failure on Ubuntu installation
>     
>     BugLink: http://bugs.launchpad.net/bugs/431963
>     
>     Original patch was from Dinh Nguyen. That one find the root cause
>     of this SATA drive failure issue. The USB2SATA chip GL830 can not
>     accept the ATA PASS THROUGH command.
>     
>     This patch will skip the ATA PASS THROUGH command only for GL830
>     USB device. So it will not effect other USB devices.
>     
>     [apw@canonical.com: fixed debug and optimised.]
>     Signed-off-by: Dinh Nguyen <r00091@freescale.com>
>     Signed-off-by: Bryan Wu <bryan.wu@canonical.com>
>     Signed-off-by: Andy Whitcroft <apw@canonical.com>
> 
> diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
> index 8060b85..961bb1a 100644
> --- a/drivers/usb/storage/usb.c
> +++ b/drivers/usb/storage/usb.c
> @@ -330,6 +330,15 @@ static int usb_stor_control_thread(void * __us)
>  		/* we've got a command, let's do it! */
>  		else {
>  			US_DEBUG(usb_stor_show_command(us->srb));
> +#ifdef CONFIG_MACH_MX51_BABBAGE
> +			if ((us->srb->cmnd[0] == 0x85) &&
> +			    (le16_to_cpu(us->pusb_dev->descriptor.idVendor)
> +								== 0x05e3) &&
> +			    (le16_to_cpu(us->pusb_dev->descriptor.idProduct)
> +								== 0x0718))
> +				US_DEBUGP("Skip ATA PASS-THROUGH command\n");
> +			else
> +#endif
>  			us->proto_handler(us->srb, us);
>  		}
>  
> 
Doh, a good catch on the additional P. And with the the checking
of vendor id and product id in place, even I seem to notice it
is done, which I totally failed to do this morning.
So, ok, works better for me.

-Stefan
Bryan Wu - Dec. 16, 2009, 3:49 p.m.
Andy Whitcroft wrote:
> On Wed, Dec 16, 2009 at 05:21:12PM +0800, Bryan Wu wrote:
>   
>> From: Dinh Nguyen <r00091@freescale.com>
>>
>> BugLink: http://bugs.launchpad.net/bugs/431963
>>
>> Original patch was from Dinh Nguyen. That one find the root cause
>> of this SATA drive failure issue. The USB2SATA chip GL830 can not
>> accept the ATA PASS THROUGH command.
>>
>> This patch will skip the ATA PASS THROUGH command only for GL830
>> USB device. So it will not effect other USB devices.
>>
>> Signed-off-by: Dinh Nguyen <r00091@freescale.com>
>> Signed-off-by: Bryan Wu <bryan.wu@canonical.com>
>> ---
>>  drivers/usb/storage/usb.c |   17 +++++++++++++++--
>>  1 files changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
>> index 8060b85..d538511 100644
>> --- a/drivers/usb/storage/usb.c
>> +++ b/drivers/usb/storage/usb.c
>> @@ -329,8 +329,21 @@ static int usb_stor_control_thread(void * __us)
>>  
>>  		/* we've got a command, let's do it! */
>>  		else {
>> -			US_DEBUG(usb_stor_show_command(us->srb));
>>     
>
> This changes ...
>
>   
>> -			us->proto_handler(us->srb, us);
>> +#ifdef CONFIG_MACH_MX51_BABBAGE
>> +			u16 vid =
>> +				le16_to_cpu(us->pusb_dev->descriptor.idVendor);
>> +			u16 pid =
>> +				le16_to_cpu(us->pusb_dev->descriptor.idProduct);
>> +#endif
>> +			US_DEBUGP(usb_stor_show_command(us->srb));
>>     
>
> ... to this, which is wrong as the routine there is a void, so there is
> nothing to print.  I think this will translate into a compile error when
> CONFIG_USB_STORAGE_DEBUG is enabled.
>
> It also seems wrong to be looking up and translating these ids for every
> command when we only care for 0x85 commands.
>
>   
>> +#ifdef CONFIG_MACH_MX51_BABBAGE
>> +			if ((us->srb->cmnd[0] == 0x85) &&
>> +				(vid == 0x05e3) &&
>> +				(pid == 0x0718))
>> +				US_DEBUGP("Skip ATA PASS-THROUGH command\n");
>> +			else
>> +#endif
>> +				us->proto_handler(us->srb, us);
>>  		}
>>  
>>  		/* lock access to the state */
>>     
>
> So I propose to apply the patch as below.
>
> Comments?
>
> -apw
>
> commit 95860a73f4c1ebff91cbf03783027c15600361d7
> Author: Dinh Nguyen <r00091@freescale.com>
> Date:   Wed Dec 16 17:21:12 2009 +0800
>
>     Ubuntu: SAUCE: Workaround for SATA drive failure on Ubuntu installation
>     
>     BugLink: http://bugs.launchpad.net/bugs/431963
>     
>     Original patch was from Dinh Nguyen. That one find the root cause
>     of this SATA drive failure issue. The USB2SATA chip GL830 can not
>     accept the ATA PASS THROUGH command.
>     
>     This patch will skip the ATA PASS THROUGH command only for GL830
>     USB device. So it will not effect other USB devices.
>     
>     [apw@canonical.com: fixed debug and optimised.]
>     Signed-off-by: Dinh Nguyen <r00091@freescale.com>
>     Signed-off-by: Bryan Wu <bryan.wu@canonical.com>
>     Signed-off-by: Andy Whitcroft <apw@canonical.com>
>
> diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
> index 8060b85..961bb1a 100644
> --- a/drivers/usb/storage/usb.c
> +++ b/drivers/usb/storage/usb.c
> @@ -330,6 +330,15 @@ static int usb_stor_control_thread(void * __us)
>  		/* we've got a command, let's do it! */
>  		else {
>  			US_DEBUG(usb_stor_show_command(us->srb));
> +#ifdef CONFIG_MACH_MX51_BABBAGE
> +			if ((us->srb->cmnd[0] == 0x85) &&
> +			    (le16_to_cpu(us->pusb_dev->descriptor.idVendor)
> +								== 0x05e3) &&
> +			    (le16_to_cpu(us->pusb_dev->descriptor.idProduct)
> +								== 0x0718))
> +				US_DEBUGP("Skip ATA PASS-THROUGH command\n");
> +			else
> +#endif
>  			us->proto_handler(us->srb, us);
>  		}
>  
>   
Thanks a lot, Andy. I missed these 2 key points you mentioned here. 
Shall I repost the patch again
or you will apply it.

Cheers,
-Bryan
Andy Whitcroft - Dec. 16, 2009, 4:52 p.m.
On Wed, Dec 16, 2009 at 11:49:08PM +0800, Bryan Wu wrote:

> Thanks a lot, Andy. I missed these 2 key points you mentioned here.
> Shall I repost the patch again
> or you will apply it.

I'll apply it as you are happy.

-apw
Stefan Bader - Dec. 16, 2009, 11:30 p.m.
Bryan Wu wrote:
> Andy Whitcroft wrote:
>> On Wed, Dec 16, 2009 at 05:21:12PM +0800, Bryan Wu wrote:
>>   
>>> From: Dinh Nguyen <r00091@freescale.com>
>>>
>>> BugLink: http://bugs.launchpad.net/bugs/431963
>>>
>>> Original patch was from Dinh Nguyen. That one find the root cause
>>> of this SATA drive failure issue. The USB2SATA chip GL830 can not
>>> accept the ATA PASS THROUGH command.
>>>
>>> This patch will skip the ATA PASS THROUGH command only for GL830
>>> USB device. So it will not effect other USB devices.
>>>
>>> Signed-off-by: Dinh Nguyen <r00091@freescale.com>
>>> Signed-off-by: Bryan Wu <bryan.wu@canonical.com>
>>> ---
>>>  drivers/usb/storage/usb.c |   17 +++++++++++++++--
>>>  1 files changed, 15 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
>>> index 8060b85..d538511 100644
>>> --- a/drivers/usb/storage/usb.c
>>> +++ b/drivers/usb/storage/usb.c
>>> @@ -329,8 +329,21 @@ static int usb_stor_control_thread(void * __us)
>>>  
>>>  		/* we've got a command, let's do it! */
>>>  		else {
>>> -			US_DEBUG(usb_stor_show_command(us->srb));
>>>     
>> This changes ...
>>
>>   
>>> -			us->proto_handler(us->srb, us);
>>> +#ifdef CONFIG_MACH_MX51_BABBAGE
>>> +			u16 vid =
>>> +				le16_to_cpu(us->pusb_dev->descriptor.idVendor);
>>> +			u16 pid =
>>> +				le16_to_cpu(us->pusb_dev->descriptor.idProduct);
>>> +#endif
>>> +			US_DEBUGP(usb_stor_show_command(us->srb));
>>>     
>> ... to this, which is wrong as the routine there is a void, so there is
>> nothing to print.  I think this will translate into a compile error when
>> CONFIG_USB_STORAGE_DEBUG is enabled.
>>
>> It also seems wrong to be looking up and translating these ids for every
>> command when we only care for 0x85 commands.
>>
>>   
>>> +#ifdef CONFIG_MACH_MX51_BABBAGE
>>> +			if ((us->srb->cmnd[0] == 0x85) &&
>>> +				(vid == 0x05e3) &&
>>> +				(pid == 0x0718))
>>> +				US_DEBUGP("Skip ATA PASS-THROUGH command\n");
>>> +			else
>>> +#endif
>>> +				us->proto_handler(us->srb, us);
>>>  		}
>>>  
>>>  		/* lock access to the state */
>>>     
>> So I propose to apply the patch as below.
>>
>> Comments?
>>
>> -apw
>>
>> commit 95860a73f4c1ebff91cbf03783027c15600361d7
>> Author: Dinh Nguyen <r00091@freescale.com>
>> Date:   Wed Dec 16 17:21:12 2009 +0800
>>
>>     Ubuntu: SAUCE: Workaround for SATA drive failure on Ubuntu installation
>>     
>>     BugLink: http://bugs.launchpad.net/bugs/431963
>>     
>>     Original patch was from Dinh Nguyen. That one find the root cause
>>     of this SATA drive failure issue. The USB2SATA chip GL830 can not
>>     accept the ATA PASS THROUGH command.
>>     
>>     This patch will skip the ATA PASS THROUGH command only for GL830
>>     USB device. So it will not effect other USB devices.
>>     
>>     [apw@canonical.com: fixed debug and optimised.]
>>     Signed-off-by: Dinh Nguyen <r00091@freescale.com>
>>     Signed-off-by: Bryan Wu <bryan.wu@canonical.com>
>>     Signed-off-by: Andy Whitcroft <apw@canonical.com>
>>
>> diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
>> index 8060b85..961bb1a 100644
>> --- a/drivers/usb/storage/usb.c
>> +++ b/drivers/usb/storage/usb.c
>> @@ -330,6 +330,15 @@ static int usb_stor_control_thread(void * __us)
>>  		/* we've got a command, let's do it! */
>>  		else {
>>  			US_DEBUG(usb_stor_show_command(us->srb));
>> +#ifdef CONFIG_MACH_MX51_BABBAGE
>> +			if ((us->srb->cmnd[0] == 0x85) &&
>> +			    (le16_to_cpu(us->pusb_dev->descriptor.idVendor)
>> +								== 0x05e3) &&
>> +			    (le16_to_cpu(us->pusb_dev->descriptor.idProduct)
>> +								== 0x0718))
>> +				US_DEBUGP("Skip ATA PASS-THROUGH command\n");
>> +			else
>> +#endif
>>  			us->proto_handler(us->srb, us);
>>  		}
>>  
>>   
> Thanks a lot, Andy. I missed these 2 key points you mentioned here. 
> Shall I repost the patch again
> or you will apply it.
> 
> Cheers,
> -Bryan
> 

Also applied to Karmic and prepared the new package. I'll upload it when
I can (Launchpad is writable again).

-Stefan

Patch

diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index 8060b85..961bb1a 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -330,6 +330,15 @@  static int usb_stor_control_thread(void * __us)
 		/* we've got a command, let's do it! */
 		else {
 			US_DEBUG(usb_stor_show_command(us->srb));
+#ifdef CONFIG_MACH_MX51_BABBAGE
+			if ((us->srb->cmnd[0] == 0x85) &&
+			    (le16_to_cpu(us->pusb_dev->descriptor.idVendor)
+								== 0x05e3) &&
+			    (le16_to_cpu(us->pusb_dev->descriptor.idProduct)
+								== 0x0718))
+				US_DEBUGP("Skip ATA PASS-THROUGH command\n");
+			else
+#endif
 			us->proto_handler(us->srb, us);
 		}