Re: [PATCH 07/12] usb: usbtmc: Add ioctl USBTMC488_IOCTL_WAIT_SRQ

[Oliver Neukum]

Am Montag, den 28.05.2018, 10:59 + schrieb guido@kiener-
muenchen.de:
> > No, the problem is that you will underflow io->mutex
> > 
> 
> Don't worry. The function usbtmc488_ioctl_wait_srq is called by
> usbtmc_ioctl which already locks the mutex. See
> https://elixir.bootlin.com/linux/v4.17-rc7/source/drivers/usb/class/usbtmc.c#L1189
> So the mutex is just unlocked here to allow other threads to still communicate
> with the device.

Hi,

thanks I had overlooked that.

Regards
Oliver

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/12] usb: usbtmc: Add ioctl USBTMC488_IOCTL_WAIT_SRQ

[guido]

Zitat von Oliver Neukum :


Am Donnerstag, den 24.05.2018, 12:59 + schrieb guido@kiener-
muenchen.de:

Zitat von Oliver Neukum :

> Am Donnerstag, den 17.05.2018, 19:03 +0200 schrieb Guido Kiener:
> > +static int usbtmc488_ioctl_wait_srq(struct usbtmc_file_data *file_data,
> > +   unsigned int __user *arg)
> > +{
> > +   struct usbtmc_device_data *data = file_data->data;
> > +   struct device *dev = >intf->dev;
> > +   int rv;
> > +   unsigned int timeout;
> > +   unsigned long expire;
> > +
> > +   if (!data->iin_ep_present) {
> > +   dev_dbg(dev, "no interrupt endpoint present\n");
> > +   return -EFAULT;
> > +   }
> > +
> > +   if (get_user(timeout, arg))
> > +   return -EFAULT;
> > +
> > +   expire = msecs_to_jiffies(timeout);
> > +
> > +   mutex_unlock(>io_mutex);
>
> There is such a thing as threads sharing file descriptors.
> That leads to the question what happens to the mutex if this
> ioctl() is called multiple times.
>
>Regards
>Oliver

Multiple threads can wait with the same or different file descriptors.
When an SRQ interrupt occurs, all threads and file descriptors are
informed concurrently with wake_up_interruptible_all(>waitq);
The "_all" is already fixed in 02/12.


No, the problem is that you will underflow io->mutex


Don't worry. The function usbtmc488_ioctl_wait_srq is called by
usbtmc_ioctl which already locks the mutex. See
https://elixir.bootlin.com/linux/v4.17-rc7/source/drivers/usb/class/usbtmc.c#L1189
So the mutex is just unlocked here to allow other threads to still communicate
with the device.

Regards,

Guido

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/12] usb: usbtmc: Add ioctl USBTMC488_IOCTL_WAIT_SRQ

[Oliver Neukum]

Am Donnerstag, den 24.05.2018, 12:59 + schrieb guido@kiener-
muenchen.de:
> Zitat von Oliver Neukum :
> 
> > Am Donnerstag, den 17.05.2018, 19:03 +0200 schrieb Guido Kiener:
> > > +static int usbtmc488_ioctl_wait_srq(struct usbtmc_file_data *file_data,
> > > +   unsigned int __user *arg)
> > > +{
> > > +   struct usbtmc_device_data *data = file_data->data;
> > > +   struct device *dev = >intf->dev;
> > > +   int rv;
> > > +   unsigned int timeout;
> > > +   unsigned long expire;
> > > +
> > > +   if (!data->iin_ep_present) {
> > > +   dev_dbg(dev, "no interrupt endpoint present\n");
> > > +   return -EFAULT;
> > > +   }
> > > +
> > > +   if (get_user(timeout, arg))
> > > +   return -EFAULT;
> > > +
> > > +   expire = msecs_to_jiffies(timeout);
> > > +
> > > +   mutex_unlock(>io_mutex);
> > 
> > There is such a thing as threads sharing file descriptors.
> > That leads to the question what happens to the mutex if this
> > ioctl() is called multiple times.
> > 
> > Regards
> > Oliver
> 
> Multiple threads can wait with the same or different file descriptors.
> When an SRQ interrupt occurs, all threads and file descriptors are
> informed concurrently with wake_up_interruptible_all(>waitq);
> The "_all" is already fixed in 02/12.

No, the problem is that you will underflow io->mutex

Regards
Oliver

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/12] usb: usbtmc: Add ioctl USBTMC488_IOCTL_WAIT_SRQ

[guido]

Zitat von Oliver Neukum :


Am Donnerstag, den 17.05.2018, 19:03 +0200 schrieb Guido Kiener:

