Re: [PATCH v2 1/3] usb: gadget: net2280: Fix overrun of OUT messages
Zitat von Alan Stern : On Mon, 18 Mar 2019, Guido Kiener wrote: The OUT endpoint normally blocks (NAK) subsequent packets when a short packet was received and returns an incomplete queue entry to the gadget driver. Thereby the gadget driver can detect a short packet when reading queue entries with a length that is not equal to a multiple of packet size. The start_queue() function enables receiving OUT packets regardless of the content of the OUT FIFO. This results in a problem: When receiving is enabled again, more OUT packets are appended to the last short packet and the gadget driver cannot determine the end of a short packet anymore. Furthermore the remaining data in the OUT FIFO is not delivered to the gadget driver immediately and can produce timeout errors. This fix stops OUT naking only when OUT naking is enabled and when the OUT FIFO is empty. It ensures that all received data is delivered to the gadget driver which can detect a short packet now before new packets overrun the last short packet. This description should explain the race which causes the problem. With the current code, it's possible that the "!ep->is_in && (readl(&ep->regs->ep_stat) & BIT(NAK_OUT_PACKETS))" test will fail, then a short packet will be received, and then start_queue() will call stop_out_naking(). That's what we don't want -- OUT naking gets turned off while there is data in the FIFO. With the patch, this race can't occur because the FIFO's state is tested after we know that OUT naking is already turned on. Thanks. Please feel free to change my wording/spelling. I'm not offended. Does this description meet your request: The OUT endpoint normally blocks (NAK) subsequent packets when a short packet was received and returns an incomplete queue entry to the gadget driver. Thereby the gadget driver can detect a short packet when reading queue entries with a length that is not equal to a multiple of packet size. The start_queue() function enables receiving OUT packets regardless of the content of the OUT FIFO. This results in a race: With the current code, it's possible that the "!ep->is_in && (readl(&ep->regs->ep_stat) & BIT(NAK_OUT_PACKETS))" test will fail, then a short packet will be received, and then start_queue() will call stop_out_naking(). That's what we don't want -- OUT naking gets turned off while there is data in the FIFO. With the patch, this race can't occur because the FIFO's state is tested after we know that OUT naking is already turned on. Guido That patch itself looks good, I just think the explanation should be improved. Alan Stern Signed-off-by: Guido Kiener --- drivers/usb/gadget/udc/net2280.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c index f63f82450bf4..e0b413e9e532 100644 --- a/drivers/usb/gadget/udc/net2280.c +++ b/drivers/usb/gadget/udc/net2280.c @@ -866,9 +866,6 @@ static void start_queue(struct net2280_ep *ep, u32 dmactl, u32 td_dma) (void) readl(&ep->dev->pci->pcimstctl); writel(BIT(DMA_START), &dma->dmastat); - - if (!ep->is_in) - stop_out_naking(ep); } static void start_dma(struct net2280_ep *ep, struct net2280_request *req) @@ -907,6 +904,7 @@ static void start_dma(struct net2280_ep *ep, struct net2280_request *req) writel(BIT(DMA_START), &dma->dmastat); return; } + stop_out_naking(ep); } tmp = dmactl_default;
Re: [PATCH v2 1/3] usb: gadget: net2280: Fix overrun of OUT messages
Zitat von Alan Stern : Here are a few more slight changes to the text. Submit the patch again with this description and it will be okay: The OUT endpoint normally blocks (NAK) subsequent packets when a short packet was received and returns an incomplete queue entry to the gadget driver. Thereby the gadget driver can detect a short packet when reading queue entries with a length that is not equal to a multiple of packet size. The start_queue() function enables receiving OUT packets regardless of the content of the OUT FIFO. This results in a race: With the current code, it's possible that the "!ep->is_in && (readl(&ep->regs->ep_stat) & BIT(NAK_OUT_PACKETS))" test in start_dma() will fail, then a short packet will be received, and then start_queue() will call stop_out_naking(). That's what we don't want (OUT naking gets turned off while there is data in the FIFO) because then the next driver request might receive a mixture of old and new packets. With the patch, this race can't occur because the FIFO's state is tested after we know that OUT naking is already turned on, and OUT naking is stopped only when both of the conditions are met. This ensures that all received data is delivered to the gadget driver, which can detect a short packet now before new packets are appended to the last short packet. Thank you. Sounds good. Guido
Re: [PATCH v2 3/3] usb: gadget: net2272: Fix net2272_dequeue()
Zitat von Guido Kiener : Restore the status of ep->stopped in function net2272_dequeue(). When the given request is not found in the endpoint queue the function returns -EINVAL without restoring the state of ep->stopped. Thus the endpoint keeps blocked and does not transfer any data anymore. This fix is only compile-tested, since we do not have a corresponding hardware. An analogous fix was tested in the sibling driver. See "usb: gadget: net2280: Fix net2280_dequeue()" Signed-off-by: Guido Kiener --- drivers/usb/gadget/udc/net2272.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/gadget/udc/net2272.c b/drivers/usb/gadget/udc/net2272.c index b77f3126580e..c2011cd7df8c 100644 --- a/drivers/usb/gadget/udc/net2272.c +++ b/drivers/usb/gadget/udc/net2272.c @@ -945,6 +945,7 @@ net2272_dequeue(struct usb_ep *_ep, struct usb_request *_req) break; } if (&req->req != _req) { + ep->stopped = stopped; spin_unlock_irqrestore(&ep->dev->lock, flags); return -EINVAL; } -- 2.17.1 Alan, do you want to add an acknowledgement for the sibling driver, too? Guido
Re: [PATCH 02/12] usb: usbtmc: Support Read Status Byte with SRQ per file handle
Zitat von Greg KH : On Thu, May 17, 2018 at 07:03:26PM +0200, Guido Kiener wrote: - Add 'struct usbtmc_file_data' for each file handle to cache last srq_byte (=Status Byte with SRQ) received by usbtmc_interrupt(..) - usbtmc488_ioctl_read_stb returns cached srq_byte when available for each file handle to avoid race conditions of concurrent applications. - SRQ now sets EPOLLPRI instead of EPOLLIN - Caches other values TermChar, TermCharEnabled, auto_abort in 'struct usbtmc_file_data' will not be changed by sysfs device attributes during an open file session. Future ioctl functions can change these values. - use consistent error return value ETIMEOUT instead of ETIME Tested-by: Dave Penkler Reviewed-by: Steve Bayless Signed-off-by: Guido Kiener --- drivers/usb/class/usbtmc.c | 176 - 1 file changed, 136 insertions(+), 40 deletions(-) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index 529295a17579..5754354429d8 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -67,6 +67,7 @@ struct usbtmc_device_data { const struct usb_device_id *id; struct usb_device *usb_dev; struct usb_interface *intf; + struct list_head file_list; unsigned int bulk_in; unsigned int bulk_out; @@ -87,12 +88,12 @@ struct usbtmc_device_data { intiin_interval; struct urb*iin_urb; u16iin_wMaxPacketSize; - atomic_t srq_asserted; /* coalesced usb488_caps from usbtmc_dev_capabilities */ __u8 usb488_caps; /* attributes from the USB TMC spec for this device */ + /* They are used as default values for file_data */ u8 TermChar; bool TermCharEnabled; bool auto_abort; @@ -104,9 +105,26 @@ struct usbtmc_device_data { struct mutex io_mutex; /* only one i/o function running at a time */ wait_queue_head_t waitq; struct fasync_struct *fasync; + spinlock_t dev_lock; /* lock for file_list */ }; #define to_usbtmc_data(d) container_of(d, struct usbtmc_device_data, kref) +/* + * This structure holds private data for each USBTMC file handle. + */ +struct usbtmc_file_data { + struct usbtmc_device_data *data; + struct list_head file_elem; + + u8 srq_byte; + atomic_t srq_asserted; + + /* These values are initialized with default values from device_data */ + u8 TermChar; + bool TermCharEnabled; I don't remember, does TermChar and TermCharEnabled come from a specification somewhere? Is that why they are in InterCaps format? TermChar and TermCharEnabled was introducted 10 years ago by your patches. We can rename these fields in term_char and term_char_enabled as well. And why duplicate these fields as they are already in the device-specific data structure? We do not need it in device-specific data structure. We need it in file-specific data structure. We keep it in device-specific data structure, since we do not want to break previous applications that wants to change it with via sysfs. + bool auto_abort; +}; + /* Forward declarations */ static struct usb_driver usbtmc_driver; @@ -114,6 +132,7 @@ static void usbtmc_delete(struct kref *kref) { struct usbtmc_device_data *data = to_usbtmc_data(kref); + pr_debug("%s - called\n", __func__); usb_put_dev(data->usb_dev); kfree(data); } @@ -122,7 +141,7 @@ static int usbtmc_open(struct inode *inode, struct file *filp) { struct usb_interface *intf; struct usbtmc_device_data *data; - int retval = 0; + struct usbtmc_file_data *file_data; intf = usb_find_interface(&usbtmc_driver, iminor(inode)); if (!intf) { @@ -130,21 +149,54 @@ static int usbtmc_open(struct inode *inode, struct file *filp) return -ENODEV; } + file_data = kzalloc(sizeof(*file_data), GFP_KERNEL); + if (!file_data) + return -ENOMEM; + + pr_debug("%s - called\n", __func__); Please do not add "tracing" functions like this. The kernel has a wonderful built-in function tracing functionality, don't try to write your own. These lines should just be removed. Ok, I agree. Sorry, I'm a newbie. + data = usb_get_intfdata(intf); /* Protect reference to data from file structure until release */ kref_get(&data->kref); + mutex_lock(&data->io_mutex); + file_data->data = data; + + /* copy default values from device settings */ + file_data->TermChar = data->TermChar; + file_data->TermCharEnabled = data->TermCharEnabled; + file_data->auto_abort = data->auto_abort; + + INIT_LIST_HEAD(&file_data->file_elem); + spin_lock_irq(&da
Re: [PATCH 00/12] usb: usbtmc: Changes needed for compatible IVI/VISA library
Zitat von Greg KH : On Thu, May 17, 2018 at 07:03:24PM +0200, Guido Kiener wrote: The working group "VISA for Linux" of the IVI Foundation www.ivifoundation.org specifies common rules, shared libraries and drivers to implement the specification of "VPP-4.3: The VISA Library" on Linux to be compatible with implementations on other operating systems. The USBTMC protocol is part of the "VISA Library" that is used by many popular T&M applications. An initial implementation for Linux based on libusb has been created. While functional it has some drawbacks: - Performance - Requires root privileges to reclaim devices already claimed by the usbtmc driver The following collaborative patches meet the requirements of the IVI Foundation to implement the library based on the usbtmc driver. Improvements in the data transfer rate of over 130 MByte/s for usb 3.x connections have been measured. Why is the libusb version "slower"? Last I checked, we could reach line-speeds for USB quite easily from userspace with no problem. If you keep the endpoints full you should be just fine. Unless you have a horrid protocol that doesn't allow for that :) Sorry, the wording is misleading. The performance of libusb is pretty well, whereas the bandwidth of the current usbtmc driver is slow. From my point of view the main advantage of the new usbtmc driver in contrast to libusb is: - Multiple applications can share access to the same instruments. - The driver handles SRQ conflicts. - usbtmc driver simplifies definition of udev rules - usbtmc driver simplifies development of applications using T&M instruments. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/12] usb: usbtmc: Add ioctls for trigger, EOM bit and TermChar
Zitat von Greg KH : On Thu, May 17, 2018 at 07:03:28PM +0200, Guido Kiener wrote: - add USBTMC488_IOCTL_TRIGGER to send TRIGGER Bulk-OUT header according to Subclass USB488 Specification The usbtmc trigger command is equivalent to the IEEE 488 GET (Group Execute Trigger) action. While the "*TRG" command can be sent as data to perform the same operation, in some situations an instrument will be busy and unable to process the data immediately in which case the USBTMC488_IOCTL_TRIGGER can be used to trigger the instrument with lower latency. - add USBTMC_IOCTL_EOM_ENABLE to specify EOM bit for next write() call. Sets Bit 0 of field 'bmTransferAttributes' of DEV_DEP_MSG_OUT Bulk-OUT Header. Allows fine grained control over end of message handling on a per file descriptor basis. - add USBTMC_IOCTL_CONFIG_TERMCHAR to control TermChar handling for next read(). Controls field 'TermChar' and Bit 1 of field 'bmTransferAttributes' of REQUEST_DEV_DEP_MSG_IN BULK-OUT header. Allows enabling/disabling of terminating a read on reception of term_char individually for each read request. Why isn't this 3 patches? Please only do "one thing per patch". I just thought it is clearly arranged. Please let me know when you want to see a breakup of other patches as well. However I would be happy when I could save some extra work here. In the middle of next week I start my holiday. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/12] usb: usbtmc: Support Read Status Byte with SRQ per file handle
Zitat von Greg KH : On Fri, May 18, 2018 at 11:52:36AM +, gu...@kiener-muenchen.de wrote: Zitat von Greg KH : > On Thu, May 17, 2018 at 07:03:26PM +0200, Guido Kiener wrote: > > - Add 'struct usbtmc_file_data' for each file handle to cache last > > srq_byte (=Status Byte with SRQ) received by usbtmc_interrupt(..) > > > > - usbtmc488_ioctl_read_stb returns cached srq_byte when available for > > each file handle to avoid race conditions of concurrent applications. > > > > - SRQ now sets EPOLLPRI instead of EPOLLIN > > > > - Caches other values TermChar, TermCharEnabled, auto_abort in > > 'struct usbtmc_file_data' will not be changed by sysfs device > > attributes during an open file session. > > Future ioctl functions can change these values. > > > > - use consistent error return value ETIMEOUT instead of ETIME > > > > Tested-by: Dave Penkler > > Reviewed-by: Steve Bayless > > Signed-off-by: Guido Kiener > > --- > > drivers/usb/class/usbtmc.c | 176 - > > 1 file changed, 136 insertions(+), 40 deletions(-) > > > > diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c > > index 529295a17579..5754354429d8 100644 > > --- a/drivers/usb/class/usbtmc.c > > +++ b/drivers/usb/class/usbtmc.c > > @@ -67,6 +67,7 @@ struct usbtmc_device_data { > > const struct usb_device_id *id; > > struct usb_device *usb_dev; > > struct usb_interface *intf; > > + struct list_head file_list; > > > > unsigned int bulk_in; > > unsigned int bulk_out; > > @@ -87,12 +88,12 @@ struct usbtmc_device_data { > > intiin_interval; > > struct urb*iin_urb; > > u16iin_wMaxPacketSize; > > - atomic_t srq_asserted; > > > > /* coalesced usb488_caps from usbtmc_dev_capabilities */ > > __u8 usb488_caps; > > > > /* attributes from the USB TMC spec for this device */ > > + /* They are used as default values for file_data */ > > u8 TermChar; > > bool TermCharEnabled; > > bool auto_abort; > > @@ -104,9 +105,26 @@ struct usbtmc_device_data { > > struct mutex io_mutex; /* only one i/o function running at a time */ > > wait_queue_head_t waitq; > > struct fasync_struct *fasync; > > + spinlock_t dev_lock; /* lock for file_list */ > > }; > > #define to_usbtmc_data(d) container_of(d, struct usbtmc_device_data, kref) > > > > +/* > > + * This structure holds private data for each USBTMC file handle. > > + */ > > +struct usbtmc_file_data { > > + struct usbtmc_device_data *data; > > + struct list_head file_elem; > > + > > + u8 srq_byte; > > + atomic_t srq_asserted; > > + > > + /* These values are initialized with default values from device_data */ > > + u8 TermChar; > > + bool TermCharEnabled; > > I don't remember, does TermChar and TermCharEnabled come from a > specification somewhere? Is that why they are in InterCaps format? TermChar and TermCharEnabled was introducted 10 years ago by your patches. Wow, 2008, I can't remember what code I wrote last week, let alone a decade ago :) We can rename these fields in term_char and term_char_enabled as well. Odds are I did it this way because it matches the names of the fields of the specification. If you all have access to the spec, it should be easy to look up, right? That's right, but I think it looks better to use term_char and term_char_enabled. I guess users will find out the relationship with the specification. > And why duplicate these fields as they are already in the > device-specific data structure? We do not need it in device-specific data structure. We need it in file-specific data structure. We keep it in device-specific data structure, since we do not want to break previous applications that wants to change it with via sysfs. Ah, well it would be good to somehow document this. But, if no one is using the existing sysfs files, they can be removed. You just have to agree that if a user shows up, you add them back. In the past (6 months) nobody told me that he is using the sysfs files. And I can promise to add them back when someone claims it. > > + spin_lock_irq(&data->dev_lock); > > + srq_asserted = atomic_xchg(&file_data->srq_asserted, srq_asserted); > > That really frightens me. Why are you messing with atomic values here? > What is it supposed to be "protecting" or "d
Re: [PATCH 05/12] usb: usbtmc: Add ioctl for generic requests on control pipe
Zitat von Greg KH : On Thu, May 17, 2018 at 07:03:29PM +0200, Guido Kiener wrote: Add USBTMC_IOCTL_CTRL_REQUEST to send arbitrary requests on the control pipe. Used by specific applications of IVI Foundation, Inc. to implement VISA API functions: viUsbControlIn/Out. Signed-off-by: Guido Kiener Reviewed-by: Steve Bayless --- drivers/usb/class/usbtmc.c | 61 include/uapi/linux/usb/tmc.h | 15 + 2 files changed, 76 insertions(+) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index 152e2daa9644..00c2e51a23a7 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -5,6 +5,7 @@ * Copyright (C) 2007 Stefan Kopp, Gechingen, Germany * Copyright (C) 2008 Novell, Inc. * Copyright (C) 2008 Greg Kroah-Hartman + * Copyright (C) 2018, IVI Foundation, Inc. */ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt @@ -1263,6 +1264,62 @@ static int usbtmc_ioctl_indicator_pulse(struct usbtmc_device_data *data) return rv; } +static int usbtmc_ioctl_request(struct usbtmc_device_data *data, + void __user *arg) +{ + struct device *dev = &data->intf->dev; + struct usbtmc_ctrlrequest request; + u8 *buffer = NULL; + int rv; + unsigned long res; + + res = copy_from_user(&request, arg, sizeof(struct usbtmc_ctrlrequest)); + if (res) + return -EFAULT; + + buffer = kmalloc(min_t(u16, 256, request.req.wLength), GFP_KERNEL); No validation that wLength is a sane number? Ooops. It should be buffer = kmalloc(max_t(u16, 256, request.req.wLength), GFP_KERNEL); Then all values of request.req.wLength (0..65535) are ok. Go to the whiteboard nearby and write a the top of it: ALL INPUT IS EVIL + if (!buffer) + return -ENOMEM; + + if ((request.req.bRequestType & USB_DIR_IN) == 0 + && request.req.wLength) { + res = copy_from_user(buffer, request.data, +request.req.wLength); + if (res) { + rv = -EFAULT; + goto exit; + } + } + + rv = usb_control_msg(data->usb_dev, + usb_rcvctrlpipe(data->usb_dev, 0), + request.req.bRequest, + request.req.bRequestType, + request.req.wValue, + request.req.wIndex, + buffer, request.req.wLength, USB_CTRL_GET_TIMEOUT); request.req.wLength might not be the actual size of buffer here due to your above min_t() check. + + if (rv < 0) { + dev_err(dev, "%s failed %d\n", __func__, rv); + goto exit; + } + if ((request.req.bRequestType & USB_DIR_IN)) { + if (rv > request.req.wLength) { + dev_warn(dev, "%s returned too much data: %d\n", +__func__, rv); + rv = request.req.wLength; Why are you returning a value that is greater than the size asked for? That's going to make userspace confused. I followed the rule INPUT IS EVIL, but I'm not sure whether this really can happen that a device can return more than requested. I will a have a closer look again. Then there is the generic question of "why are you allowing arbitrary USB control messages to be sent to devices" here. That feels like a very odd way for a device that is supposed to be following a standard to be working. You are circumventing the standard here by allowing any message to be sent to the device. While nice for that one type of device, you are breaking interoperability in horrible ways (now apps only work with one type of device.) Why did you all agree to allow this to happen? I did not define the USBTMC and VISA standard. However the API requires to send this generic control request. Otherwise we have to use the libusb again. http://zone.ni.com/reference/en-XX/help/370131S-01/ni-visa/viusbcontrolin/ thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/12] usb: usbtmc: Add vendor specific/asynchronous read/write ioctls
Zitat von Greg KH : 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/12] usb: usbtmc: Add vendor specific/asynchronous read/write ioctls
Zitat von Greg KH : 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/12] usb: usbtmc: Add ioctls to set/get usb timeout
Zitat von Greg KH : On Thu, May 17, 2018 at 07:03:27PM +0200, Guido Kiener wrote: Add ioctls USBTMC_IOCTL_GET_TIMEOUT / USBTMC_IOCTL_SET_TIMEOUT to get/set I/O timeout for specific file handle. Different operations on an instrument can take different lengths of time thus it is important to be able to set the timeout slightly longer than the expected duration of each operation to optimise the responsiveness of the application. As the instrument may be shared by multiple applications the timeout should be settable on a per file descriptor basis. Tested-by: Dave Penkler Reviewed-by: Steve Bayless Signed-off-by: Dave Penkler Signed-off-by: Guido Kiener --- drivers/usb/class/usbtmc.c | 58 +--- include/uapi/linux/usb/tmc.h | 2 ++ 2 files changed, 56 insertions(+), 4 deletions(-) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index 5754354429d8..ad7872003420 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -30,6 +30,8 @@ */ #define USBTMC_SIZE_IOBUFFER 2048 +/* Minimum USB timeout (in milliseconds) */ +#define USBTMC_MIN_TIMEOUT 100 /* Default USB timeout (in milliseconds) */ #define USBTMC_TIMEOUT 5000 @@ -116,6 +118,7 @@ struct usbtmc_file_data { struct usbtmc_device_data *data; struct list_head file_elem; + u32timeout; u8 srq_byte; atomic_t srq_asserted; @@ -163,6 +166,7 @@ static int usbtmc_open(struct inode *inode, struct file *filp) file_data->data = data; /* copy default values from device settings */ + file_data->timeout = USBTMC_TIMEOUT; file_data->TermChar = data->TermChar; file_data->TermCharEnabled = data->TermCharEnabled; file_data->auto_abort = data->auto_abort; @@ -478,7 +482,7 @@ static int usbtmc488_ioctl_read_stb(struct usbtmc_file_data *file_data, rv = wait_event_interruptible_timeout( data->waitq, atomic_read(&data->iin_data_valid) != 0, - USBTMC_TIMEOUT); + file_data->timeout); if (rv < 0) { dev_dbg(dev, "wait interrupted %d\n", rv); goto exit; @@ -613,7 +617,8 @@ static int send_request_dev_dep_msg_in(struct usbtmc_file_data *file_data, retval = usb_bulk_msg(data->usb_dev, usb_sndbulkpipe(data->usb_dev, data->bulk_out), - buffer, USBTMC_HEADER_SIZE, &actual, USBTMC_TIMEOUT); + buffer, USBTMC_HEADER_SIZE, + &actual, file_data->timeout); /* Store bTag (in case we need to abort) */ data->bTag_last_write = data->bTag; @@ -681,7 +686,7 @@ static ssize_t usbtmc_read(struct file *filp, char __user *buf, usb_rcvbulkpipe(data->usb_dev, data->bulk_in), buffer, USBTMC_SIZE_IOBUFFER, &actual, - USBTMC_TIMEOUT); + file_data->timeout); dev_dbg(dev, "usb_bulk_msg: retval(%u), done(%zu), remaining(%zu), actual(%d)\n", retval, done, remaining, actual); @@ -854,7 +859,7 @@ static ssize_t usbtmc_write(struct file *filp, const char __user *buf, usb_sndbulkpipe(data->usb_dev, data->bulk_out), buffer, n_bytes, - &actual, USBTMC_TIMEOUT); + &actual, file_data->timeout); if (retval != 0) break; n_bytes -= actual; @@ -1211,6 +1216,41 @@ static int usbtmc_ioctl_indicator_pulse(struct usbtmc_device_data *data) return rv; } +/* + * Get the usb timeout value + */ +static int usbtmc_ioctl_get_timeout(struct usbtmc_file_data *file_data, + void __user *arg) +{ + __u32 timeout; + + timeout = file_data->timeout; + + return put_user(timeout, (__u32 __user *)arg); +} + +/* + * Set the usb timeout value + */ +static int usbtmc_ioctl_set_timeout(struct usbtmc_file_data *file_data, + void __user *arg) +{ + __u32 timeout; + + if (get_user(timeout, (__u32 __user *)arg)) + return -EFAULT; + + /* Note that timeout = 0 means +* MAX_SCHEDULE_TIMEOUT in usb_control_msg +*/ + if (timeout < USBTMC_MIN_TIMEOUT) + return -EINVAL; + +
Re: [PATCH 07/12] usb: usbtmc: Add ioctl USBTMC488_IOCTL_WAIT_SRQ
Zitat von Greg KH : On Thu, May 17, 2018 at 07:03:31PM +0200, Guido Kiener wrote: @@ -2420,8 +2476,8 @@ static int usbtmc_probe(struct usb_interface *intf, retcode = usb_register_dev(intf, &usbtmc_class); if (retcode) { - dev_err(&intf->dev, "Not able to get a minor" - " (base %u, slice default): %d\n", USBTMC_MINOR_BASE, + dev_err(&intf->dev, "Not able to get a minor (base %u, slice default): %d\n", + USBTMC_MINOR_BASE, retcode); Nice change, but totally not relevant for this specific patch :( Extra patch or just omit? I thought kernel message must not be broken due to rule 80 characters per line. diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h index 1540716de293..35b63530121d 100644 --- a/include/uapi/linux/usb/tmc.h +++ b/include/uapi/linux/usb/tmc.h @@ -96,6 +96,7 @@ struct usbtmc_message { #define USBTMC488_IOCTL_GOTO_LOCAL _IO(USBTMC_IOC_NR, 20) #define USBTMC488_IOCTL_LOCAL_LOCKOUT _IO(USBTMC_IOC_NR, 21) #define USBTMC488_IOCTL_TRIGGER_IO(USBTMC_IOC_NR, 22) +#define USBTMC488_IOCTL_WAIT_SRQ _IOW(USBTMC_IOC_NR, 23, unsigned int) Again __u32? Yes. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/12] usb: usbtmc: Add vendor specific/asynchronous read/write ioctls
Zitat von Greg KH : On Fri, May 18, 2018 at 02:48:11PM +, gu...@kiener-muenchen.de wrote: Zitat von Greg KH : > On Thu, May 17, 2018 at 07:03:30PM +0200, Guido Kiener wrote: > > +/* > > + * usbtmc_message->flags: > > + */ > > +#define USBTMC_FLAG_ASYNC0x0001 > > +#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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/12] usb: usbtmc: Add ioctl USBTMC488_IOCTL_WAIT_SRQ
Zitat von Greg KH : On Fri, May 18, 2018 at 03:02:10PM +, gu...@kiener-muenchen.de wrote: Zitat von Greg KH : > On Thu, May 17, 2018 at 07:03:31PM +0200, Guido Kiener wrote: > > @@ -2420,8 +2476,8 @@ static int usbtmc_probe(struct usb_interface *intf, > > > > retcode = usb_register_dev(intf, &usbtmc_class); > > if (retcode) { > > - dev_err(&intf->dev, "Not able to get a minor" > > - " (base %u, slice default): %d\n", USBTMC_MINOR_BASE, > > + dev_err(&intf->dev, "Not able to get a minor (base %u, slice > > default): %d\n", > > + USBTMC_MINOR_BASE, > > retcode); > > Nice change, but totally not relevant for this specific patch :( Extra patch or just omit? Extra patch please :) I thought kernel message must not be broken due to rule 80 characters per line. You are correct, just don't try to "sneak" it into a patch that does something totally different. Thank you very much for you review. I will send the next "version" after weekend or after my holiday. Best regards, Guido thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/12] usb: usbtmc: Add ioctls to set/get usb timeout
Zitat von Andy Shevchenko : On Thu, May 17, 2018 at 8:03 PM, Guido Kiener wrote: Add ioctls USBTMC_IOCTL_GET_TIMEOUT / USBTMC_IOCTL_SET_TIMEOUT to get/set I/O timeout for specific file handle. +static int usbtmc_ioctl_get_timeout(struct usbtmc_file_data *file_data, + void __user *arg) +{ + __u32 timeout; + + timeout = file_data->timeout; Why do you need __u32 on kernel side? Would plain u32 work? It compiles and links with u32 and __u32 (and even double). AFAIK __u32 is for user code and u32 for kernel code. So, correct is here what the maintainer says or experienced kernel developers :-). My opinion here is that put_user(..) and get_user(..) should always copy to the same type to always have a safe copy operation from/to userland. So I prefer __u32. If you are sure that u32 is more correct here, then we use u32. Best regards, Guido + + return put_user(timeout, (__u32 __user *)arg); +} +static int usbtmc_ioctl_set_timeout(struct usbtmc_file_data *file_data, + void __user *arg) +{ + __u32 timeout; Ditto. + return 0; +} -- With Best Regards, Andy Shevchenko -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/12] usb: usbtmc: Add vendor specific/asynchronous read/write ioctls
Zitat von Greg KH : On Fri, May 18, 2018 at 02:48:11PM +, gu...@kiener-muenchen.de wrote: Zitat von Greg KH : > On Thu, May 17, 2018 at 07:03:30PM +0200, Guido Kiener wrote: > > +/* > > + * usbtmc_message->flags: > > + */ > > +#define USBTMC_FLAG_ASYNC0x0001 > > +#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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/12] usb: usbtmc: Support Read Status Byte with SRQ per file handle
Zitat von Greg KH : @@ -122,7 +141,7 @@ static int usbtmc_open(struct inode *inode, struct file *filp) { struct usb_interface *intf; struct usbtmc_device_data *data; - int retval = 0; + struct usbtmc_file_data *file_data; intf = usb_find_interface(&usbtmc_driver, iminor(inode)); if (!intf) { @@ -130,21 +149,54 @@ static int usbtmc_open(struct inode *inode, struct file *filp) return -ENODEV; } + file_data = kzalloc(sizeof(*file_data), GFP_KERNEL); + if (!file_data) + return -ENOMEM; + + pr_debug("%s - called\n", __func__); Please do not add "tracing" functions like this. The kernel has a wonderful built-in function tracing functionality, don't try to write your own. These lines should just be removed. + data = usb_get_intfdata(intf); /* Protect reference to data from file structure until release */ kref_get(&data->kref); + mutex_lock(&data->io_mutex); + file_data->data = data; + + /* copy default values from device settings */ + file_data->TermChar = data->TermChar; + file_data->TermCharEnabled = data->TermCharEnabled; + file_data->auto_abort = data->auto_abort; + + INIT_LIST_HEAD(&file_data->file_elem); + spin_lock_irq(&data->dev_lock); + list_add_tail(&file_data->file_elem, &data->file_list); + spin_unlock_irq(&data->dev_lock); + mutex_unlock(&data->io_mutex); + /* Store pointer in file structure's private data field */ - filp->private_data = data; + filp->private_data = file_data; - return retval; + return 0; } static int usbtmc_release(struct inode *inode, struct file *file) { - struct usbtmc_device_data *data = file->private_data; + struct usbtmc_file_data *file_data = file->private_data; - kref_put(&data->kref, usbtmc_delete); + pr_debug("%s - called\n", __func__); Again, these all need to be dropped. + + /* prevent IO _AND_ usbtmc_interrupt */ + mutex_lock(&file_data->data->io_mutex); + spin_lock_irq(&file_data->data->dev_lock); + + list_del(&file_data->file_elem); + + spin_unlock_irq(&file_data->data->dev_lock); + mutex_unlock(&file_data->data->io_mutex); You protect the list, but what about removing the data itself? + + kref_put(&file_data->data->kref, usbtmc_delete); What protects this from being called at the same time a kref_get is being called? Yeah, it previously probably already had this race, sorry I never noticed that. I looked for a race here, but I do not find a race between open and release, since a refcount of "file_data->data->kref" is always hold by usbtmc_probe/disconnect. However I see a race between usbtmc_open and usbtmc_disconnect. Are these callback functions called mutual exclusive? I'm not sure, but if not, then I think we have the same problem in usb-skeleton.c + file_data->data = NULL; + kfree(file_data); return 0; } thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/12] usb: usbtmc: Add ioctl for generic requests on control pipe
Zitat von Greg KH : On Fri, May 18, 2018 at 01:32:51PM +, gu...@kiener-muenchen.de wrote: Zitat von Greg KH : > On Thu, May 17, 2018 at 07:03:29PM +0200, Guido Kiener wrote: > > Add USBTMC_IOCTL_CTRL_REQUEST to send arbitrary requests on the > > control pipe. Used by specific applications of IVI Foundation, > > Inc. to implement VISA API functions: viUsbControlIn/Out. > > > > Signed-off-by: Guido Kiener > > Reviewed-by: Steve Bayless > > --- > > drivers/usb/class/usbtmc.c | 61 > > include/uapi/linux/usb/tmc.h | 15 + > > 2 files changed, 76 insertions(+) > > > > diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c > > index 152e2daa9644..00c2e51a23a7 100644 > > --- a/drivers/usb/class/usbtmc.c > > +++ b/drivers/usb/class/usbtmc.c > > @@ -5,6 +5,7 @@ > > * Copyright (C) 2007 Stefan Kopp, Gechingen, Germany > > * Copyright (C) 2008 Novell, Inc. > > * Copyright (C) 2008 Greg Kroah-Hartman > > + * Copyright (C) 2018, IVI Foundation, Inc. > > */ > > > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > @@ -1263,6 +1264,62 @@ static int > > usbtmc_ioctl_indicator_pulse(struct usbtmc_device_data *data) > > return rv; > > } > > > > +static int usbtmc_ioctl_request(struct usbtmc_device_data *data, > > + void __user *arg) > > +{ > > + struct device *dev = &data->intf->dev; > > + struct usbtmc_ctrlrequest request; > > + u8 *buffer = NULL; > > + int rv; > > + unsigned long res; > > + > > + res = copy_from_user(&request, arg, sizeof(struct usbtmc_ctrlrequest)); > > + if (res) > > + return -EFAULT; > > + > > + buffer = kmalloc(min_t(u16, 256, request.req.wLength), GFP_KERNEL); > > No validation that wLength is a sane number? Ooops. It should be buffer = kmalloc(max_t(u16, 256, request.req.wLength), GFP_KERNEL); Then all values of request.req.wLength (0..65535) are ok. Yes, that would make more sense. But you should still reject known-invalid values, and not just "silently fix them up". When I start here to refuse (current) unknown settings or flags, then I fear that people always want us to change or allow new flags. A device should always be able to deal with all possible values. We cannot prevent a user from sending crazy messages to the device and I do not want to develop something like a firewall here. > Go to the whiteboard nearby and write a the top of it: >ALL INPUT IS EVIL > > > + if (!buffer) > > + return -ENOMEM; > > + > > + if ((request.req.bRequestType & USB_DIR_IN) == 0 > > + && request.req.wLength) { > > + res = copy_from_user(buffer, request.data, > > + request.req.wLength); > > + if (res) { > > + rv = -EFAULT; > > + goto exit; > > + } > > + } > > + > > + rv = usb_control_msg(data->usb_dev, > > + usb_rcvctrlpipe(data->usb_dev, 0), > > + request.req.bRequest, > > + request.req.bRequestType, > > + request.req.wValue, > > + request.req.wIndex, > > + buffer, request.req.wLength, USB_CTRL_GET_TIMEOUT); > > request.req.wLength might not be the actual size of buffer here due to > your above min_t() check. Still wrong given the check above. I'm missing something. I do not see the error. The size of buffer is just at least 256 bytes. It doesn't matter when request.req.wLength < 256. I thought this helps the memory management, since we need not to deal with different and odd memory sizes. Maybe we should just alloc always a size of request.req.wLength. > Then there is the generic question of "why are you allowing arbitrary > USB control messages to be sent to devices" here. That feels like a > very odd way for a device that is supposed to be following a standard to > be working. You are circumventing the standard here by allowing any > message to be sent to the device. While nice for that one type of > device, you are breaking interoperability in horrible ways (now apps > only work with one type of device.) > > Why did you all agree to allow this to happen? I did not define the USBTMC and VISA standard. However the API requires to send this generic control request. Otherwise we have to use the libusb again. http://zone.ni.com/reference/en-XX/help/370131S-01/ni-visa/viusbcontrolin/ Ugh, that's horrid. And a sign of "who knows what our devices will want to support!" design by committee (note, I've worked on lots of spec groups before, I know how they work...) Ok, so that needs to be supported, fair enough, so let's get the implementation correct :) thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/12] usb: usbtmc: Add vendor specific/asynchronous read/write ioctls
Zitat von Greg KH : On Mon, May 21, 2018 at 08:41:22PM +, gu...@kiener-muenchen.de wrote: Zitat von Greg KH : > On Fri, May 18, 2018 at 02:48:11PM +, gu...@kiener-muenchen.de wrote: > > > > Zitat von Greg KH : > > > > > 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", 4000, &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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/12] usb: usbtmc: Support Read Status Byte with SRQ per file handle
Zitat von Oliver Neukum : Am Montag, den 21.05.2018, 21:00 + schrieb guido@kiener- muenchen.de: I looked for a race here, but I do not find a race between open and release, since a refcount of "file_data->data->kref" is always hold by usbtmc_probe/disconnect. However I see a race between usbtmc_open and usbtmc_disconnect. Are these callback functions called mutual exclusive? No, they are not. In the meantime I found these conflictive hints: https://github.com/torvalds/linux/commit/52a749992ca6a0fd304609af40ed3bfd6cef4660 and https://elixir.bootlin.com/linux/v4.17-rc6/source/include/linux/usb.h#L1164 What do you think? My current feeling is that open/disconnect is mutual exclusive. We also could verify what really happens. Thanks, Guido I'm not sure, but if not, then I think we have the same problem in usb-skeleton.c In usb-skeleton.c a race exists. You are right. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/12] usb: usbtmc: Add ioctls for trigger, EOM bit and TermChar
Zitat von Oliver Neukum : Am Donnerstag, den 17.05.2018, 19:03 +0200 schrieb Guido Kiener: + retval = usb_bulk_msg(data->usb_dev, + usb_sndbulkpipe(data->usb_dev, + data->bulk_out), + buffer, USBTMC_HEADER_SIZE, + &actual, file_data->timeout); + + /* Store bTag (in case we need to abort) */ + data->bTag_last_write = data->bTag; + + /* Increment bTag -- and increment again if zero */ + data->bTag++; + if (!data->bTag) + data->bTag++; + Independent of whether this needs to be split up, do you really want to do this regardless of usb_bulk_msg() returning an error? Regards Oliver I think it is ok. Our devices do not care much about the right order of sequence numbers. It is just a hint to abort the last messages. And the client should know the last message numbers itself. The current implementation does it the same way. https://elixir.bootlin.com/linux/v4.17-rc6/source/drivers/usb/class/usbtmc.c#L835 Regards Guido -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/12] usb: usbtmc: Add ioctl USBTMC488_IOCTL_WAIT_SRQ
Zitat von Oliver Neukum : Am Donnerstag, den 17.05.2018, 19:03 +0200 schrieb Guido Kiener: +static int usbtmc488_ioctl_wait_srq(struct usbtmc_file_data *file_data, + unsigned int __user *arg) +{ + struct usbtmc_device_data *data = file_data->data; + struct device *dev = &data->intf->dev; + int rv; + unsigned int timeout; + unsigned long expire; + + if (!data->iin_ep_present) { + dev_dbg(dev, "no interrupt endpoint present\n"); + return -EFAULT; + } + + if (get_user(timeout, arg)) + return -EFAULT; + + expire = msecs_to_jiffies(timeout); + + mutex_unlock(&data->io_mutex); There is such a thing as threads sharing file descriptors. That leads to the question what happens to the mutex if this ioctl() is called multiple times. Regards Oliver Multiple threads can wait with the same or different file descriptors. When an SRQ interrupt occurs, all threads and file descriptors are informed concurrently with wake_up_interruptible_all(&data->waitq); The "_all" is already fixed in 02/12. Regards, Guido -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/12] usb: usbtmc: Add ioctl USBTMC488_IOCTL_WAIT_SRQ
Zitat von Oliver Neukum : Am Donnerstag, den 24.05.2018, 12:59 + schrieb guido@kiener- muenchen.de: Zitat von Oliver Neukum : > Am Donnerstag, den 17.05.2018, 19:03 +0200 schrieb Guido Kiener: > > +static int usbtmc488_ioctl_wait_srq(struct usbtmc_file_data *file_data, > > + unsigned int __user *arg) > > +{ > > + struct usbtmc_device_data *data = file_data->data; > > + struct device *dev = &data->intf->dev; > > + int rv; > > + unsigned int timeout; > > + unsigned long expire; > > + > > + if (!data->iin_ep_present) { > > + dev_dbg(dev, "no interrupt endpoint present\n"); > > + return -EFAULT; > > + } > > + > > + if (get_user(timeout, arg)) > > + return -EFAULT; > > + > > + expire = msecs_to_jiffies(timeout); > > + > > + mutex_unlock(&data->io_mutex); > > There is such a thing as threads sharing file descriptors. > That leads to the question what happens to the mutex if this > ioctl() is called multiple times. > >Regards >Oliver Multiple threads can wait with the same or different file descriptors. When an SRQ interrupt occurs, all threads and file descriptors are informed concurrently with wake_up_interruptible_all(&data->waitq); The "_all" is already fixed in 02/12. No, the problem is that you will underflow io->mutex Don't worry. The function usbtmc488_ioctl_wait_srq is called by usbtmc_ioctl which already locks the mutex. See https://elixir.bootlin.com/linux/v4.17-rc7/source/drivers/usb/class/usbtmc.c#L1189 So the mutex is just unlocked here to allow other threads to still communicate with the device. Regards, Guido -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 08/29] usb: usbtmc: Add ioctl for generic requests on control
Zitat von Greg KH : On Wed, Jul 18, 2018 at 10:45:41AM +0200, Guido Kiener wrote: Add USBTMC_IOCTL_CTRL_REQUEST to send arbitrary requests on the control pipe. Used by specific applications of IVI Foundation, Inc. to implement VISA API functions: viUsbControlIn/Out. The maximum length of control request is set to 4k. Signed-off-by: Guido Kiener Reviewed-by: Steve Bayless --- drivers/usb/class/usbtmc.c | 151 +++ include/uapi/linux/usb/tmc.h | 15 2 files changed, 166 insertions(+) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index d685db78b80b..846599dd0c84 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -5,6 +5,7 @@ * Copyright (C) 2007 Stefan Kopp, Gechingen, Germany * Copyright (C) 2008 Novell, Inc. * Copyright (C) 2008 Greg Kroah-Hartman + * Copyright (C) 2018 IVI Foundation, Inc. */ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt @@ -36,6 +37,9 @@ /* Default USB timeout (in milliseconds) */ #define USBTMC_TIMEOUT 5000 +/* 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 @@ -126,6 +130,17 @@ struct usbtmc_file_data { bool term_char_enabled; }; +#ifdef CONFIG_COMPAT + +struct compat_usbtmc_ctrlrequest { + struct usbtmc_request req; + compat_uptr_t data; +} __packed; Wait, why? You are creating a new api here, why do you need a 32bit compat layer? Just create it in a way that works for both layouts. Just make your pointer a 64bit value and cast it to the pointer when using it. Other ioctls do this in lots of places, making things simpler overall. That way you don't need an ioctl compat call at all. Please rework the code that way, don't create new apis that have to add "old" 32bit compatibility support to them. That's just extra work you don't need to do here. thanks, greg k-h Maybe I'm still missing something. You wanted us that the driver supports 32 bit applications on 64 bit systems. Therefore we added the patch [PATCH v2 07/29] usb: usbtmc: Add support for 32 bit compat applications I do not know any other way to make ioctl functions working for 32 bit application, except adding the line .compat_ioctl = usbtmc_compat_ioctl. Please note that all other ioctl calls like USBTMC_IOCTL_CLEAR etc. do not work for 32 applications as well when we omit patch 07. Please let us first agree on patch 07 and then we can continue to discuss patch 08. Regards, -Guido -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 08/29] usb: usbtmc: Add ioctl for generic requests on control
Zitat von Greg KH : On Sat, Jul 21, 2018 at 11:11:55AM +, gu...@kiener-muenchen.de wrote: Zitat von Greg KH : > On Wed, Jul 18, 2018 at 10:45:41AM +0200, Guido Kiener wrote: > > Add USBTMC_IOCTL_CTRL_REQUEST to send arbitrary requests on the > > control pipe. Used by specific applications of IVI Foundation, > > Inc. to implement VISA API functions: viUsbControlIn/Out. > > > > The maximum length of control request is set to 4k. > > > > Signed-off-by: Guido Kiener > > Reviewed-by: Steve Bayless > > --- > > drivers/usb/class/usbtmc.c | 151 +++ > > include/uapi/linux/usb/tmc.h | 15 > > 2 files changed, 166 insertions(+) > > > > diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c > > index d685db78b80b..846599dd0c84 100644 > > --- a/drivers/usb/class/usbtmc.c > > +++ b/drivers/usb/class/usbtmc.c > > @@ -5,6 +5,7 @@ > > * Copyright (C) 2007 Stefan Kopp, Gechingen, Germany > > * Copyright (C) 2008 Novell, Inc. > > * Copyright (C) 2008 Greg Kroah-Hartman > > + * Copyright (C) 2018 IVI Foundation, Inc. > > */ > > > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > @@ -36,6 +37,9 @@ > > /* Default USB timeout (in milliseconds) */ > > #define USBTMC_TIMEOUT 5000 > > > > +/* 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 > > @@ -126,6 +130,17 @@ struct usbtmc_file_data { > > bool term_char_enabled; > > }; > > > > +#ifdef CONFIG_COMPAT > > + > > +struct compat_usbtmc_ctrlrequest { > > + struct usbtmc_request req; > > + compat_uptr_t data; > > +} __packed; > > Wait, why? You are creating a new api here, why do you need a 32bit > compat layer? Just create it in a way that works for both layouts. > Just make your pointer a 64bit value and cast it to the pointer when > using it. Other ioctls do this in lots of places, making things simpler > overall. That way you don't need an ioctl compat call at all. > > Please rework the code that way, don't create new apis that have to add > "old" 32bit compatibility support to them. That's just extra work you > don't need to do here. > Ok. I found some places where in_compat_syscall() or u64_to_uptr() is used to solve the 32/64 bit pointer problem. This makes the usbtmc kernel driver much easier, however the user needs to call something like this with double type casts to assign a 32/64 bit pointer to a __u64 value: request.data = (__u64)(uintptr_t)buf We agreed to accept this drawback and I will send a v3 patch series. Regards, Guido -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 02/23] usb: usbtmc: Add ioctl for generic requests on control
Zitat von Greg KH : On Tue, Jul 24, 2018 at 11:05:29AM +0200, Guido Kiener wrote: Add USBTMC_IOCTL_CTRL_REQUEST to send arbitrary requests on the control pipe. Used by specific applications of IVI Foundation, Inc. to implement VISA API functions: viUsbControlIn/Out. The maximum length of control request is set to 4k. Signed-off-by: Guido Kiener Reviewed-by: Steve Bayless --- drivers/usb/class/usbtmc.c | 84 include/uapi/linux/usb/tmc.h | 15 +++ 2 files changed, 99 insertions(+) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index 83ffa5a14c3d..3b3b4284d04e 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -5,6 +5,7 @@ * Copyright (C) 2007 Stefan Kopp, Gechingen, Germany * Copyright (C) 2008 Novell, Inc. * Copyright (C) 2008 Greg Kroah-Hartman + * Copyright (C) 2018 IVI Foundation, Inc. */ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt @@ -36,6 +37,9 @@ /* Default USB timeout (in milliseconds) */ #define USBTMC_TIMEOUT 5000 +/* 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 @@ -129,6 +133,21 @@ struct usbtmc_file_data { /* Forward declarations */ static struct usb_driver usbtmc_driver; +#ifdef CONFIG_COMPAT +static void __user *u64_to_uptr(u64 value) +{ + if (in_compat_syscall()) + return compat_ptr(value); + else + return (void __user *)(unsigned long)value; +} +#else +static inline void __user *u64_to_uptr(u64 value) +{ + return (void __user *)(unsigned long)value; +} +#endif /* CONFIG_COMPAT */ For Linux, if there are functions/operations that are always needed by all drivers (or even some), then those functions are provided in the core kernel with header files or in the library. So if you are writing a single driver, and find you need to create something that looks very "generic", either you are doing something that no other driver has ever needed before in the history of Linux, or you are doing something wrong. Which do you think it is here? :) Please fix this up and do it properly. Your driver should only have 1 #ifdef CONFIG_COMPAT line, and that was in patch 1 for this series. Nothing else should be needed if you create your structures correctly. I do not understand why this code is correct here: https://elixir.bootlin.com/linux/v4.18-rc6/source/drivers/firewire/core-cdev.c#L223 or here (with is_compat_task -- which might be not correct): https://elixir.bootlin.com/linux/v4.18-rc6/source/drivers/s390/char/sclp_ctl.c#L45 Which variant may I use? Regards, Guido -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 02/23] usb: usbtmc: Add ioctl for generic requests on control
Zitat von Greg KH : On Tue, Jul 24, 2018 at 11:05:29AM +0200, Guido Kiener wrote: +struct usbtmc_ctrlrequest { + struct usbtmc_request req; + __u64 data; /* pointer to user space */ +} __attribute__ ((packed)); Hint, this structure could just be: struct usbtmc_ctrlreqest { struct usbtmc_request req; __u8 data[0]; } __attribute__((__aligned__(8))); No more pointer mess anymore, right? Yes. But we prefer copy by reference and not by value. Especially for the next USBTMC_IOCTL_WRITE/READ function, we want to copy many bytes (MB and GB) by reference. I had a lengthy discussion with many developers, and we prefer pointer mess before we loose any microsecond. Regards, Guido -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 00/22] usb: usbtmc: Changes needed for compatible IVI/VISA library
Zitat von Greg KH : On Mon, Jul 30, 2018 at 10:04:30AM +0200, Guido Kiener wrote: The working group "VISA for Linux" of the IVI Foundation www.ivifoundation.org specifies common rules, shared libraries and drivers to implement the specification of "VPP-4.3: The VISA Library" on Linux to be compatible with implementations on other operating systems. The USBTMC protocol is part of the "VISA Library" that is used by many popular T&M applications. Initial implementations for Linux based on libusb has been created. However using one common USBTMC driver results in more benefits: - Multiple applications can share access to the same instruments. - The driver handles SRQ conflicts. - Simplifies definition of udev rules for USBTMC devices. - Simplifies development of applications using T&M instruments. The following collaborative patches meet the requirements of the IVI Foundation to implement the library based on the usbtmc driver. Improvements in the data transfer rate of over 130 MByte/s for usb 3.x connections have been measured. Guido Kiener, Dave Penkler, Steve Bayless (22): usb: usbtmc: Add ioctl for generic requests on control usb: usbtmc: Add ioctl for vendor specific write usb: usbtmc: Add ioctl USBTMC_IOCTL_WRITE_RESULT usb: usbtmc: Add ioctl for vendor specific read usb: usbtmc: Add ioctl USBTMC_IOCTL_CANCEL_IO usb: usbtmc: Add ioctl USBTMC_IOCTL_CLEANUP_IO usb: usbtmc: Fix suspend/resume usb: usbtmc: Add ioctl USBTMC488_IOCTL_WAIT_SRQ usb: usbtmc: add ioctl USBTMC_IOCTL_MSG_IN_ATTR usb: usbtmc: Add ioctl USBTMC_IOCTL_AUTO_ABORT usb: usbtmc: Optimize usbtmc_write usb: usbtmc: Optimize usbtmc_read usb: usbtmc: Fix ioctl USBTMC_IOCTL_CLEAR usb: usbtmc: Fix ioctl USBTMC_IOCTL_ABORT_BULK_IN usb: usbtmc: Fix ioctl USBTMC_IOCTL_ABORT_BULK_OUT usb: usbtmc: Replace USBTMC_TIMEOUT macros for control messages usb: usbtmc: Add ioctl USBTMC_IOCTL_API_VERSION usb: usbtmc: Update ioctl-number.txt usb: usbtmc: Remove redundant code usb: usbtmc: Remove redundant macro USBTMC_SIZE_IOBUFFER usb: usbtmc: Fix split quoted string in debug message usb: usbtmc: Remove sysfs group TermChar and auto_abort .../ABI/stable/sysfs-driver-usb-usbtmc| 35 - Documentation/ioctl/ioctl-number.txt |2 +- drivers/usb/class/usbtmc.c| 1652 + include/uapi/linux/usb/tmc.h | 41 + 4 files changed, 1291 insertions(+), 439 deletions(-) What changed from v3 of this patch set? That needs to be listed somewhere, right? v3 included patch "usb: usbtmc: Add support for 32 bit compat applications" which you already have added to your usb-next branch. You did not accept (2nd and now 1st) patch "usb: usbtmc: Add ioctl for generic requests on control" which had a superfluous "#ifdef CONFIG_COMPAT". So v4 just removed this superfluous "#ifdef CONFIG_COMPAT" without any change of further descriptions. Regards, -Guido -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 00/22] usb: usbtmc: Changes needed for compatible IVI/VISA library
Greg, Thank you for reviewing our usbtmc kernel patches. People are asking me about the progress of the last patches and I'm not sure whether new patches can be merged now or not. So this is just a reminder to our last patch series (four weeks ago): https://patchwork.kernel.org/cover/10554819/ Regards, Guido
Re: fixes for ioctl() of usbtmc in testing tree
Zitat von Greg Kroah-Hartman : On Mon, Sep 24, 2018 at 11:24:10AM +0200, Oliver Neukum wrote: Hi, how should I mark fixes intended for the testing branch? I got one for the usbtmc driver. Just send it like normal. You can do a "Fixes:" tag with the sha1, that should be fine. I need to push out my testing branch now, 0-day seems to be stalled :( Big sorry! There is a superflous kmalloc line 1270 til 1272. Shall I send the fix? Regards Guido
Re: fixes for ioctl() of usbtmc in testing tree
Zitat von Oliver Neukum : On Mo, 2018-09-24 at 10:56 +, gu...@kiener-muenchen.de wrote: Zitat von Greg Kroah-Hartman : > On Mon, Sep 24, 2018 at 11:24:10AM +0200, Oliver Neukum wrote: > > Hi, > > > > how should I mark fixes intended for the testing branch? > > I got one for the usbtmc driver. > > Just send it like normal. You can do a "Fixes:" tag with the sha1, that > should be fine. I need to push out my testing branch now, 0-day seems > to be stalled :( > Big sorry! There is a superflous kmalloc line 1270 til 1272. Shall I send the fix? Damn. That is the same allocation repeated, not a reuse of the buffer. I'll resend. There is also a leak in the error case. I do not see a leak in the error case. kfree(NULL) should be ok. Sorry, I referred the line 1270 to the mail of Dan Carpenter. I mean the lines: diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index 0fcb81a1399b..dfbcf418dad7 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -1895,10 +1895,6 @@ static int usbtmc_ioctl_request(struct usbtmc_device_data *data, if (res) return -EFAULT; - buffer = kmalloc(request.req.wLength, GFP_KERNEL); - if (!buffer) - return -ENOMEM; - if (request.req.wLength > USBTMC_BUFSIZE) return -EMSGSIZE; @Oliver: Where do send (resend) the fix? Is this an official fix or do you just fix your internal build system? And I still have to make an official fix, isn't it? Guido
Re: fixes for ioctl() of usbtmc in testing tree
Zitat von Greg Kroah-Hartman : On Mon, Sep 24, 2018 at 12:20:42PM +, gu...@kiener-muenchen.de wrote: Zitat von Oliver Neukum : > On Mo, 2018-09-24 at 10:56 +, gu...@kiener-muenchen.de wrote: > > Zitat von Greg Kroah-Hartman : > > > > > On Mon, Sep 24, 2018 at 11:24:10AM +0200, Oliver Neukum wrote: > > > > Hi, > > > > > > > > how should I mark fixes intended for the testing branch? > > > > I got one for the usbtmc driver. > > > > > > Just send it like normal. You can do a "Fixes:" tag with the sha1, that > > > should be fine. I need to push out my testing branch now, 0-day seems > > > to be stalled :( > > > > > > > Big sorry! There is a superflous kmalloc line 1270 til 1272. > > Shall I send the fix? > > Damn. That is the same allocation repeated, not a reuse of the buffer. > I'll resend. There is also a leak in the error case. > I do not see a leak in the error case. kfree(NULL) should be ok. Sorry, I referred the line 1270 to the mail of Dan Carpenter. I mean the lines: diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index 0fcb81a1399b..dfbcf418dad7 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -1895,10 +1895,6 @@ static int usbtmc_ioctl_request(struct usbtmc_device_data *data, if (res) return -EFAULT; - buffer = kmalloc(request.req.wLength, GFP_KERNEL); - if (!buffer) - return -ENOMEM; - if (request.req.wLength > USBTMC_BUFSIZE) return -EMSGSIZE; @Oliver: Where do send (resend) the fix? Is this an official fix or do you just fix your internal build system? And I still have to make an official fix, isn't it? Yes, you need to send a "real" patch for anyone to be able to pick it up. I sent the patch series: https://patchwork.kernel.org/cover/10612963/ Anything else I can do to relieve my bad conscience? Regards, Guido
Re: [PATCH] usb: usbtmc: check size before allocating buffer and remove duplicated allocation
Zitat von Colin King : From: Colin Ian King Currently the allocation of a buffer is performed before a sanity check on the required buffer size and if the buffer size is too large the error exit return leaks the allocated buffer. Fix this by checking the size before allocating. Also, the same buffer is allocated again inside an if statement, causing the first allocation to be leaked. Fix this by removing this second redundant allocation. Detected by CoverityScan, CID#1473697 ("Resource leak") Fixes: 658f24f4523e ("usb: usbtmc: Add ioctl for generic requests on control") Signed-off-by: Colin Ian King --- drivers/usb/class/usbtmc.c | 10 +++--- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index 0fcb81a1399b..c01edff190d2 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -1895,18 +1895,14 @@ static int usbtmc_ioctl_request(struct usbtmc_device_data *data, if (res) return -EFAULT; + if (request.req.wLength > USBTMC_BUFSIZE) + return -EMSGSIZE; + buffer = kmalloc(request.req.wLength, GFP_KERNEL); if (!buffer) return -ENOMEM; - if (request.req.wLength > USBTMC_BUFSIZE) - return -EMSGSIZE; - if (request.req.wLength) { - buffer = kmalloc(request.req.wLength, GFP_KERNEL); - if (!buffer) - return -ENOMEM; - if ((request.req.bRequestType & USB_DIR_IN) == 0) { /* Send control data to device */ res = copy_from_user(buffer, request.data, -- 2.17.1 Good to know that there are some tools finding this memory leak. I have already sent a patch series here: https://patchwork.kernel.org/cover/10612963/ This includes a similar patch to your proposal: https://patchwork.kernel.org/patch/10612965/ Thank you! Regards, Guido
Re: [PATCH 2/3] udc: net2280: Fix overrun of OUT messages
Zitat von Alan Stern : On Mon, 4 Feb 2019, Guido Kiener wrote: From: Guido Kiener The OUT endpoint normally blocks (NAK) subsequent packets when a short packet is received and returns an incomplete queue entry to the gadget driver. Thereby the gadget driver can detect a short packet when reading queue entries with a length that is not equal to a multiple of packet size. The start_queue() function enables receiving OUT packets regardless of the content of the OUT FIFO. This results in a problem: When receiving is enabled more OUT packets are appended to the last short packet and the gadget driver cannot determine the end of a short packet anymore. Furthermore the remaining data in the OUT FIFO is not delivered to the gadget driver immediately and can produce timeout errors. How can that happen? When the short packet is received, the endpoint immediately starts sending NAKs, so no more data can enter the FIFO. Furthermore, the incomplete transfer is returned to the gadget driver, which removes the short packet from the FIFO. So the FIFO has to be empty when start_queue() is called. I don't remember the exact scenario. It happens something like this: When you have only one request (e.g. 4kB) left in your endpoint OUT queue and a host sends 4kB + some extra bytes, then your request (4kB) is delivered to the gadget driver while the extra bytes (short packet) are still in the OUT FIFO. I try to reconstruct the situation (without fix) how the driver can call (re)start_dma. I do not see a reasonable path in the moment. Guido This fix only stops OUT naking when all FIFO data is delivered to the gadget driver and the OUT FIFO is empty.
Re: [PATCH 3/3] udc: net2280: Fix typo
Zitat von Alan Stern : On Mon, 4 Feb 2019, Guido Kiener wrote: From: Guido Kiener Fix spelling of automatically. Signed-off-by: Guido Kiener --- drivers/usb/gadget/udc/net2280.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c index 1cb58fd5d1c6..430d582d8b6e 100644 --- a/drivers/usb/gadget/udc/net2280.c +++ b/drivers/usb/gadget/udc/net2280.c @@ -912,7 +912,7 @@ static void start_dma(struct net2280_ep *ep, struct net2280_request *req) tmp = dmactl_default; /* force packet boundaries between dma requests, but prevent the -* controller from automagically writing a last "short" packet +* controller from automatically writing a last "short" packet This is not a misspelling. It is deliberate: http://www.hacker-dictionary.com/terms/automagically Thank you and I'm sorry for my stupid idea to revise the spelling of a native speaker. I will not trust my editor's dictionary anymore :-) Of course I withdraw this patch. Guido
Re: [PATCH 1/3] udc: net2280: Fix net2280_disable
Zitat von Alan Stern : On Mon, 4 Feb 2019, Guido Kiener wrote: From: Guido Kiener A reset e.g. calling ep_reset_338x() can happen while endpoints are enabled. How can this happen? That routine is called from only two places. One is in net2280_disable(), so after the endpoint has already been disabled. The other is in usb_reinit_338x() via usb_reinit() via stop_activity(), which disconnects the gadget driver and thereby disables all the endpoints. Yes, the routine is called when I stop my application. See callstack: Call Trace: net2280_disable: Invalid ep=341c7996 or ep->desc usb_ep_disable+0x24/0xa0 [udc_core] disableEndpoints+0x1c/0x80 [rsusbtmc] tmc_func_disable+0x29/0xa0 [rsusbtmc] reset_config+0x3e/0xa0 [libcomposite] composite_disconnect+0x36/0x60 [libcomposite] net2280_pullup+0x9e/0xf0 [net2280] usb_gadget_disconnect+0x35/0xc0 [udc_core] usb_gadget_remove_driver+0x29/0xa0 [udc_core] usb_gadget_unregister_driver+0xc1/0xf0 [udc_core] usb_composite_unregister+0x12/0x20 [libcomposite] dev_release+0x3f/0x80 [rsusbtmc] __fput+0x102/0x230 fput+0xe/0x10 task_work_run+0x90/0xc0 exit_to_usermode_loop+0xf2/0x100 do_syscall_64+0xfb/0x120 entry_SYSCALL_64_after_hwframe+0x44/0xa9 When I restart my application then the endpoints remains disabled. A workaround is to reload our (legacy) gadget driver, libcomposite and net2280. The ep_reset_338x() sets ep->desc = NULL to mark endpoint being invalid. A subsequent call of net2280_disable will fail and return -EINVAL to parent function usb_ep_disable(), which will fail, too, and do not set the member ep->enabled = false. So maybe a better fix (assuming there really is a problem) is to make usb_ep_disable() clear ep->enabled always. After all, the only reasonable way for usb_ep_disable() to fail is if the endpoint isn't enabled in the first place. I had the same idea. However the root cause is the net2280 driver. We have not seen the problem with other function drivers. The usb_ep_disable() implementation looks reasonable to me. When the ret = ep->ops->disable(ep); function fails for any other fictitious reason (e.g. conflicts, order of endpoints..) then it might be better to set ep->enabled = false when the function driver really succeeds. Regards, Guido
Re: [PATCH 2/3] udc: net2280: Fix overrun of OUT messages
Zitat von Alan Stern : On Tue, 5 Feb 2019 gu...@kiener-muenchen.de wrote: Zitat von Alan Stern : > On Mon, 4 Feb 2019, Guido Kiener wrote: > >> From: Guido Kiener >> >> The OUT endpoint normally blocks (NAK) subsequent packets when a >> short packet is received and returns an incomplete queue entry to >> the gadget driver. Thereby the gadget driver can detect >> a short packet when reading queue entries with a length that is >> not equal to a multiple of packet size. >> >> The start_queue() function enables receiving OUT packets regardless >> of the content of the OUT FIFO. This results in a problem: >> When receiving is enabled more OUT packets are appended to the last >> short packet and the gadget driver cannot determine the end of a >> short packet anymore. Furthermore the remaining data in the OUT FIFO >> is not delivered to the gadget driver immediately and can produce >> timeout errors. > > How can that happen? When the short packet is received, the endpoint > immediately starts sending NAKs, so no more data can enter the FIFO. > Furthermore, the incomplete transfer is returned to the gadget driver, > which removes the short packet from the FIFO. So the FIFO has to be > empty when start_queue() is called. I don't remember the exact scenario. It happens something like this: When you have only one request (e.g. 4kB) left in your endpoint OUT queue and a host sends 4kB + some extra bytes, then your request (4kB) is delivered to the gadget driver while the extra bytes (short packet) are still in the OUT FIFO. I try to reconstruct the situation (without fix) how the driver can call (re)start_dma. I do not see a reasonable path in the moment. Okay, I see. If there's no outstanding request when the short packet arrives, the data will remain in the FIFO. But won't this situation be detected in start_dma(), which is the only place where start_queue() is called? start_dma() has code to test specifically for a previous short OUT packet. Should the fix go there instead? You are right. Unfortunately I did not save a callstack. I guess it was more a restart_dma(). We do a lot of unusual things like stalling endpoints, clear_halt, dequeue requests and so on. The biggest problem is to stop/abort the DMA (I have some more comlexer patches in the queue). I just remember that it took me a long time to find this bug and I try to reproduce it this week. Guido
Re: [PATCH 2/3] udc: net2280: Fix overrun of OUT messages
Zitat von Alan Stern : On Tue, 5 Feb 2019 gu...@kiener-muenchen.de wrote: Zitat von Alan Stern : > On Mon, 4 Feb 2019, Guido Kiener wrote: > >> From: Guido Kiener >> >> The OUT endpoint normally blocks (NAK) subsequent packets when a >> short packet is received and returns an incomplete queue entry to >> the gadget driver. Thereby the gadget driver can detect >> a short packet when reading queue entries with a length that is >> not equal to a multiple of packet size. >> >> The start_queue() function enables receiving OUT packets regardless >> of the content of the OUT FIFO. This results in a problem: >> When receiving is enabled more OUT packets are appended to the last >> short packet and the gadget driver cannot determine the end of a >> short packet anymore. Furthermore the remaining data in the OUT FIFO >> is not delivered to the gadget driver immediately and can produce >> timeout errors. > > How can that happen? When the short packet is received, the endpoint > immediately starts sending NAKs, so no more data can enter the FIFO. > Furthermore, the incomplete transfer is returned to the gadget driver, > which removes the short packet from the FIFO. So the FIFO has to be > empty when start_queue() is called. I don't remember the exact scenario. It happens something like this: When you have only one request (e.g. 4kB) left in your endpoint OUT queue and a host sends 4kB + some extra bytes, then your request (4kB) is delivered to the gadget driver while the extra bytes (short packet) are still in the OUT FIFO. I try to reconstruct the situation (without fix) how the driver can call (re)start_dma. I do not see a reasonable path in the moment. Okay, I see. If there's no outstanding request when the short packet arrives, the data will remain in the FIFO. But won't this situation be detected in start_dma(), which is the only place where start_queue() is called? start_dma() has code to test specifically for a previous short OUT packet. Should the fix go there instead? Alan, I have inserted a bug detection and can now explain the race: Ween receiving a long transfer the gadget driver queues a new request with net2280_queue() and calls start_dma(). In the meantime the OUT FIFO is still receiving data and the short packet is not detected yet. So the start_dma() function does not use the shortcut in line: https://elixir.bootlin.com/linux/v5.0-rc5/source/drivers/usb/gadget/udc/net2280.c#L905 Thus start_queue() is called at the end of the function which calls stop_out_naking(). This will allow receiving new data, even when a short packet is arrived in the meantime. Indeed, I'm not sure now whether my patch only minimizes the race or whether we need to rethink the logic here and call stop_out_naking() when the request is really done and delivered to the gadget driver. See function done(). Here is my code sample how I can detect the race (USB 2.0 with max packet size of 512): static void start_queue(struct net2280_ep *ep, u32 dmactl, u32 td_dma) { struct net2280_dma_regs __iomem *dma = ep->dma; unsigned int tmp = BIT(VALID_BIT) | (ep->is_in << DMA_DIRECTION); if (!(ep->dev->quirks & PLX_2280)) tmp |= BIT(END_OF_CHAIN); ep_vdbg(ep->dev, "start_queue\n"); writel(tmp, &dma->dmacount); writel(readl(&dma->dmastat), &dma->dmastat); writel(td_dma, &dma->dmadesc); if (ep->dev->quirks & PLX_PCIE) dmactl |= BIT(DMA_REQUEST_OUTSTANDING); writel(dmactl, &dma->dmactl); /* erratum 0116 workaround part 3: pci arbiter away from net2280 */ (void) readl(&ep->dev->pci->pcimstctl); // Show bug begin unsigned int avail = readl(&ep->regs->ep_avail); if (!ep->is_in && (avail % 512) != 0) { u32 t1, t2; t1 = readl(&ep->cfg->ep_cfg); t2 = readl(&ep->regs->ep_rsp) & 0xff; ep_err(ep->dev, " stat %08x avail %04x (ep%d%s-%s)%s\n", readl(&ep->regs->ep_stat), readl(&ep->regs->ep_avail), t1 & 0x0f, DIR_STRING(t1), type_string(t1 >> 8), ep->stopped ? "*" : ""); WARN_ON(1); } // Show bug end writel(BIT(DMA_START), &dma->dmastat); //if (!ep->is_in && readl(&ep->regs->ep_avail) == 0) //<- patch if (!ep->is_in) stop_out_naking(ep); } This is the corresponding callstack. Flag "NAK packets" (BIT4) is set in EP_STAT and there are now 944 (0x3b0) byte
Re: [PATCH 2/3] udc: net2280: Fix overrun of OUT messages
Zitat von Alan Stern : On Wed, 6 Feb 2019 gu...@kiener-muenchen.de wrote: Alan, I have inserted a bug detection and can now explain the race: Ween receiving a long transfer the gadget driver queues a new request with net2280_queue() and calls start_dma(). In the meantime the OUT FIFO is still receiving data and the short packet is not detected yet. So the start_dma() function does not use the shortcut in line: https://elixir.bootlin.com/linux/v5.0-rc5/source/drivers/usb/gadget/udc/net2280.c#L905 Thus start_queue() is called at the end of the function which calls stop_out_naking(). This will allow receiving new data, even when a short packet is arrived in the meantime. I'm confused. A transfer is in progress, let's say for request R1. Before R1 finishes, the gadget driver queues a new request, R2. Since R1 is still running, ep->queue is not empty, so net2280_queue() will not call start_dma() -- it will call queue_dma() instead. But this is not what your stack dump shows. Maybe you meant that R1 completes and then afterward the gadget driver queues R2. Then there is a race: start_dma() sees that no short packet has arrived, but a short packet could arrive before stop_out_naking() is called. Yes, I mean the second case. Currently we put only one request into the OUT queue. Using 2-3 requests would even improve bandwidth for long transfers, however it does not help to avoid the bug when the application input is blocked. Indeed, I'm not sure now whether my patch only minimizes the race or whether we need to rethink the logic here and call stop_out_naking() when the request is really done and delivered to the gadget driver. See function done(). The right time to call stop_out_naking is immediately after we complete a short transfer (that is, a transfer which ended with a short packet). start_queue() is definitely the wrong place. Right at the end of done() is probably the right place. But do it only if the last packet of the transfer was a short one. And note that this does not necessarily mean the transfer got an error: If the request was for only 15 bytes then it could complete successfully but the (only) packet would still be short. I tried to call stop_out_naking() at the end of done(). However I have overseen that the author of the driver only wants to receive data when there is an OUT request available. E.g. see: https://elixir.bootlin.com/linux/v5.0-rc5/source/drivers/usb/gadget/udc/net2280.c#L304 So I think my patch was not so "bad" and I assume we can prevent the race with the following patch (or easier see below): static void start_queue(struct net2280_ep *ep, u32 dmactl, u32 td_dma) { struct net2280_dma_regs __iomem *dma = ep->dma; unsigned int tmp = BIT(VALID_BIT) | (ep->is_in << DMA_DIRECTION); if (!(ep->dev->quirks & PLX_2280)) tmp |= BIT(END_OF_CHAIN); writel(tmp, &dma->dmacount); writel(readl(&dma->dmastat), &dma->dmastat); writel(td_dma, &dma->dmadesc); if (ep->dev->quirks & PLX_PCIE) dmactl |= BIT(DMA_REQUEST_OUTSTANDING); writel(dmactl, &dma->dmactl); /* erratum 0116 workaround part 3: pci arbiter away from net2280 */ (void) readl(&ep->dev->pci->pcimstctl); if (!ep->is_in && (readl(&ep->regs->ep_stat) & BIT(NAK_OUT_PACKETS)) && (readl(&ep->regs->ep_avail) == 0)) { stop_out_naking(ep); } writel(BIT(DMA_START), &dma->dmastat); } start_queue() is only called by start_dma(). I see that we can simplify this patch when we just insert the stop_out_nacking(ep) between line 909 and 910: https://elixir.bootlin.com/linux/v5.0-rc5/source/drivers/usb/gadget/udc/net2280.c#L909 and remove the stop_out_naking() call in start_queue(). I will test this over weekend. Regards, Guido
usb: gadget: net2280: Fix function net2280_dequeue()
Hi Alan, My last proposal "udc: net2280: Fix overrun of OUT messages" is still under investigation. During the random stress tests I found a new rare problem: When a request must be dequeued with net2280_dequeue() e.g. due to a device clear action and the same request is finished by the function scan_dma_completions(): https://elixir.bootlin.com/linux/v5.0-rc7/source/drivers/usb/gadget/udc/net2280.c#L1269 then the following search loop does not find the request and the function returns the error -EINVAL without restoring the state ep->stopped = stopped! https://elixir.bootlin.com/linux/v5.0-rc7/source/drivers/usb/gadget/udc/net2280.c#L1269 Thus the endpoint keeps blocked and does not receive any data. When we insert ep->stopped = stopped here: if (&req->req != _req) { ep->stopped = stopped; // <<<<< spin_unlock_irqrestore(&ep->dev->lock, flags); dev_err(&ep->dev->pdev->dev, "%s: Request mismatch\n", __func__); return -EINVAL; } then the driver continues as expected, although the driver issues the false error message "Request mismatch". What do you think? It's labor-intensive to fix the error message here, or shall we leave it as it is, and only insert the "ep->stopped = stopped;" Regards, Guido
Re: usb: gadget: net2280: Fix function net2280_dequeue()
Zitat von Alan Stern : On Tue, 19 Feb 2019 gu...@kiener-muenchen.de wrote: Hi Alan, My last proposal "udc: net2280: Fix overrun of OUT messages" is still under investigation. During the random stress tests I found a new rare problem: When a request must be dequeued with net2280_dequeue() e.g. due to a device clear action and the same request is finished by the function scan_dma_completions(): https://elixir.bootlin.com/linux/v5.0-rc7/source/drivers/usb/gadget/udc/net2280.c#L1269 then the following search loop does not find the request and the function returns the error -EINVAL without restoring the state ep->stopped = stopped! https://elixir.bootlin.com/linux/v5.0-rc7/source/drivers/usb/gadget/udc/net2280.c#L1269 Thus the endpoint keeps blocked and does not receive any data. When we insert ep->stopped = stopped here: if (&req->req != _req) { ep->stopped = stopped; // <<<<< spin_unlock_irqrestore(&ep->dev->lock, flags); dev_err(&ep->dev->pdev->dev, "%s: Request mismatch\n", __func__); return -EINVAL; } then the driver continues as expected, although the driver issues the false error message "Request mismatch". What do you think? It's labor-intensive to fix the error message here, or shall we leave it as it is, and only insert the "ep->stopped = stopped;" Yes, this is a bug. I think the dev_err() should be changed to ep_dbg(). It's not a real error, after all -- it's just a race. I agree. ep_dbg() is better here. I see the same bug in the sibling driver net2272_dequeue(): https://elixir.bootlin.com/linux/v5.0-rc8/source/drivers/usb/gadget/udc/net2272.c#L948 The bug is quite obvious to me, however I'm not aware that we use this driver in any of our instruments. I cannot test it. What do you recommend? - Shall I fix it within the same commit. - Shall I fix it with another commit. - Don't care about it Do that and insert the "ep->stopped = stopped;" line. Also, consider changing the -EINVAL return code here to -EIDRM. This is analogous to what usb_unlink_urb() does on the host side. EIDRM sounds better. However I see that all udc drivers calling the _dequeue() function returns -EINVAL when a request cannot be dequeued. I do not dare to break this tradition. BTW we tested my last proposal: https://patchwork.kernel.org/patch/10796161/ ... I see that we can simplify this patch when we just insert the stop_out_nacking(ep) between line 909 and 910: https://elixir.bootlin.com/linux/v5.0-rc5/source/drivers/usb/gadget/udc/net2280.c#L909 and remove the stop_out_naking() call in start_queue()... I will add this improvement to the new patch sequence. Guido
Re: [PATCH 2/2] usb: gadget: net2280: Add workaround for AB chip Errata 11
Zitat von Benjamin Herrenschmidt : The errata description is: Workaround for Default Duration of LFPS Handshake Signaling for Device-Initiated U1 Exit is too short. The default duration of the LFPS handshake generated by USB3380 for a device-initiated U1-exit may not be long enough for certain SuperSpeed downstream ports (SuperSpeed hubs/hosts) to recognize. This could lead to USB3380 entering the recovery state pre-maturely and ending up in the SS.Inactive state. I have observed various enumeration failures, seemingly related to lost transactions or SETUP status phases on modern hosts (typically thunderbolt capable systems) without this workaround. Signed-off-by: Benjamin Herrenschmidt --- drivers/usb/gadget/udc/net2280.c | 10 ++ 1 file changed, 10 insertions(+) diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c index e0191146ba22..51efee21915f 100644 --- a/drivers/usb/gadget/udc/net2280.c +++ b/drivers/usb/gadget/udc/net2280.c @@ -2269,6 +2269,16 @@ static void usb_reinit_338x(struct net2280 *dev) val |= 0x3 << HOT_RX_RESET_TS2; writel(val, &dev->llregs->ll_tsn_counters_3); + /* +* AB errata. Errata 11. Workaround for Default Duration of LFPS +* Handshake Signaling for Device-Initiated U1 Exit is too short. +* Without this, various enumeration failures observed with +* modern superspeed hosts. +*/ + val = readl(&dev->llregs->ll_lfps_timers_2); + writel((val & 0x) | LFPS_TIMERS_2_WORKAROUND_VALUE, + &dev->llregs->ll_lfps_timers_2); + /* * Set Recovery Idle to Recover bit: * - On SS connections, setting Recovery Idle to Recover Fmw improves I'm ok with your patch sequence. -Guido
Re: [PATCH 1/2] usb: gadget: net2280: Move all "ll" registers in one structure
Zitat von Felipe Balbi : Hi, Benjamin Herrenschmidt writes: On Wed, 2019-08-28 at 13:09 +0300, Felipe Balbi wrote: Hi, Benjamin Herrenschmidt writes: > The split into multiple structures of the "ll" register bank is > impractical. It makes it hard to add ll_lfps_timers_2 which is > at offset 0x794, which is outside of the existing "lfps" structure > and would require us to add yet another one. > > Instead, move all the "ll" registers into a single usb338x_ll_regs > structure, and add ll_lfps_timers_2 while at it. It will be used > in a subsequent patch. > > Signed-off-by: Benjamin Herrenschmidt I tried applying your patches but it resulted in build break. Can you collect all the dependencies and send a single series? I'm applying on top of my testing/next branch. You mean the 2 net2280 patches ? Or something else ? What break did you get ? It's just one series of 2 patches I'll try rebasing them against your branch tomorrow. allmodconfig broke with missing fields in whatever structure. I didn't dig too much, sorry. After applying [patch 1/2] & [patch 2/2] I could build on https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/ branch usb-next. Please note that [PATCH 1/2] was sent twice. -Guido
[PATCH] usb: Wait before re-enabling a port that has been disabled due to EMI
Hello. Considering that EM interference can last for a while (generally up to seconds), I am currently testing the following patch for module usbcore so that it is possible to specify an amount of time to wait before trying to re-enable a port that has been previously disabled by the hub (supposedly because of EM interference). Hopefully, setting the right positive value (for example, 2000 milliseconds) would help overcome situations such as the following: [ 1295.575679] usb 6-2: FTDI USB Serial Device converter now attached to ttyUSB1 [ 1302.204285] usb usb6-port2: disabled by hub (EMI?), re-enabling... *** NOTE: EMI is probably still present here *** [ 1303.205202] usb 6-2: USB disconnect, device number 6 [ 1303.205907] ftdi_sio ttyUSB1: FTDI USB Serial Device converter now disconnected from ttyUSB1 [ 1303.205950] ftdi_sio 6-2:1.0: device disconnected [ 1303.414089] usb 6-2: new full-speed USB device number 7 using uhci_hcd [ 1303.526226] usb 6-2: device descriptor read/64, error -71 [ 1303.894228] usb 6-2: new full-speed USB device number 8 using uhci_hcd [ 1304.006185] usb 6-2: device descriptor read/64, error -71 [ 1304.219089] usb 6-2: device descriptor read/64, error -71 [ 1304.422107] usb 6-2: new full-speed USB device number 9 using uhci_hcd [ 1304.640020] usb 6-2: device not accepting address 9, error -71 [ 1304.752024] usb 6-2: new full-speed USB device number 10 using uhci_hcd [ 1305.160020] usb 6-2: device not accepting address 10, error -71 [ 1305.160038] hub 6-0:1.0: unable to enumerate USB device on port 2 *** NOTE: Device is permanently disabled at this point *** I don't know whether my analysis is correct (and therefore the proposed solution appropriate), as reproducing the problem is very difficult... Regards, Guido Add an option to the usbcore module to wait a specified amount of time (in milliseconds) before trying to re-enable a USB port that has been previously disabled by the hub (possibly due to EMI). Signed-off-by: Guido Trentalancia --- drivers/usb/core/hub.c |8 1 file changed, 8 insertions(+) --- linux-4.4.6-orig/drivers/usb/core/hub.c 2016-03-20 16:47:09.358674922 +0100 +++ linux-4.4.6/drivers/usb/core/hub.c 2016-03-20 16:47:51.960195139 +0100 @@ -89,6 +89,13 @@ MODULE_PARM_DESC(use_both_schemes, "try the other device initialization scheme if the " "first one fails"); +static int emi_recover_timer = 0; +module_param(emi_recover_timer, int, S_IRUGO | S_IWUSR); +MODULE_PARM_DESC(emi_recover_timer, + "wait before trying to re-enable a port " + "that has been disabled by the hub due to EMI " + "(default 0 milliseconds)"); + /* Mutual exclusion for EHCI CF initialization. This interferes with * port reset on some companion controllers. */ @@ -4960,6 +4967,7 @@ static void port_event(struct usb_hub *h if (!(portstatus & USB_PORT_STAT_ENABLE) && !connect_change && udev) { dev_err(&port_dev->dev, "disabled by hub (EMI?), re-enabling...\n"); + msleep(emi_recover_timer); connect_change = 1; } } -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: Wait before re-enabling a port that has been disabled due to EMI
Hello Greg ! On dom, 2016-03-20 at 10:34 -0700, Greg KH wrote: > On Sun, Mar 20, 2016 at 06:09:57PM +0100, Guido Trentalancia wrote: > > > > [ 1295.575679] usb 6-2: FTDI USB Serial Device converter now > > attached > > to ttyUSB1 > > [ 1302.204285] usb usb6-port2: disabled by hub (EMI?), re- > > enabling... > > > > *** NOTE: EMI is probably still present here *** > > > > [ 1303.205202] usb 6-2: USB disconnect, device number 6 > > [ 1303.205907] ftdi_sio ttyUSB1: FTDI USB Serial Device converter > > now > > disconnected from ttyUSB1 > > [ 1303.205950] ftdi_sio 6-2:1.0: device disconnected > > [ 1303.414089] usb 6-2: new full-speed USB device number 7 using > > uhci_hcd > > [ 1303.526226] usb 6-2: device descriptor read/64, error -71 > > [ 1303.894228] usb 6-2: new full-speed USB device number 8 using > > uhci_hcd > > [ 1304.006185] usb 6-2: device descriptor read/64, error -71 > > [ 1304.219089] usb 6-2: device descriptor read/64, error -71 > > [ 1304.422107] usb 6-2: new full-speed USB device number 9 using > > uhci_hcd > > [ 1304.640020] usb 6-2: device not accepting address 9, error -71 > > [ 1304.752024] usb 6-2: new full-speed USB device number 10 using > > uhci_hcd > > [ 1305.160020] usb 6-2: device not accepting address 10, error -71 > > [ 1305.160038] hub 6-0:1.0: unable to enumerate USB device on port > > 2 > > > > *** NOTE: Device is permanently disabled at this point *** > > > > I don't know whether my analysis is correct (and therefore the > > proposed > > solution appropriate), as reproducing the problem is very > > difficult... > > > Module parameters are "icky", isn't there some way we can just > dynamically determine this (i.e. fall back to longer and longer wait > times?) No, we cannot in general predict how long EMI lasts. But, for example, I have a known source of EMI close to the computer (RF transmitter) and I know it lasts a couple of seconds, therefore I am able to set up the parameter correctly. Also, I believe once it fails the first time, the device is permanently disabled by the USB driver (I am no USB driver expert, do you confirm ?). Therefore, by the way the driver is actually designed, it is not possible to try again with a longer wait interval. And it's much more complicate, plus you still have to hard-code a maximum value I suppose... > Have you been able to test this out and see if it works? So far, so good. But before several days of testing, I won't be able to tell for sure whether waiting those 2 seconds completely eliminates the issue. This is because I cannot reliably reproduce the condition, it depends on the (random) EMI. Finally, I have been conservative in the patch defaulting to zero seconds, but perhaps, it's worth defaulting to 1 second or something similar. > thanks, > > greg k-h You're welcome. Guido -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: Wait before re-enabling a port that has been disabled due to EMI
Hi Oliver, that would be a misconfiguration issue, however I think we can set an upper limit for the new parameter... Also consider that the other "initial_descriptor_timeout" parameter is not limited by an upper limit... And by the way, I should preferably modify the patch so that it correctly documents Documentation/kernel-parameters.txt. Regards, Guido Il 21 marzo 2016 09:52:39 CET, Oliver Neukum ha scritto: >On Sun, 2016-03-20 at 18:09 +0100, Guido Trentalancia wrote: >> Hello. >> >> Considering that EM interference can last for a while (generally up >to >> seconds), I am currently testing the following patch for module >usbcore >> so that it is possible to specify an amount of time to wait before >> trying to re-enable a port that has been previously disabled by the >hub >> (supposedly because of EM interference). >> >> Hopefully, setting the right positive value (for example, 2000 >> milliseconds) would help overcome situations such as the following: >> > >The idea seems sound to me, but the implementation is problematic. > > >> @@ -4960,6 +4967,7 @@ static void port_event(struct usb_hub *h >> if (!(portstatus & USB_PORT_STAT_ENABLE) >> && !connect_change && udev) { >> dev_err(&port_dev->dev, "disabled by hub (EMI?), >re-enabling...\n"); >> +msleep(emi_recover_timer); > >You cannot just stall here for seconds. > > Regards > Oliver > > >-- >To unsubscribe from this list: send the line "unsubscribe linux-usb" in >the body of a message to majord...@vger.kernel.org >More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: Wait before re-enabling a port that has been disabled due to EMI
Hello Alan, thanks for getting back on this... Il 21 marzo 2016 16:01:17 CET, Alan Stern ha scritto: >On Sun, 20 Mar 2016, Guido Trentalancia wrote: > >> Hello. >> >> Considering that EM interference can last for a while (generally up >to >> seconds), I am currently testing the following patch for module >usbcore >> so that it is possible to specify an amount of time to wait before >> trying to re-enable a port that has been previously disabled by the >hub >> (supposedly because of EM interference). >> >> Hopefully, setting the right positive value (for example, 2000 >> milliseconds) would help overcome situations such as the following: >> >> [ 1295.575679] usb 6-2: FTDI USB Serial Device converter now attached >> to ttyUSB1 >> [ 1302.204285] usb usb6-port2: disabled by hub (EMI?), re-enabling... >> >> *** NOTE: EMI is probably still present here *** >> >> [ 1303.205202] usb 6-2: USB disconnect, device number 6 >> [ 1303.205907] ftdi_sio ttyUSB1: FTDI USB Serial Device converter now >> disconnected from ttyUSB1 >> [ 1303.205950] ftdi_sio 6-2:1.0: device disconnected >> [ 1303.414089] usb 6-2: new full-speed USB device number 7 using >> uhci_hcd >> [ 1303.526226] usb 6-2: device descriptor read/64, error -71 >> [ 1303.894228] usb 6-2: new full-speed USB device number 8 using >> uhci_hcd >> [ 1304.006185] usb 6-2: device descriptor read/64, error -71 >> [ 1304.219089] usb 6-2: device descriptor read/64, error -71 >> [ 1304.422107] usb 6-2: new full-speed USB device number 9 using >> uhci_hcd >> [ 1304.640020] usb 6-2: device not accepting address 9, error -71 >> [ 1304.752024] usb 6-2: new full-speed USB device number 10 using >> uhci_hcd >> [ 1305.160020] usb 6-2: device not accepting address 10, error -71 >> [ 1305.160038] hub 6-0:1.0: unable to enumerate USB device on port 2 >> >> *** NOTE: Device is permanently disabled at this point *** >> >> I don't know whether my analysis is correct (and therefore the >proposed >> solution appropriate), as reproducing the problem is very >difficult... > >Would it be better instead to provide a mechanism for re-enabling the >port after it has been "permanently" disabled? After all, you don't >know how long the EMI is going to last, and so the kernel has to give >up at some point. This means increasing the delay will make problems >more infrequent but won't eliminate them completely. > >Alan Stern It should be automatic (e.g. for servers). The manual solution doesn't need a patch: it's hardware, i. e. unplug then plug-again ;) In some cases it is possibile to predict how long interference lasts (for example, the source is co-located and known). This is not that uncommon, I suppose... When the source is not known then only statistical assumptions can be made regarding the duration of the interference: a value between 1000 and 2000 would probably suit most cases and also seems in line with other hard-coded waiting times in the driver. As you correctly noted, further increasing the value will make problems more infrequent but not necessarily would eliminate them completely (consider, for example, a very long USB cable picking up interference from a long wireless phone call lasting minutes, although even in such case there will usually be fades of the order of several milliseconds or 1-2 seconds). There is no "one fits all" solution, I suppose: only statistical assumptions and optimal solutions based upon them (and/or full configurability such as in a kernel parameter). Consider at the moment, we have least reliability against such EMI issues, as they all have finite duration. Regards, Guido -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: Wait before re-enabling a port that has been disabled due to EMI
Consider also a couple of very common non-telecom EMI sources: vehicle engines and lifts starting up. Both of them have a typical duration which I suppose is of 1, 2 or at most 3 seconds (although I do not have exact figures or references to them). What would really help here is an ad-hoc study on typical duration of EMI inside buildings... In the meanwhile, I believe 1000 milliseconds (plus configurability) to be safe. Regards, Guido Il 21 marzo 2016 16:42:14 CET, Guido Trentalancia ha scritto: >Hello Alan, > >thanks for getting back on this... > >Il 21 marzo 2016 16:01:17 CET, Alan Stern >ha scritto: >>On Sun, 20 Mar 2016, Guido Trentalancia wrote: >> >>> Hello. >>> >>> Considering that EM interference can last for a while (generally up >>to >>> seconds), I am currently testing the following patch for module >>usbcore >>> so that it is possible to specify an amount of time to wait before >>> trying to re-enable a port that has been previously disabled by the >>hub >>> (supposedly because of EM interference). >>> >>> Hopefully, setting the right positive value (for example, 2000 >>> milliseconds) would help overcome situations such as the following: >>> >>> [ 1295.575679] usb 6-2: FTDI USB Serial Device converter now >attached >>> to ttyUSB1 >>> [ 1302.204285] usb usb6-port2: disabled by hub (EMI?), >re-enabling... >>> >>> *** NOTE: EMI is probably still present here *** >>> >>> [ 1303.205202] usb 6-2: USB disconnect, device number 6 >>> [ 1303.205907] ftdi_sio ttyUSB1: FTDI USB Serial Device converter >now >>> disconnected from ttyUSB1 >>> [ 1303.205950] ftdi_sio 6-2:1.0: device disconnected >>> [ 1303.414089] usb 6-2: new full-speed USB device number 7 using >>> uhci_hcd >>> [ 1303.526226] usb 6-2: device descriptor read/64, error -71 >>> [ 1303.894228] usb 6-2: new full-speed USB device number 8 using >>> uhci_hcd >>> [ 1304.006185] usb 6-2: device descriptor read/64, error -71 >>> [ 1304.219089] usb 6-2: device descriptor read/64, error -71 >>> [ 1304.422107] usb 6-2: new full-speed USB device number 9 using >>> uhci_hcd >>> [ 1304.640020] usb 6-2: device not accepting address 9, error -71 >>> [ 1304.752024] usb 6-2: new full-speed USB device number 10 using >>> uhci_hcd >>> [ 1305.160020] usb 6-2: device not accepting address 10, error -71 >>> [ 1305.160038] hub 6-0:1.0: unable to enumerate USB device on port 2 >>> >>> *** NOTE: Device is permanently disabled at this point *** >>> >>> I don't know whether my analysis is correct (and therefore the >>proposed >>> solution appropriate), as reproducing the problem is very >>difficult... >> >>Would it be better instead to provide a mechanism for re-enabling the >>port after it has been "permanently" disabled? After all, you don't >>know how long the EMI is going to last, and so the kernel has to give >>up at some point. This means increasing the delay will make problems >>more infrequent but won't eliminate them completely. >> >>Alan Stern > >It should be automatic (e.g. for servers). The manual solution doesn't >need a patch: it's hardware, i. >e. unplug then plug-again ;) > >In some cases it is possibile to predict how long interference lasts >(for example, the source is co-located and known). This is not that >uncommon, I suppose... > >When the source is not known then only statistical assumptions can be >made regarding the duration of the interference: a value between 1000 >and 2000 would probably suit most cases and also seems in line with >other hard-coded waiting times in the driver. > >As you correctly noted, further increasing the value will make problems >more infrequent but not necessarily would eliminate them completely >(consider, for example, a very long USB cable picking up interference >from a long wireless phone call lasting minutes, although even in such >case there will usually be fades of the order of several milliseconds >or 1-2 seconds). > >There is no "one fits all" solution, I suppose: only statistical >assumptions and optimal solutions based upon them (and/or full >configurability such as in a kernel parameter). > >Consider at the moment, we have least reliability against such EMI >issues, as they all have finite duration. > >Regards, > >Guido >-- >To unsubscribe from this list: send the line "unsubscribe linux-usb" in >the body of a message to majord...@vger.kernel.org >More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] usb: Wait before re-enabling a port that has been disabled due to EMI
Hello again. On lun, 2016-03-21 at 09:36 -0400, Greg KH wrote: > On Sun, Mar 20, 2016 at 06:57:30PM +0100, Guido Trentalancia wrote: > > > > Hello Greg ! > > > > On dom, 2016-03-20 at 10:34 -0700, Greg KH wrote: > > > > > > On Sun, Mar 20, 2016 at 06:09:57PM +0100, Guido Trentalancia > > > wrote: > > > > > > > > > > > > [ 1295.575679] usb 6-2: FTDI USB Serial Device converter now > > > > attached > > > > to ttyUSB1 > > > > [ 1302.204285] usb usb6-port2: disabled by hub (EMI?), re- > > > > enabling... > > > > > > > > *** NOTE: EMI is probably still present here *** > > > > > > > > [ 1303.205202] usb 6-2: USB disconnect, device number 6 > > > > [ 1303.205907] ftdi_sio ttyUSB1: FTDI USB Serial Device > > > > converter > > > > now > > > > disconnected from ttyUSB1 > > > > [ 1303.205950] ftdi_sio 6-2:1.0: device disconnected > > > > [ 1303.414089] usb 6-2: new full-speed USB device number 7 > > > > using > > > > uhci_hcd > > > > [ 1303.526226] usb 6-2: device descriptor read/64, error -71 > > > > [ 1303.894228] usb 6-2: new full-speed USB device number 8 > > > > using > > > > uhci_hcd > > > > [ 1304.006185] usb 6-2: device descriptor read/64, error -71 > > > > [ 1304.219089] usb 6-2: device descriptor read/64, error -71 > > > > [ 1304.422107] usb 6-2: new full-speed USB device number 9 > > > > using > > > > uhci_hcd > > > > [ 1304.640020] usb 6-2: device not accepting address 9, error > > > > -71 > > > > [ 1304.752024] usb 6-2: new full-speed USB device number 10 > > > > using > > > > uhci_hcd > > > > [ 1305.160020] usb 6-2: device not accepting address 10, error > > > > -71 > > > > [ 1305.160038] hub 6-0:1.0: unable to enumerate USB device on > > > > port > > > > 2 > > > > > > > > *** NOTE: Device is permanently disabled at this point *** > > > > > Yes, this needs more work, and again, don't use a module parameter > please, make it "automatic" if at all possible. Here is a revised patch. I've tried to keep things very simple. It's "automatic" and it doesn't make use of a module parameter as suggested by Greg. Also, it avoids the risk of stalling for seconds because of a misconfiguration of the module parameter. Regards, Guido. Modify the usbcore module to wait for a second before trying to re-enable a USB port that has been previously disabled by the hub (possibly due to EMI). In general, the duration of EMI is random and unpredictable. This patch tries to ammeliorate several common cases, while at the same time avoiding to wait for an unacceptable period of time. Signed-off-by: Guido Trentalancia --- drivers/usb/core/hub.c |1 + 1 file changed, 1 insertion(+) --- linux-4.5-orig/drivers/usb/core/hub.c 2016-03-23 23:00:40.153721641 +0100 +++ linux-4.5/drivers/usb/core/hub.c2016-03-23 21:41:41.527575138 +0100 @@ -4975,6 +4975,7 @@ static void port_event(struct usb_hub *h if (!(portstatus & USB_PORT_STAT_ENABLE) && !connect_change && udev) { dev_err(&port_dev->dev, "disabled by hub (EMI?), re-enabling...\n"); + msleep(1000); /* Wait for EMI transient to finish or fade */ connect_change = 1; } } -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/3] usb: gadget: net2280: Fix overrun of OUT messages
The OUT endpoint normally blocks (NAK) subsequent packets when a short packet was received and returns an incomplete queue entry to the gadget driver. Thereby the gadget driver can detect a short packet when reading queue entries with a length that is not equal to a multiple of packet size. The start_queue() function enables receiving OUT packets regardless of the content of the OUT FIFO. This results in a problem: When receiving is enabled again, more OUT packets are appended to the last short packet and the gadget driver cannot determine the end of a short packet anymore. Furthermore the remaining data in the OUT FIFO is not delivered to the gadget driver immediately and can produce timeout errors. This fix stops OUT naking only when OUT naking is enabled and when the OUT FIFO is empty. It ensures that all received data is delivered to the gadget driver which can detect a short packet now before new packets overrun the last short packet. Signed-off-by: Guido Kiener --- drivers/usb/gadget/udc/net2280.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c index f63f82450bf4..e0b413e9e532 100644 --- a/drivers/usb/gadget/udc/net2280.c +++ b/drivers/usb/gadget/udc/net2280.c @@ -866,9 +866,6 @@ static void start_queue(struct net2280_ep *ep, u32 dmactl, u32 td_dma) (void) readl(&ep->dev->pci->pcimstctl); writel(BIT(DMA_START), &dma->dmastat); - - if (!ep->is_in) - stop_out_naking(ep); } static void start_dma(struct net2280_ep *ep, struct net2280_request *req) @@ -907,6 +904,7 @@ static void start_dma(struct net2280_ep *ep, struct net2280_request *req) writel(BIT(DMA_START), &dma->dmastat); return; } + stop_out_naking(ep); } tmp = dmactl_default; -- 2.17.1
[PATCH v2 3/3] usb: gadget: net2272: Fix net2272_dequeue()
Restore the status of ep->stopped in function net2272_dequeue(). When the given request is not found in the endpoint queue the function returns -EINVAL without restoring the state of ep->stopped. Thus the endpoint keeps blocked and does not transfer any data anymore. This fix is only compile-tested, since we do not have a corresponding hardware. An analogous fix was tested in the sibling driver. See "usb: gadget: net2280: Fix net2280_dequeue()" Signed-off-by: Guido Kiener --- drivers/usb/gadget/udc/net2272.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/gadget/udc/net2272.c b/drivers/usb/gadget/udc/net2272.c index b77f3126580e..c2011cd7df8c 100644 --- a/drivers/usb/gadget/udc/net2272.c +++ b/drivers/usb/gadget/udc/net2272.c @@ -945,6 +945,7 @@ net2272_dequeue(struct usb_ep *_ep, struct usb_request *_req) break; } if (&req->req != _req) { + ep->stopped = stopped; spin_unlock_irqrestore(&ep->dev->lock, flags); return -EINVAL; } -- 2.17.1
[PATCH v2 2/3] usb: gadget: net2280: Fix net2280_dequeue()
When a request must be dequeued with net2280_dequeue() e.g. due to a device clear action and the same request is finished by the function scan_dma_completions() then the function net2280_dequeue() does not find the request in the following search loop and returns the error -EINVAL without restoring the status ep->stopped. Thus the endpoint keeps blocked and does not receive any data anymore. This fix restores the status and does not issue an error message. Signed-off-by: Guido Kiener --- drivers/usb/gadget/udc/net2280.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c index e0b413e9e532..898339e5df10 100644 --- a/drivers/usb/gadget/udc/net2280.c +++ b/drivers/usb/gadget/udc/net2280.c @@ -1273,9 +1273,9 @@ static int net2280_dequeue(struct usb_ep *_ep, struct usb_request *_req) break; } if (&req->req != _req) { + ep->stopped = stopped; spin_unlock_irqrestore(&ep->dev->lock, flags); - dev_err(&ep->dev->pdev->dev, "%s: Request mismatch\n", - __func__); + ep_dbg(ep->dev, "%s: Request mismatch\n", __func__); return -EINVAL; } -- 2.17.1
[PATCH v3] usb: gadget: net2280: Fix overrun of OUT messages
The OUT endpoint normally blocks (NAK) subsequent packets when a short packet was received and returns an incomplete queue entry to the gadget driver. Thereby the gadget driver can detect a short packet when reading queue entries with a length that is not equal to a multiple of packet size. The start_queue() function enables receiving OUT packets regardless of the content of the OUT FIFO. This results in a race: With the current code, it's possible that the "!ep->is_in && (readl(&ep->regs->ep_stat) & BIT(NAK_OUT_PACKETS))" test in start_dma() will fail, then a short packet will be received, and then start_queue() will call stop_out_naking(). That's what we don't want (OUT naking gets turned off while there is data in the FIFO) because then the next driver request might receive a mixture of old and new packets. With the patch, this race can't occur because the FIFO's state is tested after we know that OUT naking is already turned on, and OUT naking is stopped only when both of the conditions are met. This ensures that all received data is delivered to the gadget driver, which can detect a short packet now before new packets are appended to the last short packet. Signed-off-by: Guido Kiener --- drivers/usb/gadget/udc/net2280.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c index f63f82450bf4..e0b413e9e532 100644 --- a/drivers/usb/gadget/udc/net2280.c +++ b/drivers/usb/gadget/udc/net2280.c @@ -866,9 +866,6 @@ static void start_queue(struct net2280_ep *ep, u32 dmactl, u32 td_dma) (void) readl(&ep->dev->pci->pcimstctl); writel(BIT(DMA_START), &dma->dmastat); - - if (!ep->is_in) - stop_out_naking(ep); } static void start_dma(struct net2280_ep *ep, struct net2280_request *req) @@ -907,6 +904,7 @@ static void start_dma(struct net2280_ep *ep, struct net2280_request *req) writel(BIT(DMA_START), &dma->dmastat); return; } + stop_out_naking(ep); } tmp = dmactl_default; -- 2.17.1
JMicron JMS578 USB-to-SATA HDD enclosure not working
Hello. I am trying to use a Digitus DA-71114 USB-to-SATA HDD enclosure. Such unit is reported to use the JMicron JMS578 chipset by the same manufacturer, although it is listed with a different USB VID/PID: 0080:a001. Immediately after plugging in the USB cable, it reports I/O errors, even though the hard-drive is fine (mounts and reads/writes fine under Windows without the enclosure): [ 5432.689781] usb 2-1: new SuperSpeed Gen 1 USB device number 29 using xhci_hcd [ 5432.702547] usb 2-1: New USB device found, idVendor=0080, idProduct=a001, bcdDevice= 1.00 [ 5432.702553] usb 2-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3 [ 5432.702557] usb 2-1: Product: External USB 3.0 [ 5432.702561] usb 2-1: Manufacturer: TOSHIBA [ 5432.702565] usb 2-1: SerialNumber: 201503310008E [ 5432.730948] usbcore: registered new interface driver usb-storage [ 5432.736029] scsi host2: uas [ 5432.736373] usbcore: registered new interface driver uas [ 5432.736939] scsi 2:0:0:0: Direct-Access TO Exter nal USB 3.0 6101 PQ: 0 ANSI: 6 [ 5432.738326] sd 2:0:0:0: Attached scsi generic sg2 type 0 [ 5435.336588] sd 2:0:0:0: [sdb] 1953525168 512-byte logical blocks: (1.00 TB/932 GiB) [ 5435.336594] sd 2:0:0:0: [sdb] 4096-byte physical blocks [ 5435.336762] sd 2:0:0:0: [sdb] Write Protect is off [ 5435.336766] sd 2:0:0:0: [sdb] Mode Sense: 53 00 00 08 [ 5435.337063] sd 2:0:0:0: [sdb] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA [ 5435.337347] sd 2:0:0:0: [sdb] Optimal transfer size 33553920 bytes not a multiple of physical block size (4096 bytes) [ 5465.794203] sd 2:0:0:0: [sdb] tag#6 uas_eh_abort_handler 0 uas-tag 1 inflight: CMD IN [ 5465.794211] sd 2:0:0:0: [sdb] tag#6 CDB: Read(10) 28 00 00 00 00 00 00 00 08 00 [ 5465.800252] scsi host2: uas_eh_device_reset_handler start [ 5465.915678] usb 2-1: reset SuperSpeed Gen 1 USB device number 29 using xhci_hcd [ 5465.931925] scsi host2: uas_eh_device_reset_handler success [ 5496.510222] scsi host2: uas_eh_device_reset_handler start [ 5496.510329] sd 2:0:0:0: [sdb] tag#11 uas_zap_pending 0 uas-tag 1 inflight: CMD [ 5496.510337] sd 2:0:0:0: [sdb] tag#11 CDB: Read(10) 28 00 00 00 00 00 00 00 08 00 [ 5496.625614] usb 2-1: reset SuperSpeed Gen 1 USB device number 29 using xhci_hcd [ 5496.642411] scsi host2: uas_eh_device_reset_handler success [ 5527.230204] scsi host2: uas_eh_device_reset_handler start [ 5527.230309] sd 2:0:0:0: [sdb] tag#9 uas_zap_pending 0 uas-tag 1 inflight: CMD [ 5527.230316] sd 2:0:0:0: [sdb] tag#9 CDB: Read(10) 28 00 00 00 00 00 00 00 08 00 [ 5527.345769] usb 2-1: reset SuperSpeed Gen 1 USB device number 29 using xhci_hcd [ 5527.361964] scsi host2: uas_eh_device_reset_handler success [ 5527.780612] sd 2:0:0:0: [sdb] tag#10 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE [ 5527.780631] sd 2:0:0:0: [sdb] tag#10 Sense Key : Aborted Command [current] [ 5527.780636] sd 2:0:0:0: [sdb] tag#10 Add. Sense: No additional sense information [ 5527.780642] sd 2:0:0:0: [sdb] tag#10 CDB: Read(10) 28 00 00 00 00 00 00 00 08 00 [ 5527.780647] print_req_error: I/O error, dev sdb, sector 0 flags 0 [ 5527.780657] Buffer I/O error on dev sdb, logical block 0, async page read I have also attached the usbmon dump just before and after plugging in the device. Adding the US_FL_BROKEN_FUA in unusual_uas.h and unusual_devs.h does not help ! I have also tried adding many other quirks (such as US_FL_NO_REPORT_OPCODES, US_FL_NO_ATA_1X, US_FL_IGNORE_RESIDUE, US_FL_FIX_CAPACITY, US_FL_NO_WP_DETECT, US_FL_MAX_SECTORS_64) without any luck !! The problem also happens when not using UAS but the standard USB storage driver (fails on READ command, sector 0 and sometimes also sector 1953524992). When the drive is used in the enclosure it is completely unusable, as it fails even on fdisk... What should I do ? Thanks. Guido9d969f245c00 1517920124 S Ci:2:001:0 s a3 00 0001 0004 4 < 9d969f245c00 1517920155 C Ci:2:001:0 0 4 = 03020100 9d969f245c00 1517920163 S Co:2:001:0 s 23 01 0010 0001 0 9d969f245c00 1517920170 C Co:2:001:0 0 0 9d969f245c00 1517920173 S Ci:2:001:0 s a3 00 0002 0004 4 < 9d969f245c00 1517920179 C Ci:2:001:0 0 4 = a002 9d969f245c00 1517920182 S Ci:2:001:0 s a3 00 0003 0004 4 < 9d969f245c00 1517920187 C Ci:2:001:0 0 4 = a002 9d969f245c00 1517920190 S Ci:2:001:0 s a3 00 0004 0004 4 < 9d969f245c00 1517920195 C Ci:2:001:0 0 4 = a002 9d969f245c00 1517920198 S Ci:2:001:0 s a3 00 0005 0004 4 < 9d969f245c00 1517920203 C Ci:2:001:0 0 4 = a002 9d969f245c00 1517920206 S Ci:2:001:0 s a3 00 0006 0004 4 < 9d969f245c00 1517920211 C Ci:2:001:0 0 4 = a002 9d969ecff600 1518022914 S Ii:2:001:1 -115:2048 4 < 9d969f245c00 1518022942 S Ci:2:001:0 s a3 00 0001 0004 4 < 9d969f245c00 1518022960 C Ci:2:001:0 0 4 = 0302 9d969f245c00 1518023089 S Ci:2:001:0 s a3 00 0001 0004 4 < 9d969f245c00 1518023097 C Ci:2:001:0 0 4 =
Re: JMicron JMS578 USB-to-SATA HDD enclosure not working
Hello again. I have now upgraded the original HDD enclosure firmware (version 46.01.00.01) with the latest available one from the Hardkernel.com / Odroid.com project (version 173.01.00.02). The problem persists with similar symptoms, however the Sense Key is now different: sd 2:0:0:0: [sdb] tag#13 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE sd 2:0:0:0: [sdb] tag#13 Sense Key : Data Protect [current] sd 2:0:0:0: [sdb] tag#13 Add. Sense: Logical unit access not authorized sd 2:0:0:0: [sdb] tag#13 CDB: Read(10) 28 00 00 00 00 00 00 00 08 00 print_req_error: critical target error, dev sdb, sector 0 flags 0 Buffer I/O error on dev sdb, logical block 0, async page read sdb: unable to read partition table sd 2:0:0:0: [sdb] Attached SCSI disk sd 2:0:0:0: [sdb] tag#4 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE sd 2:0:0:0: [sdb] tag#4 Sense Key : Data Protect [current] sd 2:0:0:0: [sdb] tag#4 Add. Sense: Logical unit access not authorized sd 2:0:0:0: [sdb] tag#4 CDB: Read(10) 28 00 74 70 6d 00 00 00 08 00 print_req_error: critical target error, dev sdb, sector 1953524992 flags 80700 So, the Sense basically changed from "No additional sense" to "Logical unit access not authorized", which at least seems a bit more meaningful... The hard-drive is a brand-new Seagate 1TB HDD which works perfectly fine when connected to the SATA port directly. Is anybody aware of any kind of Data Protection or Access Authorization option that needs to be disabled or enabled, respectively ? If yes, how ? Thanks very much for your time ! Guido On Fri, 17/05/2019 at 21.32 +0200, Guido Trentalancia wrote: > Hello. > > I am trying to use a Digitus DA-71114 USB-to-SATA HDD enclosure. > > Such unit is reported to use the JMicron JMS578 chipset by the same > manufacturer, although it is listed with a different USB VID/PID: > 0080:a001. > > Immediately after plugging in the USB cable, it reports I/O errors, > even though the hard-drive is fine (mounts and reads/writes fine > under > Windows without the enclosure): > > [ 5432.689781] usb 2-1: new SuperSpeed Gen 1 USB device number 29 > using > xhci_hcd > [ 5432.702547] usb 2-1: New USB device found, idVendor=0080, > idProduct=a001, bcdDevice= 1.00 > [ 5432.702553] usb 2-1: New USB device strings: Mfr=1, Product=2, > SerialNumber=3 > [ 5432.702557] usb 2-1: Product: External USB 3.0 > [ 5432.702561] usb 2-1: Manufacturer: TOSHIBA > [ 5432.702565] usb 2-1: SerialNumber: 201503310008E > [ 5432.730948] usbcore: registered new interface driver usb-storage > [ 5432.736029] scsi host2: uas > [ 5432.736373] usbcore: registered new interface driver uas > [ 5432.736939] scsi 2:0:0:0: Direct-Access TO Exter nal USB > 3.0 6101 PQ: 0 ANSI: 6 > [ 5432.738326] sd 2:0:0:0: Attached scsi generic sg2 type 0 > [ 5435.336588] sd 2:0:0:0: [sdb] 1953525168 512-byte logical blocks: > (1.00 TB/932 GiB) > [ 5435.336594] sd 2:0:0:0: [sdb] 4096-byte physical blocks > [ 5435.336762] sd 2:0:0:0: [sdb] Write Protect is off > [ 5435.336766] sd 2:0:0:0: [sdb] Mode Sense: 53 00 00 08 > [ 5435.337063] sd 2:0:0:0: [sdb] Write cache: enabled, read cache: > enabled, doesn't support DPO or FUA > [ 5435.337347] sd 2:0:0:0: [sdb] Optimal transfer size 33553920 bytes > not a multiple of physical block size (4096 bytes) > [ 5465.794203] sd 2:0:0:0: [sdb] tag#6 uas_eh_abort_handler 0 uas-tag > 1 > inflight: CMD IN > [ 5465.794211] sd 2:0:0:0: [sdb] tag#6 CDB: Read(10) 28 00 00 00 00 > 00 > 00 00 08 00 > [ 5465.800252] scsi host2: uas_eh_device_reset_handler start > [ 5465.915678] usb 2-1: reset SuperSpeed Gen 1 USB device number 29 > using xhci_hcd > [ 5465.931925] scsi host2: uas_eh_device_reset_handler success > [ 5496.510222] scsi host2: uas_eh_device_reset_handler start > [ 5496.510329] sd 2:0:0:0: [sdb] tag#11 uas_zap_pending 0 uas-tag 1 > inflight: CMD > [ 5496.510337] sd 2:0:0:0: [sdb] tag#11 CDB: Read(10) 28 00 00 00 00 > 00 > 00 00 08 00 > [ 5496.625614] usb 2-1: reset SuperSpeed Gen 1 USB device number 29 > using xhci_hcd > [ 5496.642411] scsi host2: uas_eh_device_reset_handler success > [ 5527.230204] scsi host2: uas_eh_device_reset_handler start > [ 5527.230309] sd 2:0:0:0: [sdb] tag#9 uas_zap_pending 0 uas-tag 1 > inflight: CMD > [ 5527.230316] sd 2:0:0:0: [sdb] tag#9 CDB: Read(10) 28 00 00 00 00 > 00 > 00 00 08 00 > [ 5527.345769] usb 2-1: reset SuperSpeed Gen 1 USB device number 29 > using xhci_hcd > [ 5527.361964] scsi host2: uas_eh_device_reset_handler success > [ 5527.780612] sd 2:0:0:0: [sdb] tag#10 FAILED Result: > hostbyte=DID_OK > driverbyte=DRIVER_SENSE > [ 5527.780631] sd 2:0:0:0: [sdb] tag#10 Sense Key : Aborted Command > [current] > [ 5527.780636] sd 2:0:0:0: [sdb] tag#10 Add. Sense: No additional > sense > informat
Re: JMicron JMS578 USB-to-SATA HDD enclosure not working
Thanks to the new firmware and its more meaningful Sense Key, I finally realized that the hard-drive was simply "locked" as in the BIOS Hard- Disk Password Lock feature. Therefore this is not a bug, the UAS driver is working fine with the HDD enclosure and the issue was simply a matter of unlocking the drive in the BIOS before connecting it to the JMicron JMS578 enclosure. On Sat, 18/05/2019 at 14.31 +0200, Guido Trentalancia wrote: > Hello again. > > I have now upgraded the original HDD enclosure firmware (version > 46.01.00.01) with the latest available one from the Hardkernel.com / > Odroid.com project (version 173.01.00.02). > > The problem persists with similar symptoms, however the Sense Key is > now different: > > sd 2:0:0:0: [sdb] tag#13 FAILED Result: hostbyte=DID_OK > driverbyte=DRIVER_SENSE > sd 2:0:0:0: [sdb] tag#13 Sense Key : Data Protect [current] > sd 2:0:0:0: [sdb] tag#13 Add. Sense: Logical unit access not > authorized > sd 2:0:0:0: [sdb] tag#13 CDB: Read(10) 28 00 00 00 00 00 00 00 08 00 > print_req_error: critical target error, dev sdb, sector 0 flags 0 > Buffer I/O error on dev sdb, logical block 0, async page read > sdb: unable to read partition table > sd 2:0:0:0: [sdb] Attached SCSI disk > sd 2:0:0:0: [sdb] tag#4 FAILED Result: hostbyte=DID_OK > driverbyte=DRIVER_SENSE > sd 2:0:0:0: [sdb] tag#4 Sense Key : Data Protect [current] > sd 2:0:0:0: [sdb] tag#4 Add. Sense: Logical unit access not > authorized > sd 2:0:0:0: [sdb] tag#4 CDB: Read(10) 28 00 74 70 6d 00 00 00 08 00 > print_req_error: critical target error, dev sdb, sector 1953524992 > flags 80700 > > So, the Sense basically changed from "No additional sense" to > "Logical > unit access not authorized", which at least seems a bit more > meaningful... > > The hard-drive is a brand-new Seagate 1TB HDD which works perfectly > fine when connected to the SATA port directly. > > Is anybody aware of any kind of Data Protection or Access > Authorization > option that needs to be disabled or enabled, respectively ? If yes, > how > ? > > Thanks very much for your time ! > > Guido > > On Fri, 17/05/2019 at 21.32 +0200, Guido Trentalancia wrote: > > Hello. > > > > I am trying to use a Digitus DA-71114 USB-to-SATA HDD enclosure. > > > > Such unit is reported to use the JMicron JMS578 chipset by the same > > manufacturer, although it is listed with a different USB VID/PID: > > 0080:a001. > > > > Immediately after plugging in the USB cable, it reports I/O errors, > > even though the hard-drive is fine (mounts and reads/writes fine > > under > > Windows without the enclosure): > > > > [ 5432.689781] usb 2-1: new SuperSpeed Gen 1 USB device number 29 > > using > > xhci_hcd > > [ 5432.702547] usb 2-1: New USB device found, idVendor=0080, > > idProduct=a001, bcdDevice= 1.00 > > [ 5432.702553] usb 2-1: New USB device strings: Mfr=1, Product=2, > > SerialNumber=3 > > [ 5432.702557] usb 2-1: Product: External USB 3.0 > > [ 5432.702561] usb 2-1: Manufacturer: TOSHIBA > > [ 5432.702565] usb 2-1: SerialNumber: 201503310008E > > [ 5432.730948] usbcore: registered new interface driver usb-storage > > [ 5432.736029] scsi host2: uas > > [ 5432.736373] usbcore: registered new interface driver uas > > [ 5432.736939] scsi 2:0:0:0: Direct-Access TO Exter nal USB > > 3.0 6101 PQ: 0 ANSI: 6 > > [ 5432.738326] sd 2:0:0:0: Attached scsi generic sg2 type 0 > > [ 5435.336588] sd 2:0:0:0: [sdb] 1953525168 512-byte logical > > blocks: > > (1.00 TB/932 GiB) > > [ 5435.336594] sd 2:0:0:0: [sdb] 4096-byte physical blocks > > [ 5435.336762] sd 2:0:0:0: [sdb] Write Protect is off > > [ 5435.336766] sd 2:0:0:0: [sdb] Mode Sense: 53 00 00 08 > > [ 5435.337063] sd 2:0:0:0: [sdb] Write cache: enabled, read cache: > > enabled, doesn't support DPO or FUA > > [ 5435.337347] sd 2:0:0:0: [sdb] Optimal transfer size 33553920 > > bytes > > not a multiple of physical block size (4096 bytes) > > [ 5465.794203] sd 2:0:0:0: [sdb] tag#6 uas_eh_abort_handler 0 uas- > > tag > > 1 > > inflight: CMD IN > > [ 5465.794211] sd 2:0:0:0: [sdb] tag#6 CDB: Read(10) 28 00 00 00 00 > > 00 > > 00 00 08 00 > > [ 5465.800252] scsi host2: uas_eh_device_reset_handler start > > [ 5465.915678] usb 2-1: reset SuperSpeed Gen 1 USB device number 29 > > using xhci_hcd > > [ 5465.931925] scsi host2: uas_eh_device_reset_handler success > > [ 5496.510222] scsi host2: uas_eh_device_reset_handler start > > [ 5496.510329] sd 2:0:0:0: [sdb] tag#11 uas_zap_pending 0 uas-tag 1 > > inflight: CMD >
usb: usbtmc: Proposal for new ioctl functions
Hi all, This is a follow up mail from the discussion of thread https://marc.info/?l=linux-usb&m=149485247607564&w=2 Our working group "VISA for Linux" (IVI Foundation, www.ivifoundation.org) wants to extend the Linux USBTMC driver (linux/drivers/usb/class/usbtmc.c) that can communicate with the T&M instruments. To meet our requirements we need some additional ioctl functions that can send control, bulk in/out messages in a generic way. Kindly supported by Dave Penkler there is now a github project at https://github.com/dpenkler/linux-usbtmc that already supports new functions to set/get timeout, EOM flag (End of Message), and termchar (detection of termination character). This project and forked repos are our playground and still under development. Our experimental and generic functions are currently developed at the fork: https://github.com/GuidoKiener/linux-usbtmc For a fast read/write we want to implement new generic ioctl functions: #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_WAIT_SRQ _IOW(USBTMC_IOC_NR, 23, unsigned int) #define USBTMC_IOCTL_CANCEL_IO_IO(USBTMC_IOC_NR, 35) #define USBTMC_IOCTL_CLEANUP_IO _IO(USBTMC_IOC_NR, 36) /* For test purpose only */ #define USBTMC_IOCTL_SET_OUT_HALT _IO(USBTMC_IOC_NR, 30) #define USBTMC_IOCTL_SET_IN_HALT _IO(USBTMC_IOC_NR, 31) For further description please refer to the readme.md file at https://github.com/GuidoKiener/linux-usbtmc/blob/master/README.md Open questions and comments can be just added to the wiki: https://github.com/GuidoKiener/linux-usbtmc/wiki When our working group is happy with the proposed extensions and the driver tested by several T&M companies then we would like to submit these patches to the Linux kernel. We are looking forward to your comments. Please let us know if some of you would like to participate in more discussions. Or let us know if you do not want to get more emails about this topic. -Guido -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: usb: usbtmc: Proposal for new ioctl functions
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. 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? 2. Shall we add new sysfs parameters for timeout and EOM flag? (ioctls are already added) 3. Is there any rule when to use upper/lower case or underscore for these parameters (e.g. Timeout, EOM or end_of_message)? 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. Is there any way (without changing the USB subsystem) to detect ZLPs? 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) ? Best regards, Guido -Original Message- From: Greg KH Sent: Wednesday, March 28, 2018 7:52 AM To: Kiener Guido 1DC3 Subject: Re: usb: usbtmc: Proposal for new ioctl functions On Tue, Mar 27, 2018 at 08:57:27PM +0200, guido.kie...@rohde-schwarz.com wrote: > Hi all, > > This is a follow up mail from the discussion of thread > https://marc.info/?l=linux-usb&m=149485247607564&w=2 > > Our working group "VISA for Linux" (IVI Foundation, > www.ivifoundation.org) > > wants to extend the Linux USBTMC driver > (linux/drivers/usb/class/usbtmc.c) > that can communicate with the T&M instruments. > > To meet our requirements we need some additional ioctl functions that > can send control, bulk in/out messages in a generic way. > > Kindly supported by Dave Penkler there is now a github project at > https://github.com/dpenkler/linux-usbtmc > that already supports new functions to set/get timeout, EOM flag (End > of Message), and termchar (detection of termination character). > > This project and forked repos are our playground and still under > development. > > Our experimental and generic functions are currently developed at the > fork: > https://github.com/GuidoKiener/linux-usbtmc > > For a fast read/write we want to implement new generic ioctl functions: > #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_WAIT_SRQ _IOW(USBTMC_IOC_NR, 23, unsigned int) > #define USBTMC_IOCTL_CANCEL_IO_IO(USBTMC_IOC_NR, 35) > #define USBTMC_IOCTL_CLEANUP_IO _IO(USBTMC_IOC_NR, 36) > /* For test purpose only */ > #define USBTMC_IOCTL_SET_OUT_HALT _IO(USBTMC_IOC_NR, 30) #define > USBTMC_IOCTL_SET_IN_HALT _IO(USBTMC_IOC_NR, 31) > > For further description please refer to the readme.md file at > https://github.com/GuidoKiener/linux-usbtmc/blob/master/README.md > > Open questions and comments can be just added to the wiki: > https://github.com/GuidoKiener/linux-usbtmc/wiki > > When our working group is happy with the proposed extensions and the > driver tested by several T&M companies then we would like to submit > these patches to the Linux kernel. Great, submit patches like any other developer when you have them ready. You don't need to do anything special here, that's how Linux is developed. :) Note, please do not depend on these new apis until _after_ we have merged the patches, as there might be changes required due to the review process finding problems, so please submit changes as soon as possible. thanks, greg k-h
RE: Re: usb: usbtmc: Proposal for new ioctl functions
[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 +, 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,
RE: Re: usb: usbtmc: Proposal for new ioctl functions
Alan, Thank you for your clarification. Indeed I'm missing some details how USB bulk protocol works. I will discuss your comments with Keysight & NI, and I hope we may ask you the one or other question. Guido -Original Message- From: Alan Stern Sent: Friday, April 06, 2018 4:59 PM To: Kiener Guido Subject: RE: Re: usb: usbtmc: Proposal for new ioctl functions On Thu, 5 Apr 2018, Guido Kiener wrote: > > 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. [The correct term is "packet", not "package".] > When a driver reads a package with maximum package length then it cannot > decide whether another package will follow or not. Not true; it _can_ tell. If the total amount of data received so far was less than the amount that the URB requested, then another packet _will_ follow. If the total amount of data is equal to the amount that the URB requested then another packet will _not_ follow. (And if the total amount of data is larger than the amount that the URB requested, an overrun error has occurred.) > 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. This is incorrect, and it indicates a misunderstanding of how the USB bulk protocol works. Suppose you submit a 4-KB URB, and the driver reads exactly 4 KB. The URB will be complete at that time; no ZLP will be sent because the host will not ask the device to send any more packets. In short, the device will send a bulk ZLP _only_ under the following conditions: The amount of data it wants to send is less than what the host expects to receive, and the amount is a multiple of the maximum packet length; or The host wants to receive a zero-length message (rather unlikely, but I suppose this could happen). If your communication protocol does not allow the device to know how much data the host expects to receive, but a ZLP can mess up the protocol, then the protocol is buggy and needs to be fixed. > 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. There should not be any problem to work around. > > 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. Bulk-OUT transfers never use ZLP, unless for some reason the host wants to send a message containing no data. Alan Stern
[PATCH 00/12] usb: usbtmc: Changes needed for compatible IVI/VISA library
The working group "VISA for Linux" of the IVI Foundation www.ivifoundation.org specifies common rules, shared libraries and drivers to implement the specification of "VPP-4.3: The VISA Library" on Linux to be compatible with implementations on other operating systems. The USBTMC protocol is part of the "VISA Library" that is used by many popular T&M applications. An initial implementation for Linux based on libusb has been created. While functional it has some drawbacks: - Performance - Requires root privileges to reclaim devices already claimed by the usbtmc driver The following collaborative patches meet the requirements of the IVI Foundation to implement the library based on the usbtmc driver. Improvements in the data transfer rate of over 130 MByte/s for usb 3.x connections have been measured. Guido Kiener, Dave Penkler, Steve Bayless (12): 01 Remove rigol_quirk 02 Support Read Status Byte with SRQ per file handle 03 Add ioctls to set/get usb timeout 04 Add ioctls for trigger, EOM bit, TermChar 05 Add ioctl for generic requests on control pipe 06 Add vendor specific/asynchronous read/write ioctls 07 Add ioctl USBTMC488_IOCTL_WAIT_SRQ 08 Optimize read/write and add ioctls for auto_abort 09 Fix ioctl USBTMC_IOCTL_CLEAR 10 Add test functions to set HALT Feature (Stall) 11 Add ioctls to abort with specific tags 12 Add ioctl to return API version of usbtmc driver Documentation/ioctl/ioctl-number.txt |2 +- drivers/usb/class/usbtmc.c | 1951 +- include/uapi/linux/usb/tmc.h | 60 + 3 files changed, 1628 insertions(+), 385 deletions(-) -- 2.17.0 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 02/12] usb: usbtmc: Support Read Status Byte with SRQ per file handle
- Add 'struct usbtmc_file_data' for each file handle to cache last srq_byte (=Status Byte with SRQ) received by usbtmc_interrupt(..) - usbtmc488_ioctl_read_stb returns cached srq_byte when available for each file handle to avoid race conditions of concurrent applications. - SRQ now sets EPOLLPRI instead of EPOLLIN - Caches other values TermChar, TermCharEnabled, auto_abort in 'struct usbtmc_file_data' will not be changed by sysfs device attributes during an open file session. Future ioctl functions can change these values. - use consistent error return value ETIMEOUT instead of ETIME Tested-by: Dave Penkler Reviewed-by: Steve Bayless Signed-off-by: Guido Kiener --- drivers/usb/class/usbtmc.c | 176 - 1 file changed, 136 insertions(+), 40 deletions(-) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index 529295a17579..5754354429d8 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -67,6 +67,7 @@ struct usbtmc_device_data { const struct usb_device_id *id; struct usb_device *usb_dev; struct usb_interface *intf; + struct list_head file_list; unsigned int bulk_in; unsigned int bulk_out; @@ -87,12 +88,12 @@ struct usbtmc_device_data { intiin_interval; struct urb*iin_urb; u16iin_wMaxPacketSize; - atomic_t srq_asserted; /* coalesced usb488_caps from usbtmc_dev_capabilities */ __u8 usb488_caps; /* attributes from the USB TMC spec for this device */ + /* They are used as default values for file_data */ u8 TermChar; bool TermCharEnabled; bool auto_abort; @@ -104,9 +105,26 @@ struct usbtmc_device_data { struct mutex io_mutex; /* only one i/o function running at a time */ wait_queue_head_t waitq; struct fasync_struct *fasync; + spinlock_t dev_lock; /* lock for file_list */ }; #define to_usbtmc_data(d) container_of(d, struct usbtmc_device_data, kref) +/* + * This structure holds private data for each USBTMC file handle. + */ +struct usbtmc_file_data { + struct usbtmc_device_data *data; + struct list_head file_elem; + + u8 srq_byte; + atomic_t srq_asserted; + + /* These values are initialized with default values from device_data */ + u8 TermChar; + bool TermCharEnabled; + bool auto_abort; +}; + /* Forward declarations */ static struct usb_driver usbtmc_driver; @@ -114,6 +132,7 @@ static void usbtmc_delete(struct kref *kref) { struct usbtmc_device_data *data = to_usbtmc_data(kref); + pr_debug("%s - called\n", __func__); usb_put_dev(data->usb_dev); kfree(data); } @@ -122,7 +141,7 @@ static int usbtmc_open(struct inode *inode, struct file *filp) { struct usb_interface *intf; struct usbtmc_device_data *data; - int retval = 0; + struct usbtmc_file_data *file_data; intf = usb_find_interface(&usbtmc_driver, iminor(inode)); if (!intf) { @@ -130,21 +149,54 @@ static int usbtmc_open(struct inode *inode, struct file *filp) return -ENODEV; } + file_data = kzalloc(sizeof(*file_data), GFP_KERNEL); + if (!file_data) + return -ENOMEM; + + pr_debug("%s - called\n", __func__); + data = usb_get_intfdata(intf); /* Protect reference to data from file structure until release */ kref_get(&data->kref); + mutex_lock(&data->io_mutex); + file_data->data = data; + + /* copy default values from device settings */ + file_data->TermChar = data->TermChar; + file_data->TermCharEnabled = data->TermCharEnabled; + file_data->auto_abort = data->auto_abort; + + INIT_LIST_HEAD(&file_data->file_elem); + spin_lock_irq(&data->dev_lock); + list_add_tail(&file_data->file_elem, &data->file_list); + spin_unlock_irq(&data->dev_lock); + mutex_unlock(&data->io_mutex); + /* Store pointer in file structure's private data field */ - filp->private_data = data; + filp->private_data = file_data; - return retval; + return 0; } static int usbtmc_release(struct inode *inode, struct file *file) { - struct usbtmc_device_data *data = file->private_data; + struct usbtmc_file_data *file_data = file->private_data; - kref_put(&data->kref, usbtmc_delete); + pr_debug("%s - called\n", __func__); + + /* prevent IO _AND_ usbtmc_interrupt */ + mutex_lock(&file_data->data->io_mutex); + spin_lock_irq(&file_data->data->dev_lock); + + list_del(&file_data->file_elem); + + spin_unlock_ir
[PATCH 05/12] usb: usbtmc: Add ioctl for generic requests on control pipe
Add USBTMC_IOCTL_CTRL_REQUEST to send arbitrary requests on the control pipe. Used by specific applications of IVI Foundation, Inc. to implement VISA API functions: viUsbControlIn/Out. Signed-off-by: Guido Kiener Reviewed-by: Steve Bayless --- drivers/usb/class/usbtmc.c | 61 include/uapi/linux/usb/tmc.h | 15 + 2 files changed, 76 insertions(+) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index 152e2daa9644..00c2e51a23a7 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -5,6 +5,7 @@ * Copyright (C) 2007 Stefan Kopp, Gechingen, Germany * Copyright (C) 2008 Novell, Inc. * Copyright (C) 2008 Greg Kroah-Hartman + * Copyright (C) 2018, IVI Foundation, Inc. */ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt @@ -1263,6 +1264,62 @@ static int usbtmc_ioctl_indicator_pulse(struct usbtmc_device_data *data) return rv; } +static int usbtmc_ioctl_request(struct usbtmc_device_data *data, + void __user *arg) +{ + struct device *dev = &data->intf->dev; + struct usbtmc_ctrlrequest request; + u8 *buffer = NULL; + int rv; + unsigned long res; + + res = copy_from_user(&request, arg, sizeof(struct usbtmc_ctrlrequest)); + if (res) + return -EFAULT; + + buffer = kmalloc(min_t(u16, 256, request.req.wLength), GFP_KERNEL); + if (!buffer) + return -ENOMEM; + + if ((request.req.bRequestType & USB_DIR_IN) == 0 + && request.req.wLength) { + res = copy_from_user(buffer, request.data, +request.req.wLength); + if (res) { + rv = -EFAULT; + goto exit; + } + } + + rv = usb_control_msg(data->usb_dev, + usb_rcvctrlpipe(data->usb_dev, 0), + request.req.bRequest, + request.req.bRequestType, + request.req.wValue, + request.req.wIndex, + buffer, request.req.wLength, USB_CTRL_GET_TIMEOUT); + + if (rv < 0) { + dev_err(dev, "%s failed %d\n", __func__, rv); + goto exit; + } + if ((request.req.bRequestType & USB_DIR_IN)) { + if (rv > request.req.wLength) { + dev_warn(dev, "%s returned too much data: %d\n", +__func__, rv); + rv = request.req.wLength; + } + + res = copy_to_user(request.data, buffer, rv); + if (res) + rv = -EFAULT; + } + + exit: + kfree(buffer); + return rv; +} + /* * Get the usb timeout value */ @@ -1379,6 +1436,10 @@ static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg) retval = usbtmc_ioctl_abort_bulk_in(data); break; + case USBTMC_IOCTL_CTRL_REQUEST: + retval = usbtmc_ioctl_request(data, (void __user *)arg); + break; + case USBTMC_IOCTL_GET_TIMEOUT: retval = usbtmc_ioctl_get_timeout(file_data, (void __user *)arg); diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h index 60369825691c..c1acec9ad834 100644 --- a/include/uapi/linux/usb/tmc.h +++ b/include/uapi/linux/usb/tmc.h @@ -4,6 +4,7 @@ * Copyright (C) 2008 Novell, Inc. * Copyright (C) 2008 Greg Kroah-Hartman * Copyright (C) 2015 Dave Penkler + * Copyright (C) 2018, IVI Foundation, Inc. * * This file holds USB constants defined by the USB Device Class * and USB488 Subclass Definitions for Test and Measurement devices @@ -40,6 +41,19 @@ #define USBTMC488_REQUEST_GOTO_LOCAL 161 #define USBTMC488_REQUEST_LOCAL_LOCKOUT162 +struct usbtmc_request { + __u8 bRequestType; + __u8 bRequest; + __u16 wValue; + __u16 wIndex; + __u16 wLength; +} __attribute__ ((packed)); + +struct usbtmc_ctrlrequest { + struct usbtmc_request req; + void __user *data; +} __attribute__ ((packed)); + struct usbtmc_termchar { __u8 term_char; __u8 term_char_enabled; // bool @@ -53,6 +67,7 @@ struct usbtmc_termchar { #define USBTMC_IOCTL_ABORT_BULK_IN _IO(USBTMC_IOC_NR, 4) #define USBTMC_IOCTL_CLEAR_OUT_HALT_IO(USBTMC_IOC_NR, 6) #define USBTMC_IOCTL_CLEAR_IN_HALT _IO(USBTMC_IOC_NR, 7) +#define USBTMC_IOCTL_CTRL_REQUEST _IOWR(USBTMC_IOC_NR, 8, struct usbtmc_ctrlrequest) #define USBTMC_IOCTL_GET_TIMEOUT _IOR(USBTMC_IOC_NR, 9, unsigned int) #define USBTMC_IOCTL_SET_TIMEOUT _IOW(USBTMC_IOC_NR, 10, unsigned int) #define USBTMC_IOCTL_EOM_ENABLE_IOW(USBTMC_IOC_NR, 11, __u8)
[PATCH 04/12] usb: usbtmc: Add ioctls for trigger, EOM bit and TermChar
- add USBTMC488_IOCTL_TRIGGER to send TRIGGER Bulk-OUT header according to Subclass USB488 Specification The usbtmc trigger command is equivalent to the IEEE 488 GET (Group Execute Trigger) action. While the "*TRG" command can be sent as data to perform the same operation, in some situations an instrument will be busy and unable to process the data immediately in which case the USBTMC488_IOCTL_TRIGGER can be used to trigger the instrument with lower latency. - add USBTMC_IOCTL_EOM_ENABLE to specify EOM bit for next write() call. Sets Bit 0 of field 'bmTransferAttributes' of DEV_DEP_MSG_OUT Bulk-OUT Header. Allows fine grained control over end of message handling on a per file descriptor basis. - add USBTMC_IOCTL_CONFIG_TERMCHAR to control TermChar handling for next read(). Controls field 'TermChar' and Bit 1 of field 'bmTransferAttributes' of REQUEST_DEV_DEP_MSG_IN BULK-OUT header. Allows enabling/disabling of terminating a read on reception of term_char individually for each read request. Reviewed-by: Steve Bayless Signed-off-by: Dave Penkler Signed-off-by: Guido Kiener --- drivers/usb/class/usbtmc.c | 111 +-- include/uapi/linux/usb/tmc.h | 11 2 files changed, 116 insertions(+), 6 deletions(-) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index ad7872003420..152e2daa9644 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -126,6 +126,7 @@ struct usbtmc_file_data { u8 TermChar; bool TermCharEnabled; bool auto_abort; + u8 eom_val; }; /* Forward declarations */ @@ -170,6 +171,7 @@ static int usbtmc_open(struct inode *inode, struct file *filp) file_data->TermChar = data->TermChar; file_data->TermCharEnabled = data->TermCharEnabled; file_data->auto_abort = data->auto_abort; + file_data->eom_val = 1; INIT_LIST_HEAD(&file_data->file_elem); spin_lock_irq(&data->dev_lock); @@ -577,6 +579,51 @@ static int usbtmc488_ioctl_simple(struct usbtmc_device_data *data, return rv; } +/* + * Sends a TRIGGER Bulk-OUT command message + * See the USBTMC-USB488 specification, Table 2. + * + * Also updates bTag_last_write. + */ +static int usbtmc488_ioctl_trigger(struct usbtmc_file_data *file_data) +{ + struct usbtmc_device_data *data = file_data->data; + int retval; + u8 *buffer; + int actual; + + buffer = kzalloc(USBTMC_HEADER_SIZE, GFP_KERNEL); + if (!buffer) + return -ENOMEM; + + buffer[0] = 128; + buffer[1] = data->bTag; + buffer[2] = ~data->bTag; + + retval = usb_bulk_msg(data->usb_dev, + usb_sndbulkpipe(data->usb_dev, + data->bulk_out), + buffer, USBTMC_HEADER_SIZE, + &actual, file_data->timeout); + + /* Store bTag (in case we need to abort) */ + data->bTag_last_write = data->bTag; + + /* Increment bTag -- and increment again if zero */ + data->bTag++; + if (!data->bTag) + data->bTag++; + + kfree(buffer); + if (retval < 0) { + dev_err(&data->intf->dev, "%s returned %d\n", + __func__, retval); + return retval; + } + + return 0; +} + /* * Sends a REQUEST_DEV_DEP_MSG_IN message on the Bulk-OUT endpoint. * @transfer_size: number of bytes to request from the device. @@ -829,7 +876,7 @@ static ssize_t usbtmc_write(struct file *filp, const char __user *buf, buffer[8] = 0; } else { this_part = remaining; - buffer[8] = 1; + buffer[8] = file_data->eom_val; } /* Setup IO buffer for DEV_DEP_MSG_OUT message */ @@ -1251,6 +1298,47 @@ static int usbtmc_ioctl_set_timeout(struct usbtmc_file_data *file_data, return 0; } +/* + * enables/disables sending EOM on write + */ +static int usbtmc_ioctl_eom_enable(struct usbtmc_file_data *file_data, + void __user *arg) +{ + __u8 eom_enable; + + if (copy_from_user(&eom_enable, arg, sizeof(eom_enable))) + return -EFAULT; + + if (eom_enable > 1) + return -EINVAL; + + file_data->eom_val = eom_enable; + + return 0; +} + +/* + * Configure TermChar and TermCharEnable + */ +static int usbtmc_ioctl_config_termc(struct usbtmc_file_data *file_data, + void __user *arg) +{ + struct usbtmc_termchar termc; + + if (copy_from_user(&termc, arg, sizeof(termc))) + return -EFAULT; + +
[PATCH 03/12] usb: usbtmc: Add ioctls to set/get usb timeout
Add ioctls USBTMC_IOCTL_GET_TIMEOUT / USBTMC_IOCTL_SET_TIMEOUT to get/set I/O timeout for specific file handle. Different operations on an instrument can take different lengths of time thus it is important to be able to set the timeout slightly longer than the expected duration of each operation to optimise the responsiveness of the application. As the instrument may be shared by multiple applications the timeout should be settable on a per file descriptor basis. Tested-by: Dave Penkler Reviewed-by: Steve Bayless Signed-off-by: Dave Penkler Signed-off-by: Guido Kiener --- drivers/usb/class/usbtmc.c | 58 +--- include/uapi/linux/usb/tmc.h | 2 ++ 2 files changed, 56 insertions(+), 4 deletions(-) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index 5754354429d8..ad7872003420 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -30,6 +30,8 @@ */ #define USBTMC_SIZE_IOBUFFER 2048 +/* Minimum USB timeout (in milliseconds) */ +#define USBTMC_MIN_TIMEOUT 100 /* Default USB timeout (in milliseconds) */ #define USBTMC_TIMEOUT 5000 @@ -116,6 +118,7 @@ struct usbtmc_file_data { struct usbtmc_device_data *data; struct list_head file_elem; + u32timeout; u8 srq_byte; atomic_t srq_asserted; @@ -163,6 +166,7 @@ static int usbtmc_open(struct inode *inode, struct file *filp) file_data->data = data; /* copy default values from device settings */ + file_data->timeout = USBTMC_TIMEOUT; file_data->TermChar = data->TermChar; file_data->TermCharEnabled = data->TermCharEnabled; file_data->auto_abort = data->auto_abort; @@ -478,7 +482,7 @@ static int usbtmc488_ioctl_read_stb(struct usbtmc_file_data *file_data, rv = wait_event_interruptible_timeout( data->waitq, atomic_read(&data->iin_data_valid) != 0, - USBTMC_TIMEOUT); + file_data->timeout); if (rv < 0) { dev_dbg(dev, "wait interrupted %d\n", rv); goto exit; @@ -613,7 +617,8 @@ static int send_request_dev_dep_msg_in(struct usbtmc_file_data *file_data, retval = usb_bulk_msg(data->usb_dev, usb_sndbulkpipe(data->usb_dev, data->bulk_out), - buffer, USBTMC_HEADER_SIZE, &actual, USBTMC_TIMEOUT); + buffer, USBTMC_HEADER_SIZE, + &actual, file_data->timeout); /* Store bTag (in case we need to abort) */ data->bTag_last_write = data->bTag; @@ -681,7 +686,7 @@ static ssize_t usbtmc_read(struct file *filp, char __user *buf, usb_rcvbulkpipe(data->usb_dev, data->bulk_in), buffer, USBTMC_SIZE_IOBUFFER, &actual, - USBTMC_TIMEOUT); + file_data->timeout); dev_dbg(dev, "usb_bulk_msg: retval(%u), done(%zu), remaining(%zu), actual(%d)\n", retval, done, remaining, actual); @@ -854,7 +859,7 @@ static ssize_t usbtmc_write(struct file *filp, const char __user *buf, usb_sndbulkpipe(data->usb_dev, data->bulk_out), buffer, n_bytes, - &actual, USBTMC_TIMEOUT); + &actual, file_data->timeout); if (retval != 0) break; n_bytes -= actual; @@ -1211,6 +1216,41 @@ static int usbtmc_ioctl_indicator_pulse(struct usbtmc_device_data *data) return rv; } +/* + * Get the usb timeout value + */ +static int usbtmc_ioctl_get_timeout(struct usbtmc_file_data *file_data, + void __user *arg) +{ + __u32 timeout; + + timeout = file_data->timeout; + + return put_user(timeout, (__u32 __user *)arg); +} + +/* + * Set the usb timeout value + */ +static int usbtmc_ioctl_set_timeout(struct usbtmc_file_data *file_data, + void __user *arg) +{ + __u32 timeout; + + if (get_user(timeout, (__u32 __user *)arg)) + return -EFAULT; + + /* Note that timeout = 0 means +* MAX_SCHEDULE_TIMEOUT in usb_control_msg +*/ + if (timeout < USBTMC_MIN_TIMEOUT) + return -EINVAL; + + file_data->timeout = timeout; + + return 0; +} + static long usbtmc_ioct
[PATCH 09/12] usb: usbtmc: Fix ioctl USBTMC_IOCTL_CLEAR
Insert a sleep of 50 ms between subsequent CHECK_CLEAR_STATUS control requests to avoid stressing the instrument with repeated requests. Some instruments need time to cleanup internal I/O buffers. Polling and repeated requests slow down the response time of devices. Signed-off-by: Guido Kiener Reviewed-by: Steve Bayless --- drivers/usb/class/usbtmc.c | 46 +++--- 1 file changed, 18 insertions(+), 28 deletions(-) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index 76b0a7b083a7..165b707991f3 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -1643,20 +1643,17 @@ static ssize_t usbtmc_write(struct file *filp, const char __user *buf, static int usbtmc_ioctl_clear(struct usbtmc_device_data *data) { - struct usb_host_interface *current_setting; - struct usb_endpoint_descriptor *desc; struct device *dev; u8 *buffer; int rv; int n; int actual = 0; - int max_size; dev = &data->intf->dev; dev_dbg(dev, "Sending INITIATE_CLEAR request\n"); - buffer = kmalloc(USBTMC_SIZE_IOBUFFER, GFP_KERNEL); + buffer = kmalloc(USBTMC_BUFSIZE, GFP_KERNEL); if (!buffer) return -ENOMEM; @@ -1664,7 +1661,7 @@ static int usbtmc_ioctl_clear(struct usbtmc_device_data *data) usb_rcvctrlpipe(data->usb_dev, 0), USBTMC_REQUEST_INITIATE_CLEAR, USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE, -0, 0, buffer, 1, USBTMC_TIMEOUT); +0, 0, buffer, 1, USB_CTRL_GET_TIMEOUT); if (rv < 0) { dev_err(dev, "usb_control_msg returned %d\n", rv); goto exit; @@ -1678,22 +1675,6 @@ static int usbtmc_ioctl_clear(struct usbtmc_device_data *data) goto exit; } - max_size = 0; - current_setting = data->intf->cur_altsetting; - for (n = 0; n < current_setting->desc.bNumEndpoints; n++) { - desc = ¤t_setting->endpoint[n].desc; - if (desc->bEndpointAddress == data->bulk_in) - max_size = usb_endpoint_maxp(desc); - } - - if (max_size == 0) { - dev_err(dev, "Couldn't get wMaxPacketSize\n"); - rv = -EPERM; - goto exit; - } - - dev_dbg(dev, "wMaxPacketSize is %d\n", max_size); - n = 0; usbtmc_clear_check_status: @@ -1704,7 +1685,7 @@ static int usbtmc_ioctl_clear(struct usbtmc_device_data *data) usb_rcvctrlpipe(data->usb_dev, 0), USBTMC_REQUEST_CHECK_CLEAR_STATUS, USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE, -0, 0, buffer, 2, USBTMC_TIMEOUT); +0, 0, buffer, 2, USB_CTRL_GET_TIMEOUT); if (rv < 0) { dev_err(dev, "usb_control_msg returned %d\n", rv); goto exit; @@ -1721,15 +1702,19 @@ static int usbtmc_ioctl_clear(struct usbtmc_device_data *data) goto exit; } - if (buffer[1] == 1) + if ((buffer[1] & 1) != 0) { do { dev_dbg(dev, "Reading from bulk in EP\n"); rv = usb_bulk_msg(data->usb_dev, usb_rcvbulkpipe(data->usb_dev, data->bulk_in), - buffer, USBTMC_SIZE_IOBUFFER, - &actual, USBTMC_TIMEOUT); + buffer, USBTMC_BUFSIZE, + &actual, USB_CTRL_GET_TIMEOUT); + + print_hex_dump_debug("usbtmc ", DUMP_PREFIX_NONE, +16, 1, buffer, actual, true); + n++; if (rv < 0) { @@ -1737,10 +1722,15 @@ static int usbtmc_ioctl_clear(struct usbtmc_device_data *data) rv); goto exit; } - } while ((actual == max_size) && + } while ((actual == USBTMC_BUFSIZE) && (n < USBTMC_MAX_READS_TO_CLEAR_BULK_IN)); + } else { + /* do not stress device with subsequent requests */ + msleep(50); + n++; + } - if (actual == max_size) { + if (n >= USBTMC_MAX_READS_TO_CLEAR_BULK_IN) { dev_err(dev, "Couldn't clear device buffer within %d cycles\n", USBTMC_MAX_RE
[PATCH 11/12] usb: usbtmc: Add ioctls to abort with specific tags
- fix ioctls USBTMC_IOCTL_ABORT_BULK_OUT/IN - add ioctls USBTMC_IOCTL_ABORT_BULK_OUT_TAG and USBTMC_IOCTL_ABORT_BULK_IN_TAG for test purpose. Useful for testing devices and client applications: The ioctls allow to test the abort mechanism and the response of the device when using different or wrong tag ids. Signed-off-by: Guido Kiener Reviewed-by: Steve Bayless --- drivers/usb/class/usbtmc.c | 151 ++- include/uapi/linux/usb/tmc.h | 2 + 2 files changed, 81 insertions(+), 72 deletions(-) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index 8b464598bee5..c24efe513556 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -277,18 +277,17 @@ static int usbtmc_release(struct inode *inode, struct file *file) return 0; } -static int usbtmc_ioctl_abort_bulk_in(struct usbtmc_device_data *data) +static int usbtmc_ioctl_abort_bulk_in_tag(struct usbtmc_device_data *data, + u8 bTag) { u8 *buffer; struct device *dev; int rv; int n; int actual; - struct usb_host_interface *current_setting; - int max_size; dev = &data->intf->dev; - buffer = kmalloc(USBTMC_SIZE_IOBUFFER, GFP_KERNEL); + buffer = kmalloc(USBTMC_BUFSIZE, GFP_KERNEL); if (!buffer) return -ENOMEM; @@ -296,86 +295,87 @@ static int usbtmc_ioctl_abort_bulk_in(struct usbtmc_device_data *data) usb_rcvctrlpipe(data->usb_dev, 0), USBTMC_REQUEST_INITIATE_ABORT_BULK_IN, USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_ENDPOINT, -data->bTag_last_read, data->bulk_in, -buffer, 2, USBTMC_TIMEOUT); +bTag, data->bulk_in, +buffer, 2, USB_CTRL_GET_TIMEOUT); if (rv < 0) { dev_err(dev, "usb_control_msg returned %d\n", rv); goto exit; } - dev_dbg(dev, "INITIATE_ABORT_BULK_IN returned %x\n", buffer[0]); + dev_dbg(dev, "INITIATE_ABORT_BULK_IN returned %x with tag %02x\n", + buffer[0], buffer[1]); if (buffer[0] == USBTMC_STATUS_FAILED) { + /* No transfer in progress and the Bulk-OUT FIFO is empty. */ rv = 0; goto exit; } - if (buffer[0] != USBTMC_STATUS_SUCCESS) { - dev_err(dev, "INITIATE_ABORT_BULK_IN returned %x\n", - buffer[0]); - rv = -EPERM; + if (buffer[0] == USBTMC_STATUS_TRANSFER_NOT_IN_PROGRESS) { + /* The device returns this status if either: +* - There is a transfer in progress, but the specified bTag +* does not match. +* - There is no transfer in progress, but the Bulk-OUT FIFO +* is not empty. +*/ + rv = -ENOMSG; goto exit; } - max_size = 0; - current_setting = data->intf->cur_altsetting; - for (n = 0; n < current_setting->desc.bNumEndpoints; n++) - if (current_setting->endpoint[n].desc.bEndpointAddress == - data->bulk_in) - max_size = usb_endpoint_maxp(¤t_setting->endpoint[n].desc); - - if (max_size == 0) { - dev_err(dev, "Couldn't get wMaxPacketSize\n"); + if (buffer[0] != USBTMC_STATUS_SUCCESS) { + dev_err(dev, "INITIATE_ABORT_BULK_IN returned %x\n", + buffer[0]); rv = -EPERM; goto exit; } - dev_dbg(&data->intf->dev, "wMaxPacketSize is %d\n", max_size); - n = 0; - do { - dev_dbg(dev, "Reading from bulk in EP\n"); +usbtmc_abort_bulk_in_status: + dev_dbg(dev, "Reading from bulk in EP\n"); - rv = usb_bulk_msg(data->usb_dev, - usb_rcvbulkpipe(data->usb_dev, - data->bulk_in), - buffer, USBTMC_SIZE_IOBUFFER, - &actual, USBTMC_TIMEOUT); + /* Data must be present. So use low timeout 300 ms */ + rv = usb_bulk_msg(data->usb_dev, + usb_rcvbulkpipe(data->usb_dev, + data->bulk_in), + buffer, USBTMC_BUFSIZE, + &actual, 300); - n++; + print_hex_dump_debug("usbtmc ", DUMP_PREFIX_NONE, 16, 1, +buffer, actual, true); - if (rv < 0) { -
[PATCH 12/12] usb: usbtmc: Add ioctl to return API version of usbtmc driver
- add ioctl USBTMC_IOCTL_API_VERSION to get current API version - add info message to show API version - replace USBTMC_TIMEOUT macros with common used USB_CTRL_GET_TIMEOUT or USB_CTRL_SET_TIMEOUT macros. - delete some superfluous code lines. - update ioctl-number.txt Signed-off-by: Guido Kiener Reviewed-by: Steve Bayless --- Documentation/ioctl/ioctl-number.txt | 2 +- drivers/usb/class/usbtmc.c | 38 +++- include/uapi/linux/usb/tmc.h | 1 + 3 files changed, 23 insertions(+), 18 deletions(-) diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt index 7f7413e597f3..9112df246a77 100644 --- a/Documentation/ioctl/ioctl-number.txt +++ b/Documentation/ioctl/ioctl-number.txt @@ -200,7 +200,7 @@ Code Seq#(hex) Include FileComments 'X'01 linux/pktcdvd.h conflict! 'Y'all linux/cyclades.h 'Z'14-15 drivers/message/fusion/mptctl.h -'['00-07 linux/usb/tmc.h USB Test and Measurement Devices +'['00-3F linux/usb/tmc.h USB Test and Measurement Devices <mailto:gre...@linuxfoundation.org> 'a'all linux/atm*.h, linux/sonet.h ATM on linux <http://lrcwww.epfl.ch/> diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index c24efe513556..25c5418556ef 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -21,16 +21,14 @@ #include #include +/* Increment API VERSION when changing tmc.h with new flags or ioctls + * or when changing a significant behavior of the driver. + */ +#define USBTMC_API_VERSION (2) #define USBTMC_HEADER_SIZE 12 #define USBTMC_MINOR_BASE 176 -/* - * Size of driver internal IO buffer. Must be multiple of 4 and at least as - * large as wMaxPacketSize (which is usually 512 bytes). - */ -#define USBTMC_SIZE_IOBUFFER 2048 - /* Minimum USB timeout (in milliseconds) */ #define USBTMC_MIN_TIMEOUT 100 /* Default USB timeout (in milliseconds) */ @@ -512,8 +510,6 @@ static int usbtmc488_ioctl_read_stb(struct usbtmc_file_data *file_data, rv = put_user(stb, (__u8 __user *)arg); dev_dbg(dev, "stb:0x%02x with srq received %d\n", (unsigned int)stb, rv); - if (rv) - return -EFAULT; return rv; } spin_unlock_irq(&data->dev_lock); @@ -530,7 +526,7 @@ static int usbtmc488_ioctl_read_stb(struct usbtmc_file_data *file_data, USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE, data->iin_bTag, data->ifnum, - buffer, 0x03, USBTMC_TIMEOUT); + buffer, 0x03, USB_CTRL_GET_TIMEOUT); if (rv < 0) { dev_err(dev, "stb usb_control_msg returned %d\n", rv); goto exit; @@ -569,10 +565,7 @@ static int usbtmc488_ioctl_read_stb(struct usbtmc_file_data *file_data, stb = buffer[2]; } - if (put_user(stb, (__u8 __user *)arg)) - rv = -EFAULT; - else - rv = 0; + rv = put_user(stb, (__u8 __user *)arg); dev_dbg(dev, "stb:0x%02x received %d\n", (unsigned int)stb, rv); exit: @@ -667,7 +660,7 @@ static int usbtmc488_ioctl_simple(struct usbtmc_device_data *data, USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE, wValue, data->ifnum, - buffer, 0x01, USBTMC_TIMEOUT); + buffer, 0x01, USB_CTRL_GET_TIMEOUT); if (rv < 0) { dev_err(dev, "simple usb_control_msg failed %d\n", rv); goto exit; @@ -1861,7 +1854,7 @@ static int get_capabilities(struct usbtmc_device_data *data) rv = usb_control_msg(data->usb_dev, usb_rcvctrlpipe(data->usb_dev, 0), USBTMC_REQUEST_GET_CAPABILITIES, USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE, -0, 0, buffer, 0x18, USBTMC_TIMEOUT); +0, 0, buffer, 0x18, USB_CTRL_GET_TIMEOUT); if (rv < 0) { dev_err(dev, "usb_control_msg returned %d\n", rv); goto err_out; @@ -1984,6 +1977,9 @@ static const struct attribute_group data_attr_grp = { .attrs = data_attrs, }; +/* + * Flash activity indicator on device + */ static int usbtmc_ioctl_indicator_pulse(struct usbtmc_device_data *data) { struct device *dev; @@ -2000,7 +1996,7 @@ static int usbtmc_ioctl_indicator_pulse(struct usbtmc_device_data *data) usb_rcvctrlpipe(data->usb_dev, 0),
[PATCH 08/12] usb: usbtmc: Optimize read/write and add ioctls for auto_abort
- Optimize read/write functions for better bandwidth and fixes reading of long transfers - add ioctl USBTMC_IOCTL_AUTO_ABORT to configure auto_abort for each specific file handle. - add ioctl USBTMC_IOCTL_MSG_IN_ATTR that returns the specific bmTransferAttributes field of the last DEV_DEP_MSG_IN Bulk-IN header. This header is received by the read() function. The meaning of the (u8) bitmap bmTransferAttributes is: Bit 0 = EOM flag is set when the last of a USBTMC message is received. Bit 1 = is set when the last byte is a termchar (e.g. '\n'). Note that this bit is always zero when the device does not support termchar feature or when termchar detection is not enabled (see ioctl USBTMC_IOCTL_CONFIG_TERMCHAR). Signed-off-by: Guido Kiener Reviewed-by: Steve Bayless --- drivers/usb/class/usbtmc.c | 357 --- include/uapi/linux/usb/tmc.h | 3 + 2 files changed, 206 insertions(+), 154 deletions(-) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index 6b8b8510cfc4..76b0a7b083a7 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -131,6 +131,7 @@ struct usbtmc_file_data { u8 srq_byte; atomic_t srq_asserted; atomic_t closing; + u8 bmTransferAttributes; /* member of DEV_DEP_MSG_IN */ /* These values are initialized with default values from device_data */ u8 TermChar; @@ -1356,20 +1357,20 @@ static ssize_t usbtmc_read(struct file *filp, char __user *buf, struct usbtmc_file_data *file_data; struct usbtmc_device_data *data; struct device *dev; + const size_t bufsize = USBTMC_BUFSIZE; u32 n_characters; u8 *buffer; int actual; - size_t done; - size_t remaining; + u64 done = 0; + u64 remaining; int retval; - size_t this_part; /* Get pointer to private data structure */ file_data = filp->private_data; data = file_data->data; dev = &data->intf->dev; - buffer = kmalloc(USBTMC_SIZE_IOBUFFER, GFP_KERNEL); + buffer = kmalloc(bufsize, GFP_KERNEL); if (!buffer) return -ENOMEM; @@ -1379,124 +1380,117 @@ static ssize_t usbtmc_read(struct file *filp, char __user *buf, goto exit; } - dev_dbg(dev, "usb_bulk_msg_in: count(%zu)\n", count); + if (count > INT_MAX) + count = INT_MAX; + + dev_dbg(dev, "%s(count:%zu)\n", __func__, count); retval = send_request_dev_dep_msg_in(file_data, count); if (retval < 0) { - if (data->auto_abort) + if (file_data->auto_abort) usbtmc_ioctl_abort_bulk_out(data); goto exit; } /* Loop until we have fetched everything we requested */ remaining = count; - this_part = remaining; - done = 0; - while (remaining > 0) { - /* Send bulk URB */ - retval = usb_bulk_msg(data->usb_dev, - usb_rcvbulkpipe(data->usb_dev, - data->bulk_in), - buffer, USBTMC_SIZE_IOBUFFER, &actual, - file_data->timeout); + /* Send bulk URB */ + retval = usb_bulk_msg(data->usb_dev, + usb_rcvbulkpipe(data->usb_dev, + data->bulk_in), + buffer, bufsize, &actual, + file_data->timeout); - dev_dbg(dev, "usb_bulk_msg: retval(%u), done(%zu), remaining(%zu), actual(%d)\n", retval, done, remaining, actual); + dev_dbg(dev, "%s: bulk_msg retval(%u), actual(%d)\n", + __func__, retval, actual); - /* Store bTag (in case we need to abort) */ - data->bTag_last_read = data->bTag; + /* Store bTag (in case we need to abort) */ + data->bTag_last_read = data->bTag; - if (retval < 0) { - dev_dbg(dev, "Unable to read data, error %d\n", retval); - if (data->auto_abort) - usbtmc_ioctl_abort_bulk_in(data); - goto exit; - } + if (retval < 0) { + if (file_data->auto_abort) + usbtmc_ioctl_abort_bulk_in(data); + goto exit; + } - /* Parse header in first packet */ - if (done == 0) { - /* Sanity checks for the header */ - if (actual < USBTMC_HEADER_SIZE) { -
[PATCH 10/12] usb: usbtmc: Add test functions to set HALT Feature (Stall)
The ioctls USBTMC_IOCTL_SET_OUT_HALT or USBTMC_IOCTL_SET_IN_HALT send a SET_FEATURE(HALT) request to the corresponding OUT or IN pipe. Useful for testing devices and client applications: The ioctls force the device to simulate the error state at the specified pipe. Signed-off-by: Guido Kiener Reviewed-by: Steve Bayless --- drivers/usb/class/usbtmc.c | 78 ++-- include/uapi/linux/usb/tmc.h | 4 ++ 2 files changed, 70 insertions(+), 12 deletions(-) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index 165b707991f3..8b464598bee5 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -1754,34 +1754,80 @@ static int usbtmc_ioctl_clear(struct usbtmc_device_data *data) return rv; } +/* + * set pipe in halt state (stalled) + * Needed for test purpose or workarounds. + */ +static int usbtmc_set_halt(struct usb_device *dev, int pipe) +{ + int rv; + int endp = usb_pipeendpoint(pipe); + + if (usb_pipein(pipe)) + endp |= USB_DIR_IN; + + rv = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), +USB_REQ_SET_FEATURE, USB_RECIP_ENDPOINT, +USB_ENDPOINT_HALT, endp, NULL, 0, +USB_CTRL_SET_TIMEOUT); + + return rv; +} + +static int usbtmc_ioctl_set_out_halt(struct usbtmc_device_data *data) +{ + int rv; + + dev_dbg(&data->intf->dev, "%s - called\n", __func__); + + rv = usbtmc_set_halt(data->usb_dev, +usb_sndbulkpipe(data->usb_dev, data->bulk_out)); + + if (rv < 0) + dev_err(&data->usb_dev->dev, "%s returned %d\n", __func__, rv); + return rv; +} + +static int usbtmc_ioctl_set_in_halt(struct usbtmc_device_data *data) +{ + int rv; + + dev_dbg(&data->intf->dev, "%s - called\n", __func__); + + rv = usbtmc_set_halt(data->usb_dev, +usb_rcvbulkpipe(data->usb_dev, data->bulk_in)); + + if (rv < 0) + dev_err(&data->usb_dev->dev, "%s returned %d\n", __func__, rv); + return rv; +} + static int usbtmc_ioctl_clear_out_halt(struct usbtmc_device_data *data) { int rv; + dev_dbg(&data->intf->dev, "%s - called\n", __func__); + rv = usb_clear_halt(data->usb_dev, usb_sndbulkpipe(data->usb_dev, data->bulk_out)); - if (rv < 0) { - dev_err(&data->usb_dev->dev, "usb_control_msg returned %d\n", - rv); - return rv; - } - return 0; + if (rv < 0) + dev_err(&data->usb_dev->dev, "%s returned %d\n", __func__, rv); + return rv; } static int usbtmc_ioctl_clear_in_halt(struct usbtmc_device_data *data) { int rv; + dev_dbg(&data->intf->dev, "%s - called\n", __func__); + rv = usb_clear_halt(data->usb_dev, usb_rcvbulkpipe(data->usb_dev, data->bulk_in)); - if (rv < 0) { - dev_err(&data->usb_dev->dev, "usb_control_msg returned %d\n", - rv); - return rv; - } - return 0; + if (rv < 0) + dev_err(&data->usb_dev->dev, "%s returned %d\n", __func__, rv); + return rv; } static int usbtmc_ioctl_cancel_io(struct usbtmc_file_data *file_data) @@ -2241,6 +2287,14 @@ static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg) file_data->auto_abort = !!tmp_byte; break; + case USBTMC_IOCTL_SET_OUT_HALT: + retval = usbtmc_ioctl_set_out_halt(data); + break; + + case USBTMC_IOCTL_SET_IN_HALT: + retval = usbtmc_ioctl_set_in_halt(data); + break; + case USBTMC_IOCTL_CANCEL_IO: retval = usbtmc_ioctl_cancel_io(file_data); break; diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h index e3bdfc1935ed..886fabf5dfea 100644 --- a/include/uapi/linux/usb/tmc.h +++ b/include/uapi/linux/usb/tmc.h @@ -101,6 +101,10 @@ struct usbtmc_message { #define USBTMC_IOCTL_MSG_IN_ATTR _IOR(USBTMC_IOC_NR, 24, __u8) #define USBTMC_IOCTL_AUTO_ABORT_IOW(USBTMC_IOC_NR, 25, __u8) +/* For test purpose only */ +#define USBTMC_IOCTL_SET_OUT_HALT _IO(USBTMC_IOC_NR, 30) +#define USBTMC_IOCTL_SET_IN_HALT _IO(USBTMC_IOC_NR, 31) + /* 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) -- 2.17.0 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 01/12] usb: usbtmc: Remove rigol_quirk
All T&M instruments should also work with rigol_quirk = 1 code path. So remove unnecessary code in rigol_quirk = 0 code path to simplify the driver. Tested-by: Dave Penkler Reviewed-by: Steve Bayless Signed-off-by: Guido Kiener --- drivers/usb/class/usbtmc.c | 81 ++ 1 file changed, 12 insertions(+), 69 deletions(-) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index bdb1de0c0cef..529295a17579 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -21,7 +21,6 @@ #include -#define RIGOL 1 #define USBTMC_HEADER_SIZE 12 #define USBTMC_MINOR_BASE 176 @@ -93,8 +92,6 @@ struct usbtmc_device_data { /* coalesced usb488_caps from usbtmc_dev_capabilities */ __u8 usb488_caps; - u8 rigol_quirk; - /* attributes from the USB TMC spec for this device */ u8 TermChar; bool TermCharEnabled; @@ -110,17 +107,6 @@ struct usbtmc_device_data { }; #define to_usbtmc_data(d) container_of(d, struct usbtmc_device_data, kref) -struct usbtmc_ID_rigol_quirk { - __u16 idVendor; - __u16 idProduct; -}; - -static const struct usbtmc_ID_rigol_quirk usbtmc_id_quirk[] = { - { 0x1ab1, 0x0588 }, - { 0x1ab1, 0x04b0 }, - { 0, 0 } -}; - /* Forward declarations */ static struct usb_driver usbtmc_driver; @@ -603,16 +589,14 @@ static ssize_t usbtmc_read(struct file *filp, char __user *buf, goto exit; } - if (data->rigol_quirk) { - dev_dbg(dev, "usb_bulk_msg_in: count(%zu)\n", count); + dev_dbg(dev, "usb_bulk_msg_in: count(%zu)\n", count); - retval = send_request_dev_dep_msg_in(data, count); + retval = send_request_dev_dep_msg_in(data, count); - if (retval < 0) { - if (data->auto_abort) - usbtmc_ioctl_abort_bulk_out(data); - goto exit; - } + if (retval < 0) { + if (data->auto_abort) + usbtmc_ioctl_abort_bulk_out(data); + goto exit; } /* Loop until we have fetched everything we requested */ @@ -621,23 +605,6 @@ static ssize_t usbtmc_read(struct file *filp, char __user *buf, done = 0; while (remaining > 0) { - if (!data->rigol_quirk) { - dev_dbg(dev, "usb_bulk_msg_in: remaining(%zu), count(%zu)\n", remaining, count); - - if (remaining > USBTMC_SIZE_IOBUFFER - USBTMC_HEADER_SIZE - 3) - this_part = USBTMC_SIZE_IOBUFFER - USBTMC_HEADER_SIZE - 3; - else - this_part = remaining; - - retval = send_request_dev_dep_msg_in(data, this_part); - if (retval < 0) { - dev_err(dev, "usb_bulk_msg returned %d\n", retval); - if (data->auto_abort) - usbtmc_ioctl_abort_bulk_out(data); - goto exit; - } - } - /* Send bulk URB */ retval = usb_bulk_msg(data->usb_dev, usb_rcvbulkpipe(data->usb_dev, @@ -658,7 +625,7 @@ static ssize_t usbtmc_read(struct file *filp, char __user *buf, } /* Parse header in first packet */ - if ((done == 0) || !data->rigol_quirk) { + if (done == 0) { /* Sanity checks for the header */ if (actual < USBTMC_HEADER_SIZE) { dev_err(dev, "Device sent too small first packet: %u < %u\n", actual, USBTMC_HEADER_SIZE); @@ -698,20 +665,11 @@ static ssize_t usbtmc_read(struct file *filp, char __user *buf, actual -= USBTMC_HEADER_SIZE; /* Check if the message is smaller than requested */ - if (data->rigol_quirk) { - if (remaining > n_characters) - remaining = n_characters; - /* Remove padding if it exists */ - if (actual > remaining) - actual = remaining; - } - else { - if (this_part > n_characters) - this_part = n_characters; - /* Remove padding if it exists */ - if (actual > this_part) - actual = this_part; - } + if (remaining > n_charact
[PATCH 07/12] usb: usbtmc: Add ioctl USBTMC488_IOCTL_WAIT_SRQ
Wait until an SRQ (service request) is received on the interrupt pipe or until the given period of time is expired. In contrast to the poll() function this ioctl does not return when other (a)synchronous I/O operations fail with EPOLLERR. Signed-off-by: Guido Kiener Reviewed-by: Steve Bayless --- drivers/usb/class/usbtmc.c | 60 ++-- include/uapi/linux/usb/tmc.h | 1 + 2 files changed, 59 insertions(+), 2 deletions(-) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index de664a345e69..6b8b8510cfc4 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -594,6 +594,54 @@ static int usbtmc488_ioctl_read_stb(struct usbtmc_file_data *file_data, return rv; } +static int usbtmc488_ioctl_wait_srq(struct usbtmc_file_data *file_data, + unsigned int __user *arg) +{ + struct usbtmc_device_data *data = file_data->data; + struct device *dev = &data->intf->dev; + int rv; + unsigned int timeout; + unsigned long expire; + + if (!data->iin_ep_present) { + dev_dbg(dev, "no interrupt endpoint present\n"); + return -EFAULT; + } + + if (get_user(timeout, arg)) + return -EFAULT; + + expire = msecs_to_jiffies(timeout); + + mutex_unlock(&data->io_mutex); + + rv = wait_event_interruptible_timeout( + data->waitq, + atomic_read(&file_data->srq_asserted) != 0 || + atomic_read(&file_data->closing), + expire); + + mutex_lock(&data->io_mutex); + + /* Note! disconnect or close could be called in the meantime */ + if (atomic_read(&file_data->closing) || data->zombie) + rv = -ENODEV; + + if (rv < 0) { + /* dev can be invalid now! */ + pr_debug("%s - wait interrupted %d\n", __func__, rv); + return rv; + } + + if (rv == 0) { + dev_dbg(dev, "%s - wait timed out\n", __func__); + return -ETIMEDOUT; + } + + dev_dbg(dev, "%s - srq asserted\n", __func__); + return 0; +} + static int usbtmc488_ioctl_simple(struct usbtmc_device_data *data, void __user *arg, unsigned int cmd) { @@ -2149,6 +2197,11 @@ static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg) retval = usbtmc488_ioctl_trigger(file_data); break; + case USBTMC488_IOCTL_WAIT_SRQ: + retval = usbtmc488_ioctl_wait_srq(file_data, + (unsigned int __user *)arg); + break; + case USBTMC_IOCTL_CANCEL_IO: retval = usbtmc_ioctl_cancel_io(file_data); break; @@ -2290,6 +2343,7 @@ static void usbtmc_interrupt(struct urb *urb) case -ESHUTDOWN: case -EILSEQ: case -ETIME: + case -EPIPE: /* urb terminated, clean up */ dev_dbg(dev, "urb terminated, status: %d\n", status); return; @@ -2308,7 +2362,9 @@ static void usbtmc_free_int(struct usbtmc_device_data *data) return; usb_kill_urb(data->iin_urb); kfree(data->iin_buffer); + data->iin_buffer = NULL; usb_free_urb(data->iin_urb); + data->iin_urb = NULL; kref_put(&data->kref, usbtmc_delete); } @@ -2420,8 +2476,8 @@ static int usbtmc_probe(struct usb_interface *intf, retcode = usb_register_dev(intf, &usbtmc_class); if (retcode) { - dev_err(&intf->dev, "Not able to get a minor" - " (base %u, slice default): %d\n", USBTMC_MINOR_BASE, + dev_err(&intf->dev, "Not able to get a minor (base %u, slice default): %d\n", + USBTMC_MINOR_BASE, retcode); goto error_register; } diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h index 1540716de293..35b63530121d 100644 --- a/include/uapi/linux/usb/tmc.h +++ b/include/uapi/linux/usb/tmc.h @@ -96,6 +96,7 @@ struct usbtmc_message { #define USBTMC488_IOCTL_GOTO_LOCAL _IO(USBTMC_IOC_NR, 20) #define USBTMC488_IOCTL_LOCAL_LOCKOUT _IO(USBTMC_IOC_NR, 21) #define USBTMC488_IOCTL_TRIGGER_IO(USBTMC_IOC_NR, 22) +#define USBTMC488_IOCTL_WAIT_SRQ _IOW(USBTMC_IOC_NR, 23, unsigned int) /* Cancel and cleanup asynchronous calls */ #define USBTMC_IOCTL_CANCEL_IO _IO(USBTMC_IOC_NR, 35) -- 2.17.0 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 06/12] usb: usbtmc: Add vendor specific/asynchronous read/write ioctls
- 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. With flag USBTMC_FLAG_ASYNC the ioctl is non blocking. When no received data is available, the read function submits as many urbs as needed to receive transfer_size bytes. However the number of flying urbs (=4kB) is limited to 16 even with subsequent calls of this ioctl. Returns -EAGAIN when non blocking and no data is received. Signals EPOLLIN | EPOLLRDNORM when asynchronous urbs are ready to be read. The usbtmc_message.message pointer can be NULL to just trigger reading data. However -EINVAL is returned when data is available, and the message pointer is NULL. - ioctl USBTMC_IOCTL_CANCEL_IO cancels last USBTMC_IOCTL_READ and USBTMC_IOCTL_WRITE functions which returns -ECANCELED. A subsequent call to USBTMC_IOCTL_READ or USBTMC_IOCTL_WRITE_RESULT returns -ECANCELED with information about current transferred data. - ioctl USBTMC_IOCTL_CLEANUP_IO kills all submitted urbs to OUT and IN bulk, and clears all received data from IN bulk. Internal transfer counters and error states are reset. - Flush flying urbs when file handle is closed or device is suspended. Signed-off-by: Guido Kiener Reviewed-by: Steve Bayless --- drivers/usb/class/usbtmc.c | 778 ++- include/uapi/linux/usb/tmc.h | 21 + 2 files changed, 797 insertions(+), 2 deletions(-) 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 */ + u16wMaxPacketSize; + /* data for interrupt in endpoint handling */ u8 bNotify1; u8 bNotify2; @@ -122,16 +130,34 @@ struct usbtmc_file_data { u32timeout; 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)
usb: usbtmc: Questions of the IVI Foundation
Hi all, Some members of the IVI Foundation www.ivifoundation.org have founded a working group “VISA for Linux” that defines common rules, header files, and shared libraries for Linux to implement the specification of "VPP-4.3: The VISA Library" (see http://ivifoundation.org/specifications/default.aspx). Moreover the interoperability of the USBTMC protocol is one of our requirements. We know that you have already improved the Linux USBTMC driver (linux/drivers/usb/class/usbtmc.c) that can communicate with T&M instruments. However we are not sure whether this driver already includes all features that we need for our instruments and for the VISA API. Therefore we ask you to give us some recommendations or replies to our questions: 1. "Libusb" versus usbtmc driver: Using "libusb" seems to be a good alternative for all Linux platforms and is the right fallback solution until future versions of the USBTMC includes all required features. Do you think we should extend the USBTMC driver with our required features (e.g. vendor specific IO, SRQ handling, asynchronous IO, raw USB IO, timeout)? Or do you think we shall keep the USBTMC driver in the current state? How long would it take to get changes into the Linux kernel? 2. As we have looked at the Linux driver, we’ve noticed that performance of the usbtmc_read() function doesn’t keep up with our fastest instruments. Do you have any suggestions on how to improve the read performance? 3. Do you have any plans to modify/evolve the USBTMC driver moving forward? 4. Do you know of any other differences between the Linux USBTMC driver and the Windows USBTMC driver? We are looking forward to your comments. Please let us know if some of you would like to participate to the next WebEx conference call. -Guido N�r��yb�X��ǧv�^�){.n�+{��^n�r���z���h�&���G���h�(�階�ݢj"���m��z�ޖ���f���h���~�m�
Re: usb: usbtmc: Questions of the IVI Foundation
linux-usb-ow...@vger.kernel.org schrieb am 15.05.2017 15:20:22: > Von: Greg KH > Datum: 15.05.2017 15:20 > Betreff: Re: usb: usbtmc: Questions of the IVI Foundation > Gesendet von: linux-usb-ow...@vger.kernel.org > > On Mon, May 15, 2017 at 02:47:48PM +0200, Guido.Kiener@x > wrote: > > Hi all, > > > > Some members of the IVI Foundation www.ivifoundation.org have founded a > > working group "VISA for Linux" that defines common rules, header files, > > and shared libraries for Linux to implement the specification of "VPP-4.3: > > The VISA Library" (see > > http://ivifoundation.org/specifications/default.aspx). > > > > Moreover the interoperability of the USBTMC protocol is one of our > > requirements. We know that you have already improved the Linux USBTMC > > driver (linux/drivers/usb/class/usbtmc.c) that can communicate with T&M > > instruments. However we are not sure whether this driver already includes > > all features that we need for our instruments and for the VISA API. > > Therefore we ask you to give us some recommendations or replies to our > > questions: > > > > 1. "Libusb" versus usbtmc driver: Using "libusb" seems to be a good > > alternative for all Linux platforms and is the right fallback solution > > until future versions of the USBTMC includes all required features. > > It also works on all other operating systems as well, Windows included, > so you might find this very useful for everyone. > > > Do you think we should extend the USBTMC driver with our required > > features (e.g. vendor specific IO, SRQ handling, asynchronous IO, raw > > USB IO, timeout)? > > Sure, send patches on, we will be glad to review and evaluate them. > > > Or do you think we shall keep the USBTMC driver in the current state? > > How long would it take to get changes into the Linux kernel? > > It depends on what the patches look like :) > > Take a look at the kernel files, Documenatation/development_process/ for > how the kernel is developed. If you send us a bugfix, we can get that > merged within a week or so. If it's a new feature or driver, that has > to wait until the next merge window, which happens ever 2 1/2 months. > > But don't worry too much about this, send your changes on and we can > review them and go from there. Sounds good. I think if we decide to extend the usbtmc driver then we will hopefully find some driver developers within the companies who are experienced :-) > > > 2. As we have looked at the Linux driver, we’ve noticed that performance > > of the usbtmc_read() function doesn’t keep up with our fastest > > instruments. Do you have any suggestions on how to improve the read > > performance? > > Where exactly are things not going fast enough? Have you found any > specific bottlenecks? How fast are you needing to go? What type of > interface do you expect userspace to have to handle high rates of data? > I'm not sure for 100%, but I assume that reading the IN pipe could be setup asynchronously (e.g. with usb_submit_urb(..)) just before sending send_request_dev_dep_msg_in(..). USBTMC_SIZE_IOBUFFER = 2kB is a bottleneck when dealing with data size > 1MB (Need of Scatter/Gather). > > 3. Do you have any plans to modify/evolve the USBTMC driver moving > > forward? > > We review any patches that people submit, just like with any other > kernel driver or subsystem. Nothing different about this one tiny > driver :) > > > 4. Do you know of any other differences between the Linux USBTMC driver > > and the Windows USBTMC driver? > > Most people here have never run windows, so you are asking the wrong > group :) > > Have you evaluated both and found any differences? The Windows USBTMC driver is maintained by the IVI Foundation. The main difference is that protocol specific headers are filled up or analyzed in user mode. This allows VISA vendors to be more flexible to maintain their applications. The specification is here: http://ivifoundation.org/downloads/Protocol%20Specifications/Ivi-6%202_USBTMC_2010-03-23.pdf At the moment I'm still not sure about the right way to go. The goal is to avoid troubles between different USBTMC applications. So we could do one of the following: 1. Drop out or disable the usbtmc driver. The IVI Foundation agrees to use only libusb from now and in the future. 2. Replace or extend the usbtmc driver with a new one. For current distributions the libusb is an alternative. 3. ... Thanks, Guido > > thanks, > > greg k-h > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: usb: usbtmc: Questions of the IVI Foundation
linux-usb-ow...@vger.kernel.org schrieb am 17.05.2017 09:08:39: > > I'm not sure for 100%, but I assume that reading the IN pipe could be > > setup asynchronously (e.g. with usb_submit_urb(..)) just before sending > > send_request_dev_dep_msg_in(..). > > USBTMC_SIZE_IOBUFFER = 2kB is a bottleneck when dealing with data size > > > 1MB (Need of Scatter/Gather). > > The major problem with usbtmc_read() is that it sends a new TMC read > request for every 2033-byte buffer it wants to read. > > As a first step, try enabling the "rigol" quirk, which simply uses > a single read request for the entire buffer. (I don't know why the > driver author thought this shouldn't be done for every device.) Thank you for the hint! I think that all T&M instruments should be able to send the entire buffer and I hope the "rigol quirk" can be used for all devices. Regards, Guido > > Regards, > Clemens -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 05/29] usb: usbtmc: Add ioctl for EOM bit
add USBTMC_IOCTL_EOM_ENABLE to specify EOM bit for next write() call. Sets Bit 0 of field 'bmTransferAttributes' of DEV_DEP_MSG_OUT Bulk-OUT Header. Allows fine grained control over end of message handling on a per file descriptor basis. Reviewed-by: Steve Bayless Tested-by: Dave Penkler Signed-off-by: Dave Penkler Signed-off-by: Guido Kiener --- drivers/usb/class/usbtmc.c | 28 +++- include/uapi/linux/usb/tmc.h | 2 ++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index 38fc7abdc00c..c77e0ac6260b 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -120,6 +120,7 @@ struct usbtmc_file_data { u32timeout; u8 srq_byte; atomic_t srq_asserted; + u8 eom_val; }; /* Forward declarations */ @@ -157,6 +158,7 @@ static int usbtmc_open(struct inode *inode, struct file *filp) file_data->data = data; file_data->timeout = USBTMC_TIMEOUT; + file_data->eom_val = 1; INIT_LIST_HEAD(&file_data->file_elem); spin_lock_irq(&data->dev_lock); @@ -855,7 +857,7 @@ static ssize_t usbtmc_write(struct file *filp, const char __user *buf, buffer[8] = 0; } else { this_part = remaining; - buffer[8] = 1; + buffer[8] = file_data->eom_val; } /* Setup IO buffer for DEV_DEP_MSG_OUT message */ @@ -1277,6 +1279,25 @@ static int usbtmc_ioctl_set_timeout(struct usbtmc_file_data *file_data, return 0; } +/* + * enables/disables sending EOM on write + */ +static int usbtmc_ioctl_eom_enable(struct usbtmc_file_data *file_data, + void __user *arg) +{ + u8 eom_enable; + + if (copy_from_user(&eom_enable, arg, sizeof(eom_enable))) + return -EFAULT; + + if (eom_enable > 1) + return -EINVAL; + + file_data->eom_val = eom_enable; + + return 0; +} + static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { struct usbtmc_file_data *file_data; @@ -1327,6 +1348,11 @@ static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg) (void __user *)arg); break; + case USBTMC_IOCTL_EOM_ENABLE: + retval = usbtmc_ioctl_eom_enable(file_data, +(void __user *)arg); + break; + case USBTMC488_IOCTL_GET_CAPS: retval = copy_to_user((void __user *)arg, &data->usb488_caps, diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h index c61bad7150dd..e7317dfdd2ae 100644 --- a/include/uapi/linux/usb/tmc.h +++ b/include/uapi/linux/usb/tmc.h @@ -50,6 +50,8 @@ #define USBTMC_IOCTL_CLEAR_IN_HALT _IO(USBTMC_IOC_NR, 7) #define USBTMC_IOCTL_GET_TIMEOUT _IOR(USBTMC_IOC_NR, 9, __u32) #define USBTMC_IOCTL_SET_TIMEOUT _IOW(USBTMC_IOC_NR, 10, __u32) +#define USBTMC_IOCTL_EOM_ENABLE_IOW(USBTMC_IOC_NR, 11, __u8) + #define USBTMC488_IOCTL_GET_CAPS _IOR(USBTMC_IOC_NR, 17, unsigned char) #define USBTMC488_IOCTL_READ_STB _IOR(USBTMC_IOC_NR, 18, unsigned char) #define USBTMC488_IOCTL_REN_CONTROL_IOW(USBTMC_IOC_NR, 19, unsigned char) -- 2.17.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 11/29] usb: usbtmc: Add ioctl for vendor specific read
The USBTMC_IOCTL_READ call provides for generic synchronous and asynchronous reads on bulk IN to implement vendor specific library routines. Depending on transfer_size the function submits one or more urbs (up to 16) each with a size of up to 4kB. 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. With flag USBTMC_FLAG_ASYNC the ioctl is non blocking. When no received data is available, the read function submits as many urbs as needed to receive transfer_size bytes. However the number of flying urbs (=4kB) is limited to 16 even with subsequent calls of this ioctl. Returns -EAGAIN when non blocking and no data is received. Signals EPOLLIN | EPOLLRDNORM when asynchronous urbs are ready to be read. In non blocking mode the usbtmc_message.message pointer may be NULL and the ioctl just submits urbs to initiate receiving data. However if data is already available due to a previous non blocking call the ioctl will return -EINVAL when the message pointer is NULL. Signed-off-by: Guido Kiener Reviewed-by: Steve Bayless --- drivers/usb/class/usbtmc.c | 369 ++- include/uapi/linux/usb/tmc.h | 2 + 2 files changed, 370 insertions(+), 1 deletion(-) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index 36a35c66f676..1a06478211ea 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -85,6 +85,9 @@ struct usbtmc_device_data { u8 bTag_last_write; /* needed for abort */ u8 bTag_last_read; /* needed for abort */ + /* packet size of IN bulk */ + u16wMaxPacketSize; + /* data for interrupt in endpoint handling */ u8 bNotify1; u8 bNotify2; @@ -140,6 +143,13 @@ struct usbtmc_file_data { struct semaphore limit_write_sem; u32 out_transfer_size; int out_status; + + /* data for generic_read */ + u32 in_transfer_size; + int in_status; + int in_urbs_used; + struct usb_anchor in_anchor; + wait_queue_head_t wait_bulk_in; }; #ifdef CONFIG_COMPAT @@ -158,6 +168,7 @@ struct compat_usbtmc_message { #define USBTMC_IOCTL_CTRL_REQUEST32_IOWR(USBTMC_IOC_NR, 8, struct compat_usbtmc_ctrlrequest) #define USBTMC_IOCTL_WRITE32 _IOWR(USBTMC_IOC_NR, 13, struct compat_usbtmc_message) +#define USBTMC_IOCTL_READ32_IOWR(USBTMC_IOC_NR, 14, struct compat_usbtmc_message) #endif @@ -192,6 +203,8 @@ static int usbtmc_open(struct inode *inode, struct file *filp) 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 */ @@ -238,6 +251,9 @@ static int usbtmc_flush(struct file *file, fl_owner_t id) 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); @@ -701,6 +717,331 @@ static struct urb *usbtmc_create_urb(void) return urb; } +static void usbtmc_read_bulk_cb(struct urb *urb) +{ + struct usbtmc_file_data *file_data = urb->context; + int status = urb->status; + unsigned long flags; + + /* 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_irqsave(&file_data->err_lock, flags); + if (!file_data->in_status) + file_data->in_status = status; + spin_unlock_irqrestore(&file_data->err_lock, flags); + } + + spin_lock_irqsave(&file_data->err_lock, flags); + file_data->in_transfer_size += urb->actual_length; + dev_dbg(&file_data->data->intf->dev, + "%s - total size: %u current: %d status:
[PATCH v2 00/29] usb: usbtmc: Changes needed for compatible IVI/VISA library
The working group "VISA for Linux" of the IVI Foundation www.ivifoundation.org specifies common rules, shared libraries and drivers to implement the specification of "VPP-4.3: The VISA Library" on Linux to be compatible with implementations on other operating systems. The USBTMC protocol is part of the "VISA Library" that is used by many popular T&M applications. Initial implementations for Linux based on libusb has been created. However using one common USBTMC driver results in more benefits: - Multiple applications can share access to the same instruments. - The driver handles SRQ conflicts. - Simplifies definition of udev rules for USBTMC devices. - Simplifies development of applications using T&M instruments. The following collaborative patches meet the requirements of the IVI Foundation to implement the library based on the usbtmc driver. Improvements in the data transfer rate of over 130 MByte/s for usb 3.x connections have been measured. Guido Kiener, Dave Penkler, Steve Bayless (29): 01 Support Read Status Byte with SRQ per file 02 Use consistent timeout error 03 Add ioctls to set/get usb timeout 04 Add ioctl for trigger 05 Add ioctl for EOM bit 06 Add ioctl for termination character 07 Add support for 32 bit compat applications 08 Add ioctl for generic requests on control 09 Add ioctl for vendor specific write 10 Add ioctl USBTMC_IOCTL_WRITE_RESULT 11 Add ioctl for vendor specific read 12 Add ioctl USBTMC_IOCTL_CANCEL_IO 13 Add ioctl USBTMC_IOCTL_CLEANUP_IO 14 Fix suspend/resume 15 Add ioctl USBTMC488_IOCTL_WAIT_SRQ 16 Add ioctl USBTMC_IOCTL_MSG_IN_ATTR 17 Add ioctl USBTMC_IOCTL_AUTO_ABORT 18 Optimize usbtmc_write 19 Optimize usbtmc_read 20 Fix ioctl USBTMC_IOCTL_CLEAR 21 Fix ioctl USBTMC_IOCTL_ABORT_BULK_IN 22 Fix ioctl USBTMC_IOCTL_ABORT_BULK_OUT 23 Replace USBTMC_TIMEOUT macros for control messages 24 Add ioctl USBTMC_IOCTL_API_VERSION 25 Update ioctl-number.txt 26 Remove redundant code 27 Remove redundant macro USBTMC_SIZE_IOBUFFER 28 Fix split quoted string in debug message 29 Remove sysfs group TermChar and auto_abort .../ABI/stable/sysfs-driver-usb-usbtmc| 35 - Documentation/ioctl/ioctl-number.txt |2 +- drivers/usb/class/usbtmc.c| 2111 + include/uapi/linux/usb/tmc.h | 54 + 4 files changed, 1730 insertions(+), 472 deletions(-) -- 2.17.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 16/29] usb: usbtmc: add ioctl USBTMC_IOCTL_MSG_IN_ATTR
add ioctl USBTMC_IOCTL_MSG_IN_ATTR that returns the specific bmTransferAttributes field of the last DEV_DEP_MSG_IN Bulk-IN header. This header is received by the read() function. The meaning of the (u8) bitmap bmTransferAttributes is: Bit 0 = EOM flag is set when the last transfer of a USBTMC message is received. Bit 1 = is set when the last byte is a termchar (e.g. '\n'). Note that this bit is always zero when the device does not support the termchar feature or when termchar detection is not enabled (see ioctl USBTMC_IOCTL_CONFIG_TERMCHAR). Signed-off-by: Guido Kiener Reviewed-by: Steve Bayless --- drivers/usb/class/usbtmc.c | 8 include/uapi/linux/usb/tmc.h | 2 ++ 2 files changed, 10 insertions(+) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index 8ab8c6f33cec..1e6cecf9e981 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -131,6 +131,7 @@ struct usbtmc_file_data { u8 srq_byte; atomic_t srq_asserted; atomic_t closing; + u8 bmTransferAttributes; /* member of DEV_DEP_MSG_IN */ u8 eom_val; u8 term_char; @@ -1503,6 +1504,8 @@ static ssize_t usbtmc_read(struct file *filp, char __user *buf, (buffer[6] << 16) + (buffer[7] << 24); + file_data->bmTransferAttributes = buffer[8]; + if (n_characters > this_part) { dev_err(dev, "Device wants to return more data than requested: %u > %zu\n", n_characters, count); if (data->auto_abort) @@ -2347,6 +2350,11 @@ static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg) (__u32 __user *)arg); break; + case USBTMC_IOCTL_MSG_IN_ATTR: + retval = put_user(file_data->bmTransferAttributes, + (__u8 __user *)arg); + break; + case USBTMC_IOCTL_CANCEL_IO: retval = usbtmc_ioctl_cancel_io(file_data); break; diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h index 76d11af19aaa..35c115e68775 100644 --- a/include/uapi/linux/usb/tmc.h +++ b/include/uapi/linux/usb/tmc.h @@ -98,6 +98,8 @@ struct usbtmc_message { #define USBTMC488_IOCTL_TRIGGER_IO(USBTMC_IOC_NR, 22) #define USBTMC488_IOCTL_WAIT_SRQ _IOW(USBTMC_IOC_NR, 23, __u32) +#define USBTMC_IOCTL_MSG_IN_ATTR _IOR(USBTMC_IOC_NR, 24, __u8) + /* 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) -- 2.17.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 06/29] usb: usbtmc: Add ioctl for termination character
add USBTMC_IOCTL_CONFIG_TERMCHAR to control TermChar handling for next read(). Controls field 'TermChar' and Bit 1 of field 'bmTransferAttributes' of REQUEST_DEV_DEP_MSG_IN BULK-OUT header. Allows enabling/disabling of terminating a read on reception of term_char individually for each read request. Reviewed-by: Steve Bayless Tested-by: Dave Penkler Signed-off-by: Dave Penkler Signed-off-by: Guido Kiener --- drivers/usb/class/usbtmc.c | 36 ++-- include/uapi/linux/usb/tmc.h | 6 ++ 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index c77e0ac6260b..1b7b2e402adb 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -121,6 +121,8 @@ struct usbtmc_file_data { u8 srq_byte; atomic_t srq_asserted; u8 eom_val; + u8 term_char; + bool term_char_enabled; }; /* Forward declarations */ @@ -157,7 +159,10 @@ static int usbtmc_open(struct inode *inode, struct file *filp) mutex_lock(&data->io_mutex); file_data->data = data; + /* copy default values from device settings */ file_data->timeout = USBTMC_TIMEOUT; + file_data->term_char = data->TermChar; + file_data->term_char_enabled = data->TermCharEnabled; file_data->eom_val = 1; INIT_LIST_HEAD(&file_data->file_elem); @@ -634,9 +639,9 @@ static int send_request_dev_dep_msg_in(struct usbtmc_file_data *file_data, buffer[5] = transfer_size >> 8; buffer[6] = transfer_size >> 16; buffer[7] = transfer_size >> 24; - buffer[8] = data->TermCharEnabled * 2; + buffer[8] = file_data->term_char_enabled * 2; /* Use term character? */ - buffer[9] = data->TermChar; + buffer[9] = file_data->term_char; buffer[10] = 0; /* Reserved */ buffer[11] = 0; /* Reserved */ @@ -1298,6 +1303,28 @@ static int usbtmc_ioctl_eom_enable(struct usbtmc_file_data *file_data, return 0; } +/* + * Configure termination character for read() + */ +static int usbtmc_ioctl_config_termc(struct usbtmc_file_data *file_data, + void __user *arg) +{ + struct usbtmc_termchar termc; + + if (copy_from_user(&termc, arg, sizeof(termc))) + return -EFAULT; + + if ((termc.term_char_enabled > 1) || + (termc.term_char_enabled && + !(file_data->data->capabilities.device_capabilities & 1))) + return -EINVAL; + + file_data->term_char = termc.term_char; + file_data->term_char_enabled = termc.term_char_enabled; + + return 0; +} + static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { struct usbtmc_file_data *file_data; @@ -1353,6 +1380,11 @@ static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg) (void __user *)arg); break; + case USBTMC_IOCTL_CONFIG_TERMCHAR: + retval = usbtmc_ioctl_config_termc(file_data, + (void __user *)arg); + break; + case USBTMC488_IOCTL_GET_CAPS: retval = copy_to_user((void __user *)arg, &data->usb488_caps, diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h index e7317dfdd2ae..729af2f861a4 100644 --- a/include/uapi/linux/usb/tmc.h +++ b/include/uapi/linux/usb/tmc.h @@ -40,6 +40,11 @@ #define USBTMC488_REQUEST_GOTO_LOCAL 161 #define USBTMC488_REQUEST_LOCAL_LOCKOUT162 +struct usbtmc_termchar { + __u8 term_char; + __u8 term_char_enabled; +} __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) @@ -51,6 +56,7 @@ #define USBTMC_IOCTL_GET_TIMEOUT _IOR(USBTMC_IOC_NR, 9, __u32) #define USBTMC_IOCTL_SET_TIMEOUT _IOW(USBTMC_IOC_NR, 10, __u32) #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 USBTMC488_IOCTL_GET_CAPS _IOR(USBTMC_IOC_NR, 17, unsigned char) #define USBTMC488_IOCTL_READ_STB _IOR(USBTMC_IOC_NR, 18, unsigned char) -- 2.17.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 04/29] usb: usbtmc: Add ioctl for trigger
add USBTMC488_IOCTL_TRIGGER to send TRIGGER Bulk-OUT header according to Subclass USB488 Specification The usbtmc trigger command is equivalent to the IEEE 488 GET (Group Execute Trigger) action. While the "*TRG" command can be sent as data to perform the same operation, in some situations an instrument will be busy and unable to process the data immediately in which case the USBTMC488_IOCTL_TRIGGER can be used to trigger the instrument with lower latency. Reviewed-by: Steve Bayless Tested-by: Dave Penkler Signed-off-by: Dave Penkler Signed-off-by: Guido Kiener --- drivers/usb/class/usbtmc.c | 49 include/uapi/linux/usb/tmc.h | 1 + 2 files changed, 50 insertions(+) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index 36d740c4c6fb..38fc7abdc00c 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -557,6 +557,51 @@ static int usbtmc488_ioctl_simple(struct usbtmc_device_data *data, return rv; } +/* + * Sends a TRIGGER Bulk-OUT command message + * See the USBTMC-USB488 specification, Table 2. + * + * Also updates bTag_last_write. + */ +static int usbtmc488_ioctl_trigger(struct usbtmc_file_data *file_data) +{ + struct usbtmc_device_data *data = file_data->data; + int retval; + u8 *buffer; + int actual; + + buffer = kzalloc(USBTMC_HEADER_SIZE, GFP_KERNEL); + if (!buffer) + return -ENOMEM; + + buffer[0] = 128; + buffer[1] = data->bTag; + buffer[2] = ~data->bTag; + + retval = usb_bulk_msg(data->usb_dev, + usb_sndbulkpipe(data->usb_dev, + data->bulk_out), + buffer, USBTMC_HEADER_SIZE, + &actual, file_data->timeout); + + /* Store bTag (in case we need to abort) */ + data->bTag_last_write = data->bTag; + + /* Increment bTag -- and increment again if zero */ + data->bTag++; + if (!data->bTag) + data->bTag++; + + kfree(buffer); + if (retval < 0) { + dev_err(&data->intf->dev, "%s returned %d\n", + __func__, retval); + return retval; + } + + return 0; +} + /* * Sends a REQUEST_DEV_DEP_MSG_IN message on the Bulk-OUT endpoint. * @transfer_size: number of bytes to request from the device. @@ -1309,6 +1354,10 @@ static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg) retval = usbtmc488_ioctl_simple(data, (void __user *)arg, USBTMC488_REQUEST_LOCAL_LOCKOUT); break; + + case USBTMC488_IOCTL_TRIGGER: + retval = usbtmc488_ioctl_trigger(file_data); + break; } skip_io_on_zombie: diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h index a89ffc33532e..c61bad7150dd 100644 --- a/include/uapi/linux/usb/tmc.h +++ b/include/uapi/linux/usb/tmc.h @@ -55,6 +55,7 @@ #define USBTMC488_IOCTL_REN_CONTROL_IOW(USBTMC_IOC_NR, 19, unsigned char) #define USBTMC488_IOCTL_GOTO_LOCAL _IO(USBTMC_IOC_NR, 20) #define USBTMC488_IOCTL_LOCAL_LOCKOUT _IO(USBTMC_IOC_NR, 21) +#define USBTMC488_IOCTL_TRIGGER_IO(USBTMC_IOC_NR, 22) /* Driver encoded usb488 capabilities */ #define USBTMC488_CAPABILITY_TRIGGER 1 -- 2.17.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 13/29] usb: usbtmc: Add ioctl USBTMC_IOCTL_CLEANUP_IO
The ioctl USBTMC_IOCTL_CLEANUP_IO kills all submitted urbs to OUT and IN bulk, and clears all received data from IN bulk. Internal transfer counters and error states are reset. An application should use this ioctl after an asnychronous transfer was canceled and/or error handling has finished. Signed-off-by: Guido Kiener Reviewed-by: Steve Bayless --- drivers/usb/class/usbtmc.c | 19 +++ include/uapi/linux/usb/tmc.h | 1 + 2 files changed, 20 insertions(+) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index d9ebecce65a4..9fc468ed0dee 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -1774,6 +1774,21 @@ static int usbtmc_ioctl_cancel_io(struct usbtmc_file_data *file_data) return 0; } +static int usbtmc_ioctl_cleanup_io(struct usbtmc_file_data *file_data) +{ + 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; @@ -2278,6 +2293,10 @@ static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg) 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: diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h index 44d29e65ba80..60da62e02d9c 100644 --- a/include/uapi/linux/usb/tmc.h +++ b/include/uapi/linux/usb/tmc.h @@ -99,6 +99,7 @@ struct usbtmc_message { /* 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 -- 2.17.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 12/29] usb: usbtmc: Add ioctl USBTMC_IOCTL_CANCEL_IO
ioctl USBTMC_IOCTL_CANCEL_IO stops and kills all flying urbs of last USBTMC_IOCTL_READ and USBTMC_IOCTL_WRITE function calls. A subsequent call to USBTMC_IOCTL_READ or USBTMC_IOCTL_WRITE_RESULT returns -ECANCELED with information about current transferred data. Signed-off-by: Guido Kiener Reviewed-by: Steve Bayless --- drivers/usb/class/usbtmc.c | 4 include/uapi/linux/usb/tmc.h | 3 +++ 2 files changed, 7 insertions(+) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index 1a06478211ea..d9ebecce65a4 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -2274,6 +2274,10 @@ 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; } skip_io_on_zombie: diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h index 6a947e495865..44d29e65ba80 100644 --- a/include/uapi/linux/usb/tmc.h +++ b/include/uapi/linux/usb/tmc.h @@ -97,6 +97,9 @@ struct usbtmc_message { #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) + /* Driver encoded usb488 capabilities */ #define USBTMC488_CAPABILITY_TRIGGER 1 #define USBTMC488_CAPABILITY_SIMPLE 2 -- 2.17.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 09/29] usb: usbtmc: Add ioctl for vendor specific write
The new ioctl USBTMC_IOCTL_WRITE sends a generic message to bulk OUT. This ioctl is used for vendor specific or asynchronous I/O as well. 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. Flush flying urbs when file handle is closed or device is suspended or reset. Signed-off-by: Guido Kiener Reviewed-by: Steve Bayless --- drivers/usb/class/usbtmc.c | 416 ++- include/uapi/linux/usb/tmc.h | 14 ++ 2 files changed, 428 insertions(+), 2 deletions(-) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index 846599dd0c84..c46280f53f39 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -37,6 +37,8 @@ /* 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) @@ -125,9 +127,19 @@ struct usbtmc_file_data { u32timeout; u8 srq_byte; atomic_t srq_asserted; + u8 eom_val; u8 term_char; bool term_char_enabled; + + spinlock_t err_lock; /* lock for errors */ + + struct usb_anchor submitted; + + /* data for generic_write */ + struct semaphore limit_write_sem; + u32 out_transfer_size; + int out_status; }; #ifdef CONFIG_COMPAT @@ -137,12 +149,21 @@ struct compat_usbtmc_ctrlrequest { compat_uptr_t data; } __packed; +struct compat_usbtmc_message { + __u32 transfer_size; + __u32 transferred; + __u32 flags; + compat_uptr_t message; +} __packed; + #define USBTMC_IOCTL_CTRL_REQUEST32_IOWR(USBTMC_IOC_NR, 8, struct compat_usbtmc_ctrlrequest) +#define USBTMC_IOCTL_WRITE32 _IOWR(USBTMC_IOC_NR, 13, struct compat_usbtmc_message) #endif /* 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) { @@ -168,6 +189,10 @@ static int usbtmc_open(struct inode *inode, struct file *filp) if (!file_data) return -ENOMEM; + spin_lock_init(&file_data->err_lock); + sema_init(&file_data->limit_write_sem, MAX_URBS_IN_FLIGHT); + init_usb_anchor(&file_data->submitted); + data = usb_get_intfdata(intf); /* Protect reference to data from file structure until release */ kref_get(&data->kref); @@ -193,6 +218,36 @@ 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; + + 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->out_status = 0; + file_data->out_transfer_size = 0; + spin_unlock_irq(&file_data->err_lock); + + wake_up_interruptible_all(&data->waitq); + 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 +680,262 @@ 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_write_bulk_cb(struct urb *urb) +{ + struct usbtmc_file_data *file_data = urb->context; + int wakeup = 0; + unsigned long flags; + + spin_lock_irqsave(&file_data->err_lock, flags); + file_data->out_transfer_size += urb->actual_length; + + /* sync/async unlink faults aren't errors */ + if (urb->status) { + if
[PATCH v2 23/29] usb: usbtmc: Replace USBTMC_TIMEOUT macros for control messages
Use common timeout macro USB_CTRL_GET_TIMEOUT (=5s) for all usb_control_msg() function calls. The macro USBTMC_TIMEOUT should only be used as default value for Bulk IN/OUT transfers. Signed-off-by: Guido Kiener Reviewed-by: Steve Bayless --- drivers/usb/class/usbtmc.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index a973ce2898e1..d545b19bf346 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -541,7 +541,7 @@ static int usbtmc488_ioctl_read_stb(struct usbtmc_file_data *file_data, USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE, data->iin_bTag, data->ifnum, - buffer, 0x03, USBTMC_TIMEOUT); + buffer, 0x03, USB_CTRL_GET_TIMEOUT); if (rv < 0) { dev_err(dev, "stb usb_control_msg returned %d\n", rv); goto exit; @@ -675,7 +675,7 @@ static int usbtmc488_ioctl_simple(struct usbtmc_device_data *data, USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE, wValue, data->ifnum, - buffer, 0x01, USBTMC_TIMEOUT); + buffer, 0x01, USB_CTRL_GET_TIMEOUT); if (rv < 0) { dev_err(dev, "simple usb_control_msg failed %d\n", rv); goto exit; @@ -1870,7 +1870,7 @@ static int get_capabilities(struct usbtmc_device_data *data) rv = usb_control_msg(data->usb_dev, usb_rcvctrlpipe(data->usb_dev, 0), USBTMC_REQUEST_GET_CAPABILITIES, USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE, -0, 0, buffer, 0x18, USBTMC_TIMEOUT); +0, 0, buffer, 0x18, USB_CTRL_GET_TIMEOUT); if (rv < 0) { dev_err(dev, "usb_control_msg returned %d\n", rv); goto err_out; @@ -2009,7 +2009,7 @@ static int usbtmc_ioctl_indicator_pulse(struct usbtmc_device_data *data) usb_rcvctrlpipe(data->usb_dev, 0), USBTMC_REQUEST_INDICATOR_PULSE, USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE, -0, 0, buffer, 0x01, USBTMC_TIMEOUT); +0, 0, buffer, 0x01, USB_CTRL_GET_TIMEOUT); if (rv < 0) { dev_err(dev, "usb_control_msg returned %d\n", rv); -- 2.17.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 15/29] usb: usbtmc: Add ioctl USBTMC488_IOCTL_WAIT_SRQ
Wait until an SRQ (service request) is received on the interrupt pipe or until the given period of time is expired. In contrast to the poll() function this ioctl does not return when other (a)synchronous I/O operations fail with EPOLLERR. Signed-off-by: Guido Kiener Reviewed-by: Steve Bayless --- drivers/usb/class/usbtmc.c | 57 include/uapi/linux/usb/tmc.h | 1 + 2 files changed, 58 insertions(+) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index 71c81ceaf7e7..8ab8c6f33cec 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -130,6 +130,7 @@ struct usbtmc_file_data { u32timeout; u8 srq_byte; atomic_t srq_asserted; + atomic_t closing; u8 eom_val; u8 term_char; @@ -213,6 +214,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->term_char = data->TermChar; @@ -243,6 +246,7 @@ static int usbtmc_flush(struct file *file, fl_owner_t id) if (file_data == NULL) return -ENODEV; + atomic_set(&file_data->closing, 1); data = file_data->data; /* wait for io to stop */ @@ -596,6 +600,54 @@ static int usbtmc488_ioctl_read_stb(struct usbtmc_file_data *file_data, return rv; } +static int usbtmc488_ioctl_wait_srq(struct usbtmc_file_data *file_data, + __u32 __user *arg) +{ + struct usbtmc_device_data *data = file_data->data; + struct device *dev = &data->intf->dev; + int rv; + u32 timeout; + unsigned long expire; + + if (!data->iin_ep_present) { + dev_dbg(dev, "no interrupt endpoint present\n"); + return -EFAULT; + } + + if (get_user(timeout, arg)) + return -EFAULT; + + expire = msecs_to_jiffies(timeout); + + mutex_unlock(&data->io_mutex); + + rv = wait_event_interruptible_timeout( + data->waitq, + atomic_read(&file_data->srq_asserted) != 0 || + atomic_read(&file_data->closing), + expire); + + mutex_lock(&data->io_mutex); + + /* Note! disconnect or close could be called in the meantime */ + if (atomic_read(&file_data->closing) || data->zombie) + rv = -ENODEV; + + if (rv < 0) { + /* dev can be invalid now! */ + pr_debug("%s - wait interrupted %d\n", __func__, rv); + return rv; + } + + if (rv == 0) { + dev_dbg(dev, "%s - wait timed out\n", __func__); + return -ETIMEDOUT; + } + + dev_dbg(dev, "%s - srq asserted\n", __func__); + return 0; +} + static int usbtmc488_ioctl_simple(struct usbtmc_device_data *data, void __user *arg, unsigned int cmd) { @@ -2290,6 +2342,11 @@ static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg) retval = usbtmc488_ioctl_trigger(file_data); break; + case USBTMC488_IOCTL_WAIT_SRQ: + retval = usbtmc488_ioctl_wait_srq(file_data, + (__u32 __user *)arg); + break; + case USBTMC_IOCTL_CANCEL_IO: retval = usbtmc_ioctl_cancel_io(file_data); break; diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h index 60da62e02d9c..76d11af19aaa 100644 --- a/include/uapi/linux/usb/tmc.h +++ b/include/uapi/linux/usb/tmc.h @@ -96,6 +96,7 @@ struct usbtmc_message { #define USBTMC488_IOCTL_GOTO_LOCAL _IO(USBTMC_IOC_NR, 20) #define USBTMC488_IOCTL_LOCAL_LOCKOUT _IO(USBTMC_IOC_NR, 21) #define USBTMC488_IOCTL_TRIGGER_IO(USBTMC_IOC_NR, 22) +#define USBTMC488_IOCTL_WAIT_SRQ _IOW(USBTMC_IOC_NR, 23, __u32) /* Cancel and cleanup asynchronous calls */ #define USBTMC_IOCTL_CANCEL_IO _IO(USBTMC_IOC_NR, 35) -- 2.17.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 18/29] usb: usbtmc: Optimize usbtmc_write
Use new usbtmc_generic_write function to maximize bandwidth during long data transfer. The maximum output transfer size is limited to INT_MAX (=2GB). Signed-off-by: Guido Kiener Reviewed-by: Steve Bayless --- drivers/usb/class/usbtmc.c | 176 +++-- 1 file changed, 109 insertions(+), 67 deletions(-) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index 9ec03b74035b..f38475ee6d5c 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -1577,94 +1577,136 @@ static ssize_t usbtmc_write(struct file *filp, const char __user *buf, { struct usbtmc_file_data *file_data; struct usbtmc_device_data *data; + struct urb *urb = NULL; + ssize_t retval = 0; u8 *buffer; - int retval; - int actual; - unsigned long int n_bytes; - int remaining; - int done; - int this_part; + u32 remaining, done; + u32 transfersize, aligned, buflen; file_data = filp->private_data; data = file_data->data; - buffer = kmalloc(USBTMC_SIZE_IOBUFFER, GFP_KERNEL); - if (!buffer) - return -ENOMEM; - mutex_lock(&data->io_mutex); + if (data->zombie) { retval = -ENODEV; goto exit; } - remaining = count; done = 0; - while (remaining > 0) { - if (remaining > USBTMC_SIZE_IOBUFFER - USBTMC_HEADER_SIZE) { - this_part = USBTMC_SIZE_IOBUFFER - USBTMC_HEADER_SIZE; - buffer[8] = 0; - } else { - this_part = remaining; - buffer[8] = file_data->eom_val; - } - - /* Setup IO buffer for DEV_DEP_MSG_OUT message */ - buffer[0] = 1; - buffer[1] = data->bTag; - buffer[2] = ~data->bTag; - buffer[3] = 0; /* Reserved */ - buffer[4] = this_part >> 0; - buffer[5] = this_part >> 8; - buffer[6] = this_part >> 16; - buffer[7] = this_part >> 24; - /* buffer[8] is set above... */ - buffer[9] = 0; /* Reserved */ - buffer[10] = 0; /* Reserved */ - buffer[11] = 0; /* Reserved */ - - if (copy_from_user(&buffer[USBTMC_HEADER_SIZE], buf + done, this_part)) { - retval = -EFAULT; - goto exit; - } - - n_bytes = roundup(USBTMC_HEADER_SIZE + this_part, 4); - memset(buffer + USBTMC_HEADER_SIZE + this_part, 0, n_bytes - (USBTMC_HEADER_SIZE + this_part)); - - do { - retval = usb_bulk_msg(data->usb_dev, - usb_sndbulkpipe(data->usb_dev, - data->bulk_out), - buffer, n_bytes, - &actual, file_data->timeout); - if (retval != 0) - break; - n_bytes -= actual; - } while (n_bytes); - - data->bTag_last_write = data->bTag; + 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); + + if (!count) + goto exit; + + if (down_trylock(&file_data->limit_write_sem)) { + /* previous calls were async */ + retval = -EBUSY; + goto exit; + } + + urb = usbtmc_create_urb(); + if (!urb) { + retval = -ENOMEM; + up(&file_data->limit_write_sem); + goto exit; + } + + buffer = urb->transfer_buffer; + buflen = urb->transfer_buffer_length; + + if (count > INT_MAX) { + transfersize = INT_MAX; + buffer[8] = 0; + } else { + transfersize = count; + buffer[8] = file_data->eom_val; + } + + /* Setup IO buffer for DEV_DEP_MSG_OUT message */ + buffer[0] = 1; + buffer[1] = data->bTag; + buffer[2] = ~data->bTag; + buffer[3] = 0; /* Reserved */ + buffer[4] = transfersize >> 0; + buffer[5] = transfersize >> 8; + buffer[6] = transfersize >> 16; + buffer[7] = transfersize >> 24; + /* buffer[8] is set above... */ + buffer[9] = 0; /* Reserved */ + buffer[10] = 0; /* Reserved */ + buffer[11] = 0; /* Reserved */ + + remaining = transfersize; + + if (transfersize + USBTMC_HEADER_SIZE > buflen) { + transfersize = buflen - USBTMC_HEADER_SIZE; +
[PATCH v2 20/29] usb: usbtmc: Fix ioctl USBTMC_IOCTL_CLEAR
Remove calculation of max_size (=wMaxPacketSize) and wrong condition (actual == max_size) in while loop. A device clear should always flush the complete Bulk-IN FIFO. Insert a sleep of 50 ms between subsequent CHECK_CLEAR_STATUS control requests to avoid stressing the instrument with repeated requests. Some instruments need time to cleanup internal I/O buffers. Polling and nonbraked requests slow down the response time of devices. Use USBTMC_BUFSIZE (4k) instead of USBTMC_SIZE_IOBUFFER (2k). Using USBTMC_SIZE_IOBUFFER is deprecated. Check only bit 0 (field bmClear) of the CHECK_CLEAR_STATUS response, since other bits are reserved and can change in future versions. Signed-off-by: Guido Kiener Reviewed-by: Steve Bayless --- drivers/usb/class/usbtmc.c | 46 +++--- 1 file changed, 18 insertions(+), 28 deletions(-) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index fa8b17947792..dda5391d01e5 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -1700,20 +1700,17 @@ static ssize_t usbtmc_write(struct file *filp, const char __user *buf, static int usbtmc_ioctl_clear(struct usbtmc_device_data *data) { - struct usb_host_interface *current_setting; - struct usb_endpoint_descriptor *desc; struct device *dev; u8 *buffer; int rv; int n; int actual = 0; - int max_size; dev = &data->intf->dev; dev_dbg(dev, "Sending INITIATE_CLEAR request\n"); - buffer = kmalloc(USBTMC_SIZE_IOBUFFER, GFP_KERNEL); + buffer = kmalloc(USBTMC_BUFSIZE, GFP_KERNEL); if (!buffer) return -ENOMEM; @@ -1721,7 +1718,7 @@ static int usbtmc_ioctl_clear(struct usbtmc_device_data *data) usb_rcvctrlpipe(data->usb_dev, 0), USBTMC_REQUEST_INITIATE_CLEAR, USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE, -0, 0, buffer, 1, USBTMC_TIMEOUT); +0, 0, buffer, 1, USB_CTRL_GET_TIMEOUT); if (rv < 0) { dev_err(dev, "usb_control_msg returned %d\n", rv); goto exit; @@ -1735,22 +1732,6 @@ static int usbtmc_ioctl_clear(struct usbtmc_device_data *data) goto exit; } - max_size = 0; - current_setting = data->intf->cur_altsetting; - for (n = 0; n < current_setting->desc.bNumEndpoints; n++) { - desc = ¤t_setting->endpoint[n].desc; - if (desc->bEndpointAddress == data->bulk_in) - max_size = usb_endpoint_maxp(desc); - } - - if (max_size == 0) { - dev_err(dev, "Couldn't get wMaxPacketSize\n"); - rv = -EPERM; - goto exit; - } - - dev_dbg(dev, "wMaxPacketSize is %d\n", max_size); - n = 0; usbtmc_clear_check_status: @@ -1761,7 +1742,7 @@ static int usbtmc_ioctl_clear(struct usbtmc_device_data *data) usb_rcvctrlpipe(data->usb_dev, 0), USBTMC_REQUEST_CHECK_CLEAR_STATUS, USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE, -0, 0, buffer, 2, USBTMC_TIMEOUT); +0, 0, buffer, 2, USB_CTRL_GET_TIMEOUT); if (rv < 0) { dev_err(dev, "usb_control_msg returned %d\n", rv); goto exit; @@ -1778,15 +1759,19 @@ static int usbtmc_ioctl_clear(struct usbtmc_device_data *data) goto exit; } - if (buffer[1] == 1) + if ((buffer[1] & 1) != 0) { do { dev_dbg(dev, "Reading from bulk in EP\n"); rv = usb_bulk_msg(data->usb_dev, usb_rcvbulkpipe(data->usb_dev, data->bulk_in), - buffer, USBTMC_SIZE_IOBUFFER, - &actual, USBTMC_TIMEOUT); + buffer, USBTMC_BUFSIZE, + &actual, USB_CTRL_GET_TIMEOUT); + + print_hex_dump_debug("usbtmc ", DUMP_PREFIX_NONE, +16, 1, buffer, actual, true); + n++; if (rv < 0) { @@ -1794,10 +1779,15 @@ static int usbtmc_ioctl_clear(struct usbtmc_device_data *data) rv); goto exit; } - } while ((actual == max_size) && + } while ((actual == USBTMC_BUFSIZE) && (n < USBTMC
[PATCH v2 08/29] usb: usbtmc: Add ioctl for generic requests on control
Add USBTMC_IOCTL_CTRL_REQUEST to send arbitrary requests on the control pipe. Used by specific applications of IVI Foundation, Inc. to implement VISA API functions: viUsbControlIn/Out. The maximum length of control request is set to 4k. Signed-off-by: Guido Kiener Reviewed-by: Steve Bayless --- drivers/usb/class/usbtmc.c | 151 +++ include/uapi/linux/usb/tmc.h | 15 2 files changed, 166 insertions(+) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index d685db78b80b..846599dd0c84 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -5,6 +5,7 @@ * Copyright (C) 2007 Stefan Kopp, Gechingen, Germany * Copyright (C) 2008 Novell, Inc. * Copyright (C) 2008 Greg Kroah-Hartman + * Copyright (C) 2018 IVI Foundation, Inc. */ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt @@ -36,6 +37,9 @@ /* Default USB timeout (in milliseconds) */ #define USBTMC_TIMEOUT 5000 +/* 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 @@ -126,6 +130,17 @@ struct usbtmc_file_data { bool term_char_enabled; }; +#ifdef CONFIG_COMPAT + +struct compat_usbtmc_ctrlrequest { + struct usbtmc_request req; + compat_uptr_t data; +} __packed; + +#define USBTMC_IOCTL_CTRL_REQUEST32_IOWR(USBTMC_IOC_NR, 8, struct compat_usbtmc_ctrlrequest) + +#endif + /* Forward declarations */ static struct usb_driver usbtmc_driver; @@ -1250,6 +1265,127 @@ static int usbtmc_ioctl_indicator_pulse(struct usbtmc_device_data *data) return rv; } +#ifdef CONFIG_COMPAT +static int usbtmc_ioctl_request32(struct usbtmc_device_data *data, + void __user *arg) +{ + struct device *dev = &data->intf->dev; + struct compat_usbtmc_ctrlrequest request; + u8 *buffer = NULL; + int rv; + unsigned long res; + + res = copy_from_user(&request, arg, +sizeof(struct compat_usbtmc_ctrlrequest)); + if (res) + return -EFAULT; + + buffer = kmalloc(request.req.wLength, GFP_KERNEL); + if (!buffer) + return -ENOMEM; + + if (request.req.wLength > USBTMC_BUFSIZE) + return -EMSGSIZE; + + if (request.req.wLength) { + buffer = kmalloc(request.req.wLength, GFP_KERNEL); + if (!buffer) + return -ENOMEM; + + if ((request.req.bRequestType & USB_DIR_IN) == 0) { + /* Send control data to device */ + res = copy_from_user(buffer, compat_ptr(request.data), +request.req.wLength); + if (res) { + rv = -EFAULT; + goto exit; + } + } + } + + rv = usb_control_msg(data->usb_dev, + usb_rcvctrlpipe(data->usb_dev, 0), + request.req.bRequest, + request.req.bRequestType, + request.req.wValue, + request.req.wIndex, + buffer, request.req.wLength, USB_CTRL_GET_TIMEOUT); + + if (rv < 0) { + dev_err(dev, "%s failed %d\n", __func__, rv); + goto exit; + } + + if (rv && (request.req.bRequestType & USB_DIR_IN)) { + /* Read control data from device */ + res = copy_to_user(compat_ptr(request.data), buffer, rv); + if (res) + rv = -EFAULT; + } + + exit: + kfree(buffer); + return rv; +} +#endif + +static int usbtmc_ioctl_request(struct usbtmc_device_data *data, + void __user *arg) +{ + struct device *dev = &data->intf->dev; + struct usbtmc_ctrlrequest request; + u8 *buffer = NULL; + int rv; + unsigned long res; + + res = copy_from_user(&request, arg, sizeof(struct usbtmc_ctrlrequest)); + if (res) + return -EFAULT; + + if (request.req.wLength > USBTMC_BUFSIZE) + return -EMSGSIZE; + + if (request.req.wLength) { + buffer = kmalloc(request.req.wLength, GFP_KERNEL); + if (!buffer) + return -ENOMEM; + + if ((request.req.bRequestType & USB_DIR_IN) == 0) { + /* Send control data to device */ + res = copy_from_user(buffer, request.data, +request.req.wLength); + if (res) { +
[PATCH v2 07/29] usb: usbtmc: Add support for 32 bit compat applications
32 bit applications can only call ioctl functions on 64 bit systems when the field .compat_ioctl is defined for file operations. Tested-by: Dave Penkler Signed-off-by: Guido Kiener Reviewed-by: Steve Bayless --- drivers/usb/class/usbtmc.c | 12 1 file changed, 12 insertions(+) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index 1b7b2e402adb..d685db78b80b 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -18,6 +18,7 @@ #include #include #include +#include #include @@ -1423,6 +1424,14 @@ static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg) return retval; } +#ifdef CONFIG_COMPAT +static long usbtmc_compat_ioctl(struct file *file, unsigned int cmd, + unsigned long arg) +{ + return usbtmc_ioctl(file, cmd, (unsigned long)compat_ptr(arg)); +} +#endif + static int usbtmc_fasync(int fd, struct file *file, int on) { struct usbtmc_file_data *file_data = file->private_data; @@ -1459,6 +1468,9 @@ static const struct file_operations fops = { .open = usbtmc_open, .release= usbtmc_release, .unlocked_ioctl = usbtmc_ioctl, +#ifdef CONFIG_COMPAT + .compat_ioctl = usbtmc_compat_ioctl, +#endif .fasync = usbtmc_fasync, .poll = usbtmc_poll, .llseek = default_llseek, -- 2.17.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 21/29] usb: usbtmc: Fix ioctl USBTMC_IOCTL_ABORT_BULK_IN
Add parameter 'tag' to function usbtmc_ioctl_abort_bulk_in_tag() for future versions. Remove calculation of max_size (=wMaxPacketSize) and wrong condition (actual == max_size) in while loop. An abort operation should always flush the complete Bulk-IN until a short packet is received. Return error code ENOMSG when transfer (specified by given tag) is not in progress and device returns code USBTMC_STATUS_TRANSFER_NOT_IN_PROGRESS. Use USBTMC_BUFSIZE (4k) instead of USBTMC_SIZE_IOBUFFER (2k). Using USBTMC_SIZE_IOBUFFER is deprecated. Use common macro USB_CTRL_GET_TIMEOUT instead of USBTMC_TIMEOUT. Check only bit 0 (field bmAbortBulkIn) of the CHECK_ABORT_BULK_IN_STATUS response, since other bits are reserved and can change in future versions. Signed-off-by: Guido Kiener Reviewed-by: Steve Bayless --- drivers/usb/class/usbtmc.c | 139 - 1 file changed, 61 insertions(+), 78 deletions(-) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index dda5391d01e5..a516b0fe37a3 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -290,18 +290,17 @@ static int usbtmc_release(struct inode *inode, struct file *file) return 0; } -static int usbtmc_ioctl_abort_bulk_in(struct usbtmc_device_data *data) +static int usbtmc_ioctl_abort_bulk_in_tag(struct usbtmc_device_data *data, + u8 tag) { u8 *buffer; struct device *dev; int rv; int n; int actual; - struct usb_host_interface *current_setting; - int max_size; dev = &data->intf->dev; - buffer = kmalloc(USBTMC_SIZE_IOBUFFER, GFP_KERNEL); + buffer = kmalloc(USBTMC_BUFSIZE, GFP_KERNEL); if (!buffer) return -ENOMEM; @@ -309,21 +308,34 @@ static int usbtmc_ioctl_abort_bulk_in(struct usbtmc_device_data *data) usb_rcvctrlpipe(data->usb_dev, 0), USBTMC_REQUEST_INITIATE_ABORT_BULK_IN, USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_ENDPOINT, -data->bTag_last_read, data->bulk_in, -buffer, 2, USBTMC_TIMEOUT); +tag, data->bulk_in, +buffer, 2, USB_CTRL_GET_TIMEOUT); if (rv < 0) { dev_err(dev, "usb_control_msg returned %d\n", rv); goto exit; } - dev_dbg(dev, "INITIATE_ABORT_BULK_IN returned %x\n", buffer[0]); + dev_dbg(dev, "INITIATE_ABORT_BULK_IN returned %x with tag %02x\n", + buffer[0], buffer[1]); if (buffer[0] == USBTMC_STATUS_FAILED) { + /* No transfer in progress and the Bulk-OUT FIFO is empty. */ rv = 0; goto exit; } + if (buffer[0] == USBTMC_STATUS_TRANSFER_NOT_IN_PROGRESS) { + /* The device returns this status if either: +* - There is a transfer in progress, but the specified bTag +* does not match. +* - There is no transfer in progress, but the Bulk-OUT FIFO +* is not empty. +*/ + rv = -ENOMSG; + goto exit; + } + if (buffer[0] != USBTMC_STATUS_SUCCESS) { dev_err(dev, "INITIATE_ABORT_BULK_IN returned %x\n", buffer[0]); @@ -331,64 +343,52 @@ static int usbtmc_ioctl_abort_bulk_in(struct usbtmc_device_data *data) goto exit; } - max_size = 0; - current_setting = data->intf->cur_altsetting; - for (n = 0; n < current_setting->desc.bNumEndpoints; n++) - if (current_setting->endpoint[n].desc.bEndpointAddress == - data->bulk_in) - max_size = usb_endpoint_maxp(¤t_setting->endpoint[n].desc); - - if (max_size == 0) { - dev_err(dev, "Couldn't get wMaxPacketSize\n"); - rv = -EPERM; - goto exit; - } - - dev_dbg(&data->intf->dev, "wMaxPacketSize is %d\n", max_size); - - n = 0; - - do { - dev_dbg(dev, "Reading from bulk in EP\n"); - - rv = usb_bulk_msg(data->usb_dev, - usb_rcvbulkpipe(data->usb_dev, - data->bulk_in), - buffer, USBTMC_SIZE_IOBUFFER, - &actual, USBTMC_TIMEOUT); - - n++; - - if (rv < 0) { - dev_err(dev, "usb_bulk_msg returned %d\n", rv); - goto exit; - } - } while ((actual == max_size) &&am
[PATCH v2 01/29] usb: usbtmc: Support Read Status Byte with SRQ per file
Add 'struct usbtmc_file_data' for each file handle to cache last srq_byte (=Status Byte with SRQ) received by usbtmc_interrupt(..) usbtmc488_ioctl_read_stb returns cached srq_byte when available for each file handle to avoid race conditions of concurrent applications. SRQ now sets EPOLLPRI instead of EPOLLIN since EPOLLIN is now reserved for asynchronous reads Tested-by: Dave Penkler Reviewed-by: Steve Bayless Signed-off-by: Guido Kiener --- drivers/usb/class/usbtmc.c | 136 - 1 file changed, 105 insertions(+), 31 deletions(-) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index 529295a17579..db58b84d43ee 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -67,6 +67,7 @@ struct usbtmc_device_data { const struct usb_device_id *id; struct usb_device *usb_dev; struct usb_interface *intf; + struct list_head file_list; unsigned int bulk_in; unsigned int bulk_out; @@ -87,7 +88,6 @@ struct usbtmc_device_data { intiin_interval; struct urb*iin_urb; u16iin_wMaxPacketSize; - atomic_t srq_asserted; /* coalesced usb488_caps from usbtmc_dev_capabilities */ __u8 usb488_caps; @@ -104,9 +104,21 @@ struct usbtmc_device_data { struct mutex io_mutex; /* only one i/o function running at a time */ wait_queue_head_t waitq; struct fasync_struct *fasync; + spinlock_t dev_lock; /* lock for file_list */ }; #define to_usbtmc_data(d) container_of(d, struct usbtmc_device_data, kref) +/* + * This structure holds private data for each USBTMC file handle. + */ +struct usbtmc_file_data { + struct usbtmc_device_data *data; + struct list_head file_elem; + + u8 srq_byte; + atomic_t srq_asserted; +}; + /* Forward declarations */ static struct usb_driver usbtmc_driver; @@ -122,7 +134,7 @@ static int usbtmc_open(struct inode *inode, struct file *filp) { struct usb_interface *intf; struct usbtmc_device_data *data; - int retval = 0; + struct usbtmc_file_data *file_data; intf = usb_find_interface(&usbtmc_driver, iminor(inode)); if (!intf) { @@ -130,21 +142,45 @@ static int usbtmc_open(struct inode *inode, struct file *filp) return -ENODEV; } + file_data = kzalloc(sizeof(*file_data), GFP_KERNEL); + if (!file_data) + return -ENOMEM; + data = usb_get_intfdata(intf); /* Protect reference to data from file structure until release */ kref_get(&data->kref); + mutex_lock(&data->io_mutex); + file_data->data = data; + + INIT_LIST_HEAD(&file_data->file_elem); + spin_lock_irq(&data->dev_lock); + list_add_tail(&file_data->file_elem, &data->file_list); + spin_unlock_irq(&data->dev_lock); + mutex_unlock(&data->io_mutex); + /* Store pointer in file structure's private data field */ - filp->private_data = data; + filp->private_data = file_data; - return retval; + return 0; } static int usbtmc_release(struct inode *inode, struct file *file) { - struct usbtmc_device_data *data = file->private_data; + struct usbtmc_file_data *file_data = file->private_data; - kref_put(&data->kref, usbtmc_delete); + /* prevent IO _AND_ usbtmc_interrupt */ + mutex_lock(&file_data->data->io_mutex); + spin_lock_irq(&file_data->data->dev_lock); + + list_del(&file_data->file_elem); + + spin_unlock_irq(&file_data->data->dev_lock); + mutex_unlock(&file_data->data->io_mutex); + + kref_put(&file_data->data->kref, usbtmc_delete); + file_data->data = NULL; + kfree(file_data); return 0; } @@ -369,10 +405,12 @@ static int usbtmc_ioctl_abort_bulk_out(struct usbtmc_device_data *data) return rv; } -static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data, +static int usbtmc488_ioctl_read_stb(struct usbtmc_file_data *file_data, void __user *arg) { + struct usbtmc_device_data *data = file_data->data; struct device *dev = &data->intf->dev; + int srq_asserted = 0; u8 *buffer; u8 tag; __u8 stb; @@ -381,15 +419,25 @@ static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data, dev_dbg(dev, "Enter ioctl_read_stb iin_ep_present: %d\n", data->iin_ep_present); + spin_lock_irq(&data->dev_lock); + srq_asserted = atomic_xchg(&file_data->srq_asserted, srq_asserted); + if (srq_asserted) { + /* a STB with SRQ is already received */ +
[PATCH v2 19/29] usb: usbtmc: Optimize usbtmc_read
Use new usbtmc_generic_read function to maximize bandwidth during long data transfer. Also fix reading of zero length packet (ZLP) or trailing short packet. The maximum input transfer size is limited to INT_MAX (=2GB). Also remove redundant return in send_request_dev_dep_msg_in(). Signed-off-by: Guido Kiener Reviewed-by: Steve Bayless --- drivers/usb/class/usbtmc.c | 222 ++--- 1 file changed, 105 insertions(+), 117 deletions(-) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index f38475ee6d5c..fa8b17947792 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -1361,7 +1361,7 @@ static ssize_t usbtmc_ioctl_write_result(struct usbtmc_file_data *file_data, * Also updates bTag_last_write. */ static int send_request_dev_dep_msg_in(struct usbtmc_file_data *file_data, - size_t transfer_size) + u32 transfer_size) { struct usbtmc_device_data *data = file_data->data; int retval; @@ -1404,12 +1404,11 @@ static int send_request_dev_dep_msg_in(struct usbtmc_file_data *file_data, data->bTag++; kfree(buffer); - if (retval < 0) { - dev_err(&data->intf->dev, "usb_bulk_msg in send_request_dev_dep_msg_in() returned %d\n", retval); - return retval; - } + if (retval < 0) + dev_err(&data->intf->dev, "%s returned %d\n", + __func__, retval); - return 0; + return retval; } static ssize_t usbtmc_read(struct file *filp, char __user *buf, @@ -1418,20 +1417,20 @@ static ssize_t usbtmc_read(struct file *filp, char __user *buf, struct usbtmc_file_data *file_data; struct usbtmc_device_data *data; struct device *dev; + const u32 bufsize = USBTMC_BUFSIZE; u32 n_characters; u8 *buffer; int actual; - size_t done; - size_t remaining; + u32 done = 0; + u32 remaining; int retval; - size_t this_part; /* Get pointer to private data structure */ file_data = filp->private_data; data = file_data->data; dev = &data->intf->dev; - buffer = kmalloc(USBTMC_SIZE_IOBUFFER, GFP_KERNEL); + buffer = kmalloc(bufsize, GFP_KERNEL); if (!buffer) return -ENOMEM; @@ -1441,7 +1440,10 @@ static ssize_t usbtmc_read(struct file *filp, char __user *buf, goto exit; } - dev_dbg(dev, "usb_bulk_msg_in: count(%zu)\n", count); + if (count > INT_MAX) + count = INT_MAX; + + dev_dbg(dev, "%s(count:%zu)\n", __func__, count); retval = send_request_dev_dep_msg_in(file_data, count); @@ -1453,114 +1455,100 @@ static ssize_t usbtmc_read(struct file *filp, char __user *buf, /* Loop until we have fetched everything we requested */ remaining = count; - this_part = remaining; - done = 0; - - while (remaining > 0) { - /* Send bulk URB */ - retval = usb_bulk_msg(data->usb_dev, - usb_rcvbulkpipe(data->usb_dev, - data->bulk_in), - buffer, USBTMC_SIZE_IOBUFFER, &actual, - file_data->timeout); - - dev_dbg(dev, "usb_bulk_msg: retval(%u), done(%zu), remaining(%zu), actual(%d)\n", retval, done, remaining, actual); - - /* Store bTag (in case we need to abort) */ - data->bTag_last_read = data->bTag; - - if (retval < 0) { - dev_dbg(dev, "Unable to read data, error %d\n", retval); - if (file_data->auto_abort) - usbtmc_ioctl_abort_bulk_in(data); + + /* Send bulk URB */ + retval = usb_bulk_msg(data->usb_dev, + usb_rcvbulkpipe(data->usb_dev, + data->bulk_in), + buffer, bufsize, &actual, + file_data->timeout); + + dev_dbg(dev, "%s: bulk_msg retval(%u), actual(%d)\n", + __func__, retval, actual); + + /* Store bTag (in case we need to abort) */ + data->bTag_last_read = data->bTag; + + if (retval < 0) { + if (file_data->auto_abort) + usbtmc_ioctl_abort_bulk_in(data); + goto exit; + } + + /* Sanity checks for the header */ + if (actual < USBTMC_HEADER_SIZE) { + dev_err(dev, "Device sent too small first packet: %u < %u\n", + actual, USBTMC_HEADER_SIZE); +
[PATCH v2 14/29] usb: usbtmc: Fix suspend/resume
Submitted urbs are not allowed when system is suspended. Thus the submitted urb waiting at interrupt pipe is killed during suspend callback and submitted again when system resumes. Signed-off-by: Guido Kiener Reviewed-by: Steve Bayless --- drivers/usb/class/usbtmc.c | 16 +++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index 9fc468ed0dee..71c81ceaf7e7 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -2469,7 +2469,9 @@ static void usbtmc_free_int(struct usbtmc_device_data *data) return; usb_kill_urb(data->iin_urb); kfree(data->iin_buffer); + data->iin_buffer = NULL; usb_free_urb(data->iin_urb); + data->iin_urb = NULL; kref_put(&data->kref, usbtmc_delete); } @@ -2651,13 +2653,25 @@ static int usbtmc_suspend(struct usb_interface *intf, pm_message_t message) 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) { - return 0; + struct usbtmc_device_data *data = usb_get_intfdata(intf); + int retcode = 0; + + 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) -- 2.17.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 10/29] usb: usbtmc: Add ioctl USBTMC_IOCTL_WRITE_RESULT
To allow applications to determine the status and progress of the last asynchronous USBTMC_IOCTL_WRITE call, the ioctl USBTMC_IOCTL_WRITE_RESULT copies the current out_transfer_size to the provided __u32 pointer and returns current out_status. Signed-off-by: Guido Kiener Reviewed-by: Steve Bayless --- drivers/usb/class/usbtmc.c | 25 + include/uapi/linux/usb/tmc.h | 1 + 2 files changed, 26 insertions(+) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index c46280f53f39..36a35c66f676 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -936,6 +936,26 @@ static ssize_t usbtmc_ioctl_generic_write32(struct usbtmc_file_data *file_data, } #endif +/* + * Get the generic write result + */ +static ssize_t usbtmc_ioctl_write_result(struct usbtmc_file_data *file_data, + void __user *arg) +{ + u32 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, (__u32 __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. @@ -1864,6 +1884,11 @@ static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg) break; #endif + case USBTMC_IOCTL_WRITE_RESULT: + retval = usbtmc_ioctl_write_result(file_data, + (void __user *)arg); + break; + case USBTMC488_IOCTL_GET_CAPS: retval = copy_to_user((void __user *)arg, &data->usb488_caps, diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h index fd815d6f19ac..dc9dd55fa16f 100644 --- a/include/uapi/linux/usb/tmc.h +++ b/include/uapi/linux/usb/tmc.h @@ -86,6 +86,7 @@ struct usbtmc_message { #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_WRITE_RESULT _IOWR(USBTMC_IOC_NR, 15, __u32) #define USBTMC488_IOCTL_GET_CAPS _IOR(USBTMC_IOC_NR, 17, unsigned char) #define USBTMC488_IOCTL_READ_STB _IOR(USBTMC_IOC_NR, 18, unsigned char) -- 2.17.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 02/29] usb: usbtmc: use consistent timeout error
- use consistent error value ETIMEOUT instead of ETIME Tested-by: Dave Penkler Reviewed-by: Steve Bayless Signed-off-by: Guido Kiener --- drivers/usb/class/usbtmc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index db58b84d43ee..243e8446b8dd 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -468,7 +468,7 @@ static int usbtmc488_ioctl_read_stb(struct usbtmc_file_data *file_data, if (rv == 0) { dev_dbg(dev, "wait timed out\n"); - rv = -ETIME; + rv = -ETIMEDOUT; goto exit; } -- 2.17.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 17/29] usb: usbtmc: Add ioctl USBTMC_IOCTL_AUTO_ABORT
Add ioctl USBTMC_IOCTL_AUTO_ABORT to configure auto_abort for each specific file handle. Signed-off-by: Guido Kiener Reviewed-by: Steve Bayless --- drivers/usb/class/usbtmc.c | 23 --- include/uapi/linux/usb/tmc.h | 1 + 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index 1e6cecf9e981..9ec03b74035b 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -136,6 +136,7 @@ struct usbtmc_file_data { u8 eom_val; u8 term_char; bool term_char_enabled; + bool auto_abort; spinlock_t err_lock; /* lock for errors */ @@ -221,6 +222,7 @@ static int usbtmc_open(struct inode *inode, struct file *filp) file_data->timeout = USBTMC_TIMEOUT; file_data->term_char = data->TermChar; file_data->term_char_enabled = data->TermCharEnabled; + file_data->auto_abort = data->auto_abort; file_data->eom_val = 1; INIT_LIST_HEAD(&file_data->file_elem); @@ -1444,7 +1446,7 @@ static ssize_t usbtmc_read(struct file *filp, char __user *buf, retval = send_request_dev_dep_msg_in(file_data, count); if (retval < 0) { - if (data->auto_abort) + if (file_data->auto_abort) usbtmc_ioctl_abort_bulk_out(data); goto exit; } @@ -1469,7 +1471,7 @@ static ssize_t usbtmc_read(struct file *filp, char __user *buf, if (retval < 0) { dev_dbg(dev, "Unable to read data, error %d\n", retval); - if (data->auto_abort) + if (file_data->auto_abort) usbtmc_ioctl_abort_bulk_in(data); goto exit; } @@ -1479,21 +1481,21 @@ static ssize_t usbtmc_read(struct file *filp, char __user *buf, /* Sanity checks for the header */ if (actual < USBTMC_HEADER_SIZE) { dev_err(dev, "Device sent too small first packet: %u < %u\n", actual, USBTMC_HEADER_SIZE); - if (data->auto_abort) + if (file_data->auto_abort) usbtmc_ioctl_abort_bulk_in(data); goto exit; } if (buffer[0] != 2) { dev_err(dev, "Device sent reply with wrong MsgID: %u != 2\n", buffer[0]); - if (data->auto_abort) + if (file_data->auto_abort) usbtmc_ioctl_abort_bulk_in(data); goto exit; } if (buffer[1] != data->bTag_last_write) { dev_err(dev, "Device sent reply with wrong bTag: %u != %u\n", buffer[1], data->bTag_last_write); - if (data->auto_abort) + if (file_data->auto_abort) usbtmc_ioctl_abort_bulk_in(data); goto exit; } @@ -1508,7 +1510,7 @@ static ssize_t usbtmc_read(struct file *filp, char __user *buf, if (n_characters > this_part) { dev_err(dev, "Device wants to return more data than requested: %u > %zu\n", n_characters, count); - if (data->auto_abort) + if (file_data->auto_abort) usbtmc_ioctl_abort_bulk_in(data); goto exit; } @@ -1650,7 +1652,7 @@ static ssize_t usbtmc_write(struct file *filp, const char __user *buf, if (retval < 0) { dev_err(&data->intf->dev, "Unable to send data, error %d\n", retval); - if (data->auto_abort) + if (file_data->auto_abort) usbtmc_ioctl_abort_bulk_out(data); goto exit; } @@ -2219,6 +2221,7 @@ static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg) struct usbtmc_file_data *file_data; struct usbtmc_device_data *data; int retval = -EBADRQC; + __u8 tmp_byte; file_data = file->private_data; data = file_data->data; @@ -2355,6 +2358,12 @@ static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg) (__u8 __user
[PATCH v2 03/29] usb: usbtmc: Add ioctls to set/get usb timeout
Add ioctls USBTMC_IOCTL_GET_TIMEOUT / USBTMC_IOCTL_SET_TIMEOUT to get/set I/O timeout for specific file handle. Different operations on an instrument can take different lengths of time thus it is important to be able to set the timeout slightly longer than the expected duration of each operation to optimise the responsiveness of the application. As the instrument may be shared by multiple applications the timeout should be settable on a per file descriptor basis. Tested-by: Dave Penkler Reviewed-by: Steve Bayless Signed-off-by: Dave Penkler Signed-off-by: Guido Kiener --- drivers/usb/class/usbtmc.c | 65 include/uapi/linux/usb/tmc.h | 4 +++ 2 files changed, 63 insertions(+), 6 deletions(-) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index 243e8446b8dd..36d740c4c6fb 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -30,6 +30,8 @@ */ #define USBTMC_SIZE_IOBUFFER 2048 +/* Minimum USB timeout (in milliseconds) */ +#define USBTMC_MIN_TIMEOUT 100 /* Default USB timeout (in milliseconds) */ #define USBTMC_TIMEOUT 5000 @@ -115,6 +117,7 @@ struct usbtmc_file_data { struct usbtmc_device_data *data; struct list_head file_elem; + u32timeout; u8 srq_byte; atomic_t srq_asserted; }; @@ -153,6 +156,8 @@ static int usbtmc_open(struct inode *inode, struct file *filp) mutex_lock(&data->io_mutex); file_data->data = data; + file_data->timeout = USBTMC_TIMEOUT; + INIT_LIST_HEAD(&file_data->file_elem); spin_lock_irq(&data->dev_lock); list_add_tail(&file_data->file_elem, &data->file_list); @@ -460,7 +465,7 @@ static int usbtmc488_ioctl_read_stb(struct usbtmc_file_data *file_data, rv = wait_event_interruptible_timeout( data->waitq, atomic_read(&data->iin_data_valid) != 0, - USBTMC_TIMEOUT); + file_data->timeout); if (rv < 0) { dev_dbg(dev, "wait interrupted %d\n", rv); goto exit; @@ -560,8 +565,10 @@ static int usbtmc488_ioctl_simple(struct usbtmc_device_data *data, * * Also updates bTag_last_write. */ -static int send_request_dev_dep_msg_in(struct usbtmc_device_data *data, size_t transfer_size) +static int send_request_dev_dep_msg_in(struct usbtmc_file_data *file_data, + size_t transfer_size) { + struct usbtmc_device_data *data = file_data->data; int retval; u8 *buffer; int actual; @@ -590,7 +597,8 @@ static int send_request_dev_dep_msg_in(struct usbtmc_device_data *data, size_t t retval = usb_bulk_msg(data->usb_dev, usb_sndbulkpipe(data->usb_dev, data->bulk_out), - buffer, USBTMC_HEADER_SIZE, &actual, USBTMC_TIMEOUT); + buffer, USBTMC_HEADER_SIZE, + &actual, file_data->timeout); /* Store bTag (in case we need to abort) */ data->bTag_last_write = data->bTag; @@ -640,7 +648,7 @@ static ssize_t usbtmc_read(struct file *filp, char __user *buf, dev_dbg(dev, "usb_bulk_msg_in: count(%zu)\n", count); - retval = send_request_dev_dep_msg_in(data, count); + retval = send_request_dev_dep_msg_in(file_data, count); if (retval < 0) { if (data->auto_abort) @@ -659,7 +667,7 @@ static ssize_t usbtmc_read(struct file *filp, char __user *buf, usb_rcvbulkpipe(data->usb_dev, data->bulk_in), buffer, USBTMC_SIZE_IOBUFFER, &actual, - USBTMC_TIMEOUT); + file_data->timeout); dev_dbg(dev, "usb_bulk_msg: retval(%u), done(%zu), remaining(%zu), actual(%d)\n", retval, done, remaining, actual); @@ -832,7 +840,7 @@ static ssize_t usbtmc_write(struct file *filp, const char __user *buf, usb_sndbulkpipe(data->usb_dev, data->bulk_out), buffer, n_bytes, - &actual, USBTMC_TIMEOUT); + &actual, file_data->timeout); if (retval != 0) break; n_bytes -= actual; @@ -1189,6 +1197,41 @@ static int usbtmc_ioctl_indicator_pulse(struct usbtmc_d
[PATCH v2 22/29] usb: usbtmc: Fix ioctl USBTMC_IOCTL_ABORT_BULK_OUT
Add parameter 'tag' to function usbtmc_ioctl_abort_bulk_out_tag() for future versions. Use USBTMC_BUFSIZE (4k) instead of USBTMC_SIZE_IOBUFFER (2k). Using USBTMC_SIZE_IOBUFFER is deprecated. Insert a sleep of 50 ms between subsequent CHECK_ABORT_BULK_OUT_STATUS control requests to avoid stressing the instrument with repeated requests. Use common macro USB_CTRL_GET_TIMEOUT instead of USBTMC_TIMEOUT. Signed-off-by: Guido Kiener Reviewed-by: Steve Bayless --- drivers/usb/class/usbtmc.c | 16 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index a516b0fe37a3..a973ce2898e1 100644 --- a/drivers/usb/class/usbtmc.c +++ b/drivers/usb/class/usbtmc.c @@ -418,7 +418,8 @@ static int usbtmc_ioctl_abort_bulk_in(struct usbtmc_device_data *data) return usbtmc_ioctl_abort_bulk_in_tag(data, data->bTag_last_read); } -static int usbtmc_ioctl_abort_bulk_out(struct usbtmc_device_data *data) +static int usbtmc_ioctl_abort_bulk_out_tag(struct usbtmc_device_data *data, + u8 tag) { struct device *dev; u8 *buffer; @@ -435,8 +436,8 @@ static int usbtmc_ioctl_abort_bulk_out(struct usbtmc_device_data *data) usb_rcvctrlpipe(data->usb_dev, 0), USBTMC_REQUEST_INITIATE_ABORT_BULK_OUT, USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_ENDPOINT, -data->bTag_last_write, data->bulk_out, -buffer, 2, USBTMC_TIMEOUT); +tag, data->bulk_out, +buffer, 2, USB_CTRL_GET_TIMEOUT); if (rv < 0) { dev_err(dev, "usb_control_msg returned %d\n", rv); @@ -455,12 +456,14 @@ static int usbtmc_ioctl_abort_bulk_out(struct usbtmc_device_data *data) n = 0; usbtmc_abort_bulk_out_check_status: + /* do not stress device with subsequent requests */ + msleep(50); rv = usb_control_msg(data->usb_dev, usb_rcvctrlpipe(data->usb_dev, 0), USBTMC_REQUEST_CHECK_ABORT_BULK_OUT_STATUS, USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_ENDPOINT, 0, data->bulk_out, buffer, 0x08, -USBTMC_TIMEOUT); +USB_CTRL_GET_TIMEOUT); n++; if (rv < 0) { dev_err(dev, "usb_control_msg returned %d\n", rv); @@ -494,6 +497,11 @@ static int usbtmc_ioctl_abort_bulk_out(struct usbtmc_device_data *data) return rv; } +static int usbtmc_ioctl_abort_bulk_out(struct usbtmc_device_data *data) +{ + return usbtmc_ioctl_abort_bulk_out_tag(data, data->bTag_last_write); +} + static int usbtmc488_ioctl_read_stb(struct usbtmc_file_data *file_data, void __user *arg) { -- 2.17.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html