[02/12] usb: usbtmc: Support Read Status Byte with SRQ per file handle

[Guido Kiener <guido@kiener-muenchen.de>]

Zitat von Greg KH <gregkh@linuxfoundation.org>:

> 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@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 | 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html