[PATCH] usb: core: devio: add ioctls for suspend and resume

[PATCH] usb: core: devio: add ioctls for suspend and resume

[Mayuresh Kulkarni]

- The current driver increments the PM ref-count in its .open
API and decrements it in its .close API.
- Due to this, it is not possible for the usb_device to go into
suspend (L2) even if all of its interfaces report idle to usb-core.
- In order to allow the suspend, 2 new ioctls are added:
1. USBDEVFS_SUSPEND: calls auto-suspend and indicates to usb/pm core
to attempt suspend, if appropriate. This is a non-blocking call.
2. USBDEVFS_WAIT_RESUME: waits for resume. This is a blocking call.
The resume could happen due to:
host-wake (i.e.: any driver bound to interface(s) of device wants to
send/receive control/data)
OR
remote-wake (i.e.: when remote-wake enabled device generates a
remote-wake to host).
In both these conditions, this call will return.
- It is expected that:
1. When user-space thinks the device is idle from its point-of-view,
it calls USBDEVFS_SUSPEND.
2. After USBDEVFS_WAIT_RESUME call returns,
the callers queries the device/interface of its interest to see
what caused resume and take appropriate action on it.

The link to relevant discussion about this patch on linux-usb is -
https://www.spinics.net/lists/linux-usb/msg173285.html

Signed-off-by: Mayuresh Kulkarni <mkulkarni@opensource.cirrus.com>
---
 drivers/usb/core/devio.c          | 114 ++++++++++++++++++++++++++++++++++++--
 include/uapi/linux/usbdevice_fs.h |   2 +
 2 files changed, 112 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index fa783531..67dc326 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -68,6 +68,9 @@ struct usb_dev_state {
 	u32 disabled_bulk_eps;
 	bool privileges_dropped;
 	unsigned long interface_allowed_mask;
+	bool runtime_active;
+	bool is_suspended;
+	wait_queue_head_t wait_resume;
 };
 
 struct usb_memory {
@@ -444,6 +447,23 @@ static struct async *async_getpending(struct usb_dev_state *ps,
 	return NULL;
 }
 
+static int async_getpending_count(struct usb_dev_state *ps)
+{
+	struct async *as;
+	int count;
+	unsigned long flags;
+
+	spin_lock_irqsave(&ps->lock, flags);
+	if (list_empty(&ps->async_pending))
+		count = 0;
+	else
+		list_for_each_entry(as, &ps->async_pending, asynclist)
+			count++;
+	spin_unlock_irqrestore(&ps->lock, flags);
+
+	return count;
+}
+
 static void snoop_urb(struct usb_device *udev,
 		void __user *userurb, int pipe, unsigned length,
 		int timeout_or_status, enum snoop_when when,
@@ -699,16 +719,26 @@ static void driver_disconnect(struct usb_interface *intf)
 	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.
- */
 static int driver_suspend(struct usb_interface *intf, pm_message_t msg)
 {
+	struct usb_dev_state *ps = usb_get_intfdata(intf);
+
+	ps->is_suspended = true;
+	snoop(&ps->dev->dev, "driver-suspend\n");
+
 	return 0;
 }
 
 static int driver_resume(struct usb_interface *intf)
 {
+	struct usb_dev_state *ps = usb_get_intfdata(intf);
+
+	ps->runtime_active = true;
+	wake_up(&ps->wait_resume);
+
+	snoop(&ps->dev->dev, "driver-resume: runtime-active = %d\n",
+		ps->runtime_active);
+
 	return 0;
 }
 
@@ -718,6 +748,7 @@ struct usb_driver usbfs_driver = {
 	.disconnect =	driver_disconnect,
 	.suspend =	driver_suspend,
 	.resume =	driver_resume,
+	.supports_autosuspend = 1,
 };
 
 static int claimintf(struct usb_dev_state *ps, unsigned int ifnum)
@@ -963,6 +994,27 @@ static struct usb_device *usbdev_lookup_by_devt(dev_t devt)
 	return to_usb_device(dev);
 }
 
+/* must be called with usb-dev lock held */
+static int usbdev_do_resume(struct usb_dev_state *ps)
+{
+	int ret = 0;
+
+	if (!ps->runtime_active) {
+		snoop(&ps->dev->dev, "suspended..resume now\n");
+		ps->is_suspended = false;
+		if (usb_autoresume_device(ps->dev)) {
+			ret = -EIO;
+			goto _out;
+		}
+		snoop(&ps->dev->dev, "resume done..active now\n");
+		ps->runtime_active = true;
+		wake_up(&ps->wait_resume);
+	}
+
+_out:
+	return ret;
+}
+
 /*
  * file operations
  */
@@ -1008,6 +1060,9 @@ static int usbdev_open(struct inode *inode, struct file *file)
 	INIT_LIST_HEAD(&ps->async_completed);
 	INIT_LIST_HEAD(&ps->memory_list);
 	init_waitqueue_head(&ps->wait);
+	init_waitqueue_head(&ps->wait_resume);
+	ps->runtime_active = true;
+	ps->is_suspended = false;
 	ps->disc_pid = get_pid(task_pid(current));
 	ps->cred = get_current_cred();
 	smp_wmb();
@@ -1034,6 +1089,10 @@ static int usbdev_release(struct inode *inode, struct file *file)
 	struct async *as;
 
 	usb_lock_device(dev);
+
+	/* what can we can do if resume fails? */
+	usbdev_do_resume(ps);
+
 	usb_hub_release_all_ports(dev, ps);
 
 	list_del_init(&ps->list);
@@ -2355,6 +2414,18 @@ static int proc_drop_privileges(struct usb_dev_state *ps, void __user *arg)
 	return 0;
 }
 
+static int proc_wait_resume(struct usb_dev_state *ps)
+{
+	int ret;
+
+	snoop(&ps->dev->dev, "wait-for-resume\n");
+	ret = wait_event_interruptible(ps->wait_resume,
+		(ps->runtime_active == true));
+	snoop(&ps->dev->dev, "wait-for-resume...done\n");
+
+	return ret;
+}
+
 /*
  * NOTE:  All requests here that have interface numbers as parameters
  * are assuming that somehow the configuration has been prevented from
@@ -2373,6 +2444,25 @@ static long usbdev_do_ioctl(struct file *file, unsigned int cmd,
 
 	usb_lock_device(dev);
 
+	if (cmd != USBDEVFS_WAIT_RESUME) {
+		ret = usbdev_do_resume(ps);
+		if (ret)
+			goto done;
+	} else {
+		usb_unlock_device(dev);
+		ret = proc_wait_resume(ps);
+
+		/* Call auto-resume to balance auto-suspend of suspend-ioctl */
+		usb_lock_device(dev);
+		if (ps->is_suspended) {
+			ret = usb_autoresume_device(ps->dev);
+			ps->is_suspended = false;
+		}
+		usb_unlock_device(dev);
+
+		goto _done;
+	}
+
 	/* Reap operations are allowed even after disconnection */
 	switch (cmd) {
 	case USBDEVFS_REAPURB:
@@ -2549,10 +2639,26 @@ static long usbdev_do_ioctl(struct file *file, unsigned int cmd,
 	case USBDEVFS_GET_SPEED:
 		ret = ps->dev->speed;
 		break;
+	case USBDEVFS_SUSPEND:
+		ret = 0;
+
+		/* we cannot suspend when URB is pending */
+		if (async_getpending_count(ps)) {
+			snoop(&ps->dev->dev, "ioctl-suspend but URB pending\n");
+			ret = -EINVAL;
+		} else {
+			if (ps->runtime_active) {
+				snoop(&ps->dev->dev, "ioctl-suspend\n");
+				ps->runtime_active = false;
+				usb_autosuspend_device(ps->dev);
+			}
+		}
+		break;
 	}
 
- done:
+done:
 	usb_unlock_device(dev);
+_done:
 	if (ret >= 0)
 		inode->i_atime = current_time(inode);
 	return ret;
diff --git a/include/uapi/linux/usbdevice_fs.h b/include/uapi/linux/usbdevice_fs.h
index 964e872..ae46beb 100644
--- a/include/uapi/linux/usbdevice_fs.h
+++ b/include/uapi/linux/usbdevice_fs.h
@@ -197,5 +197,7 @@ 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_SUSPEND           _IO('U', 32)
+#define USBDEVFS_WAIT_RESUME       _IO('U', 33)
 
 #endif /* _UAPI_LINUX_USBDEVICE_FS_H */
-- 
2.7.4

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Greg KH]

On Fri, May 10, 2019 at 11:01:09AM +0100, Mayuresh Kulkarni wrote:
> - The current driver increments the PM ref-count in its .open
> API and decrements it in its .close API.
> - Due to this, it is not possible for the usb_device to go into
> suspend (L2) even if all of its interfaces report idle to usb-core.
> - In order to allow the suspend, 2 new ioctls are added:
> 1. USBDEVFS_SUSPEND: calls auto-suspend and indicates to usb/pm core
> to attempt suspend, if appropriate. This is a non-blocking call.
> 2. USBDEVFS_WAIT_RESUME: waits for resume. This is a blocking call.
> The resume could happen due to:
> host-wake (i.e.: any driver bound to interface(s) of device wants to
> send/receive control/data)
> OR
> remote-wake (i.e.: when remote-wake enabled device generates a
> remote-wake to host).
> In both these conditions, this call will return.
> - It is expected that:
> 1. When user-space thinks the device is idle from its point-of-view,
> it calls USBDEVFS_SUSPEND.
> 2. After USBDEVFS_WAIT_RESUME call returns,
> the callers queries the device/interface of its interest to see
> what caused resume and take appropriate action on it.

I'm going to make a bunch of random comments on this patch, some
cosmetic...

First off, the above is horrible to try to read, please format things in
a sane way such that we can understand it well.

> The link to relevant discussion about this patch on linux-usb is -
> https://www.spinics.net/lists/linux-usb/msg173285.html

You should not need to reference any external web site for a changelog
entry, just put the needed information in the changelog itself.

> Signed-off-by: Mayuresh Kulkarni <mkulkarni@opensource.cirrus.com>
> ---
>  drivers/usb/core/devio.c          | 114 ++++++++++++++++++++++++++++++++++++--
>  include/uapi/linux/usbdevice_fs.h |   2 +
>  2 files changed, 112 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index fa783531..67dc326 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -68,6 +68,9 @@ struct usb_dev_state {
>  	u32 disabled_bulk_eps;
>  	bool privileges_dropped;
>  	unsigned long interface_allowed_mask;
> +	bool runtime_active;
> +	bool is_suspended;
> +	wait_queue_head_t wait_resume;

That's some crazy alignment issues, please don't waste bytes for no good
reason.

And can you document these fields somewhere?


>  };
>  
>  struct usb_memory {
> @@ -444,6 +447,23 @@ static struct async *async_getpending(struct usb_dev_state *ps,
>  	return NULL;
>  }
>  
> +static int async_getpending_count(struct usb_dev_state *ps)
> +{
> +	struct async *as;
> +	int count;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&ps->lock, flags);
> +	if (list_empty(&ps->async_pending))
> +		count = 0;
> +	else
> +		list_for_each_entry(as, &ps->async_pending, asynclist)
> +			count++;

Doesn't list_for_each_entry() work just fine on an empty list?  Just set
count to 0 up above, right?

> +	spin_unlock_irqrestore(&ps->lock, flags);

So, right after you call this function, and drop the lock, the entries
can change, making your "count" invalid and stale.  So you can not trust
the value at all, right?

> +
> +	return count;
> +}
> +
>  static void snoop_urb(struct usb_device *udev,
>  		void __user *userurb, int pipe, unsigned length,
>  		int timeout_or_status, enum snoop_when when,
> @@ -699,16 +719,26 @@ static void driver_disconnect(struct usb_interface *intf)
>  	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.
> - */
>  static int driver_suspend(struct usb_interface *intf, pm_message_t msg)
>  {
> +	struct usb_dev_state *ps = usb_get_intfdata(intf);
> +
> +	ps->is_suspended = true;

No locking needed?

> +	snoop(&ps->dev->dev, "driver-suspend\n");

Why?  Does anyone use the snoop api anymore?

> +
>  	return 0;
>  }
>  
>  static int driver_resume(struct usb_interface *intf)
>  {
> +	struct usb_dev_state *ps = usb_get_intfdata(intf);
> +
> +	ps->runtime_active = true;
> +	wake_up(&ps->wait_resume);
> +
> +	snoop(&ps->dev->dev, "driver-resume: runtime-active = %d\n",
> +		ps->runtime_active);

What does runtime_active give userspace here?  Again, who is using
snoop?  And isn't runtime_active a bool?  It's always going to be "true"
here.  Is this just debugging code left in?

> +
>  	return 0;
>  }
>  
> @@ -718,6 +748,7 @@ struct usb_driver usbfs_driver = {
>  	.disconnect =	driver_disconnect,
>  	.suspend =	driver_suspend,
>  	.resume =	driver_resume,
> +	.supports_autosuspend = 1,
>  };
>  
>  static int claimintf(struct usb_dev_state *ps, unsigned int ifnum)
> @@ -963,6 +994,27 @@ static struct usb_device *usbdev_lookup_by_devt(dev_t devt)
>  	return to_usb_device(dev);
>  }
>  
> +/* must be called with usb-dev lock held */
> +static int usbdev_do_resume(struct usb_dev_state *ps)
> +{
> +	int ret = 0;
> +
> +	if (!ps->runtime_active) {
> +		snoop(&ps->dev->dev, "suspended..resume now\n");

Again snoop?

> +		ps->is_suspended = false;
> +		if (usb_autoresume_device(ps->dev)) {
> +			ret = -EIO;
> +			goto _out;
> +		}
> +		snoop(&ps->dev->dev, "resume done..active now\n");
> +		ps->runtime_active = true;
> +		wake_up(&ps->wait_resume);

No locking at all?

> +	}
> +
> +_out:

Why is this even needed, just return -EIO above and all is good.

But again, is any locking needed here?


> +	return ret;
> +}
> +
>  /*
>   * file operations
>   */
> @@ -1008,6 +1060,9 @@ static int usbdev_open(struct inode *inode, struct file *file)
>  	INIT_LIST_HEAD(&ps->async_completed);
>  	INIT_LIST_HEAD(&ps->memory_list);
>  	init_waitqueue_head(&ps->wait);
> +	init_waitqueue_head(&ps->wait_resume);
> +	ps->runtime_active = true;
> +	ps->is_suspended = false;
>  	ps->disc_pid = get_pid(task_pid(current));
>  	ps->cred = get_current_cred();
>  	smp_wmb();
> @@ -1034,6 +1089,10 @@ static int usbdev_release(struct inode *inode, struct file *file)
>  	struct async *as;
>  
>  	usb_lock_device(dev);
> +
> +	/* what can we can do if resume fails? */

You tell me!

> +	usbdev_do_resume(ps);
> +
>  	usb_hub_release_all_ports(dev, ps);
>  
>  	list_del_init(&ps->list);
> @@ -2355,6 +2414,18 @@ static int proc_drop_privileges(struct usb_dev_state *ps, void __user *arg)
>  	return 0;
>  }
>  
> +static int proc_wait_resume(struct usb_dev_state *ps)
> +{
> +	int ret;
> +
> +	snoop(&ps->dev->dev, "wait-for-resume\n");
> +	ret = wait_event_interruptible(ps->wait_resume,
> +		(ps->runtime_active == true));
> +	snoop(&ps->dev->dev, "wait-for-resume...done\n");

This is just debugging code, right?  Please remove.

> +
> +	return ret;
> +}
> +
>  /*
>   * NOTE:  All requests here that have interface numbers as parameters
>   * are assuming that somehow the configuration has been prevented from
> @@ -2373,6 +2444,25 @@ static long usbdev_do_ioctl(struct file *file, unsigned int cmd,
>  
>  	usb_lock_device(dev);
>  
> +	if (cmd != USBDEVFS_WAIT_RESUME) {
> +		ret = usbdev_do_resume(ps);

do resume for all ioctl commands?  are you sure?

> +		if (ret)
> +			goto done;
> +	} else {
> +		usb_unlock_device(dev);
> +		ret = proc_wait_resume(ps);
> +
> +		/* Call auto-resume to balance auto-suspend of suspend-ioctl */
> +		usb_lock_device(dev);
> +		if (ps->is_suspended) {
> +			ret = usb_autoresume_device(ps->dev);
> +			ps->is_suspended = false;
> +		}
> +		usb_unlock_device(dev);
> +
> +		goto _done;

What is the difference between _done and done?  Please have descriptive
labels if you are going to have any at all.

Why isn't this part of the switch statement below?

> +	}
> +
>  	/* Reap operations are allowed even after disconnection */
>  	switch (cmd) {
>  	case USBDEVFS_REAPURB:
> @@ -2549,10 +2639,26 @@ static long usbdev_do_ioctl(struct file *file, unsigned int cmd,
>  	case USBDEVFS_GET_SPEED:
>  		ret = ps->dev->speed;
>  		break;
> +	case USBDEVFS_SUSPEND:
> +		ret = 0;
> +
> +		/* we cannot suspend when URB is pending */
> +		if (async_getpending_count(ps)) {
> +			snoop(&ps->dev->dev, "ioctl-suspend but URB pending\n");

You do not know this, your value is stale :(

And again, you are using snoop() calls for debugging, not for actual USB
traffic, which is what it was designed for.  These all need to be
removed/rewritten.

> +			ret = -EINVAL;
> +		} else {
> +			if (ps->runtime_active) {
> +				snoop(&ps->dev->dev, "ioctl-suspend\n");
> +				ps->runtime_active = false;
> +				usb_autosuspend_device(ps->dev);
> +			}
> +		}
> +		break;
>  	}
>  
> - done:
> +done:
>  	usb_unlock_device(dev);
> +_done:

See, horrid names :(

>  	if (ret >= 0)
>  		inode->i_atime = current_time(inode);
>  	return ret;
> diff --git a/include/uapi/linux/usbdevice_fs.h b/include/uapi/linux/usbdevice_fs.h
> index 964e872..ae46beb 100644
> --- a/include/uapi/linux/usbdevice_fs.h
> +++ b/include/uapi/linux/usbdevice_fs.h
> @@ -197,5 +197,7 @@ 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_SUSPEND           _IO('U', 32)
> +#define USBDEVFS_WAIT_RESUME       _IO('U', 33)

No documentation for what these do?

thanks,

greg k-h

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Alan Stern]

On Wed, 5 Jun 2019, Greg KH wrote:

> On Fri, May 10, 2019 at 11:01:09AM +0100, Mayuresh Kulkarni wrote:
> > - The current driver increments the PM ref-count in its .open
> > API and decrements it in its .close API.
> > - Due to this, it is not possible for the usb_device to go into
> > suspend (L2) even if all of its interfaces report idle to usb-core.
> > - In order to allow the suspend, 2 new ioctls are added:
> > 1. USBDEVFS_SUSPEND: calls auto-suspend and indicates to usb/pm core
> > to attempt suspend, if appropriate. This is a non-blocking call.
> > 2. USBDEVFS_WAIT_RESUME: waits for resume. This is a blocking call.
> > The resume could happen due to:
> > host-wake (i.e.: any driver bound to interface(s) of device wants to
> > send/receive control/data)
> > OR
> > remote-wake (i.e.: when remote-wake enabled device generates a
> > remote-wake to host).
> > In both these conditions, this call will return.
> > - It is expected that:
> > 1. When user-space thinks the device is idle from its point-of-view,
> > it calls USBDEVFS_SUSPEND.
> > 2. After USBDEVFS_WAIT_RESUME call returns,
> > the callers queries the device/interface of its interest to see
> > what caused resume and take appropriate action on it.
> 
> I'm going to make a bunch of random comments on this patch, some
> cosmetic...
> 
> First off, the above is horrible to try to read, please format things in
> a sane way such that we can understand it well.
> 
> > The link to relevant discussion about this patch on linux-usb is -
> > https://www.spinics.net/lists/linux-usb/msg173285.html
> 
> You should not need to reference any external web site for a changelog
> entry, just put the needed information in the changelog itself.
> 
> > Signed-off-by: Mayuresh Kulkarni <mkulkarni@opensource.cirrus.com>

Mayuresh:

I'm sorry; I completely lost sight of this patch (distracted by too 
many other things).

Aside from the issues Greg raised, it isn't right because it relies on 
the suspend and resume callbacks for individual interfaces, not for the 
whole device.  There are a few other things that should be changed as 
well.

Below is my attempt at doing the same thing (not tested, and it doesn't
answer all of Greg's objections).  It is very similar to your patch.  
Does it work for your application?

(Note: I imagine you might run into trouble because devices generally 
do not get put into runtime suspend immediately.  So if you call the 
USBDEVFS_SUSPEND ioctl and then the USBDEVFS_WAIT_FOR_RESUME ioctl, the 
wait will return immediately because the device hasn't yet been 
suspended.)

Alan Stern


 drivers/usb/core/devio.c          |   66 +++++++++++++++++++++++++++++++++++---
 drivers/usb/core/generic.c        |    5 ++
 drivers/usb/core/usb.h            |    3 +
 include/uapi/linux/usbdevice_fs.h |    2 +
 4 files changed, 72 insertions(+), 4 deletions(-)

Index: usb-devel/drivers/usb/core/devio.c
===================================================================
--- usb-devel.orig/drivers/usb/core/devio.c
+++ usb-devel/drivers/usb/core/devio.c
@@ -60,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;
+	bool privileges_dropped;
+	bool is_suspended;
+	bool suspend_requested;
 };
 
 struct usb_memory {
@@ -699,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;
@@ -712,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)
+{
+	struct usb_dev_state *ps;
+
+	list_for_each_entry(ps, &udev->filelist, list)
+		ps->is_suspended = true;
+}
+
+void usbfs_notify_resume(struct usb_device *udev)
+{
+	struct usb_dev_state *ps;
+
+	list_for_each_entry(ps, &udev->filelist, list) {
+		ps->is_suspended = false;
+		wake_up_all(&ps->wait_for_resume);
+	}
+}
+
 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)