+static int usbtmc488_ioctl_wait_srq(struct usbtmc_file_data *file_data,
+   unsigned int __user *arg)
+{
+   struct usbtmc_device_data *data = file_data->data;
+   struct device *dev = >intf->dev;
+   int rv;
+   unsigned int timeout;
+   unsigned long expire;
+
+   if (!data->iin_ep_present) {
+   dev_dbg(dev, "no interrupt endpoint present\n");
+   return -EFAULT;
+   }
+
+   if (get_user(timeout, arg))
+   return -EFAULT;
+
+   expire = msecs_to_jiffies(timeout);
+
+   mutex_unlock(>io_mutex);


There is such a thing as threads sharing file descriptors.
That leads to the question what happens to the mutex if this
ioctl() is called multiple times.

Regards
Oliver


Multiple threads can wait with the same or different file descriptors.
When an SRQ interrupt occurs, all threads and file descriptors are
informed concurrently with wake_up_interruptible_all(>waitq);
The "_all" is already fixed in 02/12.

Regards,

Guido

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/12] usb: usbtmc: Add ioctl USBTMC488_IOCTL_WAIT_SRQ

[Greg KH]

On Wed, May 23, 2018 at 02:08:27PM +0200, Oliver Neukum wrote:
> Am Donnerstag, den 17.05.2018, 19:03 +0200 schrieb Guido Kiener:
> > +static int usbtmc488_ioctl_wait_srq(struct usbtmc_file_data *file_data,
> > +   unsigned int __user *arg)
> > +{
> > +   struct usbtmc_device_data *data = file_data->data;
> > +   struct device *dev = >intf->dev;
> > +   int rv;
> > +   unsigned int timeout;
> > +   unsigned long expire;
> > +
> > +   if (!data->iin_ep_present) {
> > +   dev_dbg(dev, "no interrupt endpoint present\n");
> > +   return -EFAULT;
> > +   }
> > +
> > +   if (get_user(timeout, arg))
> > +   return -EFAULT;
> > +
> > +   expire = msecs_to_jiffies(timeout);
> > +
> > +   mutex_unlock(>io_mutex);
> 
> There is such a thing as threads sharing file descriptors.
> That leads to the question what happens to the mutex if this
> ioctl() is called multiple times.

Processes can share file descriptors as well, no need to mess with a
thread :)

good catch.

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/12] usb: usbtmc: Add ioctl USBTMC488_IOCTL_WAIT_SRQ

[Oliver Neukum]

Am Donnerstag, den 17.05.2018, 19:03 +0200 schrieb Guido Kiener:
> +static int usbtmc488_ioctl_wait_srq(struct usbtmc_file_data *file_data,
> +   unsigned int __user *arg)
> +{
> +   struct usbtmc_device_data *data = file_data->data;
> +   struct device *dev = >intf->dev;
> +   int rv;
> +   unsigned int timeout;
> +   unsigned long expire;
> +
> +   if (!data->iin_ep_present) {
> +   dev_dbg(dev, "no interrupt endpoint present\n");
> +   return -EFAULT;
> +   }
> +
> +   if (get_user(timeout, arg))
> +   return -EFAULT;
> +
> +   expire = msecs_to_jiffies(timeout);
> +
> +   mutex_unlock(>io_mutex);

There is such a thing as threads sharing file descriptors.
That leads to the question what happens to the mutex if this
ioctl() is called multiple times.

Regards
Oliver

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/12] usb: usbtmc: Add ioctl USBTMC488_IOCTL_WAIT_SRQ

[guido]

Zitat von Greg KH :


On Fri, May 18, 2018 at 03:02:10PM +, gu...@kiener-muenchen.de wrote:


Zitat von Greg KH :

