xairy / raw-gadget

USB Raw Gadget — a low-level interface for the Linux USB Gadget subsystem
333 stars 35 forks source link

USB_RAW_IOCTL_EP_READ/USB_RAW_IOCTL_EP_WRITE maximum length = PAGE_SIZE #66

Open tw39124-1 opened 4 months ago

tw39124-1 commented 4 months ago

Firstly thanks for raw-gadget, it's awesome that I'm able to simulate USB gadgets in userspace. I have a question about maximum IO sizes, which might be due to my misunderstanding of how the linux USB subsystems work. I could determine the answer through experimentation but figured I'd open an issue to provide an answer for others who might have the same question.

The maximum IO size that can be given to either the USB_RAW_IOCTL_EP_READ or USB_RAW_IOCTL_EP_WRITE ioctl appears to be limited by PAGE_SIZE i.e. 4KB. If given a larger IO length then raw_alloc_io_data returns -EINVAL:

if io->length > PAGE_SIZE
    return ERR_PTR(-EINVAL);

If the host submits a URB for a larger transfer size, is the data buffered such that the userspace gadget can call ioctl(USB_RAW_IOCTL_EP_READ) multiple times to read all of the data? Or is this not supported, and the 'extra' data above 4KB is just discarded? If the latter, is it possible to increase or remove this limit? In my case the host is trying to transmit 16KB on a bulk endpoint in a single URB.

Thanks.

xairy commented 4 months ago

Hi,

Yeah, Raw Gadget only supports up to 4 KB of data per transfer. I don't remember why I chose this limit, tbh, and I think we can just increase it.

Does Raw Gadget work for you if you increase the limit locally and rebuild the module?

If so, I'll fix it up in the Linux kernel mainline when I get to working on the next batch of Raw Gadget changes. Or feel free to send a patch yourself if you want. I'll keep this issue open in the meantime.

Thanks!

tw39124-1 commented 4 months ago

Thanks for the quick reply. I've tested it out by removing the PAGE_SIZE check altogether, recompiling the module, and it seems to work with larger IO sizes (I tried 8KB and 16KB). Although I have verified that just calling ioctl(USB_RAW_IOCTL_EP_READ) repeatedly does indeed receive the 'extra' data if the host submits URBs with data larger than the io.inner.length field, so the PAGE_SIZE restriction itself doesn't seem to be causing data loss. Although allowing larger transfers would obviously increase throughput significantly.

However, I have now stumbled upon a different issue. If the host submits a URB with a transfer size which is not an integer multiple of the IO size passed to ioctl(USB_RAW_IOCTL_EP_READ) on the gadget side, then I don't appear to receive the final "short" block of data. In my case, the host is transferring a 14702592 byte file in 16KB URBs, so the final URB is smaller (6144 bytes).

With io.inner.length = 4096 and calling ioctl(USB_RAW_IOCTL_EP_READ) in a loop, I receive all data except the final 2048 bytes (the final ioctl call just blocks forever). If I change io.inner.length = 2048 , which is a divisor of the total file length, I do appear to receive all blocks.

Similarly if I set io.inner.length = 16384 then the entire final 6144 byte chunk is not received.

Any idea what's going on here? 6144 is not a multiple of 4KB or 16KB but is a multiple of 2KB, so it appears that either dummy_hcd or raw-gadget is doing something weird here.

tw39124-1 commented 4 months ago

After further debugging it looks like dummy_hcd.c has this block, in the transfer() function:

...
if (req->req.length == req->req.actual) {
    if (req->req.zero && to_host)
    rescan = 1;
    else
    req->req.status = 0;
}
...

This only changes the req->req.status from -EINPROGRESS to 0 if the req->req.actual (i.e. bytes transferred to the gadget) exactly matches req->req.length i.e. the IO length which the gadget submits in the URB. So in my case where req->req.length = 4096 but the final chunk of data from the host is 2048 bytes, the req->req.status remains as -EINPROGRESS and so the gadget URB never completes because of the block below which checks req->req.status:

...
if (req->req.status != -EINPROGRESS) {
    list_del_init(&req->queue);

    spin_unlock(&dum->lock);
    usb_gadget_giveback_request(&ep->ep, &req->req);
    spin_lock(&dum->lock);

    /* requests might have been unlinked... */
    rescan = 1;
}
...

Commenting out the req->req.length == req->req.actual check solves the issue and I now receive all data I am expecting, regardless of the size used for io.inner.length on the gadget side. I'm not sure whether just removing this check entirely is the "correct" thing to do or whether it should just be skipped under certain conditions. Any thoughts?

xairy commented 4 months ago

Do you have access to some hardware UDCs to test this? E.g. a Raspberry Pi. I'm wondering if this issue is specific to the Dummy UDC.

Regardless, I think we'll need help from the Linux kernel USB developers to figure this other issue out.

Could you send a summary of your findings wrt the req->req.length == req->req.actual check over email to me andreyknvl@gmail.com and Alan Stern stern@rowland.harvard.edu? And also please CC linux-usb@vger.kernel.org.