@@ -1008,6 +1029,7 @@ 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();
@@ -1034,6 +1056,8 @@ static int usbdev_release(struct inode *
 	struct async *as;
 
 	usb_lock_device(dev);
+	if (ps->suspend_requested)
+		usb_autoresume_device(dev);
 	usb_hub_release_all_ports(dev, ps);
 
 	list_del_init(&ps->list);
@@ -2355,6 +2379,17 @@ static int proc_drop_privileges(struct u
 	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, !ps->is_suspended);
+	usb_lock_device(ps->dev);
+
+	return ret;
+}
+
 /*
  * NOTE:  All requests here that have interface numbers as parameters
  * are assuming that somehow the configuration has been prevented from
@@ -2373,6 +2408,17 @@ static long usbdev_do_ioctl(struct file
 
 	usb_lock_device(dev);
 
+	/* Almost all operations require the device to be runtime-active */
+	if (cmd != USBDEVFS_SUSPEND && cmd != USBDEVFS_WAIT_FOR_RESUME &&
+			ps->suspend_requested) {
+		if (usb_autoresume_device(dev) == 0) {
+			ps->suspend_requested = false;
+		} else {
+			ret = -EIO;
+			goto done;
+		}
+	}
+
 	/* Reap operations are allowed even after disconnection */
 	switch (cmd) {
 	case USBDEVFS_REAPURB:
@@ -2549,6 +2595,16 @@ static long usbdev_do_ioctl(struct file
 	case USBDEVFS_GET_SPEED:
 		ret = ps->dev->speed;
 		break;
+	case USBDEVFS_SUSPEND:
+		if (!ps->suspend_requested) {
+			usb_autosuspend_device(dev);
+			ps->suspend_requested = true;
+		}
+		ret = 0;
+		break;
+	case USBDEVFS_WAIT_FOR_RESUME:
+		ret = proc_wait_for_resume(ps);
+		break;
 	}
 
  done:
@@ -2620,6 +2676,8 @@ static void usbdev_remove(struct usb_dev
 		ps = list_entry(udev->filelist.next, struct usb_dev_state, list);
 		destroy_all_async(ps);
 		wake_up_all(&ps->wait);
+		ps->is_suspended = false;
+		wake_up_all(&ps->wait_for_resume);
 		list_del_init(&ps->list);
 		if (ps->discsignr) {
 			clear_siginfo(&sinfo);
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,7 @@ 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_SUSPEND           _IO('U', 32)
+#define USBDEVFS_WAIT_FOR_RESUME   _IO('U', 33)
 
 #endif /* _UAPI_LINUX_USBDEVICE_FS_H */

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Mayuresh Kulkarni]

On Wed, 2019-06-05 at 17:02 -0400, Alan Stern wrote:
> On Wed, 5 Jun 2019, Greg KH wrote:
> 
> > 
> > On Fri, May 10, 2019 at 11:01:09AM +0100, Mayuresh Kulkarni wrote:
> > > 
> > > - The current driver increments the PM ref-count in its .open
> > > API and decrements it in its .close API.
> > > - Due to this, it is not possible for the usb_device to go into
> > > suspend (L2) even if all of its interfaces report idle to usb-core.
> > > - In order to allow the suspend, 2 new ioctls are added:
> > > 1. USBDEVFS_SUSPEND: calls auto-suspend and indicates to usb/pm core
> > > to attempt suspend, if appropriate. This is a non-blocking call.
> > > 2. USBDEVFS_WAIT_RESUME: waits for resume. This is a blocking call.
> > > The resume could happen due to:
> > > host-wake (i.e.: any driver bound to interface(s) of device wants to
> > > send/receive control/data)
> > > OR
> > > remote-wake (i.e.: when remote-wake enabled device generates a
> > > remote-wake to host).
> > > In both these conditions, this call will return.
> > > - It is expected that:
> > > 1. When user-space thinks the device is idle from its point-of-view,
> > > it calls USBDEVFS_SUSPEND.
> > > 2. After USBDEVFS_WAIT_RESUME call returns,
> > > the callers queries the device/interface of its interest to see
> > > what caused resume and take appropriate action on it.
> > I'm going to make a bunch of random comments on this patch, some
> > cosmetic...
> > 
> > First off, the above is horrible to try to read, please format things in
> > a sane way such that we can understand it well.
> > 
> > > 
> > > The link to relevant discussion about this patch on linux-usb is -
> > > https://www.spinics.net/lists/linux-usb/msg173285.html
> > You should not need to reference any external web site for a changelog
> > entry, just put the needed information in the changelog itself.
> > 
> > > 
> > > Signed-off-by: Mayuresh Kulkarni <mkulkarni@opensource.cirrus.com>
> Mayuresh:
> 
> I'm sorry; I completely lost sight of this patch (distracted by too 
> many other things).
> 

Hi Alan,

Thanks for your review and apologies for late response from me. I was on PTO last week and then in a training this week.

> Aside from the issues Greg raised, it isn't right because it relies on 
> the suspend and resume callbacks for individual interfaces, not for the 
> whole device.  There are a few other things that should be changed as 
> well.

In our use-case, we open the USB device with our VID/PID and then using that fd
we bind to our interface. So this approach probably worked for our use-case.

> Below is my attempt at doing the same thing (not tested, and it doesn't
> answer all of Greg's objections).  It is very similar to your patch.  
> Does it work for your application?
> 

I am checking your code-changes and will get back to you on this by next week.

> (Note: I imagine you might run into trouble because devices generally 
> do not get put into runtime suspend immediately.  So if you call the 
> USBDEVFS_SUSPEND ioctl and then the USBDEVFS_WAIT_FOR_RESUME ioctl, the 
> wait will return immediately because the device hasn't yet been 
> suspended.)
> 

For this point, I am suggesting below -
How about we return "udev->dev.power.usage_count" from suspend ioctl?
count = 0 -> suspend success so good to call wait-for-resume ioctl
count != 0 -> don't call resume since suspend did not happen.

Will that work?

> Alan Stern
> 
> 
>  drivers/usb/core/devio.c          |   66 +++++++++++++++++++++++++++++++++++-
> --
>  drivers/usb/core/generic.c        |    5 ++
>  drivers/usb/core/usb.h            |    3 +
>  include/uapi/linux/usbdevice_fs.h |    2 +
>  4 files changed, 72 insertions(+), 4 deletions(-)
> 
> Index: usb-devel/drivers/usb/core/devio.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/core/devio.c
> +++ usb-devel/drivers/usb/core/devio.c
> @@ -60,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;
> +	bool privileges_dropped;
> +	bool is_suspended;
> +	bool suspend_requested;
>  };
>  
>  struct usb_memory {
> @@ -699,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;
> @@ -712,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)
> +{
> +	struct usb_dev_state *ps;
> +
> +	list_for_each_entry(ps, &udev->filelist, list)
> +		ps->is_suspended = true;
> +}
> +
> +void usbfs_notify_resume(struct usb_device *udev)
> +{
> +	struct usb_dev_state *ps;
> +
> +	list_for_each_entry(ps, &udev->filelist, list) {
> +		ps->is_suspended = false;
> +		wake_up_all(&ps->wait_for_resume);
> +	}
> +}
> +
>  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)
> @@ -1008,6 +1029,7 @@ 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();
> @@ -1034,6 +1056,8 @@ static int usbdev_release(struct inode *
>  	struct async *as;
>  
>  	usb_lock_device(dev);
> +	if (ps->suspend_requested)
> +		usb_autoresume_device(dev);
>  	usb_hub_release_all_ports(dev, ps);
>  
>  	list_del_init(&ps->list);
> @@ -2355,6 +2379,17 @@ static int proc_drop_privileges(struct u
>  	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, !ps-
> >is_suspended);
> +	usb_lock_device(ps->dev);
> +
> +	return ret;
> +}
> +
>  /*
>   * NOTE:  All requests here that have interface numbers as parameters
>   * are assuming that somehow the configuration has been prevented from
> @@ -2373,6 +2408,17 @@ static long usbdev_do_ioctl(struct file
>  
>  	usb_lock_device(dev);
>  
> +	/* Almost all operations require the device to be runtime-active */
> +	if (cmd != USBDEVFS_SUSPEND && cmd != USBDEVFS_WAIT_FOR_RESUME &&
> +			ps->suspend_requested) {
> +		if (usb_autoresume_device(dev) == 0) {
> +			ps->suspend_requested = false;
> +		} else {
> +			ret = -EIO;
> +			goto done;
> +		}
> +	}
> +
>  	/* Reap operations are allowed even after disconnection */
>  	switch (cmd) {
>  	case USBDEVFS_REAPURB:
> @@ -2549,6 +2595,16 @@ static long usbdev_do_ioctl(struct file
>  	case USBDEVFS_GET_SPEED:
>  		ret = ps->dev->speed;
>  		break;
> +	case USBDEVFS_SUSPEND:
> +		if (!ps->suspend_requested) {
> +			usb_autosuspend_device(dev);
> +			ps->suspend_requested = true;
> +		}
> +		ret = 0;
> +		break;
> +	case USBDEVFS_WAIT_FOR_RESUME:
> +		ret = proc_wait_for_resume(ps);
> +		break;
>  	}
>  
>   done:
> @@ -2620,6 +2676,8 @@ static void usbdev_remove(struct usb_dev
>  		ps = list_entry(udev->filelist.next, struct usb_dev_state,
> list);
>  		destroy_all_async(ps);
>  		wake_up_all(&ps->wait);
> +		ps->is_suspended = false;
> +		wake_up_all(&ps->wait_for_resume);
>  		list_del_init(&ps->list);
>  		if (ps->discsignr) {
>  			clear_siginfo(&sinfo);
> 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,7 @@ 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_SUSPEND           _IO('U', 32)
> +#define USBDEVFS_WAIT_FOR_RESUME   _IO('U', 33)
>  
>  #endif /* _UAPI_LINUX_USBDEVICE_FS_H */
> 

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Alan Stern]

On Thu, 13 Jun 2019, Mayuresh Kulkarni wrote:

> Hi Alan,
> 
> Thanks for your review and apologies for late response from me. I was on PTO last week and then in a training this week.
> 
> > Aside from the issues Greg raised, it isn't right because it relies on 
> > the suspend and resume callbacks for individual interfaces, not for the 
> > whole device.  There are a few other things that should be changed as 
> > well.
> 
> In our use-case, we open the USB device with our VID/PID and then using that fd
> we bind to our interface. So this approach probably worked for our use-case.

Yes.  But it seems more reliable to use suspend/resume information for 
the whole device, instead of assuming that the userspace program will 
have claimed an interface.

> > Below is my attempt at doing the same thing (not tested, and it doesn't
> > answer all of Greg's objections).  It is very similar to your patch.  
> > Does it work for your application?
> > 
> 
> I am checking your code-changes and will get back to you on this by next week.
> 
> > (Note: I imagine you might run into trouble because devices generally 
> > do not get put into runtime suspend immediately.  So if you call the 
> > USBDEVFS_SUSPEND ioctl and then the USBDEVFS_WAIT_FOR_RESUME ioctl, the 
> > wait will return immediately because the device hasn't yet been 
> > suspended.)
> > 
> 
> For this point, I am suggesting below -
> How about we return "udev->dev.power.usage_count" from suspend ioctl?
> count = 0 -> suspend success so good to call wait-for-resume ioctl
> count != 0 -> don't call resume since suspend did not happen.
> 
> Will that work?

No, it will not.  The usage_count value can change at any time, so it 
will be out-of-date by the time the ioctl returns.  Furthermore, even 
if usage_count is > 0 when the suspend ioctl returns, it may become 0 
later on, and the device will be suspended some time after that.

In fact, if you use the default settings for USB autosuspend, the 
device won't be suspended until 2 seconds after the usage_count becomes 
0.  So even if the suspend ioctl decrements usage_count to 0, the 
device still won't be suspended right away.  If you call the 
wait-for-resume ioctl immediately, the call will fail.

Alan Stern

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Mayuresh Kulkarni]

On Wed, 2019-06-05 at 17:02 -0400, Alan Stern wrote:
> On Wed, 5 Jun 2019, Greg KH wrote:
> 
> > 
> > On Fri, May 10, 2019 at 11:01:09AM +0100, Mayuresh Kulkarni wrote:
> > > 
> > > - The current driver increments the PM ref-count in its .open
> > > API and decrements it in its .close API.
> > > - Due to this, it is not possible for the usb_device to go into
> > > suspend (L2) even if all of its interfaces report idle to usb-
> > > core.
> > > - In order to allow the suspend, 2 new ioctls are added:
> > > 1. USBDEVFS_SUSPEND: calls auto-suspend and indicates to usb/pm
> > > core
> > > to attempt suspend, if appropriate. This is a non-blocking call.
> > > 2. USBDEVFS_WAIT_RESUME: waits for resume. This is a blocking
> > > call.
> > > The resume could happen due to:
> > > host-wake (i.e.: any driver bound to interface(s) of device wants
> > > to
> > > send/receive control/data)
> > > OR
> > > remote-wake (i.e.: when remote-wake enabled device generates a
> > > remote-wake to host).
> > > In both these conditions, this call will return.
> > > - It is expected that:
> > > 1. When user-space thinks the device is idle from its point-of-
> > > view,
> > > it calls USBDEVFS_SUSPEND.
> > > 2. After USBDEVFS_WAIT_RESUME call returns,
> > > the callers queries the device/interface of its interest to see
> > > what caused resume and take appropriate action on it.
> > I'm going to make a bunch of random comments on this patch, some
> > cosmetic...
> > 
> > First off, the above is horrible to try to read, please format
> > things in
> > a sane way such that we can understand it well.
> > 
> > > 
> > > The link to relevant discussion about this patch on linux-usb is -
> > > https://www.spinics.net/lists/linux-usb/msg173285.html
> > You should not need to reference any external web site for a
> > changelog
> > entry, just put the needed information in the changelog itself.
> > 
> > > 
> > > Signed-off-by: Mayuresh Kulkarni <mkulkarni@opensource.cirrus.com>
> Mayuresh:
> 
> I'm sorry; I completely lost sight of this patch (distracted by too 
> many other things).
> 
> Aside from the issues Greg raised, it isn't right because it relies
> on 
> the suspend and resume callbacks for individual interfaces, not for
> the 
> whole device.  There are a few other things that should be changed as 
> well.
> 
> Below is my attempt at doing the same thing (not tested, and it
> doesn't
> answer all of Greg's objections).  It is very similar to your patch.  
> Does it work for your application?
> 
> (Note: I imagine you might run into trouble because devices generally 
> do not get put into runtime suspend immediately.  So if you call the 
> USBDEVFS_SUSPEND ioctl and then the USBDEVFS_WAIT_FOR_RESUME ioctl,
> the 
> wait will return immediately because the device hasn't yet been 
> suspended.)
> 

Hi Alan,

For this particular comment, how about we add suspend-waiter similar to
resume-waiter?
As per the below changes, usbfs_notify_suspend() can wake the suspend-
waiter, if generic_suspend() is called. So, the suspend ioctl will be
partial blocking i.e.: it will wait till suspend happens and will return
when it is safe to do resume.

Will this work?

The reason for asking this is - I think the suspend ioctl should return
appropriate status to user-space indicating weather to wait-for-resume
or not.

Or are you suggesting to always have a delay in suspend/resume in user-
space?

Please do review my comment below in this context too.

> 

> 
> 
>  drivers/usb/core/devio.c          |   66
> +++++++++++++++++++++++++++++++++++---
>  drivers/usb/core/generic.c        |    5 ++
>  drivers/usb/core/usb.h            |    3 +
>  include/uapi/linux/usbdevice_fs.h |    2 +
>  4 files changed, 72 insertions(+), 4 deletions(-)
> 
> Index: usb-devel/drivers/usb/core/devio.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/core/devio.c
> +++ usb-devel/drivers/usb/core/devio.c
> @@ -60,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;
> +	bool privileges_dropped;
> +	bool is_suspended;
> +	bool suspend_requested;
>   };

>  struct usb_memory {
> @@ -699,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;
> @@ -712,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)
> +{
> +	struct usb_dev_state *ps;
> +
> +	list_for_each_entry(ps, &udev->filelist, list)
> +		ps->is_suspended = true;
> +}
> +
> +void usbfs_notify_resume(struct usb_device *udev)
> +{
> +	struct usb_dev_state *ps;
> +
> +	list_for_each_entry(ps, &udev->filelist, list) {
> +		ps->is_suspended = false;
> +		wake_up_all(&ps->wait_for_resume);
> +	}
> +}
> +
>  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)
> @@ -1008,6 +1029,7 @@ 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();
> @@ -1034,6 +1056,8 @@ static int usbdev_release(struct inode *
>  	struct async *as;
>  
>  	usb_lock_device(dev);
> +	if (ps->suspend_requested)
> +		usb_autoresume_device(dev);
>  	usb_hub_release_all_ports(dev, ps);
>  
>  	list_del_init(&ps->list);
> @@ -2355,6 +2379,17 @@ static int proc_drop_privileges(struct u
>  	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, !ps-
> >is_suspended);
> +	usb_lock_device(ps->dev);
> +
> +	return ret;
> +}
> +
>  /*
>   * NOTE:  All requests here that have interface numbers as parameters
>   * are assuming that somehow the configuration has been prevented
> from
> @@ -2373,6 +2408,17 @@ static long usbdev_do_ioctl(struct file
>  
>  	usb_lock_device(dev);
>  
> +	/* Almost all operations require the device to be runtime-
> active */
> +	if (cmd != USBDEVFS_SUSPEND && cmd !=
> USBDEVFS_WAIT_FOR_RESUME &&
> +			ps->suspend_requested) {
> +		if (usb_autoresume_device(dev) == 0) {
> +			ps->suspend_requested = false;
> +		} else {
> +			ret = -EIO;
> +			goto done;
> +		}
> +	}
> +
>  	/* Reap operations are allowed even after disconnection */
>  	switch (cmd) {
>  	case USBDEVFS_REAPURB:
> @@ -2549,6 +2595,16 @@ static long usbdev_do_ioctl(struct file
>  	case USBDEVFS_GET_SPEED:
>  		ret = ps->dev->speed;
>  		break;
> +	case USBDEVFS_SUSPEND:
> +		if (!ps->suspend_requested) {
> +			usb_autosuspend_device(dev);
> +			ps->suspend_requested = true;
> +		}
> +		ret = 0;
> +		break;
> +	case USBDEVFS_WAIT_FOR_RESUME:
> +		ret = proc_wait_for_resume(ps);
> +		break;
>  	}
>  
>   done:
> @@ -2620,6 +2676,8 @@ static void usbdev_remove(struct usb_dev
>  		ps = list_entry(udev->filelist.next, struct
> usb_dev_state, list);
>  		destroy_all_async(ps);
>  		wake_up_all(&ps->wait);
> +		ps->is_suspended = false;
> +		wake_up_all(&ps->wait_for_resume);
>  		list_del_init(&ps->list);
>  		if (ps->discsignr) {
>  			clear_siginfo(&sinfo);
> 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;
>  }
>  

In a typical SoC based system (XHCI compliant USB host controller with
one port exposed out on PCB), wouldn't this call usbfs_notify_suspend()
twice - first for udev of connected device and then for udev of root-
hub?

If yes, how about we call usbfs_notify_*() for root-hubs only? That
would be a good indication of suspend/resume since root-hubs will be
suspended last while resumed first.

Will that work?

> @@ -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,7 @@ 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_SUSPEND           _IO('U', 32)
> +#define USBDEVFS_WAIT_FOR_RESUME   _IO('U', 33)
>  
>  #endif /* _UAPI_LINUX_USBDEVICE_FS_H */
> 

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Alan Stern]

On Mon, 17 Jun 2019, Mayuresh Kulkarni wrote:

> > (Note: I imagine you might run into trouble because devices generally 
> > do not get put into runtime suspend immediately.  So if you call the 
> > USBDEVFS_SUSPEND ioctl and then the USBDEVFS_WAIT_FOR_RESUME ioctl,
> > the 
> > wait will return immediately because the device hasn't yet been 
> > suspended.)
> > 
> 
> Hi Alan,
> 
> For this particular comment, how about we add suspend-waiter similar to
> resume-waiter?
> As per the below changes, usbfs_notify_suspend() can wake the suspend-
> waiter, if generic_suspend() is called. So, the suspend ioctl will be
> partial blocking i.e.: it will wait till suspend happens and will return
> when it is safe to do resume.
> 
> Will this work?

Probably not.  Just think about it: Your program stops communicating
with the device and tells the kernel that it's ready for the device to
be suspended.  But the device doesn't go into suspend for several
seconds (or longer!) and during that time your program has no idea
what's happening to it.  I'm pretty sure that's not what you want.

You're right that the program needs to know when the device is about to 
be suspended.  But waiting for an ioctl to return isn't a good way 
to do it; this needs to be a callback of some sort.  That is, the 
kernel also needs to know when the program is ready for the suspend.

I don't know what is the best approach.

> The reason for asking this is - I think the suspend ioctl should return
> appropriate status to user-space indicating weather to wait-for-resume
> or not.
> 
> Or are you suggesting to always have a delay in suspend/resume in user-
> space?
> 
> Please do review my comment below in this context too.

> In a typical SoC based system (XHCI compliant USB host controller with
> one port exposed out on PCB), wouldn't this call usbfs_notify_suspend()
> twice - first for udev of connected device and then for udev of root-
> hub?

Yes, it would.  This wouldn't make any difference to your program, 
since your program would have an open file reference only for the 
connected device, not for the root hub.

> If yes, how about we call usbfs_notify_*() for root-hubs only? That
> would be a good indication of suspend/resume since root-hubs will be
> suspended last while resumed first.
> 
> Will that work?

No.  Remember, this mechanism has to work on non-SoC systems too.  And 
even on an SoC, it's possible that your device is just one of several 
plugged into an external hub.  So your device might be suspended while 
the others remain active; then the root hub would not be suspended.

Alan Stern

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Mayuresh Kulkarni]

