Skip to contentSkip to conversation

usb: usbtmc: Support Read Status Byte with SRQ per file handle

PATCH New Greg Kroah-Hartman 1 participant 1 message May 17, 2018 USB

lore
Share & tools
lore

Greg Kroah-Hartman

author maintainer
#0 lore raw

PATCH 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 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

---
 drivers/usb/class/usbtmc.c | 176 ++++++++++++++++++++++++++++---------
 1 file changed, 136 insertions(+), 40 deletions(-)
drivers/usb/class/usbtmc.c +6 -1
index 529295a17579..5754354429d8 100644
@@ -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