usb: usbtmc: Support Read Status Byte with SRQ per file handle
Greg Kroah-Hartman
maintainerPATCH 2/12 usb: usbtmc: Support Read Status Byte with SRQ per file handle
Quoted 59 lines · Guido Kiener
- Add 'struct
usbtmc_file_data' for each file handle to cache lastsrq_byte(=Status Byte with SRQ) received byusbtmc_interrupt(..)-
usbtmc488_ioctl_read_stbreturns cachedsrq_bytewhen 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_abortin 'structusbtmc_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 <[email protected]>Reviewed-by Steve Bayless <[email protected]>Signed-off-by Guido Kiener <[email protected]>--- drivers/usb/class/usbtmc.c | 176 ++++++++++++++++++++++++++++--------- 1 file changed, 136 insertions(+), 40 deletions(-)drivers/usb/class/usbtmc.c +6
-1@@ -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 {int iin_interval;struct urb *iin_urb;u16 iin_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. +*/
drivers/usb/class/usbtmc.c struct usbtmc_device_data { · line 124
Quoted 10 lines
drivers/usb/class/usbtmc.c +10
-0+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?
And why duplicate these fields as they are already in the device-specific data structure?
Quoted 24 lines
drivers/usb/class/usbtmc.c +5
-1+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) {
drivers/usb/class/usbtmc.c static int usbtmc_open(struct inode *inode, struct file *filp) · line 156
Quoted 8 lines
drivers/usb/class/usbtmc.c +5
-0@@ -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.
Quoted 24 lines
drivers/usb/class/usbtmc.c +16
-2+ 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;
drivers/usb/class/usbtmc.c static int usbtmc_open(struct inode *inode, struct file *filp) · line 186
Quoted 10 lines
drivers/usb/class/usbtmc.c +3
-2+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.
drivers/usb/class/usbtmc.c static int usbtmc_open(struct inode *inode, struct file *filp) · line 195
Quoted 9 lines
drivers/usb/class/usbtmc.c +9
-0+ +/* 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?
drivers/usb/class/usbtmc.c static int usbtmc_open(struct inode *inode, struct file *filp) · line 197
Quoted 2 lines
drivers/usb/class/usbtmc.c +2
-0+ +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.
Quoted 20 lines
drivers/usb/class/usbtmc.c +5
-1+ file_data->data =NULL ; +kfree (file_data);return 0 ; }@@ -369,10 +421,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;
drivers/usb/class/usbtmc.c static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data, · line 439
Quoted 5 lines
drivers/usb/class/usbtmc.c +2
-0@@ -381,15 +435,27 @@ 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);
That really frightens me. Why are you messing with atomic values here? What is it supposed to be "protecting" or "doing"?
thanks,
greg k-h
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html