On Mon, 2019-06-17 at 11:55 -0400, Alan Stern wrote:
> On Mon, 17 Jun 2019, Mayuresh Kulkarni wrote:
> 
> > 
> > > 
> > > (Note: I imagine you might run into trouble because devices
> > > generally 
> > > do not get put into runtime suspend immediately.  So if you call
> > > the 
> > > USBDEVFS_SUSPEND ioctl and then the USBDEVFS_WAIT_FOR_RESUME
> > > ioctl,
> > > the 
> > > wait will return immediately because the device hasn't yet been 
> > > suspended.)
> > > 
> > Hi Alan,
> > 
> > For this particular comment, how about we add suspend-waiter similar
> > to
> > resume-waiter?
> > As per the below changes, usbfs_notify_suspend() can wake the
> > suspend-
> > waiter, if generic_suspend() is called. So, the suspend ioctl will
> > be
> > partial blocking i.e.: it will wait till suspend happens and will
> > return
> > when it is safe to do resume.
> > 
> > Will this work?
> Probably not.  Just think about it: Your program stops communicating
> with the device and tells the kernel that it's ready for the device to
> be suspended.  But the device doesn't go into suspend for several
> seconds (or longer!) and during that time your program has no idea
> what's happening to it.  I'm pretty sure that's not what you want.
> 

Right, sounds good. Thanks.

> You're right that the program needs to know when the device is about
> to 
> be suspended.  But waiting for an ioctl to return isn't a good way 
> to do it; this needs to be a callback of some sort.  That is, the 
> kernel also needs to know when the program is ready for the suspend.
> 
> I don't know what is the best approach.

This is becoming tricky now.

> 
> > 
> > The reason for asking this is - I think the suspend ioctl should
> > return
> > appropriate status to user-space indicating weather to wait-for-
> > resume
> > or not.
> > 
> > Or are you suggesting to always have a delay in suspend/resume in
> > user-
> > space?
> > 
> > Please do review my comment below in this context too.
> > 
> > In a typical SoC based system (XHCI compliant USB host controller
> > with
> > one port exposed out on PCB), wouldn't this
> > call usbfs_notify_suspend()
> > twice - first for udev of connected device and then for udev of
> > root-
> > hub?
> Yes, it would.  This wouldn't make any difference to your program, 
> since your program would have an open file reference only for the 
> connected device, not for the root hub.
> 
> > 
> > If yes, how about we call usbfs_notify_*() for root-hubs only? That
> > would be a good indication of suspend/resume since root-hubs will be
> > suspended last while resumed first.
> > 
> > Will that work?
> No.  Remember, this mechanism has to work on non-SoC systems
> too.  And 
> even on an SoC, it's possible that your device is just one of several 
> plugged into an external hub.  So your device might be suspended
> while 
> the others remain active; then the root hub would not be suspended.
> 

I think my point is - usbfs driver is actually doing nothing w.r.t USB-
2.0 L2 state, right? The root-hub's suspend will invoke the USB-2.0 L2
transitions. This will happen when all the USB devices on that port
report idle to USB-core.
I agree that usually driver's suspend/resume call-back will put the
device in its low power state. But that is not applicable to udev of
usbfs driver, right?

So, doesn't it makes sense to tell user-space about actual USB-2.0 L2
state transitions rather than suspend/resume entry call-backs of device-
driver model of kernel (which are stub in this context)?

> Alan Stern
> 

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Alan Stern]

On Tue, 18 Jun 2019, Mayuresh Kulkarni wrote:

> > You're right that the program needs to know when the device is about
> > to 
> > be suspended.  But waiting for an ioctl to return isn't a good way 
> > to do it; this needs to be a callback of some sort.  That is, the 
> > kernel also needs to know when the program is ready for the suspend.
> > 
> > I don't know what is the best approach.
> 
> This is becoming tricky now.

Yes.  There probably are mechanisms already in use in other parts of 
the kernel that would be suitable here, but I don't know what they are.  
We could try asking some other people for advice.

> I think my point is - usbfs driver is actually doing nothing w.r.t USB-
> 2.0 L2 state, right? The root-hub's suspend will invoke the USB-2.0 L2
> transitions. This will happen when all the USB devices on that port
> report idle to USB-core.
> I agree that usually driver's suspend/resume call-back will put the
> device in its low power state. But that is not applicable to udev of
> usbfs driver, right?

It doesn't work quite the way you think.  The suspend callback informs
the driver that the device is about to enter a low-power state.  The
callback's job is to make the driver stop trying to communicate with
the device, since all such communication attempts will fail once the
device is suspended.

The suspend callback is _not_ responsible for actually suspending the
device; that is handled by the USB subsystem core.

These ideas are indeed applicable to programs using usbfs.  The kernel
needs to have a way to inform the program that the device is about
enter (or has just left) a low-power state, so that the program can
stop (or start) trying to communicate with the device.  And the kernel 
needs to know when the program is ready for the state change.

> So, doesn't it makes sense to tell user-space about actual USB-2.0 L2
> state transitions rather than suspend/resume entry call-backs of device-
> driver model of kernel (which are stub in this context)?

Yes, that's what we need to figure out.  But things have to happen in 
the right order; otherwise you could encounter a situation where the 
userspace program's URBs start failing and the program doesn't learn 
until later that the reason is because the device is being suspended.

Alan Stern

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Oliver Neukum]

Am Dienstag, den 18.06.2019, 11:50 -0400 schrieb Alan Stern:
> On Tue, 18 Jun 2019, Mayuresh Kulkarni wrote:
> 
> > > You're right that the program needs to know when the device is about
> > > to 
> > > be suspended.  But waiting for an ioctl to return isn't a good way 
> > > to do it; this needs to be a callback of some sort.  That is, the 
> > > kernel also needs to know when the program is ready for the suspend.
> > > 
> > > I don't know what is the best approach.
> > 
> > This is becoming tricky now.
> 
> Yes.  There probably are mechanisms already in use in other parts of 
> the kernel that would be suitable here, but I don't know what they are.  
> We could try asking some other people for advice.

Waiting for an ioctl() is horrible. If you really want to do this
poll() would be the obvious API. It is made for waiting for changes
of states.

[..]
> The suspend callback is _not_ responsible for actually suspending the
> device; that is handled by the USB subsystem core.
> 
> These ideas are indeed applicable to programs using usbfs.  The kernel

Not fully. Usbfs has the same issue as FUSE here. Drivers are per
interface but power management is per device. Hence every driver
is in the block IO path for these issues. You cannot do block IO
in user space. The best you can do is notify of state changes,
but you cannot wait for them.

> needs to have a way to inform the program that the device is about
> enter (or has just left) a low-power state, so that the program can
> stop (or start) trying to communicate with the device.  And the kernel 
> needs to know when the program is ready for the state change.

That has difficulties based in fundamental issues. We can let user
space block power transitions. We can notify it. But we cannot
block on it.

It would be easiest to export the usb_autopm_* API to user space.

	Regards
		Oliver

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Alan Stern]

On Wed, 19 Jun 2019, Oliver Neukum wrote:

> Am Dienstag, den 18.06.2019, 11:50 -0400 schrieb Alan Stern:
> > On Tue, 18 Jun 2019, Mayuresh Kulkarni wrote:
> > 
> > > > You're right that the program needs to know when the device is about
> > > > to 
> > > > be suspended.  But waiting for an ioctl to return isn't a good way 
> > > > to do it; this needs to be a callback of some sort.  That is, the 
> > > > kernel also needs to know when the program is ready for the suspend.
> > > > 
> > > > I don't know what is the best approach.
> > > 
> > > This is becoming tricky now.
> > 
> > Yes.  There probably are mechanisms already in use in other parts of 
> > the kernel that would be suitable here, but I don't know what they are.  
> > We could try asking some other people for advice.
> 
> Waiting for an ioctl() is horrible. If you really want to do this
> poll() would be the obvious API. It is made for waiting for changes
> of states.
> 
> [..]
> > The suspend callback is _not_ responsible for actually suspending the
> > device; that is handled by the USB subsystem core.
> > 
> > These ideas are indeed applicable to programs using usbfs.  The kernel
> 
> Not fully. Usbfs has the same issue as FUSE here. Drivers are per
> interface but power management is per device. Hence every driver
> is in the block IO path for these issues. You cannot do block IO
> in user space. The best you can do is notify of state changes,
> but you cannot wait for them.

usbfs access is per-device, not per-interface, but your point remains 
valid.

> > needs to have a way to inform the program that the device is about
> > enter (or has just left) a low-power state, so that the program can
> > stop (or start) trying to communicate with the device.  And the kernel 
> > needs to know when the program is ready for the state change.
> 
> That has difficulties based in fundamental issues. We can let user
> space block power transitions. We can notify it. But we cannot
> block on it.
> 
> It would be easiest to export the usb_autopm_* API to user space.

But ->suspend and ->resume callbacks are part of that API, and as you 
say, it is not feasible to have these callbacks run in userspace.

The only solution I can think of is for the userspace program to first
set the device's autosuspend delay to 0.  Then whenever the
WAIT_FOR_RESUME ioctl returns -- even if it returns immediately -- the
program should assume the suspend is over or is not going to happen.  
Then the program can run normally for a short while (10 seconds,
perhaps) before making another attempt to suspend.

The only change I would make to the patch posted earlier is to have 
proc_wait_for_resume() call usb_autoresume_device() and set 
ps->suspend_requested = false before returning.

Mayuresh, how does that sound?

Alan Stern

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Oliver Neukum]

Am Mittwoch, den 19.06.2019, 10:40 -0400 schrieb Alan Stern:
> On Wed, 19 Jun 2019, Oliver Neukum wrote:

> > It would be easiest to export the usb_autopm_* API to user space.
> 
> But ->suspend and ->resume callbacks are part of that API, and as you 
> say, it is not feasible to have these callbacks run in userspace.

We can notify user space about resume(). There will be a delay, but
there will always be a delay, even in kernel space.

> The only solution I can think of is for the userspace program to first
> set the device's autosuspend delay to 0.  Then whenever the
> WAIT_FOR_RESUME ioctl returns -- even if it returns immediately -- the
> program should assume the suspend is over or is not going to happen.  
> Then the program can run normally for a short while (10 seconds,
> perhaps) before making another attempt to suspend.

That is ugly. Quite ugly actually.

Why actually do we care about suspend()? Would the user space
task do anything else but cease IO? We can do that in usbfs,
assuming we do not care about the order of killing.
User space will learn from an appropriate error code the device
went to power save. And if it does not do IO, why care?
I have never seen a driver that actually saves device state in
suspend().

	Regards
		Oliver

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Alan Stern]

On Wed, 19 Jun 2019, Oliver Neukum wrote:

> Am Mittwoch, den 19.06.2019, 10:40 -0400 schrieb Alan Stern:
> > On Wed, 19 Jun 2019, Oliver Neukum wrote:
> 
> > > It would be easiest to export the usb_autopm_* API to user space.
> > 
> > But ->suspend and ->resume callbacks are part of that API, and as you 
> > say, it is not feasible to have these callbacks run in userspace.
> 
> We can notify user space about resume(). There will be a delay, but
> there will always be a delay, even in kernel space.
> 
> > The only solution I can think of is for the userspace program to first
> > set the device's autosuspend delay to 0.  Then whenever the
> > WAIT_FOR_RESUME ioctl returns -- even if it returns immediately -- the
> > program should assume the suspend is over or is not going to happen.  
> > Then the program can run normally for a short while (10 seconds,
> > perhaps) before making another attempt to suspend.
> 
> That is ugly. Quite ugly actually.
> 
> Why actually do we care about suspend()? Would the user space
> task do anything else but cease IO? We can do that in usbfs,
> assuming we do not care about the order of killing.
> User space will learn from an appropriate error code the device
> went to power save. And if it does not do IO, why care?
> I have never seen a driver that actually saves device state in
> suspend().

Learning from error codes is problematic.  According to the current
design, any usbfs interaction (except for the specific suspend/resume
ioctls) will cause usbfs to do a usb_autoresume_device(), thus
preventing a suspend from occurring.

Evidently we need to rethink this.  In fact, the userspace program
needs to be able to interact with the device right up until the point
where it actually gets suspended.  And of course, this implies that the
program needs to be able to reap URBs even after the device is
suspended.

That sort of approach ought to be workable.  Once the program sees that 
its URBs are failing with the appropriate error codes, it can call the 
WAIT_FOR_RESUME ioctl.

Alan Stern

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Mayuresh Kulkarni]

On Wed, 2019-06-19 at 10:40 -0400, Alan Stern wrote:
> On Wed, 19 Jun 2019, Oliver Neukum wrote:
> 
> > 
> > Am Dienstag, den 18.06.2019, 11:50 -0400 schrieb Alan Stern:
> > > 
> > > On Tue, 18 Jun 2019, Mayuresh Kulkarni wrote:
> > > 
> > > > 
> > > > > 
> > > > > You're right that the program needs to know when the device is
> > > > > about
> > > > > to 
> > > > > be suspended.  But waiting for an ioctl to return isn't a good
> > > > > way 
> > > > > to do it; this needs to be a callback of some sort.  That is,
> > > > > the 
> > > > > kernel also needs to know when the program is ready for the
> > > > > suspend.
> > > > > 
> > > > > I don't know what is the best approach.
> > > > This is becoming tricky now.
> > > Yes.  There probably are mechanisms already in use in other parts
> > > of 
> > > the kernel that would be suitable here, but I don't know what they
> > > are.  
> > > We could try asking some other people for advice.
> > Waiting for an ioctl() is horrible. If you really want to do this
> > poll() would be the obvious API. It is made for waiting for changes
> > of states.
> > 
> > [..]
> > > 
> > > The suspend callback is _not_ responsible for actually suspending
> > > the
> > > device; that is handled by the USB subsystem core.
> > > 
> > > These ideas are indeed applicable to programs using usbfs.  The
> > > kernel
> > Not fully. Usbfs has the same issue as FUSE here. Drivers are per
> > interface but power management is per device. Hence every driver
> > is in the block IO path for these issues. You cannot do block IO
> > in user space. The best you can do is notify of state changes,
> > but you cannot wait for them.
> usbfs access is per-device, not per-interface, but your point remains 
> valid.
> 
> > 
> > > 
> > > needs to have a way to inform the program that the device is about
> > > enter (or has just left) a low-power state, so that the program
> > > can
> > > stop (or start) trying to communicate with the device.  And the
> > > kernel 
> > > needs to know when the program is ready for the state change.
> > That has difficulties based in fundamental issues. We can let user
> > space block power transitions. We can notify it. But we cannot
> > block on it.
> > 
> > It would be easiest to export the usb_autopm_* API to user space.
> But ->suspend and ->resume callbacks are part of that API, and as you 
> say, it is not feasible to have these callbacks run in userspace.
> 
> The only solution I can think of is for the userspace program to first
> set the device's autosuspend delay to 0.  Then whenever the
> WAIT_FOR_RESUME ioctl returns -- even if it returns immediately -- the
> program should assume the suspend is over or is not going to happen.  
> Then the program can run normally for a short while (10 seconds,
> perhaps) before making another attempt to suspend.
> 

