Den 14.06.2021 22.54, skrev Linus Walleij:
> Hi Noralf,
>
> On Mon, Mar 29, 2021 at 8:01 PM Noralf Trønnes <noralf@xxxxxxxxxxx> wrote:
>
>> There'a limit to how big a kmalloc buffer can be, and as memory gets
>> fragmented it becomes more difficult to get big buffers. The downside of
>> smaller buffers is that the driver has to split the transfer up which
>> hampers performance. Compression might also take a hit because of the
>> splitting.
>>
>> Solve this by allocating the transfer buffer using vmalloc and create a
>> SG table to be passed on to the USB subsystem. vmalloc_32() is used to
>> avoid DMA bounce buffers on USB controllers that can only access 32-bit
>> addresses.
>>
>> This also solves the problem that split transfers can give host side
>> tearing since flushing is decoupled from rendering.
>>
>> Signed-off-by: Noralf Trønnes <noralf@xxxxxxxxxxx>
>
>> + num_pages = PAGE_ALIGN(gdrm->bulk_len) >> PAGE_SHIFT;
>
> Isn't it the same to write:
>
> num_pages = round_up(gdrm->bulk_len, PAGE_SIZE)?
>
> Slightly easier to read IMO.
>
Yes it's the same, I just copied this from elsewhere in the kernel where
a vmalloc buffer is turned into an sg list. I can change that.
>> + if (max_buffer_size > SZ_64M)
>> + max_buffer_size = SZ_64M; /* safeguard */
>
> Explain this choice of max buffer in the commit message
> or as a comment please because I don't get why this size
> is the roof.
>
>> +struct gud_usb_bulk_context {
>> + struct timer_list timer;
>> + struct usb_sg_request sgr;
>> +};
>> +
>> +static void gud_usb_bulk_timeout(struct timer_list *t)
>> +{
>> + struct gud_usb_bulk_context *timer = from_timer(timer, t, timer);
>> +
>> + usb_sg_cancel(&timer->sgr);
>
> Error message here? "Timeout on sg bulk transfer".
>
A timeout is detected in gud_usb_bulk() which will return -ETIMEDOUT if
the timer did fire. gud_flush_work() will print an error message.
>> +}
>> +
>> +static int gud_usb_bulk(struct gud_device *gdrm, size_t len)
>> +{
>> + struct gud_usb_bulk_context ctx;
>> + int ret;
>> +
>> + ret = usb_sg_init(&ctx.sgr, gud_to_usb_device(gdrm), gdrm->bulk_pipe, 0,
>> + gdrm->bulk_sgt.sgl, gdrm->bulk_sgt.nents, len, GFP_KERNEL);
>> + if (ret)
>> + return ret;
>> +
>> + 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;
>> + else if (ctx.sgr.status < 0)
>> + ret = ctx.sgr.status;
>> + else if (ctx.sgr.bytes != len)
>> + ret = -EIO;
>> +
>> + destroy_timer_on_stack(&ctx.timer);
>> +
>> + return ret;
>> +}
>
> Mention in the commit message that sending USB bulk transfers with
> an sglist could be unstable so you set up a timeout around
> usb_sg_wait() (did this happen to you? then write that)
>
> The other users of usb_sg_wait() in the kernel do not have these
> timeout wrappers, I suspect the reasoning is something like
> "it's graphics, not storage, so if we timeout and lose an update,
> too bad but let's just continue hoping the lost graphics will be
> less than noticeable" so then we should write that as a comment
> about that in the code or something.
>
There are 5 users of usb_sg_wait() in the kernel:
drivers/input/touchscreen/sur40.c
drivers/misc/cardreader/rtsx_usb.c
drivers/mmc/host/vub300.c
drivers/usb/misc/usbtest.c
drivers/usb/storage/transport.c
3 of those wrap it in a timer:
drivers/misc/cardreader/rtsx_usb.c: rtsx_usb_bulk_transfer_sglist()
drivers/mmc/host/vub300.c: __command_write_data()
drivers/usb/misc/usbtest.c: perform_sglist()
And it looks to me like usb/storage has some timeout handling through
the scsi layer:
/drivers/usb/storage/scsiglue.c: command_abort() ->
usb_stor_stop_transport() -> usb_sg_cancel()
This leaves 1 out of 5 users without timeout handling?
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.
I can add a comment in the commit message about the timer.
Noralf.
> With these comments fixed up:
> Reviewed-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
>
> Yours,
> Linus Walleij
>
