Patchwork Bug #757654: UHCI fails to signal stall response patch

login
register
mail settings
Submitter Jan Vesely
Date April 15, 2011, 8:57 p.m.
Message ID <BANLkTik3hmvH6_tUuEZjR8j0L04PdCk7mw@mail.gmail.com>
Download mbox | patch
Permalink /patch/91440/
State New
Headers show

Comments

Jan Vesely - April 15, 2011, 8:57 p.m.
Hi,

I'm sending a patch for bug #757654. The bug does not really break
anything it just makes USB error detection harder.
It's a quick fix and might need some polishing but it works (I am
currently using it).

thx,
jan

PS: I guess you need this line:
Signed-off-by: Jan Vesely <jano.vesely@gmail.com>
Brad Hards - April 16, 2011, 6:33 a.m.
On Sat, 16 Apr 2011 06:57:00 am Jan Vesely wrote:
> +        s->status |= UHCI_STS_USBERR;
This is per UHCI 1.1D Section 4.1.5. Looks good.

> +        *int_mask |= 0x02;
> +        if (td->ctrl & TD_CTRL_IOC)
> +            *int_mask |= 0x01;
> +        uhci_update_irq(s);
I see "A hardware interrupt is signalled to the system", but can you provide a 
little explanation of why this particular interrupt mask?

> +        s->status |= UHCI_STS_USBERR;
This is per UHCI 1.1d Section 4.1.4. Looks good.

> +        *int_mask |= 0x02;
> +        if (td->ctrl & TD_CTRL_IOC)
> +           *int_mask |= 0x01;
> +        uhci_update_irq(s);
I see "A hardware interrupt is signalled to the system", but can you provide a 
little explanation of why this particular interrupt mask?
Jan Vesely - April 16, 2011, 7:29 a.m.
On Sat, Apr 16, 2011 at 8:33 AM, Brad Hards <bradh@frogmouth.net> wrote:
> On Sat, 16 Apr 2011 06:57:00 am Jan Vesely wrote:
>> +        s->status |= UHCI_STS_USBERR;
> This is per UHCI 1.1D Section 4.1.5. Looks good.
>
>> +        *int_mask |= 0x02;
>> +        if (td->ctrl & TD_CTRL_IOC)
>> +            *int_mask |= 0x01;
>> +        uhci_update_irq(s);
> I see "A hardware interrupt is signalled to the system", but can you provide a
> little explanation of why this particular interrupt mask?

I used th code I found around in that same file (hw/usb-uhci.c),
lines 705-724 contain both masks. "if (td->ctrl & TD_CTRL_IOC)
*int_mask |= 0x01;", is in more places so I just copied that lines.
*int_mask |= 0x2, is used when SPD condition is detected.
that is strange, SPD should use the same interrupt as IOC, but return
value indicates that it is treated as error condition (unsuccessful
td) so I figured *int_mask |= 0x2 signals error interrupt (it does not
match bits in interrupt enable register- that was my first guess)
uhci_update_irq(s); to me it looks like a duplicate functionality to
int_mask parameter, I did not investigate further and included it just
to be sure (it's used on line 775, when error countdown reaches zero).

>
>> +        s->status |= UHCI_STS_USBERR;
> This is per UHCI 1.1d Section 4.1.4. Looks good.
>
>> +        *int_mask |= 0x02;
>> +        if (td->ctrl & TD_CTRL_IOC)
>> +           *int_mask |= 0x01;
>> +        uhci_update_irq(s);
> I see "A hardware interrupt is signalled to the system", but can you provide a
> little explanation of why this particular interrupt mask?
>
>
Stefan Hajnoczi - April 16, 2011, 8:40 a.m.
On Fri, Apr 15, 2011 at 9:57 PM, Jan Vesely <jano.vesely@gmail.com> wrote:
> I'm sending a patch for bug #757654. The bug does not really break
> anything it just makes USB error detection harder.
> It's a quick fix and might need some polishing but it works (I am
> currently using it).
>
> thx,
> jan
>
> PS: I guess you need this line:
> Signed-off-by: Jan Vesely <jano.vesely@gmail.com>

Not trivial.  CCing Gerd for USB review instead.

Stefan
Gerd Hoffmann - May 5, 2011, 12:05 p.m.
> diff --git a/hw/usb-uhci.c b/hw/usb-uhci.c
> index 346db3e..a51d89b 100644
> --- a/hw/usb-uhci.c
> +++ b/hw/usb-uhci.c
> @@ -732,11 +732,21 @@ out:
>       case USB_RET_STALL:
>           td->ctrl |= TD_CTRL_STALL;
>           td->ctrl&= ~TD_CTRL_ACTIVE;
> +        s->status |= UHCI_STS_USBERR;

Just this line should be enougth.

>       case USB_RET_BABBLE:
>           td->ctrl |= TD_CTRL_BABBLE | TD_CTRL_STALL;
>           td->ctrl&= ~TD_CTRL_ACTIVE;
> +        s->status |= UHCI_STS_USBERR;

Likewise.

Tried that?

cheers,
   Gerd

Patch

diff --git a/hw/usb-uhci.c b/hw/usb-uhci.c
index 346db3e..a51d89b 100644
--- a/hw/usb-uhci.c
+++ b/hw/usb-uhci.c
@@ -732,11 +732,21 @@  out:
     case USB_RET_STALL:
         td->ctrl |= TD_CTRL_STALL;
         td->ctrl &= ~TD_CTRL_ACTIVE;
+        s->status |= UHCI_STS_USBERR;
+        *int_mask |= 0x02;
+        if (td->ctrl & TD_CTRL_IOC)
+            *int_mask |= 0x01;
+        uhci_update_irq(s);
         return 1;

     case USB_RET_BABBLE:
         td->ctrl |= TD_CTRL_BABBLE | TD_CTRL_STALL;
         td->ctrl &= ~TD_CTRL_ACTIVE;
+        s->status |= UHCI_STS_USBERR;
+        *int_mask |= 0x02;
+        if (td->ctrl & TD_CTRL_IOC)
+           *int_mask |= 0x01;
+        uhci_update_irq(s);
         /* frame interrupted */
         return -1;