Looks like usb_autosuspend_delay parameter is applicable to all USB
devices. Also, since it is a module parameter, it might be tuned on a
particular platform (e.g.: from init.<vendor>.rc on Android).
So, I am not sure if it is good idea to rely on user-space to change it
and restore it to original value when the USB device of interest is
detached.

> The only change I would make to the patch posted earlier is to have 
> proc_wait_for_resume() call usb_autoresume_device() and set 
> ps->suspend_requested = false before returning.
> 
> Mayuresh, how does that sound?

With the code-changes you send me (in response to the
patch), usb_autoresume_device() will be called when the waiter
in proc_wait_for_resume() will return and send some message to "know"
why resume happened.

With above proposed change, proc_wait_for_resume() will return
with usb_autoresume_device() and suspend_requested = false. So when the
user-space will send some message to "know" resume reason, the checks in
ioctl() handler will be skipped.

(Apologies if above is obvious, but still wanted to comment so that we
are on same page).

With that said, I think there would be an issue with "host-wake" case as
below - the sequence of operations are:
- suspend ioctl called: assume actual bus suspend happens
- wait-for-resume ioctl called
- after a while user-space wants to send a message (due to end user
interaction for example)

Here the ioctl() will do usb_autoresume_device() since suspend_requested
= true. This will end up calling usbfs_notify_resume() which will
release waiter in proc_wait_for_resume(). And due to above proposed
change, it will end up calling usb_autoresume_device() again.

As a result, suspend will not happen for next suspend ioctl call.

So, looks like the original proposed change seems better here. On the
side note, I am still in process of verifying the code changes.

> 
> Alan Stern
> 

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Alan Stern]

On Thu, 20 Jun 2019, Mayuresh Kulkarni wrote:

> On Wed, 2019-06-19 at 10:40 -0400, Alan Stern wrote:

> > The only solution I can think of is for the userspace program to first
> > set the device's autosuspend delay to 0.  Then whenever the
> > WAIT_FOR_RESUME ioctl returns -- even if it returns immediately -- the
> > program should assume the suspend is over or is not going to happen.  
> > Then the program can run normally for a short while (10 seconds,
> > perhaps) before making another attempt to suspend.
> > 
> 
> Looks like usb_autosuspend_delay parameter is applicable to all USB
> devices. Also, since it is a module parameter, it might be tuned on a
> particular platform (e.g.: from init.<vendor>.rc on Android).
> So, I am not sure if it is good idea to rely on user-space to change it
> and restore it to original value when the USB device of interest is
> detached.

That's up to you.  There are lots of different ways to set the 
autosuspend delay.  For example, you could create a udev rule that 
would do it only for the devices your program handles.

> > The only change I would make to the patch posted earlier is to have 
> > proc_wait_for_resume() call usb_autoresume_device() and set 
> > ps->suspend_requested = false before returning.
> > 
> > Mayuresh, how does that sound?
> 
> With the code-changes you send me (in response to the
> patch), usb_autoresume_device() will be called when the waiter
> in proc_wait_for_resume() will return and send some message to "know"
> why resume happened.
> 
> With above proposed change, proc_wait_for_resume() will return
> with usb_autoresume_device() and suspend_requested = false. So when the
> user-space will send some message to "know" resume reason, the checks in
> ioctl() handler will be skipped.
> 
> (Apologies if above is obvious, but still wanted to comment so that we
> are on same page).
> 
> With that said, I think there would be an issue with "host-wake" case as
> below - the sequence of operations are:
> - suspend ioctl called: assume actual bus suspend happens
> - wait-for-resume ioctl called
> - after a while user-space wants to send a message (due to end user
> interaction for example)
> 
> Here the ioctl() will do usb_autoresume_device() since suspend_requested
> = true. This will end up calling usbfs_notify_resume() which will
> release waiter in proc_wait_for_resume(). And due to above proposed
> change, it will end up calling usb_autoresume_device() again.
> 
> As a result, suspend will not happen for next suspend ioctl call.

Obviously the code would need to be more careful.  It would call 
usb_autoresume_device() only if ps->suspend_requested was true.

Alan Stern

> So, looks like the original proposed change seems better here. On the
> side note, I am still in process of verifying the code changes.

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Mayuresh Kulkarni]

On Thu, 2019-06-20 at 11:49 -0400, Alan Stern wrote:
> On Thu, 20 Jun 2019, Mayuresh Kulkarni wrote:
> 
> > 
> > On Wed, 2019-06-19 at 10:40 -0400, Alan Stern wrote:
> > 
> > > 
> > > The only solution I can think of is for the userspace program to
> > > first
> > > set the device's autosuspend delay to 0.  Then whenever the
> > > WAIT_FOR_RESUME ioctl returns -- even if it returns immediately --
> > > the
> > > program should assume the suspend is over or is not going to
> > > happen.  
> > > Then the program can run normally for a short while (10 seconds,
> > > perhaps) before making another attempt to suspend.
> > > 
> > Looks like usb_autosuspend_delay parameter is applicable to all USB
> > devices. Also, since it is a module parameter, it might be tuned on
> > a
> > particular platform (e.g.: from init.<vendor>.rc on Android).
> > So, I am not sure if it is good idea to rely on user-space to change
> > it
> > and restore it to original value when the USB device of interest is
> > detached.
> That's up to you.  There are lots of different ways to set the 
> autosuspend delay.  For example, you could create a udev rule that 
> would do it only for the devices your program handles.
> 
> > 
> > > 
> > > The only change I would make to the patch posted earlier is to
> > > have 
> > > proc_wait_for_resume() call usb_autoresume_device() and set 
> > > ps->suspend_requested = false before returning.
> > > 
> > > Mayuresh, how does that sound?
> > With the code-changes you send me (in response to the
> > patch), usb_autoresume_device() will be called when the waiter
> > in proc_wait_for_resume() will return and send some message to
> > "know"
> > why resume happened.
> > 
> > With above proposed change, proc_wait_for_resume() will return
> > with usb_autoresume_device() and suspend_requested = false. So when
> > the
> > user-space will send some message to "know" resume reason, the
> > checks in
> > ioctl() handler will be skipped.
> > 
> > (Apologies if above is obvious, but still wanted to comment so that
> > we
> > are on same page).
> > 
> > With that said, I think there would be an issue with "host-wake"
> > case as
> > below - the sequence of operations are:
> > - suspend ioctl called: assume actual bus suspend happens
> > - wait-for-resume ioctl called
> > - after a while user-space wants to send a message (due to end user
> > interaction for example)
> > 
> > Here the ioctl() will do usb_autoresume_device()
> > since suspend_requested
> > = true. This will end up calling usbfs_notify_resume() which will
> > release waiter in proc_wait_for_resume(). And due to above proposed
> > change, it will end up calling usb_autoresume_device() again.
> > 
> > As a result, suspend will not happen for next suspend ioctl call.
> Obviously the code would need to be more careful.  It would call 
> usb_autoresume_device() only if ps->suspend_requested was true.
> 
> Alan Stern
> 
> > 
> > So, looks like the original proposed change seems better here. On
> > the
> > side note, I am still in process of verifying the code changes.

Hi Alan,

With the suggested modification (of having suspend/resume of usbfs at
device level instead of interface level), looks like I am seeing a
deadlock described as below -

Pre-condition: USB device is connected but suspended before running test
program.

1. The test program calls open(/dev/bus/usb/...).
2. This ends up calling usbdev_open().
3. usbdev_open() takes the lock and calls usb_autoresume_device().
4. usb_autoresume_device() calls pm_runtime_get_sync(). Due to _sync
version, this call will return after calling the resume call-back of
driver (please correct me if wrong).
5. This ends up calling generic_resume() which
calls usbfs_notify_resume().
6. Now usbfs_notify_resume() also wants the same lock that usbdev_open()
in (3) has already taken.

My observation so far is - when the USB device is connected for first
time, Android's USB manager server is able to open the device and reads
its descriptors using usbfs. But the test is not able to. The change is
auto-suspend in between device connect and run of test program.

I am still analysing the situation here to see if pre-condition above
really makes difference or not. So please take this update with pinch of
salt. However, still I wanted send this update and get a quick review to
ensure I am not wandering in weeds.

Thanks,

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Alan Stern]

On Fri, 21 Jun 2019, Mayuresh Kulkarni wrote:

> Hi Alan,
> 
> With the suggested modification (of having suspend/resume of usbfs at
> device level instead of interface level), looks like I am seeing a
> deadlock described as below -
> 
> Pre-condition: USB device is connected but suspended before running test
> program.
> 
> 1. The test program calls open(/dev/bus/usb/...).
> 2. This ends up calling usbdev_open().
> 3. usbdev_open() takes the lock and calls usb_autoresume_device().
> 4. usb_autoresume_device() calls pm_runtime_get_sync(). Due to _sync
> version, this call will return after calling the resume call-back of
> driver (please correct me if wrong).
> 5. This ends up calling generic_resume() which
> calls usbfs_notify_resume().
> 6. Now usbfs_notify_resume() also wants the same lock that usbdev_open()
> in (3) has already taken.

What lock are you talking about?  usbfs_notify_resume() doesn't take 
any locks.

> My observation so far is - when the USB device is connected for first
> time, Android's USB manager server is able to open the device and reads
> its descriptors using usbfs. But the test is not able to. The change is
> auto-suspend in between device connect and run of test program.
> 
> I am still analysing the situation here to see if pre-condition above
> really makes difference or not. So please take this update with pinch of
> salt. However, still I wanted send this update and get a quick review to
> ensure I am not wandering in weeds.

This doesn't sound like a real problem.

However, I have been thinking about how to do all this in light of
Oliver's comments, and it seems like we should make some changes.

First, let's change the ioctls' names.  Instead of RESUME and SUSPEND,
I'll call them FORBID_SUSPEND and ALLOW_SUSPEND.  The way they work
should be clear: ALLOW_SUSPEND will permit the device to be suspended
but might not cause a suspend to happen immediately.  FORBID_SUSPEND
will cause an immediate resume if the device is currently suspended and
will prevent the device from being suspended in the future.  The new 
names more accurately reflect what the ioctls actually do.

Second, the WAIT_FOR_RESUME ioctl will wait only until a resume has
occurred more recently than the most recent ALLOW_SUSPEND ioctl.  So
for example, if the program calls ALLOW_SUSPEND, and the device
suspends, and then the device resumes, and then the device suspends
again, and then the program calls WAIT_FOR_RESUME, the ioctl will
return immediately even though the device is currently suspended.  
Thus you don't have to worry about missing a remote resume.  This also
means that when WAIT_FOR_RESUME returns, the program should call
FORBID_SUSPEND to ensure that the device is active and doesn't go back 
into suspend.

In practice, your program would use the ioctls as follows:

	When the device file is opened, the kernel will automatically
	do the equivalent of FORBID_SUSPEND (to remain compatible 
	with the current behavior).

	When the program is ready for the device to suspend, it will
	call the ALLOW_SUSPEND ioctl.  But it won't cancel the 
	outstanding URBs; instead it will continue to interact 
	normally with the device, because the device might not be 
	suspended for some time.

	When the device does go into suspend, URBs will start failing
	with an appropriate error code (EHOSTUNREACH, ESHUTDOWN, 
	EPROTO, or something similar).  In this way the program will
	realize the device is suspended.  At that point the program
	should call the WAIT_FOR_RESUME ioctl and stop trying to 
	communicate with the device.

	When WAIT_FOR_RESUME returns, the program should call the
	FORBID_SUSPEND ioctl and resume normal communication with the 
	device.

How does that sound?

The proposed patch is below.

Alan Stern


Index: usb-devel/drivers/usb/core/devio.c
===================================================================
--- usb-devel.orig/drivers/usb/core/devio.c
+++ usb-devel/drivers/usb/core/devio.c
@@ -60,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 {
@@ -699,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;
@@ -712,12 +713,29 @@ 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;
+
+	list_for_each_entry(ps, &udev->filelist, list) {
+		WRITE_ONCE(ps->not_yet_resumed, 0);
+		wake_up_all(&ps->wait_for_resume);
+	}
+}
+
 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)
@@ -1008,6 +1026,7 @@ 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();
@@ -1032,8 +1051,11 @@ static int usbdev_release(struct inode *
 	struct usb_device *dev = ps->dev;
 	unsigned int ifnum;
 	struct async *as;
+	int rc = 0;
 
 	usb_lock_device(dev);
+	if (ps->suspend_allowed)
+		rc = usb_autoresume_device(dev);
 	usb_hub_release_all_ports(dev, ps);
 
 	list_del_init(&ps->list);
@@ -1044,7 +1066,8 @@ static int usbdev_release(struct inode *
 			releaseintf(ps, ifnum);
 	}
 	destroy_all_async(ps);
-	usb_autosuspend_device(dev);
+	if (rc == 0)
+		usb_autosuspend_device(dev);
 	usb_unlock_device(dev);
 	usb_put_dev(dev);
 	put_pid(ps->disc_pid);
@@ -2355,6 +2378,45 @@ 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);
+
+	return ret;
+}
+
 /*
  * NOTE:  All requests here that have interface numbers as parameters
  * are assuming that somehow the configuration has been prevented from
@@ -2549,6 +2611,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:
@@ -2620,6 +2691,8 @@ static void usbdev_remove(struct usb_dev
 		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);
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 */

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Mayuresh Kulkarni]

On Fri, 2019-06-21 at 15:28 -0400, Alan Stern wrote:
> On Fri, 21 Jun 2019, Mayuresh Kulkarni wrote:
> 
> > 
> > Hi Alan,
> > 
> > With the suggested modification (of having suspend/resume of usbfs
> > at
> > device level instead of interface level), looks like I am seeing a
> > deadlock described as below -
> > 
> > Pre-condition: USB device is connected but suspended before running
> > test
> > program.
> > 
> > 1. The test program calls open(/dev/bus/usb/...).
> > 2. This ends up calling usbdev_open().
> > 3. usbdev_open() takes the lock and calls usb_autoresume_device().
> > 4. usb_autoresume_device() calls pm_runtime_get_sync(). Due to _sync
> > version, this call will return after calling the resume call-back of
> > driver (please correct me if wrong).
> > 5. This ends up calling generic_resume() which
> > calls usbfs_notify_resume().
> > 6. Now usbfs_notify_resume() also wants the same lock that
> > usbdev_open()
> > in (3) has already taken.
> What lock are you talking about?  usbfs_notify_resume() doesn't take 
> any locks.

The lock I am talking about is used
by usb_lock_device/usb_unlock_device. It is shared between usbdev_open()
and usbfs_notify_resume().
I think for this approach, we would also need to
call usb_autosuspend_device/usb_autoresume_device out of the
usb_lock/unlock_device pair.

> 
> > 
> > My observation so far is - when the USB device is connected for
> > first
> > time, Android's USB manager server is able to open the device and
> > reads
> > its descriptors using usbfs. But the test is not able to. The change
> > is
> > auto-suspend in between device connect and run of test program.
> > 
> > I am still analysing the situation here to see if pre-condition
> > above
> > really makes difference or not. So please take this update with
> > pinch of
> > salt. However, still I wanted send this update and get a quick
> > review to
> > ensure I am not wandering in weeds.
> This doesn't sound like a real problem.
> 
> However, I have been thinking about how to do all this in light of
> Oliver's comments, and it seems like we should make some changes.
> 
> First, let's change the ioctls' names.  Instead of RESUME and SUSPEND,
> I'll call them FORBID_SUSPEND and ALLOW_SUSPEND.  The way they work
> should be clear: ALLOW_SUSPEND will permit the device to be suspended
> but might not cause a suspend to happen immediately.  FORBID_SUSPEND
> will cause an immediate resume if the device is currently suspended
> and
> will prevent the device from being suspended in the future.  The new 
> names more accurately reflect what the ioctls actually do.
> 
> Second, the WAIT_FOR_RESUME ioctl will wait only until a resume has
> occurred more recently than the most recent ALLOW_SUSPEND ioctl.  So
> for example, if the program calls ALLOW_SUSPEND, and the device
> suspends, and then the device resumes, and then the device suspends
> again, and then the program calls WAIT_FOR_RESUME, the ioctl will
> return immediately even though the device is currently suspended.  
> Thus you don't have to worry about missing a remote resume.  This also
> means that when WAIT_FOR_RESUME returns, the program should call
> FORBID_SUSPEND to ensure that the device is active and doesn't go
> back 
> into suspend.
> 

1. For the above example, since resume can happen anytime, the user-
space's suspend and resume notion would get out-of-sync with actual
device state, right?

> In practice, your program would use the ioctls as follows:
> 
> 	When the device file is opened, the kernel will automatically
> 	do the equivalent of FORBID_SUSPEND (to remain compatible 
> 	with the current behavior).
> 
> 	When the program is ready for the device to suspend, it will
> 	call the ALLOW_SUSPEND ioctl.  But it won't cancel the 
> 	outstanding URBs; instead it will continue to interact 
> 	normally with the device, because the device might not be 
> 	suspended for some time.
> 

I think it is reasonable to expect that user-space will call
ALLOW_SUSPEND when there is no URB pending from its side. If that is not
the case, how can it expect the device to suspend when it has some
outstanding work pending?

> 	When the device does go into suspend, URBs will start failing
> 	with an appropriate error code (EHOSTUNREACH, ESHUTDOWN, 
> 	EPROTO, or something similar).  In this way the program will
> 	realize the device is suspended.  At that point the program
> 	should call the WAIT_FOR_RESUME ioctl and stop trying to 
> 	communicate with the device.
> 
> 	When WAIT_FOR_RESUME returns, the program should call the
> 	FORBID_SUSPEND ioctl and resume normal communication with the 
> 	device.
> 

Due to this condition, the example in (1) above will not cause a wait on
current resume operation, since after WAIT_FOR_RESUME returns, the user-
space will call FORBID_SUSPEND, which will bring device out of current
suspend state. So, the wait effectively didn't happen, right?

> How does that sound?

My concerns are mentioned above, hope they make sense.
I will give this approach a shot with my test program and see how that
goes.

Just to re-iterate, the main issue we have now is - how to notify user-
space of suspend, so that it can safely call wait-for-resume, right?

Thanks,

> 
> The proposed patch is below.
> 
> Alan Stern
> 
> 
> Index: usb-devel/drivers/usb/core/devio.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/core/devio.c
> +++ usb-devel/drivers/usb/core/devio.c
> @@ -60,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 {
> @@ -699,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;
> @@ -712,12 +713,29 @@ 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;
> +
> +	list_for_each_entry(ps, &udev->filelist, list) {
> +		WRITE_ONCE(ps->not_yet_resumed, 0);
> +		wake_up_all(&ps->wait_for_resume);
> +	}
> +}
> +
>  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)
> @@ -1008,6 +1026,7 @@ 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();
> @@ -1032,8 +1051,11 @@ static int usbdev_release(struct inode *
>  	struct usb_device *dev = ps->dev;
>  	unsigned int ifnum;
>  	struct async *as;
> +	int rc = 0;
>  
>  	usb_lock_device(dev);
> +	if (ps->suspend_allowed)
> +		rc = usb_autoresume_device(dev);
>  	usb_hub_release_all_ports(dev, ps);
>  
>  	list_del_init(&ps->list);
> @@ -1044,7 +1066,8 @@ static int usbdev_release(struct inode *
>  			releaseintf(ps, ifnum);
>  	}
>  	destroy_all_async(ps);
> -	usb_autosuspend_device(dev);
> +	if (rc == 0)
> +		usb_autosuspend_device(dev);
>  	usb_unlock_device(dev);
>  	usb_put_dev(dev);
>  	put_pid(ps->disc_pid);
> @@ -2355,6 +2378,45 @@ 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);
> +
> +	return ret;
> +}
> +
>  /*
>   * NOTE:  All requests here that have interface numbers as parameters
>   * are assuming that somehow the configuration has been prevented
> from
> @@ -2549,6 +2611,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:
> @@ -2620,6 +2691,8 @@ static void usbdev_remove(struct usb_dev
>  		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);
> 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 */
> 

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Alan Stern]

