Patchwork PROBLEM: OHCI HCD - computer freezes during boot

login
register
mail settings
Submitter Alan Stern
Date April 26, 2012, 2:25 p.m.
Message ID <Pine.LNX.4.44L0.1204261020040.1530-100000@iolanthe.rowland.org>
Download mbox | patch
Permalink /patch/155282/
State Not Applicable
Headers show

Comments

Alan Stern - April 26, 2012, 2:25 p.m.
On Thu, 26 Apr 2012, Calin Demian wrote:

> I don't know how, but adding the following line in function
> quirk_usb_handoff_ohci makes the system work fine:
> 
> 	/* reset requires max 10 us delay */
> +	udelay(1);
> 	for (cnt = 30; cnt > 0; --cnt) {	/* ... allow extra time */
> 		if ((readl(base + OHCI_CMDSTATUS) & OHCI_HCR) == 0)
> 
> I observed thad the condition in the for loop is allways true, so the
> delay inside the loop is never executed.
> With the unmodified source, the execution doesn't stop at this point
> but at the next readl (I tried inserting printk and readl instructions
> after the writel which restores the fminterval and the execution
> stopped before exiting quirk_usb_handoff_ohci).
> I don't know how it relates to the ACPI message... (see bug 43073 in
> bugzilla.kernel.org).

I don't know just what the problem is either, but there's nothing wrong 
with delaying startup by 1 us.  In fact, all we have to do is reorder 
the statements within the loop.  Is this patch okay for you?

Alan Stern




