Den 15.06.2021 11.17, skrev Peter Stuge:
Hi Noralf,
Noralf Trønnes wrote:
+static int gud_usb_bulk(struct gud_device *gdrm, size_t len)
..
timer_setup_on_stack(&ctx.timer, gud_usb_bulk_timeout, 0);mod_timer(&ctx.timer, jiffies + msecs_to_jiffies(3000));usb_sg_wait(&ctx.sgr);if (!del_timer_sync(&ctx.timer))ret = -ETIMEDOUT;..
Mention in the commit message that sending USB bulk transfers with an sglist could be unstable
Can you explain a bit about /how/ it is unstable?
As you write, usb_bulk_msg() (as used before) has a timeout which is passed to the host controller hardware and implemented there.
I haven't used SG with kernel USB but I would expect such a timeout to still be available with SG?
I have taken a closer look and usb_bulk_msg() calls usb_start_wait_urb() which uses wait_for_completion_timeout() so the timeout isn't handled by the hardware.
usb_sg_wait() on the other hand uses plain wait_for_completion() without the timeout. So ideally usb_sg_wait() should have had a timeout parameter and used wait_for_completion_timeout().
usb_bulk_msg() has builtin timeout handling and during development of a microcontroller gadget implementation I've triggered this timeout several times when the uC usb interrupts stopped firing.
The device not responding to bulk packets scheduled and sent by the host is a real error /in the device/ and thus not neccessarily something the kernel must handle gracefully.. I think it's quite nice to do so, but one can argue that it's not strictly required.
But more importantly: Remember that bulk transfer has no delivery time guarantee. It can take indefinitely long until a bulk transfer is scheduled by the host on a busy bus which is starved with more important things (control, interrupt, iso transfers) - that's not an error at all, and may be indistinguishable from the device not responding to packets actually sent by the host.
Having a timeout is important, I just expect the USB SG interface to support it since it is the hardware that times out in the non-SG case.
And since this is essentially real time data maybe a shorter timeout is better? 3 seconds seems really long.
I have looked at what the others are using: - rtsx_usb uses 10 seconds in one place - vub300 uses 2 seconds plus a length based addition - usbtest uses 10 seconds.
The other USB DRM driver gm12u320 uses a 1 second timeout per block, so a worst case timeout of 20 seconds per frame.
3 seconds is a "long time", but compared to the default control request timeout USB_CTRL_GET_TIMEOUT which is 5 seconds, and which gud uses, it's not that long. I don't want to put too much limitation on the device, but ofc can't allow it to hang the driver.
And a timeout is an exception so hitting that probably means something is seriously wrong. I though of adding some kind of usb bus reset handling to the driver that kicks in after the device has been unresponsive for some time, but dropped that since I have so limited understanding of things USB.
Noralf.
The timeout must include all latency for a frame, so e.g. 16ms (60 Hz) is too short for sure. But maybe something like 500ms?
//Peter