On Mon, 24 Jun 2019, Mayuresh Kulkarni wrote:

> On Fri, 2019-06-21 at 15:28 -0400, Alan Stern wrote:
> > On Fri, 21 Jun 2019, Mayuresh Kulkarni wrote:
> > 
> > > 
> > > Hi Alan,
> > > 
> > > With the suggested modification (of having suspend/resume of usbfs
> > > at
> > > device level instead of interface level), looks like I am seeing a
> > > deadlock described as below -
> > > 
> > > Pre-condition: USB device is connected but suspended before running
> > > test
> > > program.
> > > 
> > > 1. The test program calls open(/dev/bus/usb/...).
> > > 2. This ends up calling usbdev_open().
> > > 3. usbdev_open() takes the lock and calls usb_autoresume_device().
> > > 4. usb_autoresume_device() calls pm_runtime_get_sync(). Due to _sync
> > > version, this call will return after calling the resume call-back of
> > > driver (please correct me if wrong).
> > > 5. This ends up calling generic_resume() which
> > > calls usbfs_notify_resume().
> > > 6. Now usbfs_notify_resume() also wants the same lock that
> > > usbdev_open()
> > > in (3) has already taken.
> > What lock are you talking about?  usbfs_notify_resume() doesn't take 
> > any locks.
> 
> The lock I am talking about is used
> by usb_lock_device/usb_unlock_device. It is shared between usbdev_open()
> and usbfs_notify_resume().

Here's the subroutine as it appears in the patch:

void usbfs_notify_resume(struct usb_device *udev)
{
	struct usb_dev_state *ps;

	list_for_each_entry(ps, &udev->filelist, list) {
		WRITE_ONCE(ps->not_yet_resumed, 0);
		wake_up_all(&ps->wait_for_resume);
	}
}

Please point out exactly where in this code you think
usbfs_notify_resume() tries to acquire the device lock.

(Although, to be fair, I think udev->filelist needs to be protected by
usbfs_mutex.  I will add this in the next version of the patch.)

> I think for this approach, we would also need to
> call usb_autosuspend_device/usb_autoresume_device out of the
> usb_lock/unlock_device pair.

No, you're not allowed to do that.  See the documentation at the top 
of the source code for usb_autosuspend_device() and 
usb_autoresume_device() in driver.c.

> > > My observation so far is - when the USB device is connected for
> > > first
> > > time, Android's USB manager server is able to open the device and
> > > reads
> > > its descriptors using usbfs. But the test is not able to. The change
> > > is
> > > auto-suspend in between device connect and run of test program.
> > > 
> > > I am still analysing the situation here to see if pre-condition
> > > above
> > > really makes difference or not. So please take this update with
> > > pinch of
> > > salt. However, still I wanted send this update and get a quick
> > > review to
> > > ensure I am not wandering in weeds.
> > This doesn't sound like a real problem.
> > 
> > However, I have been thinking about how to do all this in light of
> > Oliver's comments, and it seems like we should make some changes.
> > 
> > First, let's change the ioctls' names.  Instead of RESUME and SUSPEND,
> > I'll call them FORBID_SUSPEND and ALLOW_SUSPEND.  The way they work
> > should be clear: ALLOW_SUSPEND will permit the device to be suspended
> > but might not cause a suspend to happen immediately.  FORBID_SUSPEND
> > will cause an immediate resume if the device is currently suspended
> > and
> > will prevent the device from being suspended in the future.  The new 
> > names more accurately reflect what the ioctls actually do.
> > 
> > Second, the WAIT_FOR_RESUME ioctl will wait only until a resume has
> > occurred more recently than the most recent ALLOW_SUSPEND ioctl.  So
> > for example, if the program calls ALLOW_SUSPEND, and the device
> > suspends, and then the device resumes, and then the device suspends
> > again, and then the program calls WAIT_FOR_RESUME, the ioctl will
> > return immediately even though the device is currently suspended.  
> > Thus you don't have to worry about missing a remote resume.  This also
> > means that when WAIT_FOR_RESUME returns, the program should call
> > FORBID_SUSPEND to ensure that the device is active and doesn't go
> > back 
> > into suspend.
> > 
> 
> 1. For the above example, since resume can happen anytime, the user-
> space's suspend and resume notion would get out-of-sync with actual
> device state, right?

That's right.  We can't notify userspace about every state transition 
the device makes, so userspace may very well get out-of-sync with the 
actual device state.

> > In practice, your program would use the ioctls as follows:
> > 
> > 	When the device file is opened, the kernel will automatically
> > 	do the equivalent of FORBID_SUSPEND (to remain compatible 
> > 	with the current behavior).
> > 
> > 	When the program is ready for the device to suspend, it will
> > 	call the ALLOW_SUSPEND ioctl.  But it won't cancel the 
> > 	outstanding URBs; instead it will continue to interact 
> > 	normally with the device, because the device might not be 
> > 	suspended for some time.
> > 
> 
> I think it is reasonable to expect that user-space will call
> ALLOW_SUSPEND when there is no URB pending from its side. If that is not
> the case, how can it expect the device to suspend when it has some
> outstanding work pending?

There are two possible ways a userspace program can monitor the 
device's state:

    1.	The program can leave an URB (typically on an interrupt 
	endpoint) running constantly, and the device can send its 
	response to the URB whenever something happens.

    2.	The program can poll the device by submitting an URB 
	periodically to see if anything has happened since the last 
	poll.

In case 1, the program would leave the URB running even after doing the 
ALLOW_SUSPEND ioctl.  That way the program will be aware of anything 
that happens to the device before it suspends.  When the device does go 
into suspend, the URB will be terminated.

In case 2, the program would continue polling the device even after 
doing the ALLOW_SUSPEND ioctl.  When the device does go into suspend, 
the polling URB will fail.

If the program doesn't do either of these (that is, if it leaves the 
device completely idle) then it won't know if the user presses a 
button or does anything else to the device before the device is put 
into suspend.

> > 	When the device does go into suspend, URBs will start failing
> > 	with an appropriate error code (EHOSTUNREACH, ESHUTDOWN, 
> > 	EPROTO, or something similar).  In this way the program will
> > 	realize the device is suspended.  At that point the program
> > 	should call the WAIT_FOR_RESUME ioctl and stop trying to 
> > 	communicate with the device.
> > 
> > 	When WAIT_FOR_RESUME returns, the program should call the
> > 	FORBID_SUSPEND ioctl and resume normal communication with the 
> > 	device.
> > 
> 
> Due to this condition, the example in (1) above will not cause a wait on
> current resume operation, since after WAIT_FOR_RESUME returns, the user-
> space will call FORBID_SUSPEND, which will bring device out of current
> suspend state. So, the wait effectively didn't happen, right?

Correct.

> > How does that sound?
> 
> My concerns are mentioned above, hope they make sense.
> I will give this approach a shot with my test program and see how that
> goes.
> 
> Just to re-iterate, the main issue we have now is - how to notify user-
> space of suspend, so that it can safely call wait-for-resume, right?

Basically there is no way to notify userspace when the device is 
suspended.  Instead, we have to assume that the userspace program will 
figure out what has happened all by itself when its URBs begin failing.

If necessary, we could add an extra ioctl (IS_SUSPENDED) which would 
return 0 or 1 depending on whether the device is active or suspended.  
But I don't think we will need this.

Alan Stern

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Mayuresh Kulkarni]

On Mon, 2019-06-24 at 13:48 -0400, Alan Stern wrote:
> On Mon, 24 Jun 2019, Mayuresh Kulkarni wrote:
> 
> > 
> > On Fri, 2019-06-21 at 15:28 -0400, Alan Stern wrote:
> > > 
> > > On Fri, 21 Jun 2019, Mayuresh Kulkarni wrote:
> > > 
> > > > 
> > > > 
> > > > Hi Alan,
> > > > 
> > > > With the suggested modification (of having suspend/resume of
> > > > usbfs
> > > > at
> > > > device level instead of interface level), looks like I am seeing
> > > > a
> > > > deadlock described as below -
> > > > 
> > > > Pre-condition: USB device is connected but suspended before
> > > > running
> > > > test
> > > > program.
> > > > 
> > > > 1. The test program calls open(/dev/bus/usb/...).
> > > > 2. This ends up calling usbdev_open().
> > > > 3. usbdev_open() takes the lock and
> > > > calls usb_autoresume_device().
> > > > 4. usb_autoresume_device() calls pm_runtime_get_sync(). Due to
> > > > _sync
> > > > version, this call will return after calling the resume call-
> > > > back of
> > > > driver (please correct me if wrong).
> > > > 5. This ends up calling generic_resume() which
> > > > calls usbfs_notify_resume().
> > > > 6. Now usbfs_notify_resume() also wants the same lock that
> > > > usbdev_open()
> > > > in (3) has already taken.
> > > What lock are you talking about?  usbfs_notify_resume() doesn't
> > > take 
> > > any locks.
> > The lock I am talking about is used
> > by usb_lock_device/usb_unlock_device. It is shared
> > between usbdev_open()
> > and usbfs_notify_resume().
> Here's the subroutine as it appears in the patch:
> 
> void usbfs_notify_resume(struct usb_device *udev)
> {
> 	struct usb_dev_state *ps;
> 
> 	list_for_each_entry(ps, &udev->filelist, list) {
> 		WRITE_ONCE(ps->not_yet_resumed, 0);
> 		wake_up_all(&ps->wait_for_resume);
> 	}
> }
> 
> Please point out exactly where in this code you think
> usbfs_notify_resume() tries to acquire the device lock.
> 

Oops, sorry. Please accept my apology on this one.
You are right, the original patch you send doesn't have the lock
in usbfs_notify_suspend/resume.

> (Although, to be fair, I think udev->filelist needs to be protected by
> usbfs_mutex.  I will add this in the next version of the patch.)

OK.

> 
> > 
> > I think for this approach, we would also need to
> > call usb_autosuspend_device/usb_autoresume_device out of the
> > usb_lock/unlock_device pair.
> No, you're not allowed to do that.  See the documentation at the top 
> of the source code for usb_autosuspend_device() and 
> usb_autoresume_device() in driver.c.
> 
> > 
> > > 
> > > > 
> > > > My observation so far is - when the USB device is connected for
> > > > first
> > > > time, Android's USB manager server is able to open the device
> > > > and
> > > > reads
> > > > its descriptors using usbfs. But the test is not able to. The
> > > > change
> > > > is
> > > > auto-suspend in between device connect and run of test program.
> > > > 
> > > > I am still analysing the situation here to see if pre-condition
> > > > above
> > > > really makes difference or not. So please take this update with
> > > > pinch of
> > > > salt. However, still I wanted send this update and get a quick
> > > > review to
> > > > ensure I am not wandering in weeds.
> > > This doesn't sound like a real problem.
> > > 
> > > However, I have been thinking about how to do all this in light of
> > > Oliver's comments, and it seems like we should make some changes.
> > > 
> > > First, let's change the ioctls' names.  Instead of RESUME and
> > > SUSPEND,
> > > I'll call them FORBID_SUSPEND and ALLOW_SUSPEND.  The way they
> > > work
> > > should be clear: ALLOW_SUSPEND will permit the device to be
> > > suspended
> > > but might not cause a suspend to happen
> > > immediately.  FORBID_SUSPEND
> > > will cause an immediate resume if the device is currently
> > > suspended
> > > and
> > > will prevent the device from being suspended in the future.  The
> > > new 
> > > names more accurately reflect what the ioctls actually do.
> > > 
> > > Second, the WAIT_FOR_RESUME ioctl will wait only until a resume
> > > has
> > > occurred more recently than the most recent ALLOW_SUSPEND
> > > ioctl.  So
> > > for example, if the program calls ALLOW_SUSPEND, and the device
> > > suspends, and then the device resumes, and then the device
> > > suspends
> > > again, and then the program calls WAIT_FOR_RESUME, the ioctl will
> > > return immediately even though the device is currently
> > > suspended.  
> > > Thus you don't have to worry about missing a remote resume.  This
> > > also
> > > means that when WAIT_FOR_RESUME returns, the program should call
> > > FORBID_SUSPEND to ensure that the device is active and doesn't go
> > > back 
> > > into suspend.
> > > 
> > 1. For the above example, since resume can happen anytime, the user-
> > space's suspend and resume notion would get out-of-sync with actual
> > device state, right?
> That's right.  We can't notify userspace about every state transition 
> the device makes, so userspace may very well get out-of-sync with the 
> actual device state.
> 
> > 
> > > 
> > > In practice, your program would use the ioctls as follows:
> > > 
> > > 	When the device file is opened, the kernel will automatically
> > > 	do the equivalent of FORBID_SUSPEND (to remain compatible 
> > > 	with the current behavior).
> > > 
> > > 	When the program is ready for the device to suspend, it will
> > > 	call the ALLOW_SUSPEND ioctl.  But it won't cancel the 
> > > 	outstanding URBs; instead it will continue to interact 
> > > 	normally with the device, because the device might not be 
> > > 	suspended for some time.
> > > 
> > I think it is reasonable to expect that user-space will call
> > ALLOW_SUSPEND when there is no URB pending from its side. If that is
> > not
> > the case, how can it expect the device to suspend when it has some
> > outstanding work pending?
> There are two possible ways a userspace program can monitor the 
> device's state:
> 
>     1.	The program can leave an URB (typically on an interrupt 
> 	endpoint) running constantly, and the device can send its 
> 	response to the URB whenever something happens.
> 
>     2.	The program can poll the device by submitting an URB 
> 	periodically to see if anything has happened since the last 
> 	poll.
> 
> In case 1, the program would leave the URB running even after doing
> the 
> ALLOW_SUSPEND ioctl.  That way the program will be aware of anything 
> that happens to the device before it suspends.  When the device does
> go 
> into suspend, the URB will be terminated.
> 
> In case 2, the program would continue polling the device even after 
> doing the ALLOW_SUSPEND ioctl.  When the device does go into suspend, 
> the polling URB will fail.
> 

Right, so user space should do the following when it determines the
device is idle from its point of view -

1. Call ALLOW_SUSPEND ioctl
2. Queue an URB and wait for its REAP. When the wait returns -EFAIL (or
something similar), that is the indication that the device is no longer
active (or suspended)
3. Call WAIT_FOR_RESUME ioctl
4. When WAIT_FOR_RESUME ioctl returns, it is guaranteed that device is
active.
5. Call FORBID_SUSPEND ioctl and read the cause of resume.
6. Go to (1) when appropriate

Have I summarized this approach correctly from user-space point of view?

> If the program doesn't do either of these (that is, if it leaves the 
> device completely idle) then it won't know if the user presses a 
> button or does anything else to the device before the device is put 
> into suspend.
> 
> > 
> > > 
> > > 	When the device does go into suspend, URBs will start failing
> > > 	with an appropriate error code (EHOSTUNREACH, ESHUTDOWN, 
> > > 	EPROTO, or something similar).  In this way the program will
> > > 	realize the device is suspended.  At that point the program
> > > 	should call the WAIT_FOR_RESUME ioctl and stop trying to 
> > > 	communicate with the device.
> > > 
> > > 	When WAIT_FOR_RESUME returns, the program should call the
> > > 	FORBID_SUSPEND ioctl and resume normal communication with the 
> > > 	device.
> > > 
> > Due to this condition, the example in (1) above will not cause a
> > wait on
> > current resume operation, since after WAIT_FOR_RESUME returns, the
> > user-
> > space will call FORBID_SUSPEND, which will bring device out of
> > current
> > suspend state. So, the wait effectively didn't happen, right?
> Correct.
> 
> > 
> > > 
> > > How does that sound?
> > My concerns are mentioned above, hope they make sense.
> > I will give this approach a shot with my test program and see how
> > that
> > goes.
> > 
> > Just to re-iterate, the main issue we have now is - how to notify
> > user-
> > space of suspend, so that it can safely call wait-for-resume, right?
> Basically there is no way to notify userspace when the device is 
> suspended.  Instead, we have to assume that the userspace program
> will 
> figure out what has happened all by itself when its URBs begin
> failing.
> 

Based on discussion so far and my understanding, how about below
approach -

1. Have ALLOW_SUSPEND and WAIT_FOR_RESUME ioctls. As before,
ALLOW_SUSPEND calls usb_autosuspend_device() while WAIT_FOR_RESUME waits
for resume.
2. Have usbfs_notify_suspend() and usbfs_notify_resume() as per your
previous patch (i.e.: system/resume callbacks at device level).
3. Extend usbdev_poll() to wait for udev->state == USB_STATE_SUSPENDED
when events == POLLPRI. Return POLLPRI when state = USB_STATE_SUSPENDED.
4. As before, any ioctl != (ALLOW_SUSPEND or WAIT_FOR_RESUME)
calls usb_autoresume_device().

The corresponding user-space calls would be -
A. When determined device is idle, call ALLOW_SUSPEND ioctl.
B. Call poll(device_fd, POLLPRI). When poll returns check revents
== POLLPRI.
C. Call WAIT_FOR_RESUME ioctl.
D. When WAIT_FOR_RESUME ioctl returns, read resume reason.
E. Go to (A).

The constraint on (1) above is - ALLOW_SUSPEND should be called when
user-space decides device is idle. I think this is not a hard constraint
since poll() for suspend will return POLLPRI when device is suspended
which is different from what it returns when ASYNC URB is completed.

Few points I am unsure of are -
1. Is it OK for a driver to re-purpose POLLPRI for its own use
or POLLPRI has a unique meaning system wide?
2. Is it safe to wait for udev->state to be of a particular value?

If this approach could work, I can spend time on this one as well.

Thanks,

> If necessary, we could add an extra ioctl (IS_SUSPENDED) which would 
> return 0 or 1 depending on whether the device is active or
> suspended.  
> But I don't think we will need this.
> 
> Alan Stern
> 

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Alan Stern]

On Tue, 25 Jun 2019, Mayuresh Kulkarni wrote:

> > There are two possible ways a userspace program can monitor the 
> > device's state:
> > 
> >     1.	The program can leave an URB (typically on an interrupt 
> > 	endpoint) running constantly, and the device can send its 
> > 	response to the URB whenever something happens.
> > 
> >     2.	The program can poll the device by submitting an URB 
> > 	periodically to see if anything has happened since the last 
> > 	poll.
> > 
> > In case 1, the program would leave the URB running even after doing
> > the 
> > ALLOW_SUSPEND ioctl.  That way the program will be aware of anything 
> > that happens to the device before it suspends.  When the device does
> > go 
> > into suspend, the URB will be terminated.
> > 
> > In case 2, the program would continue polling the device even after 
> > doing the ALLOW_SUSPEND ioctl.  When the device does go into suspend, 
> > the polling URB will fail.
> > 
> 
> Right, so user space should do the following when it determines the
> device is idle from its point of view -
> 
> 1. Call ALLOW_SUSPEND ioctl
> 2. Queue an URB and wait for its REAP. When the wait returns -EFAIL (or
> something similar), that is the indication that the device is no longer
> active (or suspended)
> 3. Call WAIT_FOR_RESUME ioctl
> 4. When WAIT_FOR_RESUME ioctl returns, it is guaranteed that device is
> active.
> 5. Call FORBID_SUSPEND ioctl and read the cause of resume.
> 6. Go to (1) when appropriate
> 
> Have I summarized this approach correctly from user-space point of view?

