diff mbox

[v2,01/29] usb: usbtmc: Support Read Status Byte with SRQ per file

Message ID 20180718084602.10568-2-guido@kiener-muenchen.de (mailing list archive)
State New, archived
Headers show

Commit Message

Guido Kiener July 18, 2018, 8:45 a.m. UTC 
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 since EPOLLIN is now reserved
for asynchronous reads

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 | 136 ++++++++++++++++++++++++++++---------
 1 file changed, 105 insertions(+), 31 deletions(-)

Comments

Greg Kroah-Hartman July 21, 2018, 6:17 a.m. UTC | #1 
On Wed, Jul 18, 2018 at 10:45:34AM +0200, Guido Kiener wrote:
>  		}
> -		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",
> @@ -1295,6 +1369,7 @@ static void usbtmc_interrupt(struct urb *urb)
>  	case -ESHUTDOWN:
>  	case -EILSEQ:
>  	case -ETIME:
> +	case -EPIPE:
>  		/* urb terminated, clean up */
>  		dev_dbg(dev, "urb terminated, status: %d\n", status);
>  		return;

Minor nit, these two changes didn't have anything to do with the larger
patch.  I'll let them slide for now, but please be more careful in the
future.

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
diff mbox

Patch

diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 529295a17579..db58b84d43ee 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,7 +88,6 @@  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;
@@ -104,9 +104,21 @@  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;
+};
+
 /* Forward declarations */
 static struct usb_driver usbtmc_driver;
 
@@ -122,7 +134,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 +142,45 @@  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;
+
 	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;
+
+	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);
+	/* 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 +405,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 +419,25 @@  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);
+		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,
@@ -435,9 +483,8 @@  static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data,
 		stb = buffer[2];
 	}
 
-	rv = copy_to_user(arg, &stb, sizeof(stb));
-	if (rv)
-		rv = -EFAULT;
+	rv = put_user(stb, (__u8 __user *)arg);
+	dev_dbg(dev, "stb:0x%02x received %d\n", (unsigned int)stb, rv);
 
  exit:
 	/* bump interrupt bTag */
@@ -565,6 +612,7 @@  static int send_request_dev_dep_msg_in(struct usbtmc_device_data *data, size_t t
 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 +624,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);
@@ -721,6 +770,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 +780,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)
@@ -1140,10 +1191,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 +1238,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 +1265,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 +1285,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 +1332,33 @@  static void usbtmc_interrupt(struct urb *urb)
 		}
 		/* check for SRQ notification */
 		if (data->iin_buffer[0] == 0x81) {
+			unsigned long flags;
+			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_irqsave(&data->dev_lock, flags);
+			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_irqrestore(&data->dev_lock, flags);
+
+			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",
@@ -1295,6 +1369,7 @@  static void usbtmc_interrupt(struct urb *urb)
 	case -ESHUTDOWN:
 	case -EILSEQ:
 	case -ETIME:
+	case -EPIPE:
 		/* urb terminated, clean up */
 		dev_dbg(dev, "urb terminated, status: %d\n", status);
 		return;
@@ -1339,7 +1414,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 +1519,14 @@  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");
-
-	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);