Re: [RFC] usbfs: Add ioctls for runtime suspend and resume

[Mayuresh Kulkarni <mkulkarni@opensource.cirrus.com>]

On Fri, 2019-07-05 at 14:51 -0400, Alan Stern wrote:
> On Wed, 3 Jul 2019, Mayuresh Kulkarni wrote:
> 
> > 
> > As you had mentioned in one of the comment before, the only addition
> > to
> > the patch I have locally is -
> > usbfs_notify_resume() has usbfs_mutex lock around list traversal.
> > 
> > Could you please send the patch for review? Please note, I think I
> > am
> > not a part of linux-usb mailing-list, so probably need to be in cc
> > to
> > get the patch email. Do let me know if something else is needed from
> > me.
> Here it is.  There are two changes from the previous version:
> 
> 1.	This is rebased on top of a separate patch which Greg has 
> 	already accepted:
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit?
> id=ffed60971f3d95923b99ea970862c6ab6a22c20f
> 
> 2.	I implemented Oliver's recommendation that the
> WAIT_FOR_RESUME
> 	ioctl should automatically do FORBID_SUSPEND before it returns, 
> 	if the return code is 0 (that is, it wasn't interrupted by a 
> 	signal).
> 
> Still to do: Write up the documentation.  In fact, the existing
> description of usbfs in Documentation/driver-api/usb/usb.rst is sadly
> out of date.  And it deserves to be split out into a separate file of
> its own -- but I'm not sure where it really belongs, considering that
> it is an API for userspace, not an internal kernel API.
> 
> Greg, suggestions?

Hi Greg,