Yes, except for one thing: In step 4, it is _not_ guaranteed that the 
device is active when WAIT_FOR_RESUME returns.  The only guarantee is 
that a resume did occur sometime after step 1, but the device might 
have gone back into suspend after that occurred.

And note that step 2 (queuing an URB) is something your program would
do anyway, even if the device wasn't going to be suspended or wasn't
idle.


> Based on discussion so far and my understanding, how about below
> approach -
> 
> 1. Have ALLOW_SUSPEND and WAIT_FOR_RESUME ioctls. As before,
> ALLOW_SUSPEND calls usb_autosuspend_device() while WAIT_FOR_RESUME waits
> for resume.
> 2. Have usbfs_notify_suspend() and usbfs_notify_resume() as per your
> previous patch (i.e.: system/resume callbacks at device level).
> 3. Extend usbdev_poll() to wait for udev->state == USB_STATE_SUSPENDED
> when events == POLLPRI. Return POLLPRI when state = USB_STATE_SUSPENDED.
> 4. As before, any ioctl != (ALLOW_SUSPEND or WAIT_FOR_RESUME)
> calls usb_autoresume_device().

3 sounds reasonable at first, but I'm not sure it would work.  
Consider what would happen if the device is suspended very briefly and
then wakes up.  The usbdev_poll() call might not return, because by the
time it checks udev->state, the state has already changed back to
USB_STATE_CONFIGURED.

In any case, we shouldn't do 4.  It would prevent the device from ever
going into suspend, because the program would want to continue making
usbfs ioctl calls while waiting for the suspend to occur.

> The corresponding user-space calls would be -
> A. When determined device is idle, call ALLOW_SUSPEND ioctl.
> B. Call poll(device_fd, POLLPRI). When poll returns check revents
> == POLLPRI.

What if the device never does go into suspend?  The poll() call
wouldn't return and the program would just stop working.

> C. Call WAIT_FOR_RESUME ioctl.
> D. When WAIT_FOR_RESUME ioctl returns, read resume reason.
> E. Go to (A).
> 
> The constraint on (1) above is - ALLOW_SUSPEND should be called when
> user-space decides device is idle. I think this is not a hard constraint
> since poll() for suspend will return POLLPRI when device is suspended
> which is different from what it returns when ASYNC URB is completed.
> 
> Few points I am unsure of are -
> 1. Is it OK for a driver to re-purpose POLLPRI for its own use
> or POLLPRI has a unique meaning system wide?

POLLPRI does not have a unique system-wide meaning.  We could use it to 
indicate the device is suspended, if we want to.  But I'm not convinced 
that it would be a good idea.

> 2. Is it safe to wait for udev->state to be of a particular value?

No, not really, because the state can change.

> If this approach could work, I can spend time on this one as well.

What advantage do you think your proposal has over my proposal?

Alan Stern

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Oliver Neukum]

Am Dienstag, den 25.06.2019, 10:08 -0400 schrieb Alan Stern:
> On Tue, 25 Jun 2019, Mayuresh Kulkarni wrote:

> > Right, so user space should do the following when it determines the
> > device is idle from its point of view -
> > 
> > 1. Call ALLOW_SUSPEND ioctl

That is a race in principle. You should reverse steps 1 and 2

> > 2. Queue an URB and wait for its REAP. When the wait returns -EFAIL (or
> > something similar), that is the indication that the device is no longer
> > active (or suspended)
> > 3. Call WAIT_FOR_RESUME ioctl

It seems to me that there ought to be one API for that. Either an
ioctl or poll.

> > 4. When WAIT_FOR_RESUME ioctl returns, it is guaranteed that device is
> > active.
> > 5. Call FORBID_SUSPEND ioctl and read the cause of resume.
> > 6. Go to (1) when appropriate
> > 
> > Have I summarized this approach correctly from user-space point of view?
> 
> Yes, except for one thing: In step 4, it is _not_ guaranteed that the 
> device is active when WAIT_FOR_RESUME returns.  The only guarantee is 
> that a resume did occur sometime after step 1, but the device might 
> have gone back into suspend after that occurred.

Now is that a good API? Shouldn't we rather have an API for either
* resume the device now and bump the counter
* bump the counter when te device resumes

I don't see a value in not having a defined power state after resume.

> > 3. Extend usbdev_poll() to wait for udev->state == USB_STATE_SUSPENDED
> > when events == POLLPRI. Return POLLPRI when state = USB_STATE_SUSPENDED.
> > 4. As before, any ioctl != (ALLOW_SUSPEND or WAIT_FOR_RESUME)
> > calls usb_autoresume_device().
> 
> 3 sounds reasonable at first, but I'm not sure it would work.  
> Consider what would happen if the device is suspended very briefly and
> then wakes up.  The usbdev_poll() call might not return, because by the
> time it checks udev->state, the state has already changed back to
> USB_STATE_CONFIGURED.

Indeed. It seems to me that any power transition should be reported
back.

> In any case, we shouldn't do 4.  It would prevent the device from ever
> going into suspend, because the program would want to continue making
> usbfs ioctl calls while waiting for the suspend to occur.

Exactly.

> > The corresponding user-space calls would be -
> > A. When determined device is idle, call ALLOW_SUSPEND ioctl.
> > B. Call poll(device_fd, POLLPRI). When poll returns check revents
> > == POLLPRI.
> 
> What if the device never does go into suspend?  The poll() call
> wouldn't return and the program would just stop working.

Well, that is why you can poll for multiple events at the same
time and the syscall has a timeout.

> > 2. Is it safe to wait for udev->state to be of a particular value?
> 
> No, not really, because the state can change.

That is a general issue with poll. You cannot be sure that there
is still data to be read when you are ready to read.

	Regards
		Oliver

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Alan Stern]

On Wed, 26 Jun 2019, Oliver Neukum wrote:

> Am Dienstag, den 25.06.2019, 10:08 -0400 schrieb Alan Stern:
> > On Tue, 25 Jun 2019, Mayuresh Kulkarni wrote:
> 
> > > Right, so user space should do the following when it determines the
> > > device is idle from its point of view -
> > > 
> > > 1. Call ALLOW_SUSPEND ioctl
> 
> That is a race in principle. You should reverse steps 1 and 2

I disagree.  First, it's not really a race.  It doesn't matter whether
the URB error occurs during completion or during submission; either way
the program will become aware that the device has suspended.

Second, in all likelihood the program will be submitting URBs more or
less continually anyway.  Think of an HID driver, for example.  It
always keeps an interrupt URB active so that it can monitor what is
happening to the device.

If you really think it would be better, we could add an IS_SUSPENDED
ioctl so that the program can poll for state changes.  But that seems
like a pretty bad approach to me.  We could even add a WAIT_FOR_SUSPEND
ioctl, but programs basically will never want to wait for a suspend to
occur.

> > > 2. Queue an URB and wait for its REAP. When the wait returns -EFAIL (or
> > > something similar), that is the indication that the device is no longer
> > > active (or suspended)
> > > 3. Call WAIT_FOR_RESUME ioctl
> 
> It seems to me that there ought to be one API for that. Either an
> ioctl or poll.

Isn't a WAIT_FOR_RESUME ioctl one API?

In general, an ioctl is more flexible than poll -- poll can report only
a very limited number of types of state change.

> > > 4. When WAIT_FOR_RESUME ioctl returns, it is guaranteed that device is
> > > active.
> > > 5. Call FORBID_SUSPEND ioctl and read the cause of resume.
> > > 6. Go to (1) when appropriate
> > > 
> > > Have I summarized this approach correctly from user-space point of view?
> > 
> > Yes, except for one thing: In step 4, it is _not_ guaranteed that the 
> > device is active when WAIT_FOR_RESUME returns.  The only guarantee is 
> > that a resume did occur sometime after step 1, but the device might 
> > have gone back into suspend after that occurred.
> 
> Now is that a good API? Shouldn't we rather have an API for either
> * resume the device now and bump the counter
> * bump the counter when te device resumes

It makes the code cleaner: The usage counter is incremented by the
FORBID_SUSPEND ioctl and decremented by the ALLOW_SUSPEND ioctl.  
Nothing else.  Now you're suggesting that WAIT_FOR_RESUME should also
increment the usage counter upon return.  Programs may not even want
that behavior (consider the possibility that WAIT_FOR_RESUME was
interrupted by a signal).

In any case, we do need two separate APIs: one to resume the device
immediately, and one to wait for a resume to occur.  Programs will want 
to do both these things.

> I don't see a value in not having a defined power state after resume.

Well, we also don't have a defined power state after ALLOW_SUSPEND.  I
don't see this as a problem.  It's more of a necessity, in fact --
there's no way to keep a userspace program fully up to date on the
device's state, because suspend and resume events are asynchronous.

> > > 3. Extend usbdev_poll() to wait for udev->state == USB_STATE_SUSPENDED
> > > when events == POLLPRI. Return POLLPRI when state = USB_STATE_SUSPENDED.
> > > 4. As before, any ioctl != (ALLOW_SUSPEND or WAIT_FOR_RESUME)
> > > calls usb_autoresume_device().
> > 
> > 3 sounds reasonable at first, but I'm not sure it would work.  
> > Consider what would happen if the device is suspended very briefly and
> > then wakes up.  The usbdev_poll() call might not return, because by the
> > time it checks udev->state, the state has already changed back to
> > USB_STATE_CONFIGURED.
> 
> Indeed. It seems to me that any power transition should be reported
> back.

Sorry, I don't understand: Reported back where?  Keep in mind that it
is not feasible to report every transition to a userspace program.  
And even if you somehow did, the last reported value would be out of
date as soon as the program received it.

> > In any case, we shouldn't do 4.  It would prevent the device from ever
> > going into suspend, because the program would want to continue making
> > usbfs ioctl calls while waiting for the suspend to occur.
> 
> Exactly.
> 
> > > The corresponding user-space calls would be -
> > > A. When determined device is idle, call ALLOW_SUSPEND ioctl.
> > > B. Call poll(device_fd, POLLPRI). When poll returns check revents
> > > == POLLPRI.
> > 
> > What if the device never does go into suspend?  The poll() call
> > wouldn't return and the program would just stop working.
> 
> Well, that is why you can poll for multiple events at the same
> time and the syscall has a timeout.

The events that poll reports on are really targeted toward
communication channels (e.g., data available to read, high-priority
data received, channel closed), not suspend/resume state changes.  
Poll really wasn't meant for this purpose.

> > > 2. Is it safe to wait for udev->state to be of a particular value?
> > 
> > No, not really, because the state can change.
> 
> That is a general issue with poll. You cannot be sure that there
> is still data to be read when you are ready to read.

Why not?  If you're dealing with a tty device and poll says there is 
data available to read, the data won't go away by itself.  It'll remain 
in the buffer until you read it.

Alan Stern

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Mayuresh Kulkarni]

On Tue, 2019-06-25 at 10:08 -0400, Alan Stern wrote:
> On Tue, 25 Jun 2019, Mayuresh Kulkarni wrote:
> 
> > 
> > > 
> > > There are two possible ways a userspace program can monitor the 
> > > device's state:
> > > 
> > >     1.	The program can leave an URB (typically on an
> > > interrupt 
> > > 	endpoint) running constantly, and the device can send its 
> > > 	response to the URB whenever something happens.
> > > 
> > >     2.	The program can poll the device by submitting an
> > > URB 
> > > 	periodically to see if anything has happened since the last 
> > > 	poll.
> > > 
> > > In case 1, the program would leave the URB running even after
> > > doing
> > > the 
> > > ALLOW_SUSPEND ioctl.  That way the program will be aware of
> > > anything 
> > > that happens to the device before it suspends.  When the device
> > > does
> > > go 
> > > into suspend, the URB will be terminated.
> > > 
> > > In case 2, the program would continue polling the device even
> > > after 
> > > doing the ALLOW_SUSPEND ioctl.  When the device does go into
> > > suspend, 
> > > the polling URB will fail.
> > > 
> > Right, so user space should do the following when it determines the
> > device is idle from its point of view -
> > 
> > 1. Call ALLOW_SUSPEND ioctl
> > 2. Queue an URB and wait for its REAP. When the wait returns -EFAIL
> > (or
> > something similar), that is the indication that the device is no
> > longer
> > active (or suspended)
> > 3. Call WAIT_FOR_RESUME ioctl
> > 4. When WAIT_FOR_RESUME ioctl returns, it is guaranteed that device
> > is
> > active.
> > 5. Call FORBID_SUSPEND ioctl and read the cause of resume.
> > 6. Go to (1) when appropriate
> > 
> > Have I summarized this approach correctly from user-space point of
> > view?
> Yes, except for one thing: In step 4, it is _not_ guaranteed that the 
> device is active when WAIT_FOR_RESUME returns.  The only guarantee is 
> that a resume did occur sometime after step 1, but the device might 
> have gone back into suspend after that occurred.
> 

Right, OK.

> And note that step 2 (queuing an URB) is something your program would
> do anyway, even if the device wasn't going to be suspended or wasn't
> idle.
> 

Yes, got it now.

> 
> > 
> > Based on discussion so far and my understanding, how about below
> > approach -
> > 
> > 1. Have ALLOW_SUSPEND and WAIT_FOR_RESUME ioctls. As before,
> > ALLOW_SUSPEND calls usb_autosuspend_device() while WAIT_FOR_RESUME
> > waits
> > for resume.
> > 2. Have usbfs_notify_suspend() and usbfs_notify_resume() as per your
> > previous patch (i.e.: system/resume callbacks at device level).
> > 3. Extend usbdev_poll() to wait for udev->state ==
> > USB_STATE_SUSPENDED
> > when events == POLLPRI. Return POLLPRI when state =
> > USB_STATE_SUSPENDED.
> > 4. As before, any ioctl != (ALLOW_SUSPEND or WAIT_FOR_RESUME)
> > calls usb_autoresume_device().
> 3 sounds reasonable at first, but I'm not sure it would work.  
> Consider what would happen if the device is suspended very briefly and
> then wakes up.  The usbdev_poll() call might not return, because by
> the
> time it checks udev->state, the state has already changed back to
> USB_STATE_CONFIGURED.
> 

I see what you mean here, could be problematic.

> In any case, we shouldn't do 4.  It would prevent the device from ever
> going into suspend, because the program would want to continue making
> usbfs ioctl calls while waiting for the suspend to occur.
> 

In poll approach, due to the contraint I mentioned, it is not expected
from user-space to interact with device *after* it calls ALLOW_SUSPEND
ioctl. This is because, it has determined that device is idle from its
point of view.

But after a while, there could be a situation where the user-space wants
to send some message to device (due to end user interaction) then,
having (4) would be handy to cancel the undone suspend or cause host-
wake.

> > 
> > The corresponding user-space calls would be -
> > A. When determined device is idle, call ALLOW_SUSPEND ioctl.
> > B. Call poll(device_fd, POLLPRI). When poll returns check revents
> > == POLLPRI.
> What if the device never does go into suspend?  The poll() call
> wouldn't return and the program would just stop working.
> 
> > 
> > C. Call WAIT_FOR_RESUME ioctl.
> > D. When WAIT_FOR_RESUME ioctl returns, read resume reason.
> > E. Go to (A).
> > 
> > The constraint on (1) above is - ALLOW_SUSPEND should be called when
> > user-space decides device is idle. I think this is not a hard
> > constraint
> > since poll() for suspend will return POLLPRI when device is
> > suspended
> > which is different from what it returns when ASYNC URB is completed.
> > 
> > Few points I am unsure of are -
> > 1. Is it OK for a driver to re-purpose POLLPRI for its own use
> > or POLLPRI has a unique meaning system wide?
> POLLPRI does not have a unique system-wide meaning.  We could use it
> to 
> indicate the device is suspended, if we want to.  But I'm not
> convinced 
> that it would be a good idea.
> 
> > 
> > 2. Is it safe to wait for udev->state to be of a particular value?
> No, not really, because the state can change.
> 

If you remember, I had also suggested to use root-hub suspend in
generic_suspend() to call usbfs_notify_suspend/_resume APIs. It might be
possible to use that in usbdev_poll() and send POLLPRI when root-hub
suspends.

> > 
> > If this approach could work, I can spend time on this one as well.
> What advantage do you think your proposal has over my proposal?
> 

Not necessarily advantage(s), but few things that I could think of are -

1. In poll based approach, user-space notion of device's state is in
sync with actual device state. This is partially true with the 3 ioctl
approach suggested by you (partially because resume might not be current
device state when wait-for-resume returns).
2. In 3 ioctl approach, user-space can still communicate with device
after calling ALLOW_SUSPEND. With poll based approach, we put a
constraint on user-space that, call ALLOW_SUSPEND only when you
determine you are not going to interact with device.

I know for (2) above, you have commented before that -
A. It is desirable to be able to communicate with the device till it is
actually suspended.
B. The possibility of device not going into suspend for long time, so
user-space will not be able to proceed.

The counter comments to them are:
For (A), *usually*, the driver by some means determine device is idle
and then schedules a suspend. After that, it is not expecting to
communicate with the device till resume happens. If it has to
communicate, it has to call resume first and then proceed.
For (B), that is why we need ability to cancel current undone suspend
operation, to allow the user-space to initiate the communication.

Hope some of these points makes sense.

> Alan Stern
> 

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Alan Stern]

On Wed, 26 Jun 2019, Mayuresh Kulkarni wrote:

> On Tue, 2019-06-25 at 10:08 -0400, Alan Stern wrote:

> > > Based on discussion so far and my understanding, how about below
> > > approach -
> > > 
> > > 1. Have ALLOW_SUSPEND and WAIT_FOR_RESUME ioctls. As before,
> > > ALLOW_SUSPEND calls usb_autosuspend_device() while WAIT_FOR_RESUME
> > > waits
> > > for resume.
> > > 2. Have usbfs_notify_suspend() and usbfs_notify_resume() as per your
> > > previous patch (i.e.: system/resume callbacks at device level).
> > > 3. Extend usbdev_poll() to wait for udev->state ==
> > > USB_STATE_SUSPENDED
> > > when events == POLLPRI. Return POLLPRI when state =
> > > USB_STATE_SUSPENDED.
> > > 4. As before, any ioctl != (ALLOW_SUSPEND or WAIT_FOR_RESUME)
> > > calls usb_autoresume_device().
> > 3 sounds reasonable at first, but I'm not sure it would work.  
> > Consider what would happen if the device is suspended very briefly and
> > then wakes up.  The usbdev_poll() call might not return, because by
> > the
> > time it checks udev->state, the state has already changed back to
> > USB_STATE_CONFIGURED.
> > 
> 
> I see what you mean here, could be problematic.
> 
> > In any case, we shouldn't do 4.  It would prevent the device from ever
> > going into suspend, because the program would want to continue making
> > usbfs ioctl calls while waiting for the suspend to occur.
> > 
> 
> In poll approach, due to the contraint I mentioned, it is not expected
> from user-space to interact with device *after* it calls ALLOW_SUSPEND
> ioctl. This is because, it has determined that device is idle from its
> point of view.

