| Message ID | 20180517170336.8426-8-guido@kiener-muenchen.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Zitat von Greg KH <gregkh@linuxfoundation.org>: > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 18, 2018 at 03:02:10PM +0000, guido@kiener-muenchen.de wrote: > > Zitat von Greg KH <gregkh@linuxfoundation.org>: > > > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Zitat von Greg KH <gregkh@linuxfoundation.org>: > On Fri, May 18, 2018 at 03:02:10PM +0000, guido@kiener-muenchen.de wrote: >> >> Zitat von Greg KH <gregkh@linuxfoundation.org>: >> >> > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Zitat von Oliver Neukum <oneukum@suse.com>: > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Donnerstag, den 24.05.2018, 12:59 +0000 schrieb guido@kiener- muenchen.de: > Zitat von Oliver Neukum <oneukum@suse.com>: > > > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Zitat von Oliver Neukum <oneukum@suse.com>: > Am Donnerstag, den 24.05.2018, 12:59 +0000 schrieb guido@kiener- > muenchen.de: >> Zitat von Oliver Neukum <oneukum@suse.com>: >> >> > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Montag, den 28.05.2018, 10:59 +0000 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)