login
Search
Here's an updated version of the usbtmc driver, with all of the different issues that have been raised, hopefully addressed. thanks, greg k-h ---------- From: Greg Kroah-Hartman <gregkh@suse.de> Subject: USB: add USB test and measurement class driver This driver was originaly written by Stefan Kopp, but massively reworked by Greg for submission. Thanks to Felipe Balbi <me@felipebalbi.com> for lots of work in cleaning up this driver. Thanks to Oliver Neukum <oliver@neukum.org> for reviewing previous versions and pointing out problems. Cc: Stefan Kopp <stefan_kopp@agilent.com> Cc: Marcel Janssen <korgull@home.nl> Cc: Felipe Balbi <me@felipebalbi.com> Cc: Oliver Neukum <oliver@neukum.org> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de> --- Documentation/ABI/stable/sysfs-driver-usb-usbtmc | 62 + Documentation/devices.txt | 3 Documentation/ioctl-number.txt | 2 drivers/usb/class/Kconfig | 10 drivers/usb/class/Makefile | 1 drivers/usb/class/usbtmc.c | 1095 +++++++++++++++++++++++ include/linux/usb/Kbuild | 2 include/linux/usb/tmc.h | 43 8 files changed, 1217 insertions(+), 1 deletion(-) --- /dev/null +++ b/Documentation/ABI/stable/sysfs-driver-usb-usbtmc @@ -0,0 +1,62 @@ +What: /sys/bus/usb/drivers/usbtmc/devices/*/interface_capabilities +What: /sys/bus/usb/drivers/usbtmc/devices/*/device_capabilities +Date: August 2008 +Contact: Greg Kroah-Hartman <gregkh@suse.de> +Description: + These files show the various USB TMC capabilities as described + by the device itself. The full description of the bitfields + can be found in the USB TMC documents from the USB-IF entitled + "Universal Serial Bus Test and Measurement Class Specification + (USBTMC) Revision 1.0" section 4.2.1.8. + + The files are read ...
This is an example of what I was discussing with Oliver. In all likelihood you simply don't need usbtmc_mutex, and using it will cause a lockdep violation. That's why so many of the other USB class drivers don't have an You must never acquire a lock in your open method if it will be held But you don't call usb_unregister_dev() during disconnect. An oversight? Alan Stern --
And this rule depends on sharing the USB major or not. This needs a big fat mention in Documentation. Regards Oliver --
Ah, yes, my fault, I converted this from using a private cdev to using the usb_register_dev() call and forgot about this. Thanks for catching it, I'll go fix this. Oh, it's usb_deregister_dev(), stupid programmers with inconsitant interfaces :) thanks, greg k-h --
Yes, once you add the call to usb_deregister_dev. You mean that the open/disconnect locking rule applies only to drivers that call usb_register_dev, i.e, drivers using the USB major. Right? I agree that it deserves to be mentioned in the documentation somewhere. Where would be a good place? None of the existing files in Documentation/usb seem appropriate. Alan Stern --
Yes. In fact drivers not using the USB major but their own char devices The USB major merits a file of its own. Regards Oliver --
IMO all char-device registration/deregistration routines should use a I don't feel competent to start writing such a file. Greg, what do you think? Maybe all that's needed to get going is to "borrow" some of the material in LDD3. :-) Alan Stern --
Well, as the USB chapter in LDD3 "borrowed" heavily from the comments in the usb code itself, that would only be fair :) That chapter needs to be reworked. The authors and Oreilly and I are currently talking about how to do this all in a framework that makes sense (the current one of a book every few years that quickly gets out of date doesn't make sense.) thanks, greg k-h --
Great, all done now. Here's the updated version. thanks, greg k-h ---------- From: Greg Kroah-Hartman <gregkh@suse.de> Subject: USB: add USB test and measurement class driver This driver was originaly written by Stefan Kopp, but massively reworked by Greg for submission. Thanks to Felipe Balbi <me@felipebalbi.com> for lots of work in cleaning up this driver. Thanks to Oliver Neukum <oliver@neukum.org> for reviewing previous versions and pointing out problems. Cc: Stefan Kopp <stefan_kopp@agilent.com> Cc: Marcel Janssen <korgull@home.nl> Cc: Felipe Balbi <me@felipebalbi.com> Cc: Oliver Neukum <oliver@neukum.org> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de> --- Documentation/ABI/stable/sysfs-driver-usb-usbtmc | 62 + Documentation/devices.txt | 3 Documentation/ioctl-number.txt | 2 drivers/usb/class/Kconfig | 10 drivers/usb/class/Makefile | 1 drivers/usb/class/usbtmc.c | 1087 +++++++++++++++++++++++ include/linux/usb/Kbuild | 2 include/linux/usb/tmc.h | 43 8 files changed, 1209 insertions(+), 1 deletion(-) --- /dev/null +++ b/Documentation/ABI/stable/sysfs-driver-usb-usbtmc @@ -0,0 +1,62 @@ +What: /sys/bus/usb/drivers/usbtmc/devices/*/interface_capabilities +What: /sys/bus/usb/drivers/usbtmc/devices/*/device_capabilities +Date: August 2008 +Contact: Greg Kroah-Hartman <gregkh@suse.de> +Description: + These files show the various USB TMC capabilities as described + by the device itself. The full description of the bitfields + can be found in the USB TMC documents from the USB-IF entitled + "Universal Serial Bus Test and Measurement Class Specification + (USBTMC) Revision 1.0" section 4.2.1.8. + + The files are read ...
This and usbtmc_read() need a test for disconnection. Open() and disconnect are guarded in usbcore, read & write are not. By reference count you've made error case? memory leak smp_rmb(); /* must hit RAM after the terminator */ \ There are subtle penalties to avoiding ioctl(). Among them is the loss of atomicity. You must set a flag for read, write and ioctl. Regards Oliver --
For splitting values up into the individual byte portions? I think this I'll leave it (casting is just a big of a mess, this obviously shows If so, then struct usb_device would be different, right? Oh, I see, disconnect() using usbfs/sysfs. Bah, is it really something that I'm not? Should I print an error and then just continue on? Would that Will do. Then I need to lock the flag with a mutex, right? thanks, greg k-h --
No. Kernel code does not exist to show how to do endianness conversion. There is no such thing as an ignorable race. On second thought Yes. You can set the intf pointer to NULL. That's sort of idiomatic. And you should NULL intfdata. Regards Oliver --
I've just installed this version. Here's what I see so far : The driver inserts well and when I connect my device it shows /dev/usbtmc0 with major 180 and minor 176. It only creates one device (Stefan's driver created two) but I'm not sure if that has changed for a reason so just let you know. When I disconnect my device, usbtmc0 will not be destroyed. After connecting the device a couple of times I have a lot of /dev/usbtmc.. files. I would expect the following to work : echo :*IDN?>/dev/usbtmc0 But it returns : No such device Using echo and cat to test the device is quite convenient, but is this supposed to work yet ? I checked /sys a bit and found that the endpoints are correctly found. I also had an issue applying the patch (documentation and makefile). Not sure if that's related to my system though, I'll check that later. regards, Marcel --
Yes, the old driver had a "control and debug " channel at minor 0, which Hm, that's not good, something's wrong here. Can you enable CONFIG_USB_DEBUG and rebuild the driver and send me the Yeah, I would expect that to work as well. The debug information might Good, how about the capability information? That should be in sysfs as well, does that show the proper values? thanks, greg k-h --
Hi all, I haven't been able to follow all the enhancements you did to the driver, but I can comment on the ratioale behind the orignal driver. The original driver used minor number 0 for "driver communication" (in contrast to "device communication"). The main purpose was to get a list of the USBTMC devices attached by doing a "cat /dev/usbtmc0" (which would point to minor number 0). The first device found would then use the next free minor number, usually 1. I believe the concept of "driver communication" has been removed from the driver, so a single minor number for a single device should be it. The issue with using cat on the shell level is that it uses fread which has the (in this case) ugly behaviour of recalling the driver's read method until the full number of characters requested has been accumulated (or until zero characters are returned, indicating the end of file). With USBTMC instruments, this behavour is bad because the retry will not just return zero characters, it will cause a timeout with the associated error condition in the device. So, to enable the use of echo/cat, I added some fread handling to the driver (which catches the retries). I believe this also has been removed, so I assume cat/fread will not work (?). BTW, removing these "features" is not a problem IMHO -- actually I have been working on a "scaled down" version of the driver myself. The reason is that I have been adding a user space library which hides the details of the USB driver and adds portability/exchangability between USB/LAN/serial devices. I expect most users to use this level and it is unlikely they will still want to use echo/cat. Best regards, Stefan -----Original Message----- From: Marcel Janssen [mailto:korgull@home.nl] Sent: Donnerstag, 28. August 2008 18:59 To: Greg KH Cc: Alan Stern; Oliver Neukum; USB list; KOPP,STEFAN (A-Germany,ex1); Felipe Balbi; Kernel development list Subject: Re: [PATCH] USB: add USB test and measurement class driver - round 2 I've just installed this ...
This may be problematic. The driver is throwing away EOF in other words. Generally this is not a good idea. User space can no longer tell how long the reply was. Regards Oliver --
Hi Oliver, USBTMC has a different concept, the "termination character". If this is enabled and set to the appropriate character (usually /n), the read operation will be terminated by the device automatically. The last character in the response will be the termination character. Note, however, this capability is optional (not every vendor supports it) and even if it is supported, is might be deactivated (for good reason, e.g. for transfer of binary data). If you are not using the termination character, the device will typically send its full output buffer contents. Usually, this works just fine because communication with the device is transactional, one query followed by a response, then another query, followed by another response, and so on. The driver does return the number of characters received in both cases (with or without use of the term character feature). The issue is more the habit of fread of "insisting" on receiving the full number of characters requested in the call to fread. Usually, you don't know how many characters will be returned by a USBTMC device, so you just ask for plenty and see how many you get. This works just fine with read(2). fread, unfortunately, retries the read by calling the driver's read entry point again. If you don't catch this retry and ask the USBTMC device for data, you will receive a timeout (unless there happens to be additional data in the device's output buffer). In the original driver, I "simulated" an EOF by just returning 0 in the retry call to the read method. Not sure there's a better way of handling this issue because you can't rely on the termination character... However, I'm very open to ideas... As I said earlier, I avoided this whole issue by using read in my next layer. Best regards Stefan -----Original Message----- From: Oliver Neukum [mailto:oliver@neukum.org] Sent: Freitag, 29. August 2008 09:47 To: KOPP,STEFAN (A-Germany,ex1) Cc: korgull@home.nl; greg@kroah.com; stern@rowland.harvard.edu; linux-usb@vger.kernel.org; ...
But how do you learn in user space how many characters the device actually did send? Using read() you'll only learn that the answer was shorter than you asked for. If you get the amount you expected the answer may have been as long as you expected or longer. How do you find out whether the answer was longer? Regards Oliver --
I may be misinterpreting your message, but read(2) does return the number of bytes read, so if you ask for plenty of data, you can be sure you will get less than that. A device's response should only appear in the output buffer when it is complete. I have never seen a partial response being made available by an instrument. USBTMC instruments are mostly controlled by short commands such as "CONF:FREQ 1.3E3\n", and they usually return short strings such as a measurement result or setting, again as a human-readable string (according to a standard called "SCPI"). It is common practice to ask for, let's say, 1000 bytes, what you really expect is maybe 10...30 bytes. And you can activate the term char handling if the device supports it. For large amounts of data (e.g. when downloading a waveform), USBTMC instruments often use a "binblock", a concept taken from the good old GPIB (IEEE488) standard, where the data is preceeded by a header and the header indicates how many bytes are following. So you first read and interpret the header and then you know exactly how many bytes to expect in the body of the message. Best regards, Stefan -----Original Message----- From: Oliver Neukum [mailto:oliver@neukum.org] Sent: Freitag, 29. August 2008 10:34 To: KOPP,STEFAN (A-Germany,ex1) Cc: korgull@home.nl; greg@kroah.com; stern@rowland.harvard.edu; linux-usb@vger.kernel.org; me@felipebalbi.com; linux-kernel@vger.kernel.org Subject: Re: [PATCH] USB: add USB test and measurement class driver - round 2 But how do you learn in user space how many characters the device actually did send? Using read() you'll only learn that the answer was shorter than you asked for. If you get the amount you expected the answer may have been as long as you expected or longer. How do you find out whether the answer was longer? Regards Oliver --
Not really ideal. We should tell user space how much really was sent. Regards Oliver --
cating that file was sending out a text file to userspace that needed to be parsed, like a /proc file. Now you can just look at all of the devices in /sys/class/usb/usbtmc* to see all of the different devices in I do not know, but we do not do wierd things in the kernel just because of broken userspace programs. This logic should be done in userspace, and programs should look at the return value of read() and handle it properly. Otherwise it is a bug. thanks, greg k-h --
I don't think this is broken in user space. The problem is that when you issue a measurement command it is not known how many bytes it will return. This is probably due to ASCII output being very common in T&M devices instead of raw data (int, float etc). The ASCII formatting is done in the device and this returns just a string which may or may not be terminated by the term char. This is of course not true for all T&M devices, but the majority works this way. I admit that the above produces a lot of overhead, but it's just a fact that T&M devices work this way, including ours for most of their data processing (not all though). I think the USBTMC spec is quite clear on how it should be implemented on messaging level. Basically when you issue the command "*IDN?" the device will process this and return the device ID string. The length of this string is set in the TransferSize of the 12 byte header that the device returns. The problem when you issue a read command is that the read command does not yet know how much data to expect. It should issue the REQUEST_DEV_DEP_MSG_IN first and set the TransferSize value high enough. In the USBTMC_488 extension you find an example (chapter 3.3.1 page 7) that shows the REQUEST_DEV_DEP_MSG_IN TransferSize being set to 64 although the actual data being returned from the device is less (only 36 bytes). What you do see in practice is that when someone would issue a read command and asking for less bytes than are available is that the user program may handle this as a warning telling the user that he did not request all available data. Stefan's driver works exactly the way I would expect from a users point of view. Whether the implementation can be improved is another issue, but the behaviour is correct and compliant with other usbtmc drivers on other platforms. Regards, Marcel --
How is this overhead in userspace, just do something like the following: char big_buffer[16000]; /* bigger than any possible request */ size = read(file_desc, &big_buffer[0], sizeof(big_buffer)); and size is the amount of data you actually read from the device, i.e. Then that's a userspace bug, don't do that in your program that reads But it's not compliant with the "standard" way of using a file descriptor in unix, and might break some POSIX requirements as well (I'm not a good POSIX follower, so I don't know for sure...) As this is trivial to handle in userspace, and requires no additional wierd code in the kernel driver, I don't see this as something we should change the driver for. thanks, greg k-h --
