Message ID | 20180517170336.8426-7-guido@kiener-muenchen.de (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Thu, May 17, 2018 at 07:03:30PM +0200, Guido Kiener wrote: > - ioctl USBTMC_IOCTL_WRITE sends an (a)synchronous generic message > to bulk OUT. The message is split into chunks of 4k (page size). > Message size is aligned to 32 bit boundaries. > > With flag USBTMC_FLAG_ASYNC the ioctl is non blocking. > With flag USBTMC_FLAG_APPEND additional urbs are queued and > out_status/out_transfer_size is not reset. EPOLLOUT | EPOLLWRNORM > is signaled when all submitted urbs are completed. > > - ioctl USBTMC_IOCTL_WRITE_RESULT copies current out_transfer_size > to given __u64 pointer and returns current out_status of the last > USBTMC_IOCTL_WRITE call. > > - ioctl USBTMC_IOCTL_READ reads an (a)synchronous generic message > from bulk IN. Depending on transfer_size the function submits one > (<=4kB) or more urbs (up to 16) with a size of 4k. > > The flag USBTMC_FLAG_IGNORE_TRAILER can be used when the transmission > size is already known. Then the function does not truncate the > transfer_size to a multiple of 4 kB, but does reserve extra space > to receive the final short or zero length packet. Note that the > instrument is allowed to send up to wMaxPacketSize - 1 bytes at the > end of a message to avoid sending a zero length packet. Again, one patch per ioctl please, it makes it easier to review. And why are you allowing arbitrary bulk messages now? What good is this spec if everyone goes around it? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 17, 2018 at 07:03:30PM +0200, Guido Kiener wrote: > +/* > + * usbtmc_message->flags: > + */ > +#define USBTMC_FLAG_ASYNC 0x0001 > +#define USBTMC_FLAG_APPEND 0x0002 > +#define USBTMC_FLAG_IGNORE_TRAILER 0x0004 > + > +struct usbtmc_message { > + void __user *message; /* pointer to header and data */ > + __u64 transfer_size; /* size of bytes to transfer */ > + __u64 transferred; /* size of received/written bytes */ > + __u32 flags; /* bit 0: 0 = synchronous; 1 = asynchronous */ > +} __attribute__ ((packed)); Very odd structure. Your userspace pointer is going to be totally out of alignment on 32bit systems running on a 64bit kernel. Why have a separate pointer at all? Why not just put the mesage at the end of this structure directly with something like: __u8 message[0]; ? Much easier and you don't have to mess with the whole compatible ioctl thunking layer (which I think you ignored here, which means you all didn't test it...) thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Zitat von Greg KH <gregkh@linuxfoundation.org>: > On Thu, May 17, 2018 at 07:03:30PM +0200, Guido Kiener wrote: >> - ioctl USBTMC_IOCTL_WRITE sends an (a)synchronous generic message >> to bulk OUT. The message is split into chunks of 4k (page size). >> Message size is aligned to 32 bit boundaries. >> >> With flag USBTMC_FLAG_ASYNC the ioctl is non blocking. >> With flag USBTMC_FLAG_APPEND additional urbs are queued and >> out_status/out_transfer_size is not reset. EPOLLOUT | EPOLLWRNORM >> is signaled when all submitted urbs are completed. >> >> - ioctl USBTMC_IOCTL_WRITE_RESULT copies current out_transfer_size >> to given __u64 pointer and returns current out_status of the last >> USBTMC_IOCTL_WRITE call. >> >> - ioctl USBTMC_IOCTL_READ reads an (a)synchronous generic message >> from bulk IN. Depending on transfer_size the function submits one >> (<=4kB) or more urbs (up to 16) with a size of 4k. >> >> The flag USBTMC_FLAG_IGNORE_TRAILER can be used when the transmission >> size is already known. Then the function does not truncate the >> transfer_size to a multiple of 4 kB, but does reserve extra space >> to receive the final short or zero length packet. Note that the >> instrument is allowed to send up to wMaxPacketSize - 1 bytes at the >> end of a message to avoid sending a zero length packet. > > Again, one patch per ioctl please, it makes it easier to review. > > And why are you allowing arbitrary bulk messages now? All VISA applications have specific flavours in dealing with timeout, algorithms to abort a message and reset a device. VISA applications need a maximum of freedom in communication with an instrument to quickly react with workarounds. There is no time to wait several weeks until a kernel fix repairs a production line. The current patches are a trade-off to meet the requirements of the IVI foundation and to give back convenient ioctls (timeout, termchar) to the community for the traditional usage of the usbtmc driver. In case of R&S I (hope) we will use the read, write functions for synchronous calls, and the new ioctls (USBTMC_IOCTL_READ/USBTMC_IOCTL_WRITE) for asynchronous calls. We need at least all generic ioctls. Other patches and ioctls for testing halt conditions and abort we can withdraw. > > What good is this spec if everyone goes around it? > > thanks, > > greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Zitat von Greg KH <gregkh@linuxfoundation.org>: > On Thu, May 17, 2018 at 07:03:30PM +0200, Guido Kiener wrote: >> +/* >> + * usbtmc_message->flags: >> + */ >> +#define USBTMC_FLAG_ASYNC 0x0001 >> +#define USBTMC_FLAG_APPEND 0x0002 >> +#define USBTMC_FLAG_IGNORE_TRAILER 0x0004 >> + >> +struct usbtmc_message { >> + void __user *message; /* pointer to header and data */ >> + __u64 transfer_size; /* size of bytes to transfer */ >> + __u64 transferred; /* size of received/written bytes */ >> + __u32 flags; /* bit 0: 0 = synchronous; 1 = asynchronous */ >> +} __attribute__ ((packed)); > > Very odd structure. Your userspace pointer is going to be totally out > of alignment on 32bit systems running on a 64bit kernel. Why have a > separate pointer at all? Why not just put the mesage at the end of this > structure directly with something like: > __u8 message[0]; > ? > Oh yes, I should know that from another project. Indead we did not test it with 32 bit applications on 64 platforms. When we use your proposal then we just can use #define USBTMC_IOCTL_WRITE _IO(USBTMC_IOC_NR, 13)? > Much easier and you don't have to mess with the whole compatible ioctl > thunking layer (which I think you ignored here, which means you all > didn't test it...) > > thanks, > > greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 18, 2018 at 02:48:11PM +0000, guido@kiener-muenchen.de wrote: > > Zitat von Greg KH <gregkh@linuxfoundation.org>: > > > On Thu, May 17, 2018 at 07:03:30PM +0200, Guido Kiener wrote: > > > +/* > > > + * usbtmc_message->flags: > > > + */ > > > +#define USBTMC_FLAG_ASYNC 0x0001 > > > +#define USBTMC_FLAG_APPEND 0x0002 > > > +#define USBTMC_FLAG_IGNORE_TRAILER 0x0004 > > > + > > > +struct usbtmc_message { > > > + void __user *message; /* pointer to header and data */ > > > + __u64 transfer_size; /* size of bytes to transfer */ > > > + __u64 transferred; /* size of received/written bytes */ > > > + __u32 flags; /* bit 0: 0 = synchronous; 1 = asynchronous */ > > > +} __attribute__ ((packed)); > > > > Very odd structure. Your userspace pointer is going to be totally out > > of alignment on 32bit systems running on a 64bit kernel. Why have a > > separate pointer at all? Why not just put the mesage at the end of this > > structure directly with something like: > > __u8 message[0]; > > ? > > > > Oh yes, I should know that from another project. Indead we did not test it > with 32 bit applications on 64 platforms. > > When we use your proposal then we just can use > #define USBTMC_IOCTL_WRITE _IO(USBTMC_IOC_NR, 13)? Why 13? Use the structure please, that way you know you always get it right. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Zitat von Greg KH <gregkh@linuxfoundation.org>: > On Fri, May 18, 2018 at 02:48:11PM +0000, guido@kiener-muenchen.de wrote: >> >> Zitat von Greg KH <gregkh@linuxfoundation.org>: >> >> > On Thu, May 17, 2018 at 07:03:30PM +0200, Guido Kiener wrote: >> > > +/* >> > > + * usbtmc_message->flags: >> > > + */ >> > > +#define USBTMC_FLAG_ASYNC 0x0001 >> > > +#define USBTMC_FLAG_APPEND 0x0002 >> > > +#define USBTMC_FLAG_IGNORE_TRAILER 0x0004 >> > > + >> > > +struct usbtmc_message { >> > > + void __user *message; /* pointer to header and data */ >> > > + __u64 transfer_size; /* size of bytes to transfer */ >> > > + __u64 transferred; /* size of received/written bytes */ >> > > + __u32 flags; /* bit 0: 0 = synchronous; 1 = asynchronous */ >> > > +} __attribute__ ((packed)); >> > >> > Very odd structure. Your userspace pointer is going to be totally out >> > of alignment on 32bit systems running on a 64bit kernel. Why have a >> > separate pointer at all? Why not just put the mesage at the end of this >> > structure directly with something like: >> > __u8 message[0]; >> > ? >> > >> >> Oh yes, I should know that from another project. Indead we did not test it >> with 32 bit applications on 64 platforms. >> >> When we use your proposal then we just can use >> #define USBTMC_IOCTL_WRITE _IO(USBTMC_IOC_NR, 13)? > > Why 13? Use the structure please, that way you know you always get it > right. Sorry. I should just read the documentation. Dave helps me. Thanks. > > thanks, > > greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Zitat von Greg KH <gregkh@linuxfoundation.org>: > On Fri, May 18, 2018 at 02:48:11PM +0000, guido@kiener-muenchen.de wrote: >> >> Zitat von Greg KH <gregkh@linuxfoundation.org>: >> >> > On Thu, May 17, 2018 at 07:03:30PM +0200, Guido Kiener wrote: >> > > +/* >> > > + * usbtmc_message->flags: >> > > + */ >> > > +#define USBTMC_FLAG_ASYNC 0x0001 >> > > +#define USBTMC_FLAG_APPEND 0x0002 >> > > +#define USBTMC_FLAG_IGNORE_TRAILER 0x0004 >> > > + >> > > +struct usbtmc_message { >> > > + void __user *message; /* pointer to header and data */ >> > > + __u64 transfer_size; /* size of bytes to transfer */ >> > > + __u64 transferred; /* size of received/written bytes */ >> > > + __u32 flags; /* bit 0: 0 = synchronous; 1 = asynchronous */ >> > > +} __attribute__ ((packed)); >> > >> > Very odd structure. Your userspace pointer is going to be totally out >> > of alignment on 32bit systems running on a 64bit kernel. Why have a >> > separate pointer at all? Why not just put the mesage at the end of this >> > structure directly with something like: >> > __u8 message[0]; >> > ? >> > Using a __u8 message[0] ends up with an extra malloc, memcpy, and free operation in userland, since we always have to append the data of a given user pointer to this struct. E.g. when I send 4 MByte on my (old) laptop this takes about plus 6 ms, and decreases bandwidth from 31,0 MByte/s to 29,5 MByte/s with USB 2.0. I hope this struct looks better (in compat mode): struct usbtmc_message { __u64 transfer_size; /* size of bytes to transfer */ __u64 transferred; /* size of received/written bytes */ void __user *message; /* pointer to header and data */ __u32 flags; /* bit 0: 0 = synchronous; 1 = asynchronous */ } __attribute__ ((packed)); BTW the driver has no .compat_ioctl entry. So I wonder how it could work with 32 bit applications on 64 bit systems before submission of our patches. Do I miss something? >> >> Oh yes, I should know that from another project. Indead we did not test it >> with 32 bit applications on 64 platforms. >> >> When we use your proposal then we just can use >> #define USBTMC_IOCTL_WRITE _IO(USBTMC_IOC_NR, 13)? > > Why 13? Use the structure please, that way you know you always get it > right. > > thanks, > > greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 21, 2018 at 08:41:22PM +0000, guido@kiener-muenchen.de wrote: > > Zitat von Greg KH <gregkh@linuxfoundation.org>: > > > On Fri, May 18, 2018 at 02:48:11PM +0000, guido@kiener-muenchen.de wrote: > > > > > > Zitat von Greg KH <gregkh@linuxfoundation.org>: > > > > > > > On Thu, May 17, 2018 at 07:03:30PM +0200, Guido Kiener wrote: > > > > > +/* > > > > > + * usbtmc_message->flags: > > > > > + */ > > > > > +#define USBTMC_FLAG_ASYNC 0x0001 > > > > > +#define USBTMC_FLAG_APPEND 0x0002 > > > > > +#define USBTMC_FLAG_IGNORE_TRAILER 0x0004 > > > > > + > > > > > +struct usbtmc_message { > > > > > + void __user *message; /* pointer to header and data */ > > > > > + __u64 transfer_size; /* size of bytes to transfer */ > > > > > + __u64 transferred; /* size of received/written bytes */ > > > > > + __u32 flags; /* bit 0: 0 = synchronous; 1 = asynchronous */ > > > > > +} __attribute__ ((packed)); > > > > > > > > Very odd structure. Your userspace pointer is going to be totally out > > > > of alignment on 32bit systems running on a 64bit kernel. Why have a > > > > separate pointer at all? Why not just put the mesage at the end of this > > > > structure directly with something like: > > > > __u8 message[0]; > > > > ? > > > > > > Using a __u8 message[0] ends up with an extra malloc, memcpy, and free > operation in userland, since we always have to append the data of a given > user pointer to this struct. E.g. when I send 4 MByte on my (old) laptop > this takes about plus 6 ms, and decreases bandwidth from 31,0 MByte/s > to 29,5 MByte/s with USB 2.0. Really? That feels like you are doing something really odd. I guess you need to figure out how you get the data to/from userspace. > I hope this struct looks better (in compat mode): > > struct usbtmc_message { > __u64 transfer_size; /* size of bytes to transfer */ > __u64 transferred; /* size of received/written bytes */ > void __user *message; /* pointer to header and data */ > __u32 flags; /* bit 0: 0 = synchronous; 1 = asynchronous */ > } __attribute__ ((packed)); No, that will not work at all. Again, think about the size of that pointer. > BTW the driver has no .compat_ioctl entry. Because you didn't need it until now. > So I wonder how it could work with 32 bit applications on 64 bit > systems before submission of our patches. Do I miss something? You are creating structures that have different sizes on those two different userspaces. Because of that, you would be forced to have a compat layer. The smart thing to do is to design the interface to not have that type of problem at all, don't create new apis that are not just 64-bit sane to start with. Copying memory is fast, really fast, much much faster than sending the data across the USB connection. If you are seeing major problems here, then I would first look at your userspace code, and then see what the kernel code does. How about you wait for this new api until you get all of the other stuff implemented/merged? It looks like you all need to really think about how this will all work out. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Zitat von Greg KH <gregkh@linuxfoundation.org>: > On Mon, May 21, 2018 at 08:41:22PM +0000, guido@kiener-muenchen.de wrote: >> >> Zitat von Greg KH <gregkh@linuxfoundation.org>: >> >> > On Fri, May 18, 2018 at 02:48:11PM +0000, guido@kiener-muenchen.de wrote: >> > > >> > > Zitat von Greg KH <gregkh@linuxfoundation.org>: >> > > >> > > > On Thu, May 17, 2018 at 07:03:30PM +0200, Guido Kiener wrote: >> > > > > +/* >> > > > > + * usbtmc_message->flags: >> > > > > + */ >> > > > > +#define USBTMC_FLAG_ASYNC 0x0001 >> > > > > +#define USBTMC_FLAG_APPEND 0x0002 >> > > > > +#define USBTMC_FLAG_IGNORE_TRAILER 0x0004 >> > > > > + >> > > > > +struct usbtmc_message { >> > > > > + void __user *message; /* pointer to header and data */ >> > > > > + __u64 transfer_size; /* size of bytes to transfer */ >> > > > > + __u64 transferred; /* size of received/written bytes */ >> > > > > + __u32 flags; /* bit 0: 0 = synchronous; 1 = asynchronous */ >> > > > > +} __attribute__ ((packed)); >> > > > >> > > > Very odd structure. Your userspace pointer is going to be totally out >> > > > of alignment on 32bit systems running on a 64bit kernel. Why have a >> > > > separate pointer at all? Why not just put the mesage at the >> end of this >> > > > structure directly with something like: >> > > > __u8 message[0]; >> > > > ? >> > > > >> >> Using a __u8 message[0] ends up with an extra malloc, memcpy, and free >> operation in userland, since we always have to append the data of a given >> user pointer to this struct. E.g. when I send 4 MByte on my (old) laptop >> this takes about plus 6 ms, and decreases bandwidth from 31,0 MByte/s >> to 29,5 MByte/s with USB 2.0. > > Really? That feels like you are doing something really odd. I guess > you need to figure out how you get the data to/from userspace. A typically user API is something like this: ssize_t write(int fd, const void *buf, size_t count); ssize_t send(int sockfd, const void *buf, size_t len, int flags); We use a similar function: ViStatus viWrite(ViSession vi, ViBuf buf, ViUInt32 count, ViPUInt32 retCount) When a user calls viWrite(vi, "*IDN? + 4 MB ...\n", 40000000, &retcont) how can we pass the big buf to the kernel without any copy operation? Here is a sample algorithm with a minimum (4kB) of copy operation: See function: tmc_raw_write_common(..) in https://github.com/GuidoKiener/linux-usbtmc/blob/master/test-raw.c > >> I hope this struct looks better (in compat mode): >> >> struct usbtmc_message { >> __u64 transfer_size; /* size of bytes to transfer */ >> __u64 transferred; /* size of received/written bytes */ >> void __user *message; /* pointer to header and data */ >> __u32 flags; /* bit 0: 0 = synchronous; 1 = asynchronous */ >> } __attribute__ ((packed)); > > No, that will not work at all. Again, think about the size of that > pointer. > The compat structure is: struct compat_usbtmc_message { u64 transfer_size; u64 transferred; compat_uptr_t message; u32 flags; } __packed; For AMD64 it works. >> BTW the driver has no .compat_ioctl entry. > > Because you didn't need it until now. ioctl(fd,USBTMC_IOCTL_CLEAR) returns errno = 25 (=ENOTTY) in Linux Kernel 4.15 when running test-raw32 created with gcc -m32 test-raw.c -o test-raw32 Does it work with other kernel versions? > >> So I wonder how it could work with 32 bit applications on 64 bit >> systems before submission of our patches. Do I miss something? > > You are creating structures that have different sizes on those two > different userspaces. Because of that, you would be forced to have a > compat layer. The smart thing to do is to design the interface to not > have that type of problem at all, don't create new apis that are not > just 64-bit sane to start with. > > Copying memory is fast, really fast, much much faster than sending the > data across the USB connection. If you are seeing major problems here, > then I would first look at your userspace code, and then see what the > kernel code does. I just do not like to be slower than libusb or other operating systems. > > How about you wait for this new api until you get all of the other stuff > implemented/merged? It looks like you all need to really think about > how this will all work out. > > thanks, > > greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 22, 2018 at 11:36:13AM +0000, guido@kiener-muenchen.de wrote: > > Zitat von Greg KH <gregkh@linuxfoundation.org>: > > > On Mon, May 21, 2018 at 08:41:22PM +0000, guido@kiener-muenchen.de wrote: > > > > > > Zitat von Greg KH <gregkh@linuxfoundation.org>: > > > > > > > On Fri, May 18, 2018 at 02:48:11PM +0000, guido@kiener-muenchen.de wrote: > > > > > > > > > > Zitat von Greg KH <gregkh@linuxfoundation.org>: > > > > > > > > > > > On Thu, May 17, 2018 at 07:03:30PM +0200, Guido Kiener wrote: > > > > > > > +/* > > > > > > > + * usbtmc_message->flags: > > > > > > > + */ > > > > > > > +#define USBTMC_FLAG_ASYNC 0x0001 > > > > > > > +#define USBTMC_FLAG_APPEND 0x0002 > > > > > > > +#define USBTMC_FLAG_IGNORE_TRAILER 0x0004 > > > > > > > + > > > > > > > +struct usbtmc_message { > > > > > > > + void __user *message; /* pointer to header and data */ > > > > > > > + __u64 transfer_size; /* size of bytes to transfer */ > > > > > > > + __u64 transferred; /* size of received/written bytes */ > > > > > > > + __u32 flags; /* bit 0: 0 = synchronous; 1 = asynchronous */ > > > > > > > +} __attribute__ ((packed)); > > > > > > > > > > > > Very odd structure. Your userspace pointer is going to be totally out > > > > > > of alignment on 32bit systems running on a 64bit kernel. Why have a > > > > > > separate pointer at all? Why not just put the mesage at the > > > end of this > > > > > > structure directly with something like: > > > > > > __u8 message[0]; > > > > > > ? > > > > > > > > > > > > Using a __u8 message[0] ends up with an extra malloc, memcpy, and free > > > operation in userland, since we always have to append the data of a given > > > user pointer to this struct. E.g. when I send 4 MByte on my (old) laptop > > > this takes about plus 6 ms, and decreases bandwidth from 31,0 MByte/s > > > to 29,5 MByte/s with USB 2.0. > > > > Really? That feels like you are doing something really odd. I guess > > you need to figure out how you get the data to/from userspace. > > A typically user API is something like this: > ssize_t write(int fd, const void *buf, size_t count); > ssize_t send(int sockfd, const void *buf, size_t len, int flags); > > We use a similar function: > ViStatus viWrite(ViSession vi, ViBuf buf, ViUInt32 count, ViPUInt32 retCount) > > When a user calls viWrite(vi, "*IDN? + 4 MB ...\n", 40000000, &retcont) how > can we pass the big buf to the kernel without any copy operation? Your function looks _just_ like a normal write() call, why are you even trying to wrap it in an ioctl? Just use write()! > > > I hope this struct looks better (in compat mode): > > > > > > struct usbtmc_message { > > > __u64 transfer_size; /* size of bytes to transfer */ > > > __u64 transferred; /* size of received/written bytes */ > > > void __user *message; /* pointer to header and data */ > > > __u32 flags; /* bit 0: 0 = synchronous; 1 = asynchronous */ > > > } __attribute__ ((packed)); > > > > No, that will not work at all. Again, think about the size of that > > pointer. > > > > The compat structure is: > > struct compat_usbtmc_message { > u64 transfer_size; > u64 transferred; > compat_uptr_t message; > u32 flags; > } __packed; > > For AMD64 it works. Show me the size and layout of both structures for a 32bit and 64bit userspace please. There are tools that do this. Again, this is not how to do it! You do not put a variable sized variable in the middle of a structure. void * changes size! Either align everything properly or throw it at the end or even better yet, DON'T DO THIS AT ALL, JUST USE write()! :) > > > BTW the driver has no .compat_ioctl entry. > > > > Because you didn't need it until now. > > ioctl(fd,USBTMC_IOCTL_CLEAR) returns errno = 25 (=ENOTTY) in Linux > Kernel 4.15 when running test-raw32 created with > gcc -m32 test-raw.c -o test-raw32 > Does it work with other kernel versions? I have no idea what you are testing here, sorry. > > > So I wonder how it could work with 32 bit applications on 64 bit > > > systems before submission of our patches. Do I miss something? > > > > You are creating structures that have different sizes on those two > > different userspaces. Because of that, you would be forced to have a > > compat layer. The smart thing to do is to design the interface to not > > have that type of problem at all, don't create new apis that are not > > just 64-bit sane to start with. > > > > Copying memory is fast, really fast, much much faster than sending the > > data across the USB connection. If you are seeing major problems here, > > then I would first look at your userspace code, and then see what the > > kernel code does. > > I just do not like to be slower than libusb or other operating systems. If you do this correctly, your bottleneck will be the USB connection. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index 00c2e51a23a7..de664a345e69 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -36,6 +36,11 @@ /* Default USB timeout (in milliseconds) */ #define USBTMC_TIMEOUT 5000 +/* Max number of urbs used in write transfers */ +#define MAX_URBS_IN_FLIGHT 16 +/* I/O buffer size used in generic read/write functions */ +#define USBTMC_BUFSIZE (4096) + /* * Maximum number of read cycles to empty bulk in endpoint during CLEAR and * ABORT_BULK_IN requests. Ends the loop if (for whatever reason) a short @@ -79,6 +84,9 @@ struct usbtmc_device_data { u8 bTag_last_write; /* needed for abort */ u8 bTag_last_read; /* needed for abort */ + /* packet size of IN bulk */ + u16 wMaxPacketSize; + /* data for interrupt in endpoint handling */ u8 bNotify1; u8 bNotify2; @@ -122,16 +130,34 @@ struct usbtmc_file_data { u32 timeout; u8 srq_byte; atomic_t srq_asserted; + atomic_t closing; /* These values are initialized with default values from device_data */ u8 TermChar; bool TermCharEnabled; bool auto_abort; u8 eom_val; + + spinlock_t err_lock; /* lock for errors */ + + struct usb_anchor submitted; + + /* data for generic_write */ + struct semaphore limit_write_sem; + u64 out_transfer_size; + int out_status; + + /* data for generic_read */ + u64 in_transfer_size; + int in_status; + int in_urbs_used; + struct usb_anchor in_anchor; + wait_queue_head_t wait_bulk_in; }; /* Forward declarations */ static struct usb_driver usbtmc_driver; +static void usbtmc_draw_down(struct usbtmc_file_data *file_data); static void usbtmc_delete(struct kref *kref) { @@ -160,6 +186,12 @@ static int usbtmc_open(struct inode *inode, struct file *filp) pr_debug("%s - called\n", __func__); + spin_lock_init(&file_data->err_lock); + sema_init(&file_data->limit_write_sem, MAX_URBS_IN_FLIGHT); + init_usb_anchor(&file_data->submitted); + init_usb_anchor(&file_data->in_anchor); + init_waitqueue_head(&file_data->wait_bulk_in); + data = usb_get_intfdata(intf); /* Protect reference to data from file structure until release */ kref_get(&data->kref); @@ -167,6 +199,8 @@ static int usbtmc_open(struct inode *inode, struct file *filp) mutex_lock(&data->io_mutex); file_data->data = data; + atomic_set(&file_data->closing, 0); + /* copy default values from device settings */ file_data->timeout = USBTMC_TIMEOUT; file_data->TermChar = data->TermChar; @@ -186,6 +220,41 @@ static int usbtmc_open(struct inode *inode, struct file *filp) return 0; } +/* + * usbtmc_flush - called before file handle is closed + */ +static int usbtmc_flush(struct file *file, fl_owner_t id) +{ + struct usbtmc_file_data *file_data; + struct usbtmc_device_data *data; + + file_data = file->private_data; + if (file_data == NULL) + return -ENODEV; + + atomic_set(&file_data->closing, 1); + data = file_data->data; + + /* wait for io to stop */ + mutex_lock(&data->io_mutex); + + usbtmc_draw_down(file_data); + + spin_lock_irq(&file_data->err_lock); + file_data->in_status = 0; + file_data->in_transfer_size = 0; + file_data->in_urbs_used = 0; + file_data->out_status = 0; + file_data->out_transfer_size = 0; + spin_unlock_irq(&file_data->err_lock); + + wake_up_interruptible_all(&data->waitq); + pr_debug("%s - called\n", __func__); + mutex_unlock(&data->io_mutex); + + return 0; +} + static int usbtmc_release(struct inode *inode, struct file *file) { struct usbtmc_file_data *file_data = file->private_data; @@ -625,6 +694,555 @@ static int usbtmc488_ioctl_trigger(struct usbtmc_file_data *file_data) return 0; } +static struct urb *usbtmc_create_urb(void) +{ + const size_t bufsize = USBTMC_BUFSIZE; + u8 *dmabuf = NULL; + struct urb *urb = usb_alloc_urb(0, GFP_KERNEL); + + if (!urb) + return NULL; + + dmabuf = kmalloc(bufsize, GFP_KERNEL); + if (!dmabuf) { + usb_free_urb(urb); + return NULL; + } + + urb->transfer_buffer = dmabuf; + urb->transfer_buffer_length = bufsize; + urb->transfer_flags |= URB_FREE_BUFFER; + return urb; +} + +static void usbtmc_read_bulk_cb(struct urb *urb) +{ + struct usbtmc_file_data *file_data = urb->context; + int status = urb->status; + + /* sync/async unlink faults aren't errors */ + if (status) { + if (!(/* status == -ENOENT || */ + status == -ECONNRESET || + status == -EREMOTEIO || /* Short packet */ + status == -ESHUTDOWN)) + dev_err(&file_data->data->intf->dev, + "%s - nonzero read bulk status received: %d\n", + __func__, status); + + spin_lock(&file_data->err_lock); + if (!file_data->in_status) + file_data->in_status = status; + spin_unlock(&file_data->err_lock); + } + + spin_lock(&file_data->err_lock); + file_data->in_transfer_size += urb->actual_length; + dev_dbg(&file_data->data->intf->dev, + "%s - total size: %llu current: %d status: %d\n", + __func__, file_data->in_transfer_size, + urb->actual_length, status); + spin_unlock(&file_data->err_lock); + usb_anchor_urb(urb, &file_data->in_anchor); + + wake_up_interruptible(&file_data->wait_bulk_in); + wake_up_interruptible(&file_data->data->waitq); +} + +static inline bool usbtmc_do_transfer(struct usbtmc_file_data *file_data) +{ + bool data_or_error; + + spin_lock_irq(&file_data->err_lock); + data_or_error = !usb_anchor_empty(&file_data->in_anchor) + || file_data->in_status; + spin_unlock_irq(&file_data->err_lock); + dev_dbg(&file_data->data->intf->dev, "%s: returns %d\n", __func__, + data_or_error); + return data_or_error; +} + +static ssize_t usbtmc_generic_read(struct usbtmc_file_data *file_data, + void __user *user_buffer, + u64 transfer_size, + u64 *transferred, + u32 flags) +{ + struct usbtmc_device_data *data = file_data->data; + struct device *dev = &data->intf->dev; + u64 done = 0; + u64 remaining; + const size_t bufsize = USBTMC_BUFSIZE; + int retval = 0; + u64 max_transfer_size; + unsigned long expire; + int bufcount = 1; + int again = 0; + + /* mutex already locked */ + + *transferred = done; + + max_transfer_size = transfer_size; + + if (flags & USBTMC_FLAG_IGNORE_TRAILER) { + /* The device may send extra alignment bytes (up to + * wMaxPacketSize – 1) to avoid sending a zero-length + * packet + */ + remaining = transfer_size; + if (((u32)max_transfer_size % data->wMaxPacketSize) == 0) + max_transfer_size += (data->wMaxPacketSize - 1); + } else { + /* round down to bufsize to avoid truncated data left */ + if (max_transfer_size > bufsize) { + max_transfer_size = + roundup(max_transfer_size + 1 - bufsize, + bufsize); + } + remaining = max_transfer_size; + } + + spin_lock_irq(&file_data->err_lock); + + if (file_data->in_status) { + /* return the very first error */ + retval = file_data->in_status; + spin_unlock_irq(&file_data->err_lock); + goto error; + } + + if (flags & USBTMC_FLAG_ASYNC) { + if (usb_anchor_empty(&file_data->in_anchor)) + again = 1; + + if (file_data->in_urbs_used == 0) { + file_data->in_transfer_size = 0; + file_data->in_status = 0; + } + } else { + file_data->in_transfer_size = 0; + file_data->in_status = 0; + } + + if (max_transfer_size == 0) { + bufcount = 0; + } else { + bufcount = roundup(max_transfer_size, bufsize) / bufsize; + if (bufcount > file_data->in_urbs_used) + bufcount -= file_data->in_urbs_used; + else + bufcount = 0; + + if (bufcount + file_data->in_urbs_used > MAX_URBS_IN_FLIGHT) { + bufcount = MAX_URBS_IN_FLIGHT - + file_data->in_urbs_used; + } + } + spin_unlock_irq(&file_data->err_lock); + + dev_dbg(dev, "%s: requested=%llu flags=0x%X size=%llu bufs=%d used=%d\n", + __func__, (u64)transfer_size, (unsigned int)flags, + max_transfer_size, bufcount, file_data->in_urbs_used); + + while (bufcount > 0) { + u8 *dmabuf = NULL; + struct urb *urb = usbtmc_create_urb(); + + if (!urb) { + retval = -ENOMEM; + goto error; + } + + dmabuf = urb->transfer_buffer; + + usb_fill_bulk_urb(urb, data->usb_dev, + usb_rcvbulkpipe(data->usb_dev, data->bulk_in), + dmabuf, bufsize, + usbtmc_read_bulk_cb, file_data); + + usb_anchor_urb(urb, &file_data->submitted); + retval = usb_submit_urb(urb, GFP_KERNEL); + /* urb is anchored. We can release our reference. */ + usb_free_urb(urb); + if (unlikely(retval)) { + usb_unanchor_urb(urb); + goto error; + } + file_data->in_urbs_used++; + bufcount--; + } + + if (again) { + dev_dbg(dev, "%s: ret=again\n", __func__); + return -EAGAIN; + } + + if (user_buffer == NULL) + return -EINVAL; + + expire = msecs_to_jiffies(file_data->timeout); + + while (max_transfer_size > 0) { + size_t this_part; + struct urb *urb = NULL; + + if (!(flags & USBTMC_FLAG_ASYNC)) { + dev_dbg(dev, "%s: before wait time %lu\n", + __func__, expire); + retval = wait_event_interruptible_timeout( + file_data->wait_bulk_in, + usbtmc_do_transfer(file_data), + expire); + + dev_dbg(dev, "%s: wait returned %d\n", + __func__, retval); + + if (retval <= 0) { + if (retval == 0) + retval = -ETIMEDOUT; + goto error; + } + } + + urb = usb_get_from_anchor(&file_data->in_anchor); + if (!urb) { + if (!(flags & USBTMC_FLAG_ASYNC)) { + /* synchronous case: must not happen */ + retval = -EFAULT; + goto error; + } + + /* asynchronous case: ready, do not block or wait */ + *transferred = done; + dev_dbg(dev, "%s: (async) done=%llu ret=0\n", + __func__, done); + return 0; + } + + file_data->in_urbs_used--; + + if (max_transfer_size > urb->actual_length) + max_transfer_size -= urb->actual_length; + else + max_transfer_size = 0; + + if (remaining > urb->actual_length) + this_part = urb->actual_length; + else + this_part = remaining; + + print_hex_dump_debug("usbtmc ", DUMP_PREFIX_NONE, 16, 1, + urb->transfer_buffer, urb->actual_length, true); + + if (copy_to_user(user_buffer + done, + urb->transfer_buffer, this_part)) { + usb_free_urb(urb); + retval = -EFAULT; + goto error; + } + + remaining -= this_part; + done += this_part; + + spin_lock_irq(&file_data->err_lock); + if (urb->status) { + /* return the very first error */ + retval = file_data->in_status; + spin_unlock_irq(&file_data->err_lock); + usb_free_urb(urb); + goto error; + } + spin_unlock_irq(&file_data->err_lock); + + if (urb->actual_length < bufsize) { + /* short packet or ZLP received => ready */ + usb_free_urb(urb); + retval = 1; + break; + } + + if (!(flags & USBTMC_FLAG_ASYNC) && + max_transfer_size > (bufsize * file_data->in_urbs_used)) { + /* resubmit, since other buffers still not enough */ + usb_anchor_urb(urb, &file_data->submitted); + retval = usb_submit_urb(urb, GFP_KERNEL); + if (unlikely(retval)) { + usb_unanchor_urb(urb); + usb_free_urb(urb); + goto error; + } + file_data->in_urbs_used++; + } + usb_free_urb(urb); + retval = 0; + } + +error: + *transferred = done; + + dev_dbg(dev, "%s: before kill\n", __func__); + /* Attention: killing urbs can take long time (2 ms) */ + usb_kill_anchored_urbs(&file_data->submitted); + dev_dbg(dev, "%s: after kill\n", __func__); + usb_scuttle_anchored_urbs(&file_data->in_anchor); + file_data->in_urbs_used = 0; + file_data->in_status = 0; /* no spinlock needed here */ + dev_dbg(dev, "%s: done=%llu ret=%d\n", __func__, done, retval); + + return retval; +} + +static ssize_t usbtmc_ioctl_generic_read(struct usbtmc_file_data *file_data, + void __user *arg) +{ + struct usbtmc_message msg; + ssize_t retval = 0; + + /* mutex already locked */ + + if (copy_from_user(&msg, arg, sizeof(struct usbtmc_message))) + return -EFAULT; + + retval = usbtmc_generic_read(file_data, msg.message, + msg.transfer_size, &msg.transferred, + msg.flags); + + if (put_user(msg.transferred, + &((struct usbtmc_message __user *)arg)->transferred)) + return -EFAULT; + + return retval; +} + +static void usbtmc_write_bulk_cb(struct urb *urb) +{ + struct usbtmc_file_data *file_data = urb->context; + int wakeup = 0; + + spin_lock(&file_data->err_lock); + file_data->out_transfer_size += urb->actual_length; + + /* sync/async unlink faults aren't errors */ + if (urb->status) { + if (!(urb->status == -ENOENT || + urb->status == -ECONNRESET || + urb->status == -ESHUTDOWN)) + dev_err(&file_data->data->intf->dev, + "%s - nonzero write bulk status received: %d\n", + __func__, urb->status); + + if (!file_data->out_status) { + file_data->out_status = urb->status; + wakeup = 1; + } + } + spin_unlock(&file_data->err_lock); + + dev_dbg(&file_data->data->intf->dev, + "%s - write bulk total size: %llu\n", + __func__, file_data->out_transfer_size); + + up(&file_data->limit_write_sem); + if (usb_anchor_empty(&file_data->submitted) || wakeup) + wake_up_interruptible(&file_data->data->waitq); +} + +static ssize_t usbtmc_generic_write(struct usbtmc_file_data *file_data, + const void __user *user_buffer, + u64 transfer_size, + u64 *transferred, + u32 flags) +{ + struct usbtmc_device_data *data = file_data->data; + struct device *dev; + u64 done = 0; + u64 remaining; + unsigned long expire; + const size_t bufsize = USBTMC_BUFSIZE; + struct urb *urb = NULL; + int retval = 0; + u32 timeout; + + *transferred = 0; + + /* Get pointer to private data structure */ + dev = &data->intf->dev; + + dev_dbg(dev, "%s: size=%llu flags=0x%X sema=%u\n", + __func__, transfer_size, (unsigned int)flags, + file_data->limit_write_sem.count); + + if (flags & USBTMC_FLAG_APPEND) { + spin_lock_irq(&file_data->err_lock); + retval = file_data->out_status; + spin_unlock_irq(&file_data->err_lock); + if (retval < 0) + return retval; + } else { + spin_lock_irq(&file_data->err_lock); + file_data->out_transfer_size = 0; + file_data->out_status = 0; + spin_unlock_irq(&file_data->err_lock); + } + + remaining = transfer_size; + + timeout = file_data->timeout; + expire = msecs_to_jiffies(timeout); + + while (remaining > 0) { + size_t this_part, aligned; + u8 *buffer = NULL; + + if (flags & USBTMC_FLAG_ASYNC) { + if (down_trylock(&file_data->limit_write_sem)) { + retval = (done)?(0):(-EAGAIN); + goto exit; + } + } else { + retval = down_timeout(&file_data->limit_write_sem, + expire); + if (retval < 0) { + retval = -ETIMEDOUT; + goto error; + } + } + + spin_lock_irq(&file_data->err_lock); + retval = file_data->out_status; + spin_unlock_irq(&file_data->err_lock); + if (retval < 0) { + up(&file_data->limit_write_sem); + goto error; + } + + /* prepare next urb to send */ + urb = usbtmc_create_urb(); + if (!urb) { + retval = -ENOMEM; + up(&file_data->limit_write_sem); + goto error; + } + buffer = urb->transfer_buffer; + + if (remaining > bufsize) + this_part = bufsize; + else + this_part = remaining; + + if (copy_from_user(buffer, user_buffer + done, this_part)) { + retval = -EFAULT; + up(&file_data->limit_write_sem); + goto error; + } + + print_hex_dump_debug("usbtmc ", DUMP_PREFIX_NONE, + 16, 1, buffer, this_part, true); + + /* fill bulk with 32 bit alignment to meet USBTMC specification + * (size + 3 & ~3) rounds up and simplifies user code + */ + aligned = (this_part + 3) & ~3; + dev_dbg(dev, "write(size:%u align:%u done:%u)\n", + (unsigned int)this_part, + (unsigned int)aligned, + (unsigned int)done); + + usb_fill_bulk_urb(urb, data->usb_dev, + usb_sndbulkpipe(data->usb_dev, data->bulk_out), + urb->transfer_buffer, aligned, + usbtmc_write_bulk_cb, file_data); + + usb_anchor_urb(urb, &file_data->submitted); + retval = usb_submit_urb(urb, GFP_KERNEL); + if (unlikely(retval)) { + usb_unanchor_urb(urb); + up(&file_data->limit_write_sem); + goto error; + } + + usb_free_urb(urb); + urb = NULL; /* urb will be finally released by usb driver */ + + remaining -= this_part; + done += this_part; + } + + /* All urbs are on the fly */ + if (!(flags & USBTMC_FLAG_ASYNC)) { + if (!usb_wait_anchor_empty_timeout(&file_data->submitted, + timeout)) { + retval = -ETIMEDOUT; + goto error; + } + } + + retval = 0; + goto exit; + +error: + usb_kill_anchored_urbs(&file_data->submitted); +exit: + usb_free_urb(urb); + + spin_lock_irq(&file_data->err_lock); + if (!(flags & USBTMC_FLAG_ASYNC)) + done = file_data->out_transfer_size; + if (!retval && file_data->out_status) + retval = file_data->out_status; + spin_unlock_irq(&file_data->err_lock); + + *transferred = done; + + dev_dbg(dev, "%s: done=%llu, retval=%d, urbstat=%d\n", + __func__, done, retval, file_data->out_status); + + return retval; +} + +static ssize_t usbtmc_ioctl_generic_write(struct usbtmc_file_data *file_data, + void __user *arg) +{ + struct usbtmc_message msg; + ssize_t retval = 0; + + /* mutex already locked */ + + if (copy_from_user(&msg, arg, sizeof(struct usbtmc_message))) + return -EFAULT; + + retval = usbtmc_generic_write(file_data, msg.message, + msg.transfer_size, &msg.transferred, + msg.flags); + + if (put_user(msg.transferred, + &((struct usbtmc_message __user *)arg)->transferred)) + return -EFAULT; + + return retval; +} + +/* + * Get the generic write result + */ +static ssize_t usbtmc_ioctl_write_result(struct usbtmc_file_data *file_data, + void __user *arg) +{ + __u64 transferred; + int retval; + + spin_lock_irq(&file_data->err_lock); + transferred = file_data->out_transfer_size; + retval = file_data->out_status; + spin_unlock_irq(&file_data->err_lock); + + if (put_user(transferred, (__u64 __user *)arg)) + return -EFAULT; + + return retval; +} + /* * Sends a REQUEST_DEV_DEP_MSG_IN message on the Bulk-OUT endpoint. * @transfer_size: number of bytes to request from the device. @@ -1091,6 +1709,33 @@ static int usbtmc_ioctl_clear_in_halt(struct usbtmc_device_data *data) return 0; } +static int usbtmc_ioctl_cancel_io(struct usbtmc_file_data *file_data) +{ + dev_dbg(&file_data->data->intf->dev, "%s - called: %d\n", __func__, 0); + spin_lock_irq(&file_data->err_lock); + file_data->in_status = -ECANCELED; + file_data->out_status = -ECANCELED; + spin_unlock_irq(&file_data->err_lock); + usb_kill_anchored_urbs(&file_data->submitted); + return 0; +} + +static int usbtmc_ioctl_cleanup_io(struct usbtmc_file_data *file_data) +{ + dev_dbg(&file_data->data->intf->dev, "%s - called: %d\n", __func__, 0); + usb_kill_anchored_urbs(&file_data->submitted); + usb_scuttle_anchored_urbs(&file_data->in_anchor); + spin_lock_irq(&file_data->err_lock); + file_data->in_status = 0; + file_data->in_transfer_size = 0; + file_data->out_status = 0; + file_data->out_transfer_size = 0; + spin_unlock_irq(&file_data->err_lock); + + file_data->in_urbs_used = 0; + return 0; +} + static int get_capabilities(struct usbtmc_device_data *data) { struct device *dev = &data->usb_dev->dev; @@ -1460,6 +2105,21 @@ static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg) (void __user *)arg); break; + case USBTMC_IOCTL_WRITE: + retval = usbtmc_ioctl_generic_write(file_data, + (void __user *)arg); + break; + + case USBTMC_IOCTL_READ: + retval = usbtmc_ioctl_generic_read(file_data, + (void __user *)arg); + break; + + case USBTMC_IOCTL_WRITE_RESULT: + retval = usbtmc_ioctl_write_result(file_data, + (void __user *)arg); + break; + case USBTMC488_IOCTL_GET_CAPS: retval = put_user(data->usb488_caps, (unsigned char __user *)arg); @@ -1488,6 +2148,14 @@ static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg) case USBTMC488_IOCTL_TRIGGER: retval = usbtmc488_ioctl_trigger(file_data); break; + + case USBTMC_IOCTL_CANCEL_IO: + retval = usbtmc_ioctl_cancel_io(file_data); + break; + + case USBTMC_IOCTL_CLEANUP_IO: + retval = usbtmc_ioctl_cleanup_io(file_data); + break; } skip_io_on_zombie: @@ -1517,7 +2185,28 @@ static __poll_t usbtmc_poll(struct file *file, poll_table *wait) poll_wait(file, &data->waitq, wait); - mask = (atomic_read(&file_data->srq_asserted)) ? EPOLLPRI : 0; + /* Note that EPOLLPRI is now assigned to SRQ, and + * EPOLLIN|EPOLLRDNORM to normal read data. + */ + mask = 0; + if (atomic_read(&file_data->srq_asserted)) + mask |= EPOLLPRI; + + /* Note that the anchor submitted includes all urbs for BULK IN + * and OUT. So EPOLLOUT is signaled when BULK OUT is empty and + * all BULK IN urbs are completed and moved to in_anchor. + */ + if (usb_anchor_empty(&file_data->submitted)) + mask |= (EPOLLOUT | EPOLLWRNORM); + if (!usb_anchor_empty(&file_data->in_anchor)) + mask |= (EPOLLIN | EPOLLRDNORM); + + spin_lock_irq(&file_data->err_lock); + if (file_data->in_status || file_data->out_status) + mask |= EPOLLERR; + spin_unlock_irq(&file_data->err_lock); + + dev_dbg(&data->intf->dev, "poll mask = %x\n", (unsigned int)mask); no_poll: mutex_unlock(&data->io_mutex); @@ -1530,6 +2219,7 @@ static const struct file_operations fops = { .write = usbtmc_write, .open = usbtmc_open, .release = usbtmc_release, + .flush = usbtmc_flush, .unlocked_ioctl = usbtmc_ioctl, .fasync = usbtmc_fasync, .poll = usbtmc_poll, @@ -1669,6 +2359,7 @@ static int usbtmc_probe(struct usb_interface *intf, } data->bulk_in = bulk_in->bEndpointAddress; + data->wMaxPacketSize = usb_endpoint_maxp(bulk_in); dev_dbg(&intf->dev, "Found bulk in endpoint at %u\n", data->bulk_in); data->bulk_out = bulk_out->bEndpointAddress; @@ -1750,6 +2441,7 @@ static int usbtmc_probe(struct usb_interface *intf, static void usbtmc_disconnect(struct usb_interface *intf) { struct usbtmc_device_data *data = usb_get_intfdata(intf); + struct list_head *elem; dev_dbg(&intf->dev, "%s - called\n", __func__); @@ -1759,19 +2451,99 @@ static void usbtmc_disconnect(struct usb_interface *intf) mutex_lock(&data->io_mutex); data->zombie = 1; wake_up_interruptible_all(&data->waitq); + list_for_each(elem, &data->file_list) { + struct usbtmc_file_data *file_data; + + file_data = list_entry(elem, + struct usbtmc_file_data, + file_elem); + usb_kill_anchored_urbs(&file_data->submitted); + usb_scuttle_anchored_urbs(&file_data->in_anchor); + } mutex_unlock(&data->io_mutex); usbtmc_free_int(data); kref_put(&data->kref, usbtmc_delete); } +static void usbtmc_draw_down(struct usbtmc_file_data *file_data) +{ + int time; + + time = usb_wait_anchor_empty_timeout(&file_data->submitted, 1000); + if (!time) + usb_kill_anchored_urbs(&file_data->submitted); + usb_scuttle_anchored_urbs(&file_data->in_anchor); +} + static int usbtmc_suspend(struct usb_interface *intf, pm_message_t message) { - /* this driver does not have pending URBs */ + struct usbtmc_device_data *data = usb_get_intfdata(intf); + struct list_head *elem; + + dev_dbg(&intf->dev, "%s - called\n", __func__); + + if (!data) + return 0; + + mutex_lock(&data->io_mutex); + list_for_each(elem, &data->file_list) { + struct usbtmc_file_data *file_data; + + file_data = list_entry(elem, + struct usbtmc_file_data, + file_elem); + usbtmc_draw_down(file_data); + } + + if (data->iin_ep_present && data->iin_urb) + usb_kill_urb(data->iin_urb); + + mutex_unlock(&data->io_mutex); return 0; } static int usbtmc_resume(struct usb_interface *intf) { + struct usbtmc_device_data *data = usb_get_intfdata(intf); + int retcode = 0; + + dev_dbg(&intf->dev, "%s - called\n", __func__); + if (data->iin_ep_present && data->iin_urb) + retcode = usb_submit_urb(data->iin_urb, GFP_KERNEL); + if (retcode) + dev_err(&intf->dev, "Failed to submit iin_urb\n"); + + return retcode; +} + +static int usbtmc_pre_reset(struct usb_interface *intf) +{ + struct usbtmc_device_data *data = usb_get_intfdata(intf); + struct list_head *elem; + + if (!data) + return 0; + + mutex_lock(&data->io_mutex); + + list_for_each(elem, &data->file_list) { + struct usbtmc_file_data *file_data; + + file_data = list_entry(elem, + struct usbtmc_file_data, + file_elem); + usbtmc_ioctl_cancel_io(file_data); + } + + return 0; +} + +static int usbtmc_post_reset(struct usb_interface *intf) +{ + struct usbtmc_device_data *data = usb_get_intfdata(intf); + + mutex_unlock(&data->io_mutex); + return 0; } @@ -1782,6 +2554,8 @@ static struct usb_driver usbtmc_driver = { .disconnect = usbtmc_disconnect, .suspend = usbtmc_suspend, .resume = usbtmc_resume, + .pre_reset = usbtmc_pre_reset, + .post_reset = usbtmc_post_reset, }; module_usb_driver(usbtmc_driver); diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h index c1acec9ad834..1540716de293 100644 --- a/include/uapi/linux/usb/tmc.h +++ b/include/uapi/linux/usb/tmc.h @@ -59,6 +59,20 @@ struct usbtmc_termchar { __u8 term_char_enabled; // bool } __attribute__ ((packed)); +/* + * usbtmc_message->flags: + */ +#define USBTMC_FLAG_ASYNC 0x0001 +#define USBTMC_FLAG_APPEND 0x0002 +#define USBTMC_FLAG_IGNORE_TRAILER 0x0004 + +struct usbtmc_message { + void __user *message; /* pointer to header and data */ + __u64 transfer_size; /* size of bytes to transfer */ + __u64 transferred; /* size of received/written bytes */ + __u32 flags; /* bit 0: 0 = synchronous; 1 = asynchronous */ +} __attribute__ ((packed)); + /* Request values for USBTMC driver's ioctl entry point */ #define USBTMC_IOC_NR 91 #define USBTMC_IOCTL_INDICATOR_PULSE _IO(USBTMC_IOC_NR, 1) @@ -72,6 +86,9 @@ struct usbtmc_termchar { #define USBTMC_IOCTL_SET_TIMEOUT _IOW(USBTMC_IOC_NR, 10, unsigned int) #define USBTMC_IOCTL_EOM_ENABLE _IOW(USBTMC_IOC_NR, 11, __u8) #define USBTMC_IOCTL_CONFIG_TERMCHAR _IOW(USBTMC_IOC_NR, 12, struct usbtmc_termchar) +#define USBTMC_IOCTL_WRITE _IOWR(USBTMC_IOC_NR, 13, struct usbtmc_message) +#define USBTMC_IOCTL_READ _IOWR(USBTMC_IOC_NR, 14, struct usbtmc_message) +#define USBTMC_IOCTL_WRITE_RESULT _IOWR(USBTMC_IOC_NR, 15, __u64) #define USBTMC488_IOCTL_GET_CAPS _IOR(USBTMC_IOC_NR, 17, unsigned char) #define USBTMC488_IOCTL_READ_STB _IOR(USBTMC_IOC_NR, 18, unsigned char) @@ -80,6 +97,10 @@ struct usbtmc_termchar { #define USBTMC488_IOCTL_LOCAL_LOCKOUT _IO(USBTMC_IOC_NR, 21) #define USBTMC488_IOCTL_TRIGGER _IO(USBTMC_IOC_NR, 22) +/* Cancel and cleanup asynchronous calls */ +#define USBTMC_IOCTL_CANCEL_IO _IO(USBTMC_IOC_NR, 35) +#define USBTMC_IOCTL_CLEANUP_IO _IO(USBTMC_IOC_NR, 36) + /* Driver encoded usb488 capabilities */ #define USBTMC488_CAPABILITY_TRIGGER 1 #define USBTMC488_CAPABILITY_SIMPLE 2