Re: [PATCH][RFC] USB: zerocopy support for usbfs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 02.07.2014 20:24, Alan Stern wrote:
Overall this implementation seems quite straightforward.  However, it
appears that your email client has mangled the whitespace in the patch.

The patch contains many style violations; you should run it through
checkpatch.pl.  It also has one or two mistakes, such as the
calculation of u for the amount of memory used.

I'll fix these...

Questions/Notes:
- I'm quite unhappy with the added member async::is_user_mem. Is there a
better place
    where I could store this information?
No, async is the right place.  Why are you unhappy about it?
A whole byte for this flag felt a bit like an overkill, but I'm fine with that.

- I had a look at some other drivers using the get_user_pages ->
sg_set_page -> page_cache_release
    technique and didn't see any special code to handle corner cases.
    Are there corner cases? - Like usb controllers not supporting the
whole memory etc. ?
Indeed yes.  The page addresses have to be checked against the DMA
mask.  Also, many host controllers cannot handle arbitrary alignment.
It would be best to require that the buffer start at a page boundary.

Is there any way to check if the host controller supports arbitrary alignment? If I read the xhci spec correctly arbitrary alignment is explicitly permitted. In my use case the user allocates large amounts of memory, which is passed down to submiturb in smaller chunks. So asking the kernel for aligned memory for every chunk would force me to recombine to urbs on the user side, adding another copy operation. So I would vote for a solution where the user can allocate the memory and pass it down
without restrictions.
The system should fallback to copying in kernel space, if the buffer is not supported by the
host-controller (but this is only the case for USB 2 and 1, right?).


- In the kernel log I see messages like:
    xhci_hcd 0000:00:14.0: WARN Event TRB for slot 4 ep 8 with no TDs queued?
    after enabling zerocopy. Any idea where this might come from?
Not directly related; that's a bug in xhci-hcd.

Whew. That saves me a lot of trouble :-)

Using a global module parameter to control the use of zerocopy (for
anything other than debugging or testing) is a bad idea.  If you think
people will have a reason for avoiding zerocopy then you should make it
possible to decide for each URB, by adding a new flag to struct
usbdevfs_urb.

I'd like to leave the parameter in for debugging purposes mostly, at least as long as this is not stable. And it is a security measure. If anything goes wrong with zerocopy (are there buggy host-controllers out there?),
the user is able to disable it on a system wide basis.

People will want to use zerocopy with isochronous transfers as well as
bulk.  Implementing that isn't going to be quite so easy; it will be
necessary for the user to set up a memory mapping.  In fact, once
that's done the same mechanism could be used for bulk transfers too.

Yes you are right. I'll look into that. Is this a either both or nothing decision? As I don't use isochronous transfers at all, it will be quite difficult for me to correctly test that.


Thanks a lot for your input.
Stefan

--

Stefan Klug
Software Developer

Basler AG
An der Strusbek 60-62
22926 Ahrensburg
Germany

Tel. +49 4102 463 582
Fax +49 4102 463 46 582

Stefan.Klug@xxxxxxxxxxxxx

www.baslerweb.com

Vorstand: Dr.-Ing. Dietmar Ley (Vorsitzender) · John P. Jennings · Arndt Bake · Hardy Mehl
Aufsichtsratsvorsitzender: Norbert Basler Basler AG · Amtsgericht Lübeck HRB 4090 · Ust-IdNr.: DE 135 098 121 · Steuer-Nr.: 30 292 04497 · WEEE-Reg.-Nr. DE 83888045
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux