Re: [PATCH 2/2] drm/gud: Use scatter-gather USB bulk transfer

["Noralf Trønnes" <noralf@tronnes.org>]

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
>