[Qemu-devel] [PATCH 0/2] usb-ohci: fixes to allow usb-net to work

[Qemu-devel] [PATCH 0/2] usb-ohci: fixes to allow usb-net to work

[Peter Maydell]

This patchset contains a couple of fixes to usb-ohci which I had to make
in the course of getting usb-net to work on the Beagle (which uses OHCI
rather than UHCI). The main one is making the usb-ohci controller model
honour the endpoint maximum packet size setting -- usb-net.c will just
ignore any attempt by the controller to give it a packet larger than
64 bytes. The fix to OHCI_TD_T1 is probably not strictly required but
I noticed it in passing.

Peter Maydell (2):
  hw/usb-ohci: Honour endpoint maximum packet size
  hw/usb-ohci: Fix OHCI_TD_T1 bit position definition

 hw/usb-ohci.c |   39 ++++++++++++++++++++++++++++-----------
 1 files changed, 28 insertions(+), 11 deletions(-)

[Qemu-devel] [PATCH 1/2] hw/usb-ohci: Honour endpoint maximum packet size

[Peter Maydell]

Honour the maximum packet size for endpoints; this applies when
sending non-isochronous data and means we transfer only as
much as the endpoint allows, leaving the transfer descriptor
on the list for another go next time around. This allows
usb-net to work when connected to an OHCI controller model.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/usb-ohci.c |   37 +++++++++++++++++++++++++++----------
 1 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/hw/usb-ohci.c b/hw/usb-ohci.c
index 503ca2d..7487188 100644
--- a/hw/usb-ohci.c
+++ b/hw/usb-ohci.c
@@ -872,7 +872,7 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
 static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
 {
     int dir;
-    size_t len = 0;
+    size_t len = 0, pktlen = 0;
 #ifdef DEBUG_PACKET
     const char *str = NULL;
 #endif
@@ -940,20 +940,30 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
             len = (td.be - td.cbp) + 1;
         }
 
-        if (len && dir != OHCI_TD_DIR_IN && !completion) {
-            ohci_copy_td(ohci, &td, ohci->usb_buf, len, 0);
+        pktlen = len;
+        if (len && dir != OHCI_TD_DIR_IN) {
+            /* The endpoint may not allow us to transfer it all now */
+            pktlen = (ed->flags & OHCI_ED_MPS_MASK) >> OHCI_ED_MPS_SHIFT;
+            if (pktlen > len) {
+                pktlen = len;
+            }
+            if (!completion) {
+                ohci_copy_td(ohci, &td, ohci->usb_buf, pktlen, 0);
+            }
         }
     }
 
     flag_r = (td.flags & OHCI_TD_R) != 0;
 #ifdef DEBUG_PACKET
-    DPRINTF(" TD @ 0x%.8x %" PRId64 " bytes %s r=%d cbp=0x%.8x be=0x%.8x\n",
-            addr, (int64_t)len, str, flag_r, td.cbp, td.be);
+    DPRINTF(" TD @ 0x%.8x %" PRId64 " of %" PRId64
+            " bytes %s r=%d cbp=0x%.8x be=0x%.8x\n",
+            addr, (int64_t)pktlen, (int64_t)len, str, flag_r, td.cbp, td.be);
 
-    if (len > 0 && dir != OHCI_TD_DIR_IN) {
+    if (pktlen > 0 && dir != OHCI_TD_DIR_IN) {
         DPRINTF("  data:");
-        for (i = 0; i < len; i++)
+        for (i = 0; i < pktlen; i++) {
             printf(" %.2x", ohci->usb_buf[i]);
+        }
         DPRINTF("\n");
     }
 #endif
@@ -982,7 +992,7 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
             usb_packet_setup(&ohci->usb_packet, pid,
                              OHCI_BM(ed->flags, ED_FA),
                              OHCI_BM(ed->flags, ED_EN));
-            usb_packet_addbuf(&ohci->usb_packet, ohci->usb_buf, len);
+            usb_packet_addbuf(&ohci->usb_packet, ohci->usb_buf, pktlen);
             ret = usb_handle_packet(dev, &ohci->usb_packet);
             if (ret != USB_RET_NODEV)
                 break;
@@ -1005,12 +1015,12 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
             DPRINTF("\n");
 #endif
         } else {
-            ret = len;
+            ret = pktlen;
         }
     }
 
     /* Writeback */