What if the user interacts with the device at this point?  Wouldn't 
the program want to know about it?

Even if your program doesn't care about user interaction with an idle
device, there undoubtedly will be other programs that _do_ care.  This 
mechanism we are designing needs to work with all programs.

> But after a while, there could be a situation where the user-space wants
> to send some message to device (due to end user interaction) then,
> having (4) would be handy to cancel the undone suspend or cause host-
> wake.

That's what the FORBID_SUSPEND ioctl does.  We don't need (4) for this.

> > > 2. Is it safe to wait for udev->state to be of a particular value?
> > No, not really, because the state can change.
> > 
> 
> If you remember, I had also suggested to use root-hub suspend in
> generic_suspend() to call usbfs_notify_suspend/_resume APIs. It might be
> possible to use that in usbdev_poll() and send POLLPRI when root-hub
> suspends.

I don't see how that would help.  It would just make things more 
confusing.  Forget about root-hub events for now.

> > > If this approach could work, I can spend time on this one as well.
> > What advantage do you think your proposal has over my proposal?
> > 
> 
> Not necessarily advantage(s), but few things that I could think of are -
> 
> 1. In poll based approach, user-space notion of device's state is in
> sync with actual device state.

NO!  That simply is not true.  You don't seem to be able to grasp this
point.

Consider this: Suppose the computer is busy running many other 
programs.  Your program gets swapped out because a bunch of other 
higher-priority tasks need to run.  By the time your program gets a 
chance to run again, the device could have gone through several 
suspend/resume transitions.  There's no way the program can keep track 
of all that.

If you want a more likely example, consider this: Your program calls 
ALLOW_SUSPEND and then calls poll().  The device suspends and the 
poll() returns.  But then, before your program can do anything else, 
the device resumes.  Now the device is active but your program thinks 
it is suspended -- the program is out of sync.

Face it: It is _impossible_ for a program to truly know the device's 
current state at all times (unless the device is not allowed to suspend 
at all).

> This is partially true with the 3 ioctl
> approach suggested by you (partially because resume might not be current
> device state when wait-for-resume returns).

Agreed.  But so what?  What abilities does your program lose as a 
result of the fact that the device might be suspended when 
WAIT_FOR_RESUME returns?

> 2. In 3 ioctl approach, user-space can still communicate with device
> after calling ALLOW_SUSPEND. With poll based approach, we put a
> constraint on user-space that, call ALLOW_SUSPEND only when you
> determine you are not going to interact with device.

That sounds like a disadvantage for your poll-based approach: It 
constrains the behavior of userspace programs.

> I know for (2) above, you have commented before that -
> A. It is desirable to be able to communicate with the device till it is
> actually suspended.
> B. The possibility of device not going into suspend for long time, so
> user-space will not be able to proceed.
> 
> The counter comments to them are:
> For (A), *usually*, the driver by some means determine device is idle
> and then schedules a suspend. After that, it is not expecting to
> communicate with the device till resume happens. If it has to
> communicate, it has to call resume first and then proceed.

_Some_ programs will behave that way but other programs will not;
they will want to continue communicating with the device right up until
the time it suspends.  The kernel has to work with _all_ programs.

> For (B), that is why we need ability to cancel current undone suspend
> operation, to allow the user-space to initiate the communication.

And that is what FORBID_SUSPEND does.  You don't need to give up on (A) 
in order to handle (B).

> Hope some of these points makes sense.

None of them seem convincing, at least, not to me.

Alan Stern

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Mayuresh Kulkarni]

On Wed, 2019-06-26 at 11:07 -0400, Alan Stern wrote:
> On Wed, 26 Jun 2019, Mayuresh Kulkarni wrote:
> 
> > 
> > On Tue, 2019-06-25 at 10:08 -0400, Alan Stern wrote:
> > 
> > > 
> > > > 
> > > > Based on discussion so far and my understanding, how about below
> > > > approach -
> > > > 
> > > > 1. Have ALLOW_SUSPEND and WAIT_FOR_RESUME ioctls. As before,
> > > > ALLOW_SUSPEND calls usb_autosuspend_device() while
> > > > WAIT_FOR_RESUME
> > > > waits
> > > > for resume.
> > > > 2. Have usbfs_notify_suspend() and usbfs_notify_resume() as per
> > > > your
> > > > previous patch (i.e.: system/resume callbacks at device level).
> > > > 3. Extend usbdev_poll() to wait for udev->state ==
> > > > USB_STATE_SUSPENDED
> > > > when events == POLLPRI. Return POLLPRI when state =
> > > > USB_STATE_SUSPENDED.
> > > > 4. As before, any ioctl != (ALLOW_SUSPEND or WAIT_FOR_RESUME)
> > > > calls usb_autoresume_device().
> > > 3 sounds reasonable at first, but I'm not sure it would work.  
> > > Consider what would happen if the device is suspended very briefly
> > > and
> > > then wakes up.  The usbdev_poll() call might not return, because
> > > by
> > > the
> > > time it checks udev->state, the state has already changed back to
> > > USB_STATE_CONFIGURED.
> > > 
> > I see what you mean here, could be problematic.
> > 
> > > 
> > > In any case, we shouldn't do 4.  It would prevent the device from
> > > ever
> > > going into suspend, because the program would want to continue
> > > making
> > > usbfs ioctl calls while waiting for the suspend to occur.
> > > 
> > In poll approach, due to the contraint I mentioned, it is not
> > expected
> > from user-space to interact with device *after* it calls
> > ALLOW_SUSPEND
> > ioctl. This is because, it has determined that device is idle from
> > its
> > point of view.
> What if the user interacts with the device at this point?  Wouldn't 
> the program want to know about it?
> 
> Even if your program doesn't care about user interaction with an idle
> device, there undoubtedly will be other programs that _do_
> care.  This 
> mechanism we are designing needs to work with all programs.
> 

OK.

> > 
> > But after a while, there could be a situation where the user-space
> > wants
> > to send some message to device (due to end user interaction) then,
> > having (4) would be handy to cancel the undone suspend or cause
> > host-
> > wake.
> That's what the FORBID_SUSPEND ioctl does.  We don't need (4) for
> this.
> 

Right, OK.

> > 
> > > 
> > > > 
> > > > 2. Is it safe to wait for udev->state to be of a particular
> > > > value?
> > > No, not really, because the state can change.
> > > m[c]++;
> > If you remember, I had also suggested to use root-hub suspend in
> > generic_suspend() to call usbfs_notify_suspend/_resume APIs. It
> > might be
> > possible to use that in usbdev_poll() and send POLLPRI when root-hub
> > suspends.
> I don't see how that would help.  It would just make things more 
> confusing.  Forget about root-hub events for now.
> 

Yes sure. Thanks.

> > 
> > > 
> > > > 
> > > > If this approach could work, I can spend time on this one as
> > > > well.
> > > What advantage do you think your proposal has over my proposal?
> > > 
> > Not necessarily advantage(s), but few things that I could think of
> > are -
> > 
> > 1. In poll based approach, user-space notion of device's state is in
> > sync with actual device state.
> NO!  That simply is not true.  You don't seem to be able to grasp this
> point.
> 
> Consider this: Suppose the computer is busy running many other 
> programs.  Your program gets swapped out because a bunch of other 
> higher-priority tasks need to run.  By the time your program gets a 
> chance to run again, the device could have gone through several 
> suspend/resume transitions.  There's no way the program can keep
> track 
> of all that.
> 

Yeah I see what you mean.

> If you want a more likely example, consider this: Your program calls 
> ALLOW_SUSPEND and then calls poll().  The device suspends and the 
> poll() returns.  But then, before your program can do anything else, 
> the device resumes.  Now the device is active but your program thinks 
> it is suspended -- the program is out of sync.
> 
> Face it: It is _impossible_ for a program to truly know the device's 
> current state at all times (unless the device is not allowed to
> suspend 
> at all).
> 

Yep, thanks.

> > 
> > This is partially true with the 3 ioctl
> > approach suggested by you (partially because resume might not be
> > current
> > device state when wait-for-resume returns).
> Agreed.  But so what?  What abilities does your program lose as a 
> result of the fact that the device might be suspended when 
> WAIT_FOR_RESUME returns?
> 
> > 
> > 2. In 3 ioctl approach, user-space can still communicate with device
> > after calling ALLOW_SUSPEND. With poll based approach, we put a
> > constraint on user-space that, call ALLOW_SUSPEND only when you
> > determine you are not going to interact with device.
> That sounds like a disadvantage for your poll-based approach: It 
> constrains the behavior of userspace programs.
> 
> > 
> > I know for (2) above, you have commented before that -
> > A. It is desirable to be able to communicate with the device till it
> > is
> > actually suspended.
> > B. The possibility of device not going into suspend for long time,
> > so
> > user-space will not be able to proceed.
> > 
> > The counter comments to them are:
> > For (A), *usually*, the driver by some means determine device is
> > idle
> > and then schedules a suspend. After that, it is not expecting to
> > communicate with the device till resume happens. If it has to
> > communicate, it has to call resume first and then proceed.
> _Some_ programs will behave that way but other programs will not;
> they will want to continue communicating with the device right up
> until
> the time it suspends.  The kernel has to work with _all_ programs.
> 

Right, OK.

> > 
> > For (B), that is why we need ability to cancel current undone
> > suspend
> > operation, to allow the user-space to initiate the communication.
> And that is what FORBID_SUSPEND does.  You don't need to give up on
> (A) 
> in order to handle (B).
> 
> > 
> > Hope some of these points makes sense.
> None of them seem convincing, at least, not to me.
> 

Thanks for all the comments and clarifications, Alan.

I will check the 3-ioctl approach on the platform I have using the test
program I had used to send my original patch.

Just for my understanding, the below sequence should also work -
1. Call ALLOW_SUSPEND
2. Previously queued URB fails ==> device no longer active
3. Call WAIT_FOR_RESUME
4. After a while (say > 10 sec), assume no remote-wake by device. But
user-space wants to communicate with the device (due to some end-user
activity).
In this case, the user space needs to call FORBID_SUSPEND ioctl. When
that returns, it is safe to assume device is active.
5. Once done, go-to (1).

Could you please cross-confirm? Thanks,

> Alan Stern
> 

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Alan Stern]

On Thu, 27 Jun 2019, Mayuresh Kulkarni wrote:

> Thanks for all the comments and clarifications, Alan.
> 
> I will check the 3-ioctl approach on the platform I have using the test
> program I had used to send my original patch.
> 
> Just for my understanding, the below sequence should also work -
> 1. Call ALLOW_SUSPEND
> 2. Previously queued URB fails ==> device no longer active
> 3. Call WAIT_FOR_RESUME

You don't have to perform (2) if you don't want to.  You can proceed
directly to (3); the WAIT_FOR_RESUME ioctl won't return until the
device has both suspended and resumed (or the call is interrupted by a
signal -- such as a 10-second timer expiring).

> 4. After a while (say > 10 sec), assume no remote-wake by device. But
> user-space wants to communicate with the device (due to some end-user
> activity).
> In this case, the user space needs to call FORBID_SUSPEND ioctl. When
> that returns, it is safe to assume device is active.

Or maybe the WAIT_FOR_RESUME ioctl returns because there was a remote 
wakeup.  In this case also you would call FORBID_SUSPEND.

In fact, you should call FORBID_SUSPEND _whenever_ WAIT_FOR_RESUME
returns, unless your program has decided not to use the device any more
(in which case you don't care whether the device is suspended or
resumed).

> 5. Once done, go-to (1).
> 
> Could you please cross-confirm? Thanks,

That's all correct.

Alan Stern

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Oliver Neukum]

Am Donnerstag, den 27.06.2019, 09:52 -0400 schrieb Alan Stern:

> 
> Or maybe the WAIT_FOR_RESUME ioctl returns because there was a remote 
> wakeup.  In this case also you would call FORBID_SUSPEND.
> 
> In fact, you should call FORBID_SUSPEND _whenever_ WAIT_FOR_RESUME

Well, this kind of indicates that the original syscall should bump
the counter.

> returns, unless your program has decided not to use the device any more
> (in which case you don't care whether the device is suspended or
> resumed).

Then you should close the device.

	Regards
		Oliver

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Alan Stern]

On Mon, 1 Jul 2019, Oliver Neukum wrote:

> Am Donnerstag, den 27.06.2019, 09:52 -0400 schrieb Alan Stern:
> 
> > 
> > Or maybe the WAIT_FOR_RESUME ioctl returns because there was a remote 
> > wakeup.  In this case also you would call FORBID_SUSPEND.
> > 
> > In fact, you should call FORBID_SUSPEND _whenever_ WAIT_FOR_RESUME
> 
> Well, this kind of indicates that the original syscall should bump
> the counter.

Perhaps it does, but...

> > returns, unless your program has decided not to use the device any more
> > (in which case you don't care whether the device is suspended or
> > resumed).
> 
> Then you should close the device.

Exactly.  Suppose WAIT_FOR_RESUME is interrupted and then the program
closes the device.  There's no need to force the device back to full 
power in this situation.

Alan Stern

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Oliver Neukum]

Am Montag, den 01.07.2019, 10:17 -0400 schrieb Alan Stern:
> On Mon, 1 Jul 2019, Oliver Neukum wrote:
> 
> > Am Donnerstag, den 27.06.2019, 09:52 -0400 schrieb Alan Stern:
> > 
> > > 
> > > Or maybe the WAIT_FOR_RESUME ioctl returns because there was a remote 
> > > wakeup.  In this case also you would call FORBID_SUSPEND.
> > > 
> > > In fact, you should call FORBID_SUSPEND _whenever_ WAIT_FOR_RESUME
> > 
> > Well, this kind of indicates that the original syscall should bump
> > the counter.
> 
> Perhaps it does, but...
> 
> > > returns, unless your program has decided not to use the device any more
> > > (in which case you don't care whether the device is suspended or
> > > resumed).
> > 
> > Then you should close the device.
> 
> Exactly.  Suppose WAIT_FOR_RESUME is interrupted and then the program
> closes the device.  There's no need to force the device back to full 
> power in this situation.

But that is the error case. You return an error code. The point of that
is to report that a syscall did not have all requested effects.

	Regards
		Oliver

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Alan Stern]

On Tue, 2 Jul 2019, Oliver Neukum wrote:

> Am Montag, den 01.07.2019, 10:17 -0400 schrieb Alan Stern:
> > On Mon, 1 Jul 2019, Oliver Neukum wrote:
> > 
> > > Am Donnerstag, den 27.06.2019, 09:52 -0400 schrieb Alan Stern:
> > > 
> > > > 
> > > > Or maybe the WAIT_FOR_RESUME ioctl returns because there was a remote 
> > > > wakeup.  In this case also you would call FORBID_SUSPEND.
> > > > 
> > > > In fact, you should call FORBID_SUSPEND _whenever_ WAIT_FOR_RESUME
> > > 
> > > Well, this kind of indicates that the original syscall should bump
> > > the counter.
> > 
> > Perhaps it does, but...
> > 
> > > > returns, unless your program has decided not to use the device any more
> > > > (in which case you don't care whether the device is suspended or
> > > > resumed).
> > > 
> > > Then you should close the device.
> > 
> > Exactly.  Suppose WAIT_FOR_RESUME is interrupted and then the program
> > closes the device.  There's no need to force the device back to full 
> > power in this situation.
> 
> But that is the error case. You return an error code. The point of that
> is to report that a syscall did not have all requested effects.

Okay, I can change the patch to work as you suggest.  The 
WAIT_FOR_RESUME ioctl will do the equivalent of FORBID_SUSPEND whenever 
it returns with no error.

Alan Stern

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Mayuresh Kulkarni]

On Fri, 2019-06-21 at 15:28 -0400, Alan Stern wrote:
> On Fri, 21 Jun 2019, Mayuresh Kulkarni wrote:
> 
> However, I have been thinking about how to do all this in light of
> Oliver's comments, and it seems like we should make some changes.
> 
> First, let's change the ioctls' names.  Instead of RESUME and SUSPEND,
> I'll call them FORBID_SUSPEND and ALLOW_SUSPEND.  The way they work
> should be clear: ALLOW_SUSPEND will permit the device to be suspended
> but might not cause a suspend to happen immediately.  FORBID_SUSPEND
> will cause an immediate resume if the device is currently suspended
> and
> will prevent the device from being suspended in the future.  The new 
> names more accurately reflect what the ioctls actually do.
> 
> Second, the WAIT_FOR_RESUME ioctl will wait only until a resume has
> occurred more recently than the most recent ALLOW_SUSPEND ioctl.  So
> for example, if the program calls ALLOW_SUSPEND, and the device
> suspends, and then the device resumes, and then the device suspends
> again, and then the program calls WAIT_FOR_RESUME, the ioctl will
> return immediately even though the device is currently suspended.  
> Thus you don't have to worry about missing a remote resume.  This also
> means that when WAIT_FOR_RESUME returns, the program should call
> FORBID_SUSPEND to ensure that the device is active and doesn't go
> back 
> into suspend.
> 
> In practice, your program would use the ioctls as follows:
> 
> 	When the device file is opened, the kernel will automatically
> 	do the equivalent of FORBID_SUSPEND (to remain compatible 
> 	with the current behavior).
> 
> 	When the program is ready for the device to suspend, it will
> 	call the ALLOW_SUSPEND ioctl.  But it won't cancel the 
> 	outstanding URBs; instead it will continue to interact 
> 	normally with the device, because the device might not be 
> 	suspended for some time.
> 
> 	When the device does go into suspend, URBs will start failing
> 	with an appropriate error code (EHOSTUNREACH, ESHUTDOWN, 
> 	EPROTO, or something similar).  In this way the program will
> 	realize the device is suspended.  At that point the program
> 	should call the WAIT_FOR_RESUME ioctl and stop trying to 
> 	communicate with the device.
> 
> 	When WAIT_FOR_RESUME returns, the program should call the
> 	FORBID_SUSPEND ioctl and resume normal communication with the 
> 	device.
> 
> How does that sound?
> 
> The proposed patch is below.
> 
> Alan Stern
> 

Hi Alan,

With the 3-ioctl patch you had send, I checked the behaviour using my
test program on our reference platform.

AFAIU, the patch seems to work fine for our use-cases of: remote-wake
and host-wake.
For our typical use-cases, the user-space does not need to communicate
with the device even after calling ALLOW_SUSPEND, so I am not in
position to verify this point.

The test does the below steps -
----------------
A. REMOTE-WAKE -
----------------
1. Open the device file.
2. Find our interface and bind to it.
3. Send multiple commands to our interface.
4. Call ALLOW_SUSPEND.
5. Call WAIT_FOR_RESUME.
6. Wait for sometime (10-20 sec).
7. Do the activity on device which I know causes remote-wake to host.
8. Waiter in (5) above, calls FORBID_SUSPEND and reads the cause of
resume.
9. Release the interface and close the device file.

After (5) + auto-suspend time-out expires, I can see device, root-hub
and host-controller (DWC-3) going into suspend.
After (7), host-controller, root-hub and device resume are seen.

--------------
B. HOST-WAKE -
--------------
1. Open the device file.
2. Find our interface and bind to it.
3. Send multiple commands to our interface.
4. Spawn a thread to cause host-wake.
5. Call ALLOW_SUSPEND.
6. Call WAIT_FOR_RESUME.
7. Signal thread after a delay (10-20 sec).
8. Thread calls FORBID_SUSPEND and sends a command to device.
9. Release the interface, wait for thread-join and close the device
file.

