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

Zitat von Greg KH <[email protected]>:

Quoted 82 lines · On Fri, May 18, 2018 at 11:52:36AM +0000, [email protected] wrote:

Quoted 73 lines

Zitat von Greg KH <[email protected]>:

Quoted 44 lines · Guido Kiener

Quoted 43 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 +1 -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;
 

Quoted 12 lines

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

Quoted 12 lines

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

 	/* 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 12 lines

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

Quoted 12 lines

drivers/usb/class/usbtmc.c +11 -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 */

Quoted 7 lines

Quoted 5 lines

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

Quoted 2 lines

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

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

Quoted 8 lines

Wow, 2008, I can't remember what code I wrote last week, let alone a decade ago :)

Quoted 1 line

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.

Quoted 9 lines

Quoted 8 lines

Quoted 3 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 4 lines

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.

Quoted 13 lines

Quoted 12 lines

Quoted 6 lines

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

Quoted 3 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.

Quoted 3 lines

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?