> On Thu, May 17, 2018 at 07:03:31PM +0200, Guido Kiener wrote:
> > @@ -2420,8 +2476,8 @@ static int usbtmc_probe(struct  
usb_interface *intf,

> >
> >   retcode = usb_register_dev(intf, _class);
> >   if (retcode) {
> > - dev_err(>dev, "Not able to get a minor"
> > - " (base %u, slice default): %d\n", USBTMC_MINOR_BASE,
> > + dev_err(>dev, "Not able to get a minor (base %u, slice
> > default): %d\n",
> > + USBTMC_MINOR_BASE,
> >   retcode);
>
> Nice change, but totally not relevant for this specific patch :(

Extra patch or just omit?


Extra patch please :)


I thought kernel message must not be broken due to rule 80 characters per
line.


You are correct, just don't try to "sneak" it into a patch that does
something totally different.



Thank you very much for you review.
I will send the next "version" after weekend or after my holiday.

Best regards,

Guido


thanks,

greg k-h




--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/12] usb: usbtmc: Add ioctl USBTMC488_IOCTL_WAIT_SRQ

[Greg KH]

On Fri, May 18, 2018 at 03:02:10PM +, gu...@kiener-muenchen.de wrote:
> 
> Zitat von Greg KH :
> 
> > On Thu, May 17, 2018 at 07:03:31PM +0200, Guido Kiener wrote:
> > > @@ -2420,8 +2476,8 @@ static int usbtmc_probe(struct usb_interface *intf,
> > > 
> > >   retcode = usb_register_dev(intf, _class);
> > >   if (retcode) {
> > > - dev_err(>dev, "Not able to get a minor"
> > > - " (base %u, slice default): %d\n", USBTMC_MINOR_BASE,
> > > + dev_err(>dev, "Not able to get a minor (base %u, slice
> > > default): %d\n",
> > > + USBTMC_MINOR_BASE,
> > >   retcode);
> > 
> > Nice change, but totally not relevant for this specific patch :(
> 
> Extra patch or just omit?

Extra patch please :)

> I thought kernel message must not be broken due to rule 80 characters per
> line.

You are correct, just don't try to "sneak" it into a patch that does
something totally different.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/12] usb: usbtmc: Add ioctl USBTMC488_IOCTL_WAIT_SRQ

[guido]

Zitat von Greg KH :


On Thu, May 17, 2018 at 07:03:31PM +0200, Guido Kiener wrote:

@@ -2420,8 +2476,8 @@ static int usbtmc_probe(struct usb_interface *intf,

retcode = usb_register_dev(intf, _class);
if (retcode) {
-   dev_err(>dev, "Not able to get a minor"
-   " (base %u, slice default): %d\n", USBTMC_MINOR_BASE,
+		dev_err(>dev, "Not able to get a minor (base %u, slice  
default): %d\n",

+   USBTMC_MINOR_BASE,
retcode);


Nice change, but totally not relevant for this specific patch :(


Extra patch or just omit?
I thought kernel message must not be broken due to rule 80 characters  
per line.





diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h
index 1540716de293..35b63530121d 100644
--- a/include/uapi/linux/usb/tmc.h
+++ b/include/uapi/linux/usb/tmc.h
@@ -96,6 +96,7 @@ struct usbtmc_message {
 #define USBTMC488_IOCTL_GOTO_LOCAL _IO(USBTMC_IOC_NR, 20)
 #define USBTMC488_IOCTL_LOCAL_LOCKOUT  _IO(USBTMC_IOC_NR, 21)
 #define USBTMC488_IOCTL_TRIGGER_IO(USBTMC_IOC_NR, 22)
+#define USBTMC488_IOCTL_WAIT_SRQ   _IOW(USBTMC_IOC_NR, 23, unsigned int)


Again __u32?


Yes.



thanks,

greg k-h




--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/12] usb: usbtmc: Add ioctl USBTMC488_IOCTL_WAIT_SRQ

[Greg KH]

On Thu, May 17, 2018 at 07:03:31PM +0200, Guido Kiener wrote:
> @@ -2420,8 +2476,8 @@ static int usbtmc_probe(struct usb_interface *intf,
>  
>   retcode = usb_register_dev(intf, _class);
>   if (retcode) {
> - dev_err(>dev, "Not able to get a minor"
> - " (base %u, slice default): %d\n", USBTMC_MINOR_BASE,
> + dev_err(>dev, "Not able to get a minor (base %u, slice 
> default): %d\n",
> + USBTMC_MINOR_BASE,
>   retcode);

Nice change, but totally not relevant for this specific patch :(

> diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h
> index 1540716de293..35b63530121d 100644
> --- a/include/uapi/linux/usb/tmc.h
> +++ b/include/uapi/linux/usb/tmc.h
> @@ -96,6 +96,7 @@ struct usbtmc_message {
>  #define USBTMC488_IOCTL_GOTO_LOCAL   _IO(USBTMC_IOC_NR, 20)
>  #define USBTMC488_IOCTL_LOCAL_LOCKOUT_IO(USBTMC_IOC_NR, 21)
>  #define USBTMC488_IOCTL_TRIGGER  _IO(USBTMC_IOC_NR, 22)
> +#define USBTMC488_IOCTL_WAIT_SRQ _IOW(USBTMC_IOC_NR, 23, unsigned int)

Again __u32?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 07/12] usb: usbtmc: Add ioctl USBTMC488_IOCTL_WAIT_SRQ

[Guido Kiener]

Wait until an SRQ (service request) is received on the interrupt pipe
or until the given period of time is expired. In contrast to the
poll() function this ioctl does not return when other (a)synchronous
I/O operations fail with EPOLLERR.

Signed-off-by: Guido Kiener 
Reviewed-by: Steve Bayless 
---
 drivers/usb/class/usbtmc.c   | 60 ++--
 include/uapi/linux/usb/tmc.h |  1 +
 2 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index de664a345e69..6b8b8510cfc4 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -594,6 +594,54 @@ static int usbtmc488_ioctl_read_stb(struct 
usbtmc_file_data *file_data,
return rv;
 }
 
+static int usbtmc488_ioctl_wait_srq(struct usbtmc_file_data *file_data,
+   unsigned int __user *arg)
+{
+   struct usbtmc_device_data *data = file_data->data;
+   struct device *dev = >intf->dev;
+   int rv;
+   unsigned int timeout;
+   unsigned long expire;
+
+   if (!data->iin_ep_present) {
+   dev_dbg(dev, "no interrupt endpoint present\n");
+   return -EFAULT;
+   }
+
+   if (get_user(timeout, arg))
+   return -EFAULT;
+
+   expire = msecs_to_jiffies(timeout);
+
+   mutex_unlock(>io_mutex);
+
+   rv = wait_event_interruptible_timeout(
+   data->waitq,
+   atomic_read(_data->srq_asserted) != 0 ||
+   atomic_read(_data->closing),
+   expire);
+
+   mutex_lock(>io_mutex);
+
+   /* Note! disconnect or close could be called in the meantime */
+   if (atomic_read(_data->closing) || data->zombie)
+   rv = -ENODEV;
+
+   if (rv < 0) {
+   /* dev can be invalid now! */
+   pr_debug("%s - wait interrupted %d\n", __func__, rv);
+   return rv;
+   }
+
+   if (rv == 0) {
+   dev_dbg(dev, "%s - wait timed out\n", __func__);
+   return -ETIMEDOUT;
+   }
+
+   dev_dbg(dev, "%s - srq asserted\n", __func__);
+   return 0;
+}
+
 static int usbtmc488_ioctl_simple(struct usbtmc_device_data *data,
void __user *arg, unsigned int cmd)
 {
@@ -2149,6 +2197,11 @@ static long usbtmc_ioctl(struct file *file, unsigned int 
cmd, unsigned long arg)
retval = usbtmc488_ioctl_trigger(file_data);
break;
 
+   case USBTMC488_IOCTL_WAIT_SRQ:
+   retval = usbtmc488_ioctl_wait_srq(file_data,
+ (unsigned int __user *)arg);
+   break;
+
case USBTMC_IOCTL_CANCEL_IO:
retval = usbtmc_ioctl_cancel_io(file_data);
break;
@@ -2290,6 +2343,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;
@@ -2308,7 +2362,9 @@ static void usbtmc_free_int(struct usbtmc_device_data 
*data)
return;
usb_kill_urb(data->iin_urb);
kfree(data->iin_buffer);
+   data->iin_buffer = NULL;
usb_free_urb(data->iin_urb);
+   data->iin_urb = NULL;
kref_put(>kref, usbtmc_delete);
 }
 
@@ -2420,8 +2476,8 @@ static int usbtmc_probe(struct usb_interface *intf,
 
retcode = usb_register_dev(intf, _class);
if (retcode) {
-   dev_err(>dev, "Not able to get a minor"
-   " (base %u, slice default): %d\n", USBTMC_MINOR_BASE,
+   dev_err(>dev, "Not able to get a minor (base %u, slice 
default): %d\n",
+   USBTMC_MINOR_BASE,
retcode);
goto error_register;
}
diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h
index 1540716de293..35b63530121d 100644
--- a/include/uapi/linux/usb/tmc.h
+++ b/include/uapi/linux/usb/tmc.h
@@ -96,6 +96,7 @@ struct usbtmc_message {
 #define USBTMC488_IOCTL_GOTO_LOCAL _IO(USBTMC_IOC_NR, 20)
 #define USBTMC488_IOCTL_LOCAL_LOCKOUT  _IO(USBTMC_IOC_NR, 21)
 #define USBTMC488_IOCTL_TRIGGER_IO(USBTMC_IOC_NR, 22)
+#define USBTMC488_IOCTL_WAIT_SRQ   _IOW(USBTMC_IOC_NR, 23, unsigned int)
 
 /* Cancel and cleanup asynchronous calls */
 #define USBTMC_IOCTL_CANCEL_IO _IO(USBTMC_IOC_NR, 35)
-- 
2.17.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html