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

Zitat von Greg KH <[email protected]>:

Quoted 57 lines · Guido Kiener

Quoted 56 lines

- 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 <[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 +3 -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)
 

Quoted 16 lines

drivers/usb/class/usbtmc.c struct usbtmc_device_data { · line 124

Quoted 13 lines

drivers/usb/class/usbtmc.c +13 -0

+/*
+ * 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.

Quoted 2 lines

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.

Quoted 5 lines

drivers/usb/class/usbtmc.c struct usbtmc_device_data { · line 126

Quoted 2 lines

drivers/usb/class/usbtmc.c +2 -0

+	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.