-    if (ret == len || (dir == OHCI_TD_DIR_IN && ret >= 0 && flag_r)) {
+    if (ret == pktlen || (dir == OHCI_TD_DIR_IN && ret >= 0 && flag_r)) {
         /* Transmission succeeded.  */
         if (ret == len) {
             td.cbp = 0;
@@ -1026,6 +1036,12 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
         OHCI_SET_BM(td.flags, TD_CC, OHCI_CC_NOERROR);
         OHCI_SET_BM(td.flags, TD_EC, 0);
 
+        if ((dir != OHCI_TD_DIR_IN) && (ret != len)) {
+            /* Partial packet transfer: TD not ready to retire yet */
+            goto exit_no_retire;
+        }
+
+        /* Setting ED_C is part of the TD retirement process */
         ed->head &= ~OHCI_ED_C;
         if (td.flags & OHCI_TD_T0)
             ed->head |= OHCI_ED_C;
@@ -1066,6 +1082,7 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
     i = OHCI_BM(td.flags, TD_DI);
     if (i < ohci->done_count)
         ohci->done_count = i;
+exit_no_retire:
     ohci_put_td(ohci, addr, &td);
     return OHCI_BM(td.flags, TD_CC) != OHCI_CC_NOERROR;
 }
-- 
1.7.1

Re: [Qemu-devel] [PATCH 1/2] hw/usb-ohci: Honour endpoint maximum packet size

[Gerd Hoffmann]

On 09/14/11 19:48, Peter Maydell wrote:
> Honour the maximum packet size for endpoints; this applies when
> sending non-isochronous data and means we transfer only as
> much as the endpoint allows, leaving the transfer descriptor
> on the list for another go next time around. This allows
> usb-net to work when connected to an OHCI controller model.

Hmm, I'd tend to fix it the other way around:  Fix usb-net to deal with 
transfers larger than the endpoint packet size.  What do you think?

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 1/2] hw/usb-ohci: Honour endpoint maximum packet size

[Peter Maydell]

On 15 September 2011 08:33, Gerd Hoffmann <kraxel@redhat.com> wrote:
> On 09/14/11 19:48, Peter Maydell wrote:
>>
>> Honour the maximum packet size for endpoints; this applies when
>> sending non-isochronous data and means we transfer only as
>> much as the endpoint allows, leaving the transfer descriptor
>> on the list for another go next time around. This allows
>> usb-net to work when connected to an OHCI controller model.
>
> Hmm, I'd tend to fix it the other way around:  Fix usb-net to deal with
> transfers larger than the endpoint packet size.  What do you think?

Honouring maximum packet size is mandated by the OHCI spec:
see section 4.3.1.3.2 "Packet Size" in OHCI specification 1.0a.
Our failure to do it is just a bug in our controller model.

If you want to make our usb-net implementation permit and
advertise a larger max-packet-size (and test it for
interoperability with a pile of OSes :-)) that's a different
thing.

-- PMM

Re: [Qemu-devel] [PATCH 1/2] hw/usb-ohci: Honour endpoint maximum packet size

[Gerd Hoffmann]

On 09/15/11 10:36, Peter Maydell wrote:
> On 15 September 2011 08:33, Gerd Hoffmann<kraxel@redhat.com>  wrote:
>> On 09/14/11 19:48, Peter Maydell wrote:
>>>
>>> Honour the maximum packet size for endpoints; this applies when
>>> sending non-isochronous data and means we transfer only as
>>> much as the endpoint allows, leaving the transfer descriptor
>>> on the list for another go next time around. This allows
>>> usb-net to work when connected to an OHCI controller model.
>>
>> Hmm, I'd tend to fix it the other way around:  Fix usb-net to deal with
>> transfers larger than the endpoint packet size.  What do you think?
>
> Honouring maximum packet size is mandated by the OHCI spec:
> see section 4.3.1.3.2 "Packet Size" in OHCI specification 1.0a.
> Our failure to do it is just a bug in our controller model.

No.  What I think is that USBPacket shouldn't be required to be an 
actual USB packet, but a transfer, i.e. do the splitting of larger 
transfers into smaller packets in the usb driver emulation (if needed), 
not the host adapter emulation.

Maybe it is a good idea to rename USBPacket to USBTransfer to make that 
clear.

 > If you want to make our usb-net implementation permit and
 > advertise a larger max-packet-size (and test it for
 > interoperability with a pile of OSes :-)) that's a different
 > thing.

Also add usb 2.0 support while being at it.
But that is another story ...

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 1/2] hw/usb-ohci: Honour endpoint maximum packet size

[Peter Maydell]

On 15 September 2011 10:13, Gerd Hoffmann <kraxel@redhat.com> wrote:
> On 09/15/11 10:36, Peter Maydell wrote:
>>
>> On 15 September 2011 08:33, Gerd Hoffmann<kraxel@redhat.com>  wrote:
>>>
>>> On 09/14/11 19:48, Peter Maydell wrote:
>>>>
>>>> Honour the maximum packet size for endpoints; this applies when
>>>> sending non-isochronous data and means we transfer only as
>>>> much as the endpoint allows, leaving the transfer descriptor
>>>> on the list for another go next time around. This allows
>>>> usb-net to work when connected to an OHCI controller model.
>>>
>>> Hmm, I'd tend to fix it the other way around:  Fix usb-net to deal with
>>> transfers larger than the endpoint packet size.  What do you think?
>>
>> Honouring maximum packet size is mandated by the OHCI spec:
>> see section 4.3.1.3.2 "Packet Size" in OHCI specification 1.0a.
>> Our failure to do it is just a bug in our controller model.
>
> No.  What I think is that USBPacket shouldn't be required to be an actual
> USB packet, but a transfer, i.e. do the splitting of larger transfers into
> smaller packets in the usb driver emulation (if needed), not the host
> adapter emulation.

The OHCI spec still requires us to only process one max-packet-size
worth of data from this TransferDescriptor before we move on to
the next one, though, doesn't it? (cf the flowchart in fig 6-7).
It seems to me that at least some of that is likely to be
guest-visible, especially in the case where an endpoint returns an
error partway through. So it's not clear to me that you could
validly batch up everything in the TD and do it all at once.

-- PMM

Re: [Qemu-devel] [PATCH 1/2] hw/usb-ohci: Honour endpoint maximum packet size

[Gerd Hoffmann]

   Hi,

>> No.  What I think is that USBPacket shouldn't be required to be an actual
>> USB packet, but a transfer, i.e. do the splitting of larger transfers into
>> smaller packets in the usb driver emulation (if needed), not the host
>> adapter emulation.
>
> The OHCI spec still requires us to only process one max-packet-size
> worth of data from this TransferDescriptor before we move on to
> the next one, though, doesn't it? (cf the flowchart in fig 6-7).

[ didn't look at specs ]

Yes, EHCI has a simliar rule, on real hardware it is needed to ensure 
the available USB bandwidth is fairly splitted across all devices by 
serving them in a robin-round fashion.

I don't think this makes much sense for emulation as it behaves very 
different compared to real hardware.  There simply is no bus with 
limited bandwidth we have to care about.

> It seems to me that at least some of that is likely to be
> guest-visible, especially in the case where an endpoint returns an
> error partway through. So it's not clear to me that you could
> validly batch up everything in the TD and do it all at once.

I think the only guest-visible effect is that the guest will never ever 
see an partially filled transfer block, it is either empty or completely 
filled (except on short transfers).

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 1/2] hw/usb-ohci: Honour endpoint maximum packet size

[Peter Maydell]

On 15 September 2011 11:49, Gerd Hoffmann <kraxel@redhat.com> wrote:
>Peter Maydell wrote:
>> It seems to me that at least some of that is likely to be
>> guest-visible, especially in the case where an endpoint returns an
>> error partway through. So it's not clear to me that you could
>> validly batch up everything in the TD and do it all at once.
>
> I think the only guest-visible effect is that the guest will never ever see
> an partially filled transfer block, it is either empty or completely filled
> (except on short transfers).

Hmm, maybe. Anyway, can we have this committed as a bug fix
for the current design pending you redoing this with your
new approach, please?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/2] hw/usb-ohci: Honour endpoint maximum packet size

[Gerd Hoffmann]

On 09/15/11 13:24, Peter Maydell wrote:
> On 15 September 2011 11:49, Gerd Hoffmann<kraxel@redhat.com>  wrote:
>> Peter Maydell wrote:
>>> It seems to me that at least some of that is likely to be
>>> guest-visible, especially in the case where an endpoint returns an
>>> error partway through. So it's not clear to me that you could
>>> validly batch up everything in the TD and do it all at once.
>>
>> I think the only guest-visible effect is that the guest will never ever see
>> an partially filled transfer block, it is either empty or completely filled
>> (except on short transfers).
>
> Hmm, maybe. Anyway, can we have this committed as a bug fix
> for the current design pending you redoing this with your
> new approach, please?

Fair enouth.  Added to the patch queue.

cheers,
   Gerd

[Qemu-devel] [PATCH 2/2] hw/usb-ohci: Fix OHCI_TD_T1 bit position definition

[Peter Maydell]

The OHCI Transfer Descriptor T (DataToggle) bits are 24 and 25;
fix an error which accidentally overlaid them both on the same bit.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/usb-ohci.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/usb-ohci.c b/hw/usb-ohci.c
index 7487188..d314743 100644
--- a/hw/usb-ohci.c
+++ b/hw/usb-ohci.c
@@ -150,7 +150,7 @@ static void ohci_async_cancel_device(OHCIState *ohci, USBDevice *dev);
 #define OHCI_TD_DI_SHIFT  21
 #define OHCI_TD_DI_MASK   (7<<OHCI_TD_DI_SHIFT)
 #define OHCI_TD_T0        (1<<24)
-#define OHCI_TD_T1        (1<<24)
+#define OHCI_TD_T1        (1<<25)
 #define OHCI_TD_EC_SHIFT  26
 #define OHCI_TD_EC_MASK   (3<<OHCI_TD_EC_SHIFT)
 #define OHCI_TD_CC_SHIFT  28
-- 
1.7.1

Re: [Qemu-devel] [PATCH 2/2] hw/usb-ohci: Fix OHCI_TD_T1 bit position definition

[Gerd Hoffmann]

On 09/14/11 19:49, Peter Maydell wrote:
> The OHCI Transfer Descriptor T (DataToggle) bits are 24 and 25;
> fix an error which accidentally overlaid them both on the same bit.
>
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>

Patch added to usb patch queue.

thanks,
   Gerd

Re: [Qemu-devel] [PATCH 0/2] usb-ohci: fixes to allow usb-net to work

[Peter Maydell]

On 14 September 2011 18:48, Peter Maydell <peter.maydell@linaro.org> wrote:
> This patchset contains a couple of fixes to usb-ohci which I had to make
> in the course of getting usb-net to work on the Beagle (which uses OHCI
> rather than UHCI). The main one is making the usb-ohci controller model
> honour the endpoint maximum packet size setting -- usb-net.c will just
> ignore any attempt by the controller to give it a packet larger than
> 64 bytes. The fix to OHCI_TD_T1 is probably not strictly required but
> I noticed it in passing.
>
> Peter Maydell (2):
>  hw/usb-ohci: Honour endpoint maximum packet size
>  hw/usb-ohci: Fix OHCI_TD_T1 bit position definition

Ping. Gerd, I got acks from you for both of these patches but they
don't seem to have made it into master yet?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/2] usb-ohci: fixes to allow usb-net to work

[Gerd Hoffmann]

On 10/10/11 18:44, Peter Maydell wrote:
> On 14 September 2011 18:48, Peter Maydell<peter.maydell@linaro.org>  wrote:
>> This patchset contains a couple of fixes to usb-ohci which I had to make
>> in the course of getting usb-net to work on the Beagle (which uses OHCI
>> rather than UHCI). The main one is making the usb-ohci controller model
>> honour the endpoint maximum packet size setting -- usb-net.c will just
>> ignore any attempt by the controller to give it a packet larger than
>> 64 bytes. The fix to OHCI_TD_T1 is probably not strictly required but
>> I noticed it in passing.
>>
>> Peter Maydell (2):
>>   hw/usb-ohci: Honour endpoint maximum packet size
>>   hw/usb-ohci: Fix OHCI_TD_T1 bit position definition
>
> Ping. Gerd, I got acks from you for both of these patches but they
> don't seem to have made it into master yet?

Was on vacation two weeks which delayed things a bit (and I'm busy with 
the backlog now ...)

The patches are sitting in my local queue and will go upstream with the 
next usb pull request.

cheers,
   Gerd