--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greg KH - April 26, 2012, 2:43 p.m.
On Thu, Apr 26, 2012 at 10:25:41AM -0400, Alan Stern wrote:
> On Thu, 26 Apr 2012, Calin Demian wrote:
> 
> > I don't know how, but adding the following line in function
> > quirk_usb_handoff_ohci makes the system work fine:
> > 
> > 	/* reset requires max 10 us delay */
> > +	udelay(1);
> > 	for (cnt = 30; cnt > 0; --cnt) {	/* ... allow extra time */
> > 		if ((readl(base + OHCI_CMDSTATUS) & OHCI_HCR) == 0)
> > 
> > I observed thad the condition in the for loop is allways true, so the
> > delay inside the loop is never executed.

That implies that something is wrong with the complier here, and it
optimized out the loop?  Are you sure that really happened?

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Stern - April 26, 2012, 3:16 p.m.
On Thu, 26 Apr 2012, Greg KH wrote:

> On Thu, Apr 26, 2012 at 10:25:41AM -0400, Alan Stern wrote:
> > On Thu, 26 Apr 2012, Calin Demian wrote:
> > 
> > > I don't know how, but adding the following line in function
> > > quirk_usb_handoff_ohci makes the system work fine:
> > > 
> > > 	/* reset requires max 10 us delay */
> > > +	udelay(1);
> > > 	for (cnt = 30; cnt > 0; --cnt) {	/* ... allow extra time */
> > > 		if ((readl(base + OHCI_CMDSTATUS) & OHCI_HCR) == 0)
> > > 
> > > I observed thad the condition in the for loop is allways true, so the
> > > delay inside the loop is never executed.
> 
> That implies that something is wrong with the complier here, and it
> optimized out the loop?  Are you sure that really happened?

Here's the complete context:

	/* reset requires max 10 us delay */
	for (cnt = 30; cnt > 0; --cnt) {	/* ... allow extra time */
		if ((readl(base + OHCI_CMDSTATUS) & OHCI_HCR) == 0)
			break;
		udelay(1);
	}

I think Calim means that the "(readl(base + OHCI_CMDSTATUS) & OHCI_HCR)  
== 0" test succeeds on the first pass through the loop, so we break out
early and the udelay(1) statement never gets executed.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greg KH - April 26, 2012, 3:25 p.m.
On Thu, Apr 26, 2012 at 11:16:17AM -0400, Alan Stern wrote:
> On Thu, 26 Apr 2012, Greg KH wrote:
> 
> > On Thu, Apr 26, 2012 at 10:25:41AM -0400, Alan Stern wrote:
> > > On Thu, 26 Apr 2012, Calin Demian wrote:
> > > 
> > > > I don't know how, but adding the following line in function
> > > > quirk_usb_handoff_ohci makes the system work fine:
> > > > 
> > > > 	/* reset requires max 10 us delay */
> > > > +	udelay(1);
> > > > 	for (cnt = 30; cnt > 0; --cnt) {	/* ... allow extra time */
> > > > 		if ((readl(base + OHCI_CMDSTATUS) & OHCI_HCR) == 0)
> > > > 
> > > > I observed thad the condition in the for loop is allways true, so the
> > > > delay inside the loop is never executed.
> > 
> > That implies that something is wrong with the complier here, and it
> > optimized out the loop?  Are you sure that really happened?
> 
> Here's the complete context:
> 
> 	/* reset requires max 10 us delay */
> 	for (cnt = 30; cnt > 0; --cnt) {	/* ... allow extra time */
> 		if ((readl(base + OHCI_CMDSTATUS) & OHCI_HCR) == 0)
> 			break;
> 		udelay(1);
> 	}
> 
> I think Calim means that the "(readl(base + OHCI_CMDSTATUS) & OHCI_HCR)  
> == 0" test succeeds on the first pass through the loop, so we break out
> early and the udelay(1) statement never gets executed.

Ok, but if the readl() succeeds, then all should be fine?

Ah, unless the hardware is really lying, but no, that would never
happen, right?

Nevermind,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Calin Demian - April 26, 2012, 3:49 p.m.
2012/4/26 Alan Stern <stern@rowland.harvard.edu>:
> On Thu, 26 Apr 2012, Greg KH wrote:
>
>> On Thu, Apr 26, 2012 at 10:25:41AM -0400, Alan Stern wrote:
>> > On Thu, 26 Apr 2012, Calin Demian wrote:
>> >
>> > > I don't know how, but adding the following line in function
>> > > quirk_usb_handoff_ohci makes the system work fine:
>> > >
>> > >   /* reset requires max 10 us delay */
>> > > + udelay(1);
>> > >   for (cnt = 30; cnt > 0; --cnt) {        /* ... allow extra time */
>> > >           if ((readl(base + OHCI_CMDSTATUS) & OHCI_HCR) == 0)
>> > >
>> > > I observed thad the condition in the for loop is allways true, so the
>> > > delay inside the loop is never executed.
>>
>> That implies that something is wrong with the complier here, and it
>> optimized out the loop?  Are you sure that really happened?
>
> Here's the complete context:
>
>        /* reset requires max 10 us delay */
>        for (cnt = 30; cnt > 0; --cnt) {        /* ... allow extra time */
>                if ((readl(base + OHCI_CMDSTATUS) & OHCI_HCR) == 0)
>                        break;
>                udelay(1);
>        }
>
> I think Calim means that the "(readl(base + OHCI_CMDSTATUS) & OHCI_HCR)
> == 0" test succeeds on the first pass through the loop, so we break out
> early and the udelay(1) statement never gets executed.
>
> Alan Stern
>

Yes, that's what I was saying. Storing the readl result in a variable
and displaying it later shows that the (readl_result & OHCI_HCR) is 0
at first pass, but that doesn't imply reset is finished, I think. It
would be too fast. Maybe the readl doesn't work for some reason...
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Calin Demian - April 26, 2012, 3:53 p.m.
I admit it might be a hardware problem. As I said in another mail, I
couldn't duplicate the problem on other computers...
If that's true than I'm sorry for bothering you.

2012/4/26 Greg KH <greg@kroah.com>:
> On Thu, Apr 26, 2012 at 11:16:17AM -0400, Alan Stern wrote:
>> On Thu, 26 Apr 2012, Greg KH wrote:
>>
>> > On Thu, Apr 26, 2012 at 10:25:41AM -0400, Alan Stern wrote:
>> > > On Thu, 26 Apr 2012, Calin Demian wrote:
>> > >
>> > > > I don't know how, but adding the following line in function
>> > > > quirk_usb_handoff_ohci makes the system work fine:
>> > > >
>> > > >         /* reset requires max 10 us delay */
>> > > > +       udelay(1);
>> > > >         for (cnt = 30; cnt > 0; --cnt) {        /* ... allow extra time */
>> > > >                 if ((readl(base + OHCI_CMDSTATUS) & OHCI_HCR) == 0)
>> > > >
>> > > > I observed thad the condition in the for loop is allways true, so the
>> > > > delay inside the loop is never executed.
>> >
>> > That implies that something is wrong with the complier here, and it
>> > optimized out the loop?  Are you sure that really happened?
>>
>> Here's the complete context:
>>
>>       /* reset requires max 10 us delay */
>>       for (cnt = 30; cnt > 0; --cnt) {        /* ... allow extra time */
>>               if ((readl(base + OHCI_CMDSTATUS) & OHCI_HCR) == 0)
>>                       break;
>>               udelay(1);
>>       }
>>
>> I think Calim means that the "(readl(base + OHCI_CMDSTATUS) & OHCI_HCR)
>> == 0" test succeeds on the first pass through the loop, so we break out
>> early and the udelay(1) statement never gets executed.
>
> Ok, but if the readl() succeeds, then all should be fine?
>
> Ah, unless the hardware is really lying, but no, that would never
> happen, right?
>
> Nevermind,
>
> greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Calin Demian - April 26, 2012, 3:59 p.m.
Yes, it's ok. Thank you.
Nevertheless, I think I will try to find out exactly what's happening.

2012/4/26 Alan Stern <stern@rowland.harvard.edu>:
> On Thu, 26 Apr 2012, Calin Demian wrote:
>
>> I don't know how, but adding the following line in function
>> quirk_usb_handoff_ohci makes the system work fine:
>>
>>       /* reset requires max 10 us delay */
>> +     udelay(1);
>>       for (cnt = 30; cnt > 0; --cnt) {        /* ... allow extra time */
>>               if ((readl(base + OHCI_CMDSTATUS) & OHCI_HCR) == 0)
>>
>> I observed thad the condition in the for loop is allways true, so the
>> delay inside the loop is never executed.
>> With the unmodified source, the execution doesn't stop at this point
>> but at the next readl (I tried inserting printk and readl instructions
>> after the writel which restores the fminterval and the execution
>> stopped before exiting quirk_usb_handoff_ohci).
>> I don't know how it relates to the ACPI message... (see bug 43073 in
>> bugzilla.kernel.org).
>
> I don't know just what the problem is either, but there's nothing wrong
> with delaying startup by 1 us.  In fact, all we have to do is reorder
> the statements within the loop.  Is this patch okay for you?
>
> Alan Stern
>
>
>
> Index: usb-3.4/drivers/usb/host/pci-quirks.c
> ===================================================================
> --- usb-3.4.orig/drivers/usb/host/pci-quirks.c
> +++ usb-3.4/drivers/usb/host/pci-quirks.c
> @@ -520,9 +520,10 @@ static void __devinit quirk_usb_handoff_
>
>        /* reset requires max 10 us delay */
>        for (cnt = 30; cnt > 0; --cnt) {        /* ... allow extra time */
> +               /* Some controllers enable HCR too soon, so always delay 1 us */
> +               udelay(1);
>                if ((readl(base + OHCI_CMDSTATUS) & OHCI_HCR) == 0)
>                        break;
> -               udelay(1);
>        }
>        writel(fminterval, base + OHCI_FMINTERVAL);
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Stern - April 26, 2012, 5:05 p.m.
On Thu, 26 Apr 2012, Calin Demian wrote:

> Yes, it's ok. Thank you.
> Nevertheless, I think I will try to find out exactly what's happening.

Okay.  It may be that your hardware never turns on the HCR bit in the 
first place.

Remember, there will be no problem merging that patch, if it helps your
system.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Calin Demian - April 26, 2012, 5:50 p.m.
Ok, then. In my case it solves the problem.
Thank you again.

Calin Demian

2012/4/26 Alan Stern <stern@rowland.harvard.edu>:
> On Thu, 26 Apr 2012, Calin Demian wrote:
>
>> Yes, it's ok. Thank you.
>> Nevertheless, I think I will try to find out exactly what's happening.
>
> Okay.  It may be that your hardware never turns on the HCR bit in the
> first place.
>
> Remember, there will be no problem merging that patch, if it helps your
> system.
>
> Alan Stern
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

Index: usb-3.4/drivers/usb/host/pci-quirks.c
===================================================================
--- usb-3.4.orig/drivers/usb/host/pci-quirks.c
+++ usb-3.4/drivers/usb/host/pci-quirks.c
@@ -520,9 +520,10 @@  static void __devinit quirk_usb_handoff_
 
 	/* reset requires max 10 us delay */
 	for (cnt = 30; cnt > 0; --cnt) {	/* ... allow extra time */
+		/* Some controllers enable HCR too soon, so always delay 1 us */
+		udelay(1);
 		if ((readl(base + OHCI_CMDSTATUS) & OHCI_HCR) == 0)
 			break;
-		udelay(1);
 	}
 	writel(fminterval, base + OHCI_FMINTERVAL);