Re: [Qemu-devel] [PATCH 04/12] usb: Add support for input pipelining

[Gerd Hoffmann <kraxel@redhat.com>]

  Hi,

> One possible alternative to re-combining would be to pass the
> short-not-ok flag along the chain all the way down into usbfs,

Yes, this is what I have in mind.

> but
> that won't work since the USBFS_URB_SHORT_NOT_OK flag does not
> work with XHCI controllers!

Oops.

> So lets try to split this discussion into multiple questions:
> 
> 1) Can we agree that re-combining input packets back into their original
> transfers as done at the guest device driver level, is useful and given
> the serial <-> usb converters case, even necessary to have ?

Given that the USBFS_URB_SHORT_NOT_OK doesn't work there is no way
around that I guess.

> 2) If we agree on 1), what is the right place to do them ?
> 
> To be able to properly combine packets into one larger transfer, an
> uncombine them again later, some coordination is needed between the
> entity doing the combining / uncombining and the hcd code:
> 
> 2a) The combining entity needs to know when the hcd is done with
> queuing up packets for an ep.

Yes.  We can add a USBDeviceClass method for that.

> 2b) When uncombining it is possible that less packets are actually
> used / filled with data, then were combined into one transfer. When
> this happens the hcd code will stop processing the queue after the
> short packet, and wait for the guest to restart it. When the guest
> restarts the queue it will be modified and the un-used packets will
> be gone from it. Since the hcd code keeps its own queue, when
> when uncombining the hcd code needs to be told to forget about
> the unused packets.

Yes.  We can add a new USBPortOps for that, or reuse
USBPortOps->complete with USBPacket->result == USB_RET_NOT_USED.

> Note that currently we have the same problem on a halt of the ep
> queue already, and currently we expect the hcd to completely empty
> the queue on a stall. I'm planning to change this code path over
> to also let the core tell the hcd to forget about the packets it
> had queued beyond the stall, as this seems something to do at the
> core level rather then reproducing it in every hcd emulation
> separately.

The core can and should do that for packets it owns (USBPacketState ==
USB_PACKET_QUEUED) because they are not (yet) passed to the USBDevice.

Packets owned by USBDevice (USBPacketState == USB_PACKET_ASYNC) must be
handled by the USBDevice itself.

> Given the necessary close interaction between the hcd code and
> the combine / uncombine code + not wanting to reproduce the
> same code in both host-linux.c and redirect.c I believe that
> the usb core is the right place for this.

I think the combining should happen just before submitting the transfer
to usbfs, in usb-host.  I'd like usb-host see what is really going on,
and the usb core not hiding this by magically creating jumbo packets.

Likewise I think usbredir should do the combining on the *server* side,
not in the qemu redir code.

>> Maybe add a callback to notify USBDevice
>> when the host controller is done filling the queue, so USBDevices can
>> process all queued packets in one go (bottom half would work too
>> though).
> 
> Changing the callback which I added for this to a bh is fine with me,
> I think that both from a making clear what is going on when reading
> the code, and from a having all the data hot in cache pov, the callback
> is better though.

Yes, callback is probably better, that is just a little minor detail though.

cheers,
  Gerd