After (6) + auto-suspend time-out expires, device, root-hub and host
controller goes into suspend.
After (8), host-controller, root-hub and device resume are seen. Also
the response to command is correct.

With that said, at this point, the above observations are in-sync with
what I had verified when I had send-out the old patch (with 2-ioctls and
interface based suspend-resume). I am checking if I can verify a bit
more complex use-cases. But not sure, if I can do that with current
setup.

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.

Thanks to You/Oliver/Greg KH for the all the comments and reviews.

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

[Alan Stern]

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?

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 */

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

[Mayuresh Kulkarni]

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).
> 

Hi Alan,

This patch looks good.
Let me know the next step(s) and if anything else is needed from me.

Thanks.

> 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?
> 
> Alan Stern
> 

A request -

I will highly appreciate if someone from Google/Android's USB team
comment on this patch. The reason being, when this is merged, I imagine
there will be suitable APIs in some future version of Google/Android's
USB manager to enable suspend/resume for apps.

Thanks.

> 
>  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 */
> 

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

[Alan Stern]

On Thu, 11 Jul 2019, Mayuresh Kulkarni wrote:

> 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).
> > 
> 
> Hi Alan,
> 
> This patch looks good.
> Let me know the next step(s) and if anything else is needed from me.

The next step is to see if there are any comments.  If there aren't, I
will submit the patch officially.

> Thanks.
> 
> > 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?
> > 
> > Alan Stern
> > 
> 
> A request -
> 
> I will highly appreciate if someone from Google/Android's USB team
> comment on this patch. The reason being, when this is merged, I imagine
> there will be suitable APIs in some future version of Google/Android's
> USB manager to enable suspend/resume for apps.

Nobody on Google/Android's USB team is likely to comment unless you 
bring this directly to their attention.  Don't assume they will just 
happen to see it on the mailing list.

Also, the last I looked, Android was using version 4.9 of the kernel.  
Since this patch won't get into the kernel until version 5.4 at the
earliest, it may be quite a while before it shows up in Android.

Alan Stern

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

[Greg KH]

On Thu, Jul 11, 2019 at 10:20:10AM -0400, Alan Stern wrote:
> Also, the last I looked, Android was using version 4.9 of the kernel.  
> Since this patch won't get into the kernel until version 5.4 at the
> earliest, it may be quite a while before it shows up in Android.

Android does not "use" a specific version of the kernel, they only
specify the "minimum kernel version" for a specific Android release.

As an example, for Android Q, they require the 4.9 kernel as a minimum,
and they have a specific LTS version as well, with updates required over
time as well.

That being said, you can always use a newer kernel version for Android,
and they publish 4.14 and 4.19 kernel trees if you wish to use them, as
well as they have a "mainline" branch that is now at 5.2 for companies
that want to use those kernels.

But for new features, like this one, you will need Android userspace
changes to start relying on the kernel change, so that will take a bit
of work, and by then the new kernel feature could have been backported
as well if it is something they want to rely on.

So yes, it would be at least a year before it showed up in Android given
they do a release on a yearly cadence.

thanks,

greg k-h

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

[Mayuresh Kulkarni]

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 */
> 

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

[Greg KH]

On Thu, Jul 25, 2019 at 10:10:27AM +0100, Mayuresh Kulkarni wrote:
> 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?

Not yet, sorry, dealing with the backlog of patches after the merge
window closed.

Give me a week or so...

But if you want to try your hand at it first, it's always easier to
review a patch than it is to come up with a new one.

thanks,

greg k-h

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

[Alan Stern]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 1335 bytes --]

On Thu, 25 Jul 2019, Greg KH wrote:

> > > 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?
> 
> Not yet, sorry, dealing with the backlog of patches after the merge
> window closed.
> 
> Give me a week or so...
> 
> But if you want to try your hand at it first, it's always easier to
> review a patch than it is to come up with a new one.

Would Documentation/userspace-api/ be the right place to put this 
information?  It looks like we could take a large chunk of 
driver-api/usb/usb.rst (most of it, in fact) and move it over there.

By the way, do you know anything about how the information in
Documentation/userspace-api gets presented to users in general?  Is
there anything comparable to the Linux man-pages project?  Or are
people just supposed to get hold of the kernel source from somewhere
and read the files there?

Alan Stern

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

[Greg KH]

On Thu, Jul 25, 2019 at 11:18:09AM -0400, Alan Stern wrote:
> On Thu, 25 Jul 2019, Greg KH wrote:
> 
> > > > 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?
> > 
> > Not yet, sorry, dealing with the backlog of patches after the merge
> > window closed.
> > 
> > Give me a week or so...
> > 
> > But if you want to try your hand at it first, it's always easier to
> > review a patch than it is to come up with a new one.
> 
> Would Documentation/userspace-api/ be the right place to put this 
> information?  It looks like we could take a large chunk of 
> driver-api/usb/usb.rst (most of it, in fact) and move it over there.

Sounds reasonable.

> By the way, do you know anything about how the information in
> Documentation/userspace-api gets presented to users in general?  Is
> there anything comparable to the Linux man-pages project?  Or are
> people just supposed to get hold of the kernel source from somewhere
> and read the files there?

No idea, but we do build it on every kernel release and put it up on
kernel.org to be easily indexed by search engines:
	https://www.kernel.org/doc/html/latest/userspace-api/index.html

thanks,

greg k-h

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Mayuresh Kulkarni]

On Wed, 2019-06-19 at 11:19 +0200, Oliver Neukum wrote:
> Am Dienstag, den 18.06.2019, 11:50 -0400 schrieb Alan Stern:
> > 
> > On Tue, 18 Jun 2019, Mayuresh Kulkarni wrote:
> > 
> > > 
> > > > 
> > > > You're right that the program needs to know when the device is
> > > > about
> > > > to 
> > > > be suspended.  But waiting for an ioctl to return isn't a good
> > > > way 
> > > > to do it; this needs to be a callback of some sort.  That is,
> > > > the 
> > > > kernel also needs to know when the program is ready for the
> > > > suspend.
> > > > 
> > > > I don't know what is the best approach.
> > > This is becoming tricky now.
> > Yes.  There probably are mechanisms already in use in other parts
> > of 
> > the kernel that would be suitable here, but I don't know what they
> > are.  
> > We could try asking some other people for advice.
> Waiting for an ioctl() is horrible. If you really want to do this
> poll() would be the obvious API. It is made for waiting for changes
> of states.
> 

Understood and agreed.

> [..]
> > 
> > The suspend callback is _not_ responsible for actually suspending
> > the
> > device; that is handled by the USB subsystem core.
> > 
> > These ideas are indeed applicable to programs using usbfs.  The
> > kernel
> Not fully. Usbfs has the same issue as FUSE here. Drivers are per
> interface but power management is per device. Hence every driver
> is in the block IO path for these issues. You cannot do block IO
> in user space. The best you can do is notify of state changes,
> but you cannot wait for them.
> 
> > 
> > needs to have a way to inform the program that the device is about
> > enter (or has just left) a low-power state, so that the program can
> > stop (or start) trying to communicate with the device.  And the
> > kernel 
> > needs to know when the program is ready for the state change.
> That has difficulties based in fundamental issues. We can let user
> space block power transitions. We can notify it. But we cannot
> block on it.
> 
> It would be easiest to export the usb_autopm_* API to user space.

AFAIU, usb_autopm_* API operate on interface rather than on device.
Due to this, they are *indirectly* exposed by appropriate class drivers
to the user-space class drivers cater to. E.g.: USB audio class driver
calls usb_autopm_* APIs when user space calls pcm_open(playback_stream).

> 
> 	Regards
> 		Oliver
> 

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Alan Stern]

On Thu, 20 Jun 2019, Mayuresh Kulkarni wrote:

> > It would be easiest to export the usb_autopm_* API to user space.
> 
> AFAIU, usb_autopm_* API operate on interface rather than on device.
> Due to this, they are *indirectly* exposed by appropriate class drivers
> to the user-space class drivers cater to. E.g.: USB audio class driver
> calls usb_autopm_* APIs when user space calls pcm_open(playback_stream).

In fact, the usb_autopm_* API can operate both on interfaces and on
devices.  It is used both ways in the kernel.

Alan Stern

Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Mayuresh Kulkarni]

On Wed, 2019-06-05 at 11:41 +0200, Greg KH wrote:
> On Fri, May 10, 2019 at 11:01:09AM +0100, Mayuresh Kulkarni wrote:
> > 
> > - The current driver increments the PM ref-count in its .open
> > API and decrements it in its .close API.
> > - Due to this, it is not possible for the usb_device to go into
> > suspend (L2) even if all of its interfaces report idle to usb-core.
> > - In order to allow the suspend, 2 new ioctls are added:
> > 1. USBDEVFS_SUSPEND: calls auto-suspend and indicates to usb/pm core
> > to attempt suspend, if appropriate. This is a non-blocking call.
> > 2. USBDEVFS_WAIT_RESUME: waits for resume. This is a blocking call.
> > The resume could happen due to:
> > host-wake (i.e.: any driver bound to interface(s) of device wants to
> > send/receive control/data)
> > OR
> > remote-wake (i.e.: when remote-wake enabled device generates a
> > remote-wake to host).
> > In both these conditions, this call will return.
> > - It is expected that:
> > 1. When user-space thinks the device is idle from its point-of-view,
> > it calls USBDEVFS_SUSPEND.
> > 2. After USBDEVFS_WAIT_RESUME call returns,
> > the callers queries the device/interface of its interest to see
> > what caused resume and take appropriate action on it.
> I'm going to make a bunch of random comments on this patch, some
> cosmetic...
> 

Hi Greg,

Apologies for late response from me. I was on PTO last week and then in a
training this week.

Thanks for your comments. I will fix most of them in next patch as Alan has
suggested some more code-changes (in the later response than this one).

> First off, the above is horrible to try to read, please format things in
> a sane way such that we can understand it well.
> 
> > 
> > The link to relevant discussion about this patch on linux-usb is -
> > https://www.spinics.net/lists/linux-usb/msg173285.html
> You should not need to reference any external web site for a changelog
> entry, just put the needed information in the changelog itself.
> 
> > 
> > Signed-off-by: Mayuresh Kulkarni <mkulkarni@opensource.cirrus.com>
> > ---
> >  drivers/usb/core/devio.c          | 114
> > ++++++++++++++++++++++++++++++++++++--
> >  include/uapi/linux/usbdevice_fs.h |   2 +
> >  2 files changed, 112 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> > index fa783531..67dc326 100644
> > --- a/drivers/usb/core/devio.c
> > +++ b/drivers/usb/core/devio.c
> > @@ -68,6 +68,9 @@ struct usb_dev_state {
> >  	u32 disabled_bulk_eps;
> >  	bool privileges_dropped;
> >  	unsigned long interface_allowed_mask;
> > +	bool runtime_active;
> > +	bool is_suspended;
> > +	wait_queue_head_t wait_resume;
> That's some crazy alignment issues, please don't waste bytes for no good
> reason.
> 
> And can you document these fields somewhere?
> 
> 
> > 
> >  };
> >  
> >  struct usb_memory {
> > @@ -444,6 +447,23 @@ static struct async *async_getpending(struct
> > usb_dev_state *ps,
> >  	return NULL;
> >  }
> >  
> > +static int async_getpending_count(struct usb_dev_state *ps)
> > +{
> > +	struct async *as;
> > +	int count;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&ps->lock, flags);
> > +	if (list_empty(&ps->async_pending))
> > +		count = 0;
> > +	else
> > +		list_for_each_entry(as, &ps->async_pending, asynclist)
> > +			count++;
> Doesn't list_for_each_entry() work just fine on an empty list?  Just set
> count to 0 up above, right?
> 
> > 
> > +	spin_unlock_irqrestore(&ps->lock, flags);
> So, right after you call this function, and drop the lock, the entries
> can change, making your "count" invalid and stale.  So you can not trust
> the value at all, right?
> 
> > 
> > +
> > +	return count;
> > +}
> > +
> >  static void snoop_urb(struct usb_device *udev,
> >  		void __user *userurb, int pipe, unsigned length,
> >  		int timeout_or_status, enum snoop_when when,
> > @@ -699,16 +719,26 @@ static void driver_disconnect(struct usb_interface
> > *intf)
> >  	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.
> > - */
> >  static int driver_suspend(struct usb_interface *intf, pm_message_t msg)
> >  {
> > +	struct usb_dev_state *ps = usb_get_intfdata(intf);
> > +
> > +	ps->is_suspended = true;
> No locking needed?
> 
> > 
> > +	snoop(&ps->dev->dev, "driver-suspend\n");
> Why?  Does anyone use the snoop api anymore?
> 
> > 
> > +
> >  	return 0;
> >  }
> >  
> >  static int driver_resume(struct usb_interface *intf)
> >  {
> > +	struct usb_dev_state *ps = usb_get_intfdata(intf);
> > +
> > +	ps->runtime_active = true;
> > +	wake_up(&ps->wait_resume);
> > +
> > +	snoop(&ps->dev->dev, "driver-resume: runtime-active = %d\n",
> > +		ps->runtime_active);
> What does runtime_active give userspace here?  Again, who is using
> snoop?  And isn't runtime_active a bool?  It's always going to be "true"
> here.  Is this just debugging code left in?
> 
> > 
> > +
> >  	return 0;
> >  }
> >  
> > @@ -718,6 +748,7 @@ struct usb_driver usbfs_driver = {
> >  	.disconnect =	driver_disconnect,
> >  	.suspend =	driver_suspend,
> >  	.resume =	driver_resume,
> > +	.supports_autosuspend = 1,
> >  };
> >  
> >  static int claimintf(struct usb_dev_state *ps, unsigned int ifnum)
> > @@ -963,6 +994,27 @@ static struct usb_device *usbdev_lookup_by_devt(dev_t
> > devt)
> >  	return to_usb_device(dev);
> >  }
> >  
> > +/* must be called with usb-dev lock held */
> > +static int usbdev_do_resume(struct usb_dev_state *ps)
> > +{
> > +	int ret = 0;
> > +
> > +	if (!ps->runtime_active) {
> > +		snoop(&ps->dev->dev, "suspended..resume now\n");
> Again snoop?
> 
> > 
> > +		ps->is_suspended = false;
> > +		if (usb_autoresume_device(ps->dev)) {
> > +			ret = -EIO;
> > +			goto _out;
> > +		}
> > +		snoop(&ps->dev->dev, "resume done..active now\n");
> > +		ps->runtime_active = true;
> > +		wake_up(&ps->wait_resume);
> No locking at all?
> 
> > 
> > +	}
> > +
> > +_out:
> Why is this even needed, just return -EIO above and all is good.
> 
> But again, is any locking needed here?
> 
> 
> > 
> > +	return ret;
> > +}
> > +
> >  /*
> >   * file operations
> >   */
> > @@ -1008,6 +1060,9 @@ static int usbdev_open(struct inode *inode, struct
> > file *file)
> >  	INIT_LIST_HEAD(&ps->async_completed);
> >  	INIT_LIST_HEAD(&ps->memory_list);
> >  	init_waitqueue_head(&ps->wait);
> > +	init_waitqueue_head(&ps->wait_resume);
> > +	ps->runtime_active = true;
> > +	ps->is_suspended = false;
> >  	ps->disc_pid = get_pid(task_pid(current));
> >  	ps->cred = get_current_cred();
> >  	smp_wmb();
> > @@ -1034,6 +1089,10 @@ static int usbdev_release(struct inode *inode, struct
> > file *file)
> >  	struct async *as;
> >  
> >  	usb_lock_device(dev);
> > +
> > +	/* what can we can do if resume fails? */
> You tell me!
> 
> > 
> > +	usbdev_do_resume(ps);
> > +
> >  	usb_hub_release_all_ports(dev, ps);
> >  
> >  	list_del_init(&ps->list);
> > @@ -2355,6 +2414,18 @@ static int proc_drop_privileges(struct usb_dev_state
> > *ps, void __user *arg)
> >  	return 0;
> >  }
> >  
> > +static int proc_wait_resume(struct usb_dev_state *ps)
> > +{
> > +	int ret;
> > +
> > +	snoop(&ps->dev->dev, "wait-for-resume\n");
> > +	ret = wait_event_interruptible(ps->wait_resume,
> > +		(ps->runtime_active == true));
> > +	snoop(&ps->dev->dev, "wait-for-resume...done\n");
> This is just debugging code, right?  Please remove.
> 
> > 
> > +
> > +	return ret;
> > +}
> > +
> >  /*
> >   * NOTE:  All requests here that have interface numbers as parameters
> >   * are assuming that somehow the configuration has been prevented from
> > @@ -2373,6 +2444,25 @@ static long usbdev_do_ioctl(struct file *file,
> > unsigned int cmd,
> >  
> >  	usb_lock_device(dev);
> >  
> > +	if (cmd != USBDEVFS_WAIT_RESUME) {
> > +		ret = usbdev_do_resume(ps);
> do resume for all ioctl commands?  are you sure?
> 
> > 
> > +		if (ret)
> > +			goto done;
> > +	} else {
> > +		usb_unlock_device(dev);
> > +		ret = proc_wait_resume(ps);
> > +
> > +		/* Call auto-resume to balance auto-suspend of suspend-
> > ioctl */
> > +		usb_lock_device(dev);
> > +		if (ps->is_suspended) {
> > +			ret = usb_autoresume_device(ps->dev);
> > +			ps->is_suspended = false;
> > +		}
> > +		usb_unlock_device(dev);
> > +
> > +		goto _done;
> What is the difference between _done and done?  Please have descriptive
> labels if you are going to have any at all.
> 
> Why isn't this part of the switch statement below?
> 
> > 
> > +	}
> > +
> >  	/* Reap operations are allowed even after disconnection */
> >  	switch (cmd) {
> >  	case USBDEVFS_REAPURB:
> > @@ -2549,10 +2639,26 @@ static long usbdev_do_ioctl(struct file *file,
> > unsigned int cmd,
> >  	case USBDEVFS_GET_SPEED:
> >  		ret = ps->dev->speed;
> >  		break;
> > +	case USBDEVFS_SUSPEND:
> > +		ret = 0;
> > +
> > +		/* we cannot suspend when URB is pending */
> > +		if (async_getpending_count(ps)) {
> > +			snoop(&ps->dev->dev, "ioctl-suspend but URB
> > pending\n");
> You do not know this, your value is stale :(
> 
> And again, you are using snoop() calls for debugging, not for actual USB
> traffic, which is what it was designed for.  These all need to be
> removed/rewritten.
> 
> > 
> > +			ret = -EINVAL;
> > +		} else {
> > +			if (ps->runtime_active) {
> > +				snoop(&ps->dev->dev, "ioctl-suspend\n");
> > +				ps->runtime_active = false;
> > +				usb_autosuspend_device(ps->dev);
> > +			}
> > +		}
> > +		break;
> >  	}
> >  
> > - done:
> > +done:
> >  	usb_unlock_device(dev);
> > +_done:
> See, horrid names :(
> 
> > 
> >  	if (ret >= 0)
> >  		inode->i_atime = current_time(inode);
> >  	return ret;
> > diff --git a/include/uapi/linux/usbdevice_fs.h
> > b/include/uapi/linux/usbdevice_fs.h
> > index 964e872..ae46beb 100644
> > --- a/include/uapi/linux/usbdevice_fs.h
> > +++ b/include/uapi/linux/usbdevice_fs.h
> > @@ -197,5 +197,7 @@ 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_SUSPEND           _IO('U', 32)
> > +#define USBDEVFS_WAIT_RESUME       _IO('U', 33)
> No documentation for what these do?
> 
> thanks,
> 
> greg k-h