[gk] See below -----Original Message----- From: Greg KH Sent: Thursday, April 05, 2018 3:45 PM To: Kiener Guido Subject: Re: usb: usbtmc: Proposal for new ioctl functions On Wed, Apr 04, 2018 at 10:03:22PM +0000, Guido Kiener wrote: > Greg, > > Before sending patches we want to be sure doing the right things. I'm not sure how we should deal with the sysfs parameters for USBTMC devices: > See > https://github.com/torvalds/linux/blob/master/Documentation/ABI/stable > /sysfs-driver-usb-usbtmc There are some parameters defined for each > device instance: TermChar, TermCharEnabled, and auto_abort. > The new driver should include ioctl functions to change these parameters for each file handle and not for each device instance. Why do you need this on a file-handle basis and not a device instance basis? [gk] Different vendor applications and libraries can open the same usbtmc device. Therefore most parameters like termchar, timeout, eom must not be overwritten by another process to avoid difficult trouble shooting. Indeed concurrent communication of different file handles can still confuse the read/write operations, however a user will see the nonsense. There also might exist sophisticated applications e.g. waiting for SRQ in one process and reading data streams in another thread. So even file locking could be an option in future driver versions. > So we could reassign the device instance parameters (TermChar, TermCharEnabled) to be default values for each file handle. > Furthermore the new driver will have more ioctl functions to change > timeout and EOM flag, Here are my questions: > 1. Are there any (test) scripts that are using the sysfs parameters TermChar, TermCharEnabled where we can check that the new driver is doing the same things? I do not know, you all are the ones that use and have these devices, not me :) [gk] Well, I know it's cool to change some driver parameters with sysfs parameters. However our preferred way to manipulate file handle parameters is using ioctl functions. As far as I know these parameters (TermChar, TermCharEnabled) were added by one of your patches. So I assumed you are using some magic file operations to change TermChar and TermCharEnabled before calling a read() operation. The "auto_abort" parameter can be kept related to the device instance, but "TermChar" and "TermCharEnabled" parameters should be related to the file handle. > 2. Shall we add new sysfs parameters for timeout and EOM flag? (ioctls > are already added) If you want to. [gk] For our VISA Libraries we do not need any sysfs parameters here. @Dave: Please let me know when you need more sysfs parameters here. > 3. Is there any rule when to use upper/lower case or underscore for these parameters (e.g. Timeout, EOM or end_of_message)? If a specification names something, then use that same name, otherwise use Linux kernel naming schemes. [gk] ok > Well, I'm quite happy with the performance of the new ioctl functions > for generic read/write and I don't want to start a lengthy discussion > here. > However I need some expert knowledge and hints about the following questions: > 1. We can detect short USB packages. A ZLP (Zero Length Package) can > happen when the previous package has the maximum packet length. > The driver could be simplified if it would be possible to detect ZLPs. How can it be simplified? [gk] A generic_read operation (from Bulk IN) could return immediately upon detecting a short package or a ZLP. When a driver reads a package with maximum package length then it cannot decide whether another package will follow or not. That means when I submit an URB with 4kB and the driver reads exactly 4kB + ZLP then the URB is completed but it does not signal that a ZLP was received as well. Submitting an extra URB will just wait for the next data package and does not return, since a ZLP is like an empty string. Of course we can work around this problem when we read the header with a single URB and calculate the expected package size and then submit more URBs. > Is there any way (without changing the USB subsystem) to detect ZLPs? On an urb you send, just set URB_ZERO_PACKET, right? What else do you need? [gk] For Bulk OUT transfers the USBTMC protocol does not use ZLP. The generic_write operation works pretty well. > 2. For ultimate performance: Is there any trick where a usb_complete_t > function or submitted urb can transfer (copy) received data directly > to a userland buffer (e.g. scatter/gather streams) ? Are you _sure_ your speed issue is with copying the data from the urb into your userspace buffer? Do you have benchmarks showing this? I think you will find that there are alot of other places you can do things to improve speed before having to worry about this. That being said, yes, you can use scatter/gather on urb buffers, look at the storage driver for one example. But first do a lot of benchmarking, CPUs copy data really really fast, faster than switching contexts, so it's almost always faster to copy data instead of messing with page tables. The break-even point is higher than you think... [gk] Thank you for the hint. Currently I'm fine with the performance and we will compare existing applications with the new driver on USB 3.0. If we really see a significant difference in bandwidth than I will have a deeper look at the storage drivers. Now I have a good feeling that we can start submitting patches in a few weeks. Regards, Guido thanks, greg k-h ��.n��������+%������w��{.n�����{���)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
