| Message ID | 1316022540-31355-2-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Accepted |
| Commit | 905fb0342ca8fc480dd3e08f8a7aaabc339da796 |
| Headers | show |
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
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
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
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
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
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
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 --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; }
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(-)