Re: [PATCH 07/12] usb: usbtmc: Add ioctl USBTMC488_IOCTL_WAIT_SRQ
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
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 = &data->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(&data->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(&data->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
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 = &data->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(&data->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(&data->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
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 = &data->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(&data->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(&data->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
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 = &data->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(&data->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
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 = &data->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(&data->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
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, &usbtmc_class); > > if (retcode) { > > - dev_err(&intf->dev, "Not able to get a minor" > > - " (base %u, slice default): %d\n", USBTMC_MINOR_BASE, > > + dev_err(&intf->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
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, &usbtmc_class); > > > if (retcode) { > > > - dev_err(&intf->dev, "Not able to get a minor" > > > - " (base %u, slice default): %d\n", USBTMC_MINOR_BASE, > > > + dev_err(&intf->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
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, &usbtmc_class); if (retcode) { - dev_err(&intf->dev, "Not able to get a minor" - " (base %u, slice default): %d\n", USBTMC_MINOR_BASE, + dev_err(&intf->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
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, &usbtmc_class); > if (retcode) { > - dev_err(&intf->dev, "Not able to get a minor" > - " (base %u, slice default): %d\n", USBTMC_MINOR_BASE, > + dev_err(&intf->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
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 = &data->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(&data->io_mutex); + + rv = wait_event_interruptible_timeout( + data->waitq, + atomic_read(&file_data->srq_asserted) != 0 || + atomic_read(&file_data->closing), + expire); + + mutex_lock(&data->io_mutex); + + /* Note! disconnect or close could be called in the meantime */ + if (atomic_read(&file_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(&data->kref, usbtmc_delete); } @@ -2420,8 +2476,8 @@ static int usbtmc_probe(struct usb_interface *intf, retcode = usb_register_dev(intf, &usbtmc_class); if (retcode) { - dev_err(&intf->dev, "Not able to get a minor" - " (base %u, slice default): %d\n", USBTMC_MINOR_BASE, + dev_err(&intf->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