Zitat von Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>:
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@xxxxxxxxx>
Reviewed-by: Steve Bayless <steve_bayless@xxxxxxxxxxxx>
Signed-off-by: Guido Kiener <guido.kiener@xxxxxxxxxxxxxxxxx>
---
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
