| Message ID | 20180517170336.8426-4-guido@kiener-muenchen.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <dpenkler@gmail.com> > Reviewed-by: Steve Bayless <steve_bayless@keysight.com> > Signed-off-by: Dave Penkler <dpenkler@gmail.com> > Signed-off-by: Guido Kiener <guido.kiener@rohde-schwarz.com> > --- > 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; > > + u32 timeout; > 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_ioctl(struct file *file, unsigned int cmd, unsigned long arg) > { > struct usbtmc_file_data *file_data; > @@ -1251,6 +1291,16 @@ static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg) > retval = usbtmc_ioctl_abort_bulk_in(data); > break; > > + case USBTMC_IOCTL_GET_TIMEOUT: > + retval = usbtmc_ioctl_get_timeout(file_data, > + (void __user *)arg); > + break; > + > + case USBTMC_IOCTL_SET_TIMEOUT: > + retval = usbtmc_ioctl_set_timeout(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 03f6adc8f35b..d6de192150f4 100644 > --- a/include/uapi/linux/usb/tmc.h > +++ b/include/uapi/linux/usb/tmc.h > @@ -46,6 +46,8 @@ > #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_GET_TIMEOUT _IOR(USBTMC_IOC_NR, 9, unsigned int) > +#define USBTMC_IOCTL_SET_TIMEOUT _IOW(USBTMC_IOC_NR, 10, unsigned int) What is the "size" of unsigned int in 32 vs. 64 bit kernels? Shouldn't this be __u32? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Zitat von Greg KH <gregkh@linuxfoundation.org>: > On Thu, May 17, 2018 at 07:03: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 <dpenkler@gmail.com> >> Reviewed-by: Steve Bayless <steve_bayless@keysight.com> >> Signed-off-by: Dave Penkler <dpenkler@gmail.com> >> Signed-off-by: Guido Kiener <guido.kiener@rohde-schwarz.com> >> --- >> 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; >> >> + u32 timeout; >> 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_ioctl(struct file *file, unsigned int cmd, >> unsigned long arg) >> { >> struct usbtmc_file_data *file_data; >> @@ -1251,6 +1291,16 @@ static long usbtmc_ioctl(struct file *file, >> unsigned int cmd, unsigned long arg) >> retval = usbtmc_ioctl_abort_bulk_in(data); >> break; >> >> + case USBTMC_IOCTL_GET_TIMEOUT: >> + retval = usbtmc_ioctl_get_timeout(file_data, >> + (void __user *)arg); >> + break; >> + >> + case USBTMC_IOCTL_SET_TIMEOUT: >> + retval = usbtmc_ioctl_set_timeout(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 03f6adc8f35b..d6de192150f4 100644 >> --- a/include/uapi/linux/usb/tmc.h >> +++ b/include/uapi/linux/usb/tmc.h >> @@ -46,6 +46,8 @@ >> #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_GET_TIMEOUT _IOR(USBTMC_IOC_NR, 9, unsigned int) >> +#define USBTMC_IOCTL_SET_TIMEOUT _IOW(USBTMC_IOC_NR, 10, unsigned int) > > What is the "size" of unsigned int in 32 vs. 64 bit kernels? Shouldn't > this be __u32? Thanks. Of course, same problem. We make __u32. > > thanks, > > greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 17, 2018 at 8:03 PM, Guido Kiener <guido@kiener-muenchen.de> 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? > + > + 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; > +}
Zitat von Andy Shevchenko <andy.shevchenko@gmail.com>: > On Thu, May 17, 2018 at 8:03 PM, Guido Kiener > <guido@kiener-muenchen.de> 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 18, 2018 at 10:48:30PM +0300, Andy Shevchenko wrote: > On Thu, May 17, 2018 at 8:03 PM, Guido Kiener <guido@kiener-muenchen.de> 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? Yes, on the kernel side that is the correct type to use after you copy it over from userspace. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c index 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; + u32 timeout; 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_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { struct usbtmc_file_data *file_data; @@ -1251,6 +1291,16 @@ static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg) retval = usbtmc_ioctl_abort_bulk_in(data); break; + case USBTMC_IOCTL_GET_TIMEOUT: + retval = usbtmc_ioctl_get_timeout(file_data, + (void __user *)arg); + break; + + case USBTMC_IOCTL_SET_TIMEOUT: + retval = usbtmc_ioctl_set_timeout(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 03f6adc8f35b..d6de192150f4 100644 --- a/include/uapi/linux/usb/tmc.h +++ b/include/uapi/linux/usb/tmc.h @@ -46,6 +46,8 @@ #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_GET_TIMEOUT _IOR(USBTMC_IOC_NR, 9, unsigned int) +#define USBTMC_IOCTL_SET_TIMEOUT _IOW(USBTMC_IOC_NR, 10, unsigned int) #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)