Den 14.06.2021 22.54, skrev Linus Walleij:
Hi Noralf,
On Mon, Mar 29, 2021 at 8:01 PM Noralf Trønnes noralf@tronnes.org 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@tronnes.org
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@linaro.org
Yours, Linus Walleij