| Message ID | 20180517170336.8426-3-guido@kiener-muenchen.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <dpenkler@gmail.com> > Reviewed-by: Steve Bayless <steve_bayless@keysight.com> > Signed-off-by: Guido Kiener <guido.kiener@rohde-schwarz.com> > --- > 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 { > 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. > + */ > +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? > + 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. > + > 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. > + 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; > @@ -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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/17/2018 09:20 AM, Greg KH wrote: >> + 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. but pr_debug() works great with DYNAMIC_DEBUG. Someone does not need to go all the way to tracing to get decent debug capability.
On Thu, May 17, 2018 at 09:44:40AM -0700, Randy Dunlap wrote: > On 05/17/2018 09:20 AM, Greg KH wrote: > >> + 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. > > > but pr_debug() works great with DYNAMIC_DEBUG. Sure, but don't do that for "I made it to this function!" type calls. We have been ripping them out for a long time now, don't add new ones please. > Someone does not need to go all the way to tracing to get decent > debug capability. ftrace is not hard to use, it's even easier than dynamic_debug at times :) 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: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 <dpenkler@gmail.com> >> Reviewed-by: Steve Bayless <steve_bayless@keysight.com> >> Signed-off-by: Guido Kiener <guido.kiener@rohde-schwarz.com> >> --- >> 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 { >> 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. >> + */ >> +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(&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. I agree. > >> + >> + /* 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 will need more time to think about the race here. > > > >> + 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; >> @@ -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"? file_data->srq_asserted is of type atomic_t. In this patch we could also use the simple type int. However in patch 07/12 we need an atomic_read(&file_data->srq_asserted) != 0 in usbtmc488_ioctl_wait_srq. I assumed that we should use the atomic_* functions when using atomic_t. > > thanks, > > greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 18, 2018 at 11:52:36AM +0000, guido@kiener-muenchen.de wrote: > > Zitat von Greg KH <gregkh@linuxfoundation.org>: > > > 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 <dpenkler@gmail.com> > > > Reviewed-by: Steve Bayless <steve_bayless@keysight.com> > > > Signed-off-by: Guido Kiener <guido.kiener@rohde-schwarz.com> > > > --- > > > 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 { > > > 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. > > > + */ > > > +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? > > 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. > > > + 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"? > > file_data->srq_asserted is of type atomic_t. In this patch we could also use > the simple type int. However in patch 07/12 we need an > atomic_read(&file_data->srq_asserted) != 0 in usbtmc488_ioctl_wait_srq. > > I assumed that we should use the atomic_* functions when using atomic_t. Yes, but why is srq_asserted an atomic value in the first place? That's the larger question here, that feels very odd to me. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Zitat von Greg KH <gregkh@linuxfoundation.org>: > On Fri, May 18, 2018 at 11:52:36AM +0000, guido@kiener-muenchen.de wrote: >> >> Zitat von Greg KH <gregkh@linuxfoundation.org>: >> >> > 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 <dpenkler@gmail.com> >> > > Reviewed-by: Steve Bayless <steve_bayless@keysight.com> >> > > Signed-off-by: Guido Kiener <guido.kiener@rohde-schwarz.com> >> > > --- >> > > 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 { >> > > 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. >> > > + */ >> > > +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 "doing"? >> >> file_data->srq_asserted is of type atomic_t. In this patch we could also use >> the simple type int. However in patch 07/12 we need an >> atomic_read(&file_data->srq_asserted) != 0 in usbtmc488_ioctl_wait_srq. >> >> I assumed that we should use the atomic_* functions when using atomic_t. > > Yes, but why is srq_asserted an atomic value in the first place? > > That's the larger question here, that feels very odd to me. I'm not sure if I understand you right here. Do you want me to change the current definition atomic_t srq_asserted to int srq_asserted and back to atomic_t srq_asserted with patch 07/12. To be honest, I started with one "big" patch until Dave told that I have to breakdown the "big" patch in a sequence. That was a hard weekend for me (as a newbie) :-). Do you need a guarantee that each single patch survives a 24 hour stress test or is it (hopefully) enough that all patches together are working stable? > > thanks, > > greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 18, 2018 at 12:36:56PM +0000, guido@kiener-muenchen.de wrote: > > Zitat von Greg KH <gregkh@linuxfoundation.org>: > > > On Fri, May 18, 2018 at 11:52:36AM +0000, guido@kiener-muenchen.de wrote: > > > > > > Zitat von Greg KH <gregkh@linuxfoundation.org>: > > > > > > > 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 <dpenkler@gmail.com> > > > > > Reviewed-by: Steve Bayless <steve_bayless@keysight.com> > > > > > Signed-off-by: Guido Kiener <guido.kiener@rohde-schwarz.com> > > > > > --- > > > > > 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 { > > > > > 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. > > > > > + */ > > > > > +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 "doing"? > > > > > > file_data->srq_asserted is of type atomic_t. In this patch we could also use > > > the simple type int. However in patch 07/12 we need an > > > atomic_read(&file_data->srq_asserted) != 0 in usbtmc488_ioctl_wait_srq. > > > > > > I assumed that we should use the atomic_* functions when using atomic_t. > > > > Yes, but why is srq_asserted an atomic value in the first place? > > > > That's the larger question here, that feels very odd to me. > > I'm not sure if I understand you right here. Do you want me to change the > current definition atomic_t srq_asserted to int srq_asserted and back to > atomic_t srq_asserted with patch 07/12. Ah, I missed that this was already an atomic variable, sorry about that. Keeping it as-is is fine for now, my fault. > To be honest, I started with one "big" patch until Dave told that I have to > breakdown the "big" patch in a sequence. That was a hard weekend for me > (as a newbie) :-). Do you need a guarantee that each single patch survives > a 24 hour stress test or is it (hopefully) enough that all patches together > are working stable? Each patch should not break the build and be "obviously correct". As we don't have a way to do any stress testing of this driver in automated build systems, don't worry about doing it yourself, just test the full set together for functional working stability. 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>: >> @@ -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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Montag, den 21.05.2018, 21:00 +0000 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. > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Zitat von Oliver Neukum <oneukum@suse.com>: > Am Montag, den 21.05.2018, 21:00 +0000 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Donnerstag, den 24.05.2018, 12:31 +0000 schrieb guido@kiener- muenchen.de: > Zitat von Oliver Neukum <oneukum@suse.com>: > > > Am Montag, den 21.05.2018, 21:00 +0000 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. Ok, I remember. You are safe, if and only if you share the USB minor number space. Regards Oliver -- 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 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 { 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. + */ +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_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 +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; @@ -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); + if (srq_asserted) { + /* a STB with SRQ is already received */ + stb = file_data->srq_byte; + spin_unlock_irq(&data->dev_lock); + 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); + buffer = kmalloc(8, GFP_KERNEL); if (!buffer) return -ENOMEM; atomic_set(&data->iin_data_valid, 0); - /* must issue read_stb before using poll or select */ - atomic_set(&data->srq_asserted, 0); - rv = usb_control_msg(data->usb_dev, usb_rcvctrlpipe(data->usb_dev, 0), USBTMC488_REQUEST_READ_STATUS_BYTE, @@ -420,7 +486,7 @@ static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data, if (rv == 0) { dev_dbg(dev, "wait timed out\n"); - rv = -ETIME; + rv = -ETIMEDOUT; goto exit; } @@ -435,9 +501,11 @@ static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data, stb = buffer[2]; } - rv = copy_to_user(arg, &stb, sizeof(stb)); - if (rv) + if (put_user(stb, (__u8 __user *)arg)) rv = -EFAULT; + else + rv = 0; + dev_dbg(dev, "stb:0x%02x received %d\n", (unsigned int)stb, rv); exit: /* bump interrupt bTag */ @@ -513,8 +581,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; @@ -533,9 +603,9 @@ static int send_request_dev_dep_msg_in(struct usbtmc_device_data *data, size_t t buffer[5] = transfer_size >> 8; buffer[6] = transfer_size >> 16; buffer[7] = transfer_size >> 24; - buffer[8] = data->TermCharEnabled * 2; + buffer[8] = file_data->TermCharEnabled * 2; /* Use term character? */ - buffer[9] = data->TermChar; + buffer[9] = file_data->TermChar; buffer[10] = 0; /* Reserved */ buffer[11] = 0; /* Reserved */ @@ -554,17 +624,17 @@ static int send_request_dev_dep_msg_in(struct usbtmc_device_data *data, size_t t 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, size_t count, loff_t *f_pos) { + struct usbtmc_file_data *file_data; struct usbtmc_device_data *data; struct device *dev; u32 n_characters; @@ -576,7 +646,8 @@ static ssize_t usbtmc_read(struct file *filp, char __user *buf, size_t this_part; /* Get pointer to private data structure */ - data = filp->private_data; + file_data = filp->private_data; + data = file_data->data; dev = &data->intf->dev; buffer = kmalloc(USBTMC_SIZE_IOBUFFER, GFP_KERNEL); @@ -591,7 +662,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) @@ -721,6 +792,7 @@ static ssize_t usbtmc_read(struct file *filp, char __user *buf, static ssize_t usbtmc_write(struct file *filp, const char __user *buf, size_t count, loff_t *f_pos) { + struct usbtmc_file_data *file_data; struct usbtmc_device_data *data; u8 *buffer; int retval; @@ -730,7 +802,8 @@ static ssize_t usbtmc_write(struct file *filp, const char __user *buf, int done; int this_part; - data = filp->private_data; + file_data = filp->private_data; + data = file_data->data; buffer = kmalloc(USBTMC_SIZE_IOBUFFER, GFP_KERNEL); if (!buffer) @@ -1074,7 +1147,7 @@ static ssize_t name##_store(struct device *dev, \ struct usb_interface *intf = to_usb_interface(dev); \ struct usbtmc_device_data *data = usb_get_intfdata(intf); \ ssize_t result; \ - unsigned val; \ + unsigned int val; \ \ result = sscanf(buf, "%u\n", &val); \ if (result != 1) \ @@ -1140,10 +1213,13 @@ static int usbtmc_ioctl_indicator_pulse(struct usbtmc_device_data *data) 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; - data = file->private_data; + file_data = file->private_data; + data = file_data->data; + mutex_lock(&data->io_mutex); if (data->zombie) { retval = -ENODEV; @@ -1184,7 +1260,8 @@ static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg) break; case USBTMC488_IOCTL_READ_STB: - retval = usbtmc488_ioctl_read_stb(data, (void __user *)arg); + retval = usbtmc488_ioctl_read_stb(file_data, + (void __user *)arg); break; case USBTMC488_IOCTL_REN_CONTROL: @@ -1210,14 +1287,15 @@ static long usbtmc_ioctl(struct file *file, unsigned int cmd, unsigned long arg) static int usbtmc_fasync(int fd, struct file *file, int on) { - struct usbtmc_device_data *data = file->private_data; + struct usbtmc_file_data *file_data = file->private_data; - return fasync_helper(fd, file, on, &data->fasync); + return fasync_helper(fd, file, on, &file_data->data->fasync); } static __poll_t usbtmc_poll(struct file *file, poll_table *wait) { - struct usbtmc_device_data *data = file->private_data; + struct usbtmc_file_data *file_data = file->private_data; + struct usbtmc_device_data *data = file_data->data; __poll_t mask; mutex_lock(&data->io_mutex); @@ -1229,7 +1307,7 @@ static __poll_t usbtmc_poll(struct file *file, poll_table *wait) poll_wait(file, &data->waitq, wait); - mask = (atomic_read(&data->srq_asserted)) ? EPOLLIN | EPOLLRDNORM : 0; + mask = (atomic_read(&file_data->srq_asserted)) ? EPOLLPRI : 0; no_poll: mutex_unlock(&data->io_mutex); @@ -1276,15 +1354,32 @@ static void usbtmc_interrupt(struct urb *urb) } /* check for SRQ notification */ if (data->iin_buffer[0] == 0x81) { + struct list_head *elem; + if (data->fasync) kill_fasync(&data->fasync, - SIGIO, POLL_IN); + SIGIO, POLL_PRI); - atomic_set(&data->srq_asserted, 1); - wake_up_interruptible(&data->waitq); + spin_lock(&data->dev_lock); + list_for_each(elem, &data->file_list) { + struct usbtmc_file_data *file_data; + + file_data = list_entry(elem, + struct usbtmc_file_data, + file_elem); + file_data->srq_byte = data->iin_buffer[1]; + atomic_set(&file_data->srq_asserted, 1); + } + spin_unlock(&data->dev_lock); + + dev_dbg(dev, "srq received bTag %x stb %x\n", + (unsigned int)data->iin_buffer[0], + (unsigned int)data->iin_buffer[1]); + wake_up_interruptible_all(&data->waitq); goto exit; } - dev_warn(dev, "invalid notification: %x\n", data->iin_buffer[0]); + dev_warn(dev, "invalid notification: %x\n", + data->iin_buffer[0]); break; case -EOVERFLOW: dev_err(dev, "overflow with length %d, actual length is %d\n", @@ -1339,7 +1434,9 @@ static int usbtmc_probe(struct usb_interface *intf, mutex_init(&data->io_mutex); init_waitqueue_head(&data->waitq); atomic_set(&data->iin_data_valid, 0); - atomic_set(&data->srq_asserted, 0); + INIT_LIST_HEAD(&data->file_list); + spin_lock_init(&data->dev_lock); + data->zombie = 0; /* Initialize USBTMC bTag and other fields */ @@ -1442,17 +1539,16 @@ static int usbtmc_probe(struct usb_interface *intf, static void usbtmc_disconnect(struct usb_interface *intf) { - struct usbtmc_device_data *data; + struct usbtmc_device_data *data = usb_get_intfdata(intf); - dev_dbg(&intf->dev, "usbtmc_disconnect called\n"); + dev_dbg(&intf->dev, "%s - called\n", __func__); - data = usb_get_intfdata(intf); usb_deregister_dev(intf, &usbtmc_class); sysfs_remove_group(&intf->dev.kobj, &capability_attr_grp); sysfs_remove_group(&intf->dev.kobj, &data_attr_grp); mutex_lock(&data->io_mutex); data->zombie = 1; - wake_up_all(&data->waitq); + wake_up_interruptible_all(&data->waitq); mutex_unlock(&data->io_mutex); usbtmc_free_int(data); kref_put(&data->kref, usbtmc_delete);