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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Fri, May 18, 2018 at 12:36:56PM +0000, guido@kiener-muenchen.de wrote:
> 
> Zitat von Greg KH <gregkh@linuxfoundation.org>:
> 
> > On Fri, May 18, 2018 at 11:52:36AM +0000, guido@kiener-muenchen.de wrote:
> > > 
> > > 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.
> > 
> > Wow, 2008, I can't remember what code I wrote last week, let alone a
> > decade ago :)
> > 
> > > 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.
> 
> > 
> > > > 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.
> > 
> > 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.
> 
> > 
> > > > > +	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.
> > 
> > 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.

Ah, I missed that this was already an atomic variable, sorry about that.
Keeping it as-is is fine for now, my fault.

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

Each patch should not break the build and be "obviously correct".  As we
don't have a way to do any stress testing of this driver in automated
build systems, don't worry about doing it yourself, just test the full
set together for functional working stability.

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