| Message ID | 20180724090550.29201-3-guido@kiener-muenchen.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: usbtmc: Changes needed for compatible IVI/VISA library | expand |
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 <guido.kiener@rohde-schwarz.com> > Reviewed-by: Steve Bayless <steve_bayless@keysight.com> > --- > 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 <gregkh@suse.de> > + * 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. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 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? 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 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 <guido.kiener@rohde-schwarz.com> >> Reviewed-by: Steve Bayless <steve_bayless@keysight.com> >> --- >> 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 <gregkh@suse.de> >> + * 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Zitat von Greg KH <gregkh@linuxfoundation.org>: > 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 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 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 <gregkh@suse.de> + * 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 */ + static void usbtmc_delete(struct kref *kref) { struct usbtmc_device_data *data = to_usbtmc_data(kref); @@ -1250,6 +1269,67 @@ 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(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, u64_to_uptr(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(u64_to_uptr(request.data), buffer, rv); + if (res) + rv = -EFAULT; + } + + exit: + kfree(buffer); + return rv; +} + /* * Get the usb timeout value */ @@ -1366,6 +1446,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 729af2f861a4..95533118ac34 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 <gregkh@suse.de> * Copyright (C) 2015 Dave Penkler <dpenkler@gmail.com> + * 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_LOCKOUT 162 +struct usbtmc_request { + __u8 bRequestType; + __u8 bRequest; + __u16 wValue; + __u16 wIndex; + __u16 wLength; +} __attribute__ ((packed)); + +struct usbtmc_ctrlrequest { + struct usbtmc_request req; + __u64 data; /* pointer to user space */ +} __attribute__ ((packed)); + struct usbtmc_termchar { __u8 term_char; __u8 term_char_enabled; @@ -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, __u32) #define USBTMC_IOCTL_SET_TIMEOUT _IOW(USBTMC_IOC_NR, 10, __u32) #define USBTMC_IOCTL_EOM_ENABLE _IOW(USBTMC_IOC_NR, 11, __u8)