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
