Patchwork [1/1] Fix SATA drive failure on Ubuntu 9.10 installation

login
register
mail settings
Submitter Bryan Wu
Date Dec. 1, 2009, 9:32 a.m.
Message ID <1259659962-4113-2-git-send-email-bryan.wu@canonical.com>
Download mbox | patch
Permalink /patch/39894/
State Changes Requested
Headers show

Comments

Bryan Wu - Dec. 1, 2009, 9:32 a.m.
From: Dinh Nguyen <r00091@freescale.com>

BugLink: https://bugs.launchpad.net/bug/431963

Signed-off-by: Bryan Wu <bryan.wu@canonical.com>
---
 drivers/usb/storage/usb.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)
Andy Whitcroft - Dec. 1, 2009, 10:01 a.m.
On Tue, Dec 01, 2009 at 05:32:42PM +0800, Bryan Wu wrote:
> From: Dinh Nguyen <r00091@freescale.com>
> 
> BugLink: https://bugs.launchpad.net/bug/431963

Missing an 's' here.  Had me going for a bit there.

I had a quick look at the bug, and the following is the text associated
with this patch:

    I think I have found a fix for this issue. I have attached the
    patch. It is probably not the correct fix at the moment, and more
    analysis is needed, but for now it appears that with 9.10, there is an
    invalid command that is coming to the USB/SATA bridge chip. Instead
    of gracefully handling this command, the USB/SATA bridge chip on the
    Babbage board is failing on this command.

This tends to imply its just a debug patch and not the final fix?  Can
we confirm its the final fix?

-apw
Bryan Wu - Dec. 1, 2009, 10:11 a.m.
Andy Whitcroft 写道:
> On Tue, Dec 01, 2009 at 05:32:42PM +0800, Bryan Wu wrote:
>> From: Dinh Nguyen <r00091@freescale.com>
>>
>> BugLink: https://bugs.launchpad.net/bug/431963
> 
> Missing an 's' here.  Had me going for a bit there.
> 

Sorry about that, -:).

> I had a quick look at the bug, and the following is the text associated
> with this patch:
> 
>     I think I have found a fix for this issue. I have attached the
>     patch. It is probably not the correct fix at the moment, and more
>     analysis is needed, but for now it appears that with 9.10, there is an
>     invalid command that is coming to the USB/SATA bridge chip. Instead
>     of gracefully handling this command, the USB/SATA bridge chip on the
>     Babbage board is failing on this command.
> 
> This tends to imply its just a debug patch and not the final fix?  Can
> we confirm its the final fix?
> 

This patch is not a final fix, from my point of view. It is a workaround to make 
the bad USB chip work. Please find the comments from the USB chip vendor:

---
We found that the key issue deciding Ubuntu 9.10 boot-up or not is “how we 
handle this ATA PASS-THROUGH command.” Original GL830 just pass the ATA 
PASS-THROUGH command and the SECTOR_COUNT field of its IDENTIFY command is 0, so 
GL830 seems it is an invalid command. As a result, GL830 just bypass this ATA 
PASS-THROUGH command to HDD and return USB Host a CSW OK. The hang-out situation 
is because HDD is processing the IDENTIFY command, but GL830 did not response HDD.

We have tried to ask GL830 response USB Host a CSW FAIL of CSW STALL to skip 
this ATA PASS-THROUGH command, and the Ubuntu 9.10 can successfully boot-up. The 
USB/SATA cable you’re using must be performance this similar solution, so it can 
boot system up.

The root cause of this weakness (the glitch you mentioned) is because GL830 
doesn’t check the integrity of ATA PASS-THROUGH command. Since there are too 
many exceptions, we suppose the command sent by USB Host should be a valid one.
---

Although it is a workaround and might not be accept by upstream, it is important 
to fix this critical bug in our Karmic i.MX51 release.

Thanks
-Bryan
Andy Whitcroft - Dec. 1, 2009, 10:22 a.m.
On Tue, Dec 01, 2009 at 06:11:18PM +0800, Bryan Wu wrote:

> Although it is a workaround and might not be accept by upstream, it
> is important to fix this critical bug in our Karmic i.MX51 release.

I can see that we might need to prevent command 0x85 passing thorugh for
this controller to fix this issue, but the code seems to my eye to just
stop all 0x85 commands to all USB storage devices regardless of where
they are connected?  That seems rather wide ranging and risky?

-apw
Stefan Bader - Dec. 1, 2009, 10:38 a.m.
Andy Whitcroft wrote:
> On Tue, Dec 01, 2009 at 06:11:18PM +0800, Bryan Wu wrote:
> 
>> Although it is a workaround and might not be accept by upstream, it
>> is important to fix this critical bug in our Karmic i.MX51 release.
> 
> I can see that we might need to prevent command 0x85 passing thorugh for
> this controller to fix this issue, but the code seems to my eye to just
> stop all 0x85 commands to all USB storage devices regardless of where
> they are connected?  That seems rather wide ranging and risky?
> 
> -apw
> 

At least all USB storage devices on all controllers. So it really should
be more specific to the controller on the board.

-Stefan
Bryan Wu - Dec. 1, 2009, 10:50 a.m.
Stefan Bader 写道:
> Andy Whitcroft wrote:
>> On Tue, Dec 01, 2009 at 06:11:18PM +0800, Bryan Wu wrote:
>>
>>> Although it is a workaround and might not be accept by upstream, it
>>> is important to fix this critical bug in our Karmic i.MX51 release.
>> I can see that we might need to prevent command 0x85 passing thorugh for
>> this controller to fix this issue, but the code seems to my eye to just
>> stop all 0x85 commands to all USB storage devices regardless of where
>> they are connected?  That seems rather wide ranging and risky?
>>
>> -apw
>>
> 
> At least all USB storage devices on all controllers. So it really should
> be more specific to the controller on the board.
> 
> -Stefan

Yeah, we need to only stop passing 0x85 to GL830 chip, while keep others intact.

Thanks for pointing that out, I will rework the patch and resend them out after 
testing.

-Bryan

Patch

diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index 8060b85..11dd37d 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -329,8 +329,11 @@  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));
-			us->proto_handler(us->srb, us);
+			US_DEBUGP(usb_stor_show_command(us->srb));
+#ifdef CONFIG_MACH_MX51_BABBAGE
+			if (us->srb->cmnd[0] != 0x85)
+#endif
+				us->proto_handler(us->srb, us);
 		}
 
 		/* lock access to the state */