Did you got a chance to look into the above documentation query by Alan?
How should we go about documenting these new IOCTLs?
> 
> Alan Stern
> 
> 
>  drivers/usb/core/devio.c          |   96
> ++++++++++++++++++++++++++++++++++++--
>  drivers/usb/core/generic.c        |    5 +
>  drivers/usb/core/usb.h            |    3 +
>  include/uapi/linux/usbdevice_fs.h |    3 +
>  4 files changed, 102 insertions(+), 5 deletions(-)
> 
> Index: usb-devel/drivers/usb/core/devio.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/core/devio.c
> +++ usb-devel/drivers/usb/core/devio.c
> @@ -48,6 +48,9 @@
>  #define USB_DEVICE_MAX			(USB_MAXBUS * 128)
>  #define USB_SG_SIZE			16384 /* split-size for
> large txs */
>  
> +/* Mutual exclusion for ps->list in resume vs. release and remove */
> +static DEFINE_MUTEX(usbfs_mutex);
> +
>  struct usb_dev_state {
>  	struct list_head list;      /* state list */
>  	struct usb_device *dev;
> @@ -57,14 +60,17 @@ struct usb_dev_state {
>  	struct list_head async_completed;
>  	struct list_head memory_list;
>  	wait_queue_head_t wait;     /* wake up if a request completed
> */
> +	wait_queue_head_t wait_for_resume;   /* wake up upon runtime
> resume */
>  	unsigned int discsignr;
>  	struct pid *disc_pid;
>  	const struct cred *cred;
>  	void __user *disccontext;
>  	unsigned long ifclaimed;
>  	u32 disabled_bulk_eps;
> -	bool privileges_dropped;
>  	unsigned long interface_allowed_mask;
> +	int not_yet_resumed;
> +	bool suspend_allowed;
> +	bool privileges_dropped;
>  };
>  
>  struct usb_memory {
> @@ -696,9 +702,7 @@ static void driver_disconnect(struct usb
>  	destroy_async_on_interface(ps, ifnum);
>  }
>  
> -/* The following routines are merely placeholders.  There is no way
> - * to inform a user task about suspend or resumes.
> - */
> +/* We don't care about suspend/resume of claimed interfaces */
>  static int driver_suspend(struct usb_interface *intf, pm_message_t
> msg)
>  {
>  	return 0;
> @@ -709,12 +713,32 @@ static int driver_resume(struct usb_inte
>  	return 0;
>  }
>  
> +/* The following routines apply to the entire device, not interfaces
> */
> +void usbfs_notify_suspend(struct usb_device *udev)
> +{
> +	/* We don't need to handle this */
> +}
> +
> +void usbfs_notify_resume(struct usb_device *udev)
> +{
> +	struct usb_dev_state *ps;
> +
> +	/* Protect against simultaneous remove or release */
> +	mutex_lock(&usbfs_mutex);
> +	list_for_each_entry(ps, &udev->filelist, list) {
> +		WRITE_ONCE(ps->not_yet_resumed, 0);
> +		wake_up_all(&ps->wait_for_resume);
> +	}
> +	mutex_unlock(&usbfs_mutex);
> +}
> +
>  struct usb_driver usbfs_driver = {
>  	.name =		"usbfs",
>  	.probe =	driver_probe,
>  	.disconnect =	driver_disconnect,
>  	.suspend =	driver_suspend,
>  	.resume =	driver_resume,
> +	.supports_autosuspend = 1,
>  };
>  
>  static int claimintf(struct usb_dev_state *ps, unsigned int ifnum)
> @@ -999,9 +1023,12 @@ static int usbdev_open(struct inode *ino
>  	INIT_LIST_HEAD(&ps->async_completed);
>  	INIT_LIST_HEAD(&ps->memory_list);
>  	init_waitqueue_head(&ps->wait);
> +	init_waitqueue_head(&ps->wait_for_resume);
>  	ps->disc_pid = get_pid(task_pid(current));
>  	ps->cred = get_current_cred();
>  	smp_wmb();
> +
> +	/* Can't race with resume; the device is already active */
>  	list_add_tail(&ps->list, &dev->filelist);
>  	file->private_data = ps;
>  	usb_unlock_device(dev);
> @@ -1027,7 +1054,10 @@ static int usbdev_release(struct inode *
>  	usb_lock_device(dev);
>  	usb_hub_release_all_ports(dev, ps);
>  
> +	/* Protect against simultaneous resume */
> +	mutex_lock(&usbfs_mutex);
>  	list_del_init(&ps->list);
> +	mutex_unlock(&usbfs_mutex);
>  
>  	for (ifnum = 0; ps->ifclaimed && ifnum < 8*sizeof(ps-
> >ifclaimed);
>  			ifnum++) {
> @@ -1035,7 +1065,8 @@ static int usbdev_release(struct inode *
>  			releaseintf(ps, ifnum);
>  	}
>  	destroy_all_async(ps);
> -	usb_autosuspend_device(dev);
> +	if (!ps->suspend_allowed)
> +		usb_autosuspend_device(dev);
>  	usb_unlock_device(dev);
>  	usb_put_dev(dev);
>  	put_pid(ps->disc_pid);
> @@ -2346,6 +2377,47 @@ static int proc_drop_privileges(struct u
>  	return 0;
>  }
>  
> +static int proc_forbid_suspend(struct usb_dev_state *ps)
> +{
> +	int ret = 0;
> +
> +	if (ps->suspend_allowed) {
> +		ret = usb_autoresume_device(ps->dev);
> +		if (ret == 0)
> +			ps->suspend_allowed = false;
> +		else if (ret != -ENODEV)
> +			ret = -EIO;
> +	}
> +	return ret;
> +}
> +
> +static int proc_allow_suspend(struct usb_dev_state *ps)
> +{
> +	if (!connected(ps))
> +		return -ENODEV;
> +
> +	WRITE_ONCE(ps->not_yet_resumed, 1);
> +	if (!ps->suspend_allowed) {
> +		usb_autosuspend_device(ps->dev);
> +		ps->suspend_allowed = true;
> +	}
> +	return 0;
> +}
> +
> +static int proc_wait_for_resume(struct usb_dev_state *ps)
> +{
> +	int ret;
> +
> +	usb_unlock_device(ps->dev);
> +	ret = wait_event_interruptible(ps->wait_for_resume,
> +			READ_ONCE(ps->not_yet_resumed) == 0);
> +	usb_lock_device(ps->dev);
> +
> +	if (ret != 0)
> +		return ret;
> +	return proc_forbid_suspend(ps);
> +}
> +
>  /*
>   * NOTE:  All requests here that have interface numbers as parameters
>   * are assuming that somehow the configuration has been prevented
> from
> @@ -2540,6 +2612,15 @@ static long usbdev_do_ioctl(struct file
>  	case USBDEVFS_GET_SPEED:
>  		ret = ps->dev->speed;
>  		break;
> +	case USBDEVFS_FORBID_SUSPEND:
> +		ret = proc_forbid_suspend(ps);
> +		break;
> +	case USBDEVFS_ALLOW_SUSPEND:
> +		ret = proc_allow_suspend(ps);
> +		break;
> +	case USBDEVFS_WAIT_FOR_RESUME:
> +		ret = proc_wait_for_resume(ps);
> +		break;
>  	}
>  
>   done:
> @@ -2607,10 +2688,14 @@ static void usbdev_remove(struct usb_dev
>  	struct usb_dev_state *ps;
>  	struct kernel_siginfo sinfo;
>  
> +	/* Protect against simultaneous resume */
> +	mutex_lock(&usbfs_mutex);
>  	while (!list_empty(&udev->filelist)) {
>  		ps = list_entry(udev->filelist.next, struct
> usb_dev_state, list);
>  		destroy_all_async(ps);
>  		wake_up_all(&ps->wait);
> +		WRITE_ONCE(ps->not_yet_resumed, 0);
> +		wake_up_all(&ps->wait_for_resume);
>  		list_del_init(&ps->list);
>  		if (ps->discsignr) {
>  			clear_siginfo(&sinfo);
> @@ -2622,6 +2707,7 @@ static void usbdev_remove(struct usb_dev
>  					ps->disc_pid, ps->cred);
>  		}
>  	}
> +	mutex_unlock(&usbfs_mutex);
>  }
>  
>  static int usbdev_notify(struct notifier_block *self,
> Index: usb-devel/drivers/usb/core/generic.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/core/generic.c
> +++ usb-devel/drivers/usb/core/generic.c
> @@ -257,6 +257,8 @@ static int generic_suspend(struct usb_de
>  	else
>  		rc = usb_port_suspend(udev, msg);
>  
> +	if (rc == 0)
> +		usbfs_notify_suspend(udev);
>  	return rc;
>  }
>  
> @@ -273,6 +275,9 @@ static int generic_resume(struct usb_dev
>  		rc = hcd_bus_resume(udev, msg);
>  	else
>  		rc = usb_port_resume(udev, msg);
> +
> +	if (rc == 0)
> +		usbfs_notify_resume(udev);
>  	return rc;
>  }
>  
> Index: usb-devel/drivers/usb/core/usb.h
> ===================================================================
> --- usb-devel.orig/drivers/usb/core/usb.h
> +++ usb-devel/drivers/usb/core/usb.h
> @@ -95,6 +95,9 @@ extern int usb_runtime_idle(struct devic
>  extern int usb_enable_usb2_hardware_lpm(struct usb_device *udev);
>  extern int usb_disable_usb2_hardware_lpm(struct usb_device *udev);
>  
> +extern void usbfs_notify_suspend(struct usb_device *udev);
> +extern void usbfs_notify_resume(struct usb_device *udev);
> +
>  #else
>  
>  static inline int usb_port_suspend(struct usb_device *udev,
> pm_message_t msg)
> Index: usb-devel/include/uapi/linux/usbdevice_fs.h
> ===================================================================
> --- usb-devel.orig/include/uapi/linux/usbdevice_fs.h
> +++ usb-devel/include/uapi/linux/usbdevice_fs.h
> @@ -197,5 +197,8 @@ struct usbdevfs_streams {
>  #define USBDEVFS_FREE_STREAMS      _IOR('U', 29, struct
> usbdevfs_streams)
>  #define USBDEVFS_DROP_PRIVILEGES   _IOW('U', 30, __u32)
>  #define USBDEVFS_GET_SPEED         _IO('U', 31)
> +#define USBDEVFS_FORBID_SUSPEND    _IO('U', 32)
> +#define USBDEVFS_ALLOW_SUSPEND     _IO('U', 33)
> +#define USBDEVFS_WAIT_FOR_RESUME   _IO('U', 34)
>  
>  #endif /* _UAPI_LINUX_USBDEVICE_FS_H */
>