diff mbox

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

Message ID 1316022540-31355-2-git-send-email-peter.maydell@linaro.org
State Accepted
Commit 905fb0342ca8fc480dd3e08f8a7aaabc339da796
Headers show

Commit Message

Peter Maydell Sept. 14, 2011, 5:48 p.m. UTC
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(-)

Comments

Gerd Hoffmann Sept. 15, 2011, 7:33 a.m. UTC | #1
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
Peter Maydell Sept. 15, 2011, 8:36 a.m. UTC | #2
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
Gerd Hoffmann Sept. 15, 2011, 9:13 a.m. UTC | #3
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
Peter Maydell Sept. 15, 2011, 10:08 a.m. UTC | #4
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
Gerd Hoffmann Sept. 15, 2011, 10:49 a.m. UTC | #5
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
Peter Maydell Sept. 15, 2011, 11:24 a.m. UTC | #6
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
Gerd Hoffmann Sept. 15, 2011, 11:45 a.m. UTC | #7
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
diff mbox

Patch

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;
 }