Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 18 Aug 2020 11:36:40 +0200
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     Sergey Korolev <s.korolev@ndmsystems.com>
Cc:     Al Viro <viro@zeniv.linux.org.uk>,
        Rob Gill <rrobgill@protonmail.com>,
        Bastien Nocera <hadess@hadess.net>,
        Alan Stern <stern@rowland.harvard.edu>,
        Johan Hovold <johan@kernel.org>,
        Nishad Kamdar <nishadkamdar@gmail.com>,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] USB: core: remove polling for
 /sys/kernel/debug/usb/devices
Message-ID: <20200818093640.GC34785@kroah.com>
References: <20200809161233.13135-1-s.korolev@ndmsystems.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200809161233.13135-1-s.korolev@ndmsystems.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, Aug 09, 2020 at 07:12:30PM +0300, Sergey Korolev wrote:
> The latest reference to usbfs_conn_disc_event() removed in
> commit fb28d58b72aa ("USB: remove CONFIG_USB_DEVICEFS")
> in 2012 and now a user poll() waits infinitely for content changes.
> 
> Signed-off-by: Sergey Korolev <s.korolev@ndmsystems.com>
> ---
>  drivers/usb/core/devices.c | 41 --------------------------------------
>  drivers/usb/core/usb.h     |  1 -
>  2 files changed, 42 deletions(-)
> 
> diff --git a/drivers/usb/core/devices.c b/drivers/usb/core/devices.c
> index 696b2b692b83..1ef2de6e375a 100644
> --- a/drivers/usb/core/devices.c
> +++ b/drivers/usb/core/devices.c
> @@ -39,7 +39,6 @@
>  #include <linux/fs.h>
>  #include <linux/mm.h>
>  #include <linux/gfp.h>
> -#include <linux/poll.h>
>  #include <linux/usb.h>
>  #include <linux/usbdevice_fs.h>
>  #include <linux/usb/hcd.h>
> @@ -97,22 +96,6 @@ static const char format_endpt[] =
>  /* E:  Ad=xx(s) Atr=xx(ssss) MxPS=dddd Ivl=D?s */
>    "E:  Ad=%02x(%c) Atr=%02x(%-4s) MxPS=%4d Ivl=%d%cs\n";
>  
> -/*
> - * Wait for an connect/disconnect event to happen. We initialize
> - * the event counter with an odd number, and each event will increment
> - * the event counter by two, so it will always _stay_ odd. That means
> - * that it will never be zero, so "event 0" will never match a current
> - * event, and thus 'poll' will always trigger as readable for the first
> - * time it gets called.
> - */
> -static struct device_connect_event {
> -	atomic_t count;
> -	wait_queue_head_t wait;
> -} device_event = {
> -	.count = ATOMIC_INIT(1),
> -	.wait = __WAIT_QUEUE_HEAD_INITIALIZER(device_event.wait)
> -};
> -
>  struct class_info {
>  	int class;
>  	char *class_name;
> @@ -146,12 +129,6 @@ static const struct class_info clas_info[] = {
>  
>  /*****************************************************************/
>  
> -void usbfs_conn_disc_event(void)
> -{
> -	atomic_add(2, &device_event.count);
> -	wake_up(&device_event.wait);
> -}
> -
>  static const char *class_decode(const int class)
>  {
>  	int ix;
> @@ -623,25 +600,7 @@ static ssize_t usb_device_read(struct file *file, char __user *buf,
>  	return total_written;
>  }
>  
> -/* Kernel lock for "lastev" protection */
> -static __poll_t usb_device_poll(struct file *file,
> -				    struct poll_table_struct *wait)
> -{
> -	unsigned int event_count;
> -
> -	poll_wait(file, &device_event.wait, wait);
> -
> -	event_count = atomic_read(&device_event.count);
> -	if (file->f_version != event_count) {
> -		file->f_version = event_count;
> -		return EPOLLIN | EPOLLRDNORM;
> -	}
> -
> -	return 0;
> -}
> -
>  const struct file_operations usbfs_devices_fops = {
>  	.llseek =	no_seek_end_llseek,
>  	.read =		usb_device_read,
> -	.poll =		usb_device_poll,
>  };
> diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
> index 98e7d1ee63dc..c893f54a3420 100644
> --- a/drivers/usb/core/usb.h
> +++ b/drivers/usb/core/usb.h
> @@ -191,7 +191,6 @@ extern const struct attribute_group *usb_interface_groups[];
>  extern struct usb_driver usbfs_driver;
>  extern const struct file_operations usbfs_devices_fops;
>  extern const struct file_operations usbdev_file_operations;
> -extern void usbfs_conn_disc_event(void);
>  
>  extern int usb_devio_init(void);
>  extern void usb_devio_cleanup(void);

Why not fix this up instead?  The debugfs file can be polled and this
should be fixed to accurately handle that.

It's kind of proof though that no one really cares about this option in
that no one has noticed it being gone for so long, so if you don't want
to fix it, I will just take this patch for now.

Let me know.

thanks,

greg k-h