Re: [RFC PATCH v2 1/6] usb: Register usb port's acpi power resources

Re: [RFC PATCH v2 1/6] usb: Register usb port's acpi power resources

[Rafael J. Wysocki]

On Tuesday, November 13, 2012 08:36:15 PM Lan Tianyu wrote:
> 于 2012/11/13 19:07, Sergei Shtylyov 写道:
> > Hello.
> >
> > On 13-11-2012 12:00, Lan Tianyu wrote:
> >
> >> This patch is to register usb port's acpi power resources. Create
> >> link between usb port device and its acpi power resource.
> >
> >> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> > [...]
> >
> >> diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
> >> index cef4252..c58ebc0 100644
> >> --- a/drivers/usb/core/usb-acpi.c
> >> +++ b/drivers/usb/core/usb-acpi.c
> >> @@ -216,6 +216,28 @@ static struct acpi_bus_type usb_acpi_bus = {
> >>       .find_device = usb_acpi_find_device,
> >>   };
> >>
> >> +int usb_acpi_register_power_resources(struct device *dev)
> >> +{
> >> +    acpi_handle port_handle = DEVICE_ACPI_HANDLE(dev);
> >> +
> >> +    if (!port_handle)
> >> +        return -ENODEV;
> >> +
> >> +    if (acpi_power_resource_register_device(dev, port_handle) < 0)
> >> +        return -ENODEV;
> >> +    return 0;
> >> +}
> >> +
> >> +void usb_acpi_unregister_power_resources(struct device *dev)
> >> +{
> >> +    acpi_handle port_handle = DEVICE_ACPI_HANDLE(dev);
> >> +
> >> +    if (!port_handle)
> >> +        return;
> >> +
> >> +    acpi_power_resource_register_device(dev, port_handle);
> >
> >     I thinbk you have been askied already, but shouldn't it be
> > acpi_power_resource_unregister_device()?
> >
> Oh. Sorry. Too focus on the other modification. Thanks for your reminder.

Besides, it would be a bit more natural to do

if (port_handle)
	acpi_power_resource_unregister_device(dev, port_handle);

instead of doing that return when port_handle is NULL.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH v2 2/6] usb: Add driver/usb/core/port.c file to fill usb port related code.

[Rafael J. Wysocki]

On Tuesday, November 13, 2012 04:00:01 PM Lan Tianyu wrote:
> This patch is to create driver/usb/core/port.c and move usb port related
> code into it.

It does seem to make functional changes in addition to that, however.

If I'm not mistaken and that really is the case, can you (briefly)
describe those changes here too?

Rafael


> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> ---
>  drivers/usb/core/Makefile |    1 +
>  drivers/usb/core/hub.c    |  113 +++++++--------------------------------------
>  drivers/usb/core/port.c   |   82 ++++++++++++++++++++++++++++++++
>  drivers/usb/core/usb.h    |    3 ++
>  include/linux/usb.h       |   16 +++++++
>  5 files changed, 118 insertions(+), 97 deletions(-)
>  create mode 100644 drivers/usb/core/port.c
> 
> diff --git a/drivers/usb/core/Makefile b/drivers/usb/core/Makefile
> index 26059b9..5e847ad 100644
> --- a/drivers/usb/core/Makefile
> +++ b/drivers/usb/core/Makefile
> @@ -7,6 +7,7 @@ ccflags-$(CONFIG_USB_DEBUG) := -DDEBUG
>  usbcore-y := usb.o hub.o hcd.o urb.o message.o driver.o
>  usbcore-y += config.o file.o buffer.o sysfs.o endpoint.o
>  usbcore-y += devio.o notify.o generic.o quirks.o devices.o
> +usbcore-y += port.o
>  
>  usbcore-$(CONFIG_PCI)		+= hcd-pci.o
>  usbcore-$(CONFIG_ACPI)		+= usb-acpi.o
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 3c85fe1c..60746aa 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -42,13 +42,6 @@
>  #define USB_VENDOR_GENESYS_LOGIC		0x05e3
>  #define HUB_QUIRK_CHECK_PORT_AUTOSUSPEND	0x01
>  
> -struct usb_port {
> -	struct usb_device *child;
> -	struct device dev;
> -	struct dev_state *port_owner;
> -	enum usb_port_connect_type connect_type;
> -};
> -
>  struct usb_hub {
>  	struct device		*intfdev;	/* the "interface" device */
>  	struct usb_device	*hdev;
> @@ -170,9 +163,6 @@ EXPORT_SYMBOL_GPL(ehci_cf_port_reset_rwsem);
>  #define HUB_DEBOUNCE_STEP	  25
>  #define HUB_DEBOUNCE_STABLE	 100
>  
> -#define to_usb_port(_dev) \
> -	container_of(_dev, struct usb_port, dev)
> -
>  static int usb_reset_and_verify_device(struct usb_device *udev);
>  
>  static inline char *portspeed(struct usb_hub *hub, int portstatus)
> @@ -1237,57 +1227,12 @@ static int hub_post_reset(struct usb_interface *intf)
>  	return 0;
>  }
>  
> -static void usb_port_device_release(struct device *dev)
> -{
> -	struct usb_port *port_dev = to_usb_port(dev);
> -
> -	usb_acpi_unregister_power_resources(dev);
> -	kfree(port_dev);
> -}
> -
>  static void usb_hub_remove_port_device(struct usb_hub *hub,
>  				       int port1)
>  {
>  	device_unregister(&hub->ports[port1 - 1]->dev);
>  }
>  
> -struct device_type usb_port_device_type = {
> -	.name =		"usb_port",
> -	.release =	usb_port_device_release,
> -};
> -
> -static int usb_hub_create_port_device(struct usb_hub *hub,
> -				      int port1)
> -{
> -	struct usb_port *port_dev = NULL;
> -	int retval;
> -
> -	port_dev = kzalloc(sizeof(*port_dev), GFP_KERNEL);
> -	if (!port_dev) {
> -		retval = -ENOMEM;
> -		goto exit;
> -	}
> -
> -	hub->ports[port1 - 1] = port_dev;
> -	port_dev->dev.parent = hub->intfdev;
> -	port_dev->dev.groups = port_dev_group;
> -	port_dev->dev.type = &usb_port_device_type;
> -	dev_set_name(&port_dev->dev, "port%d", port1);
> -
> -	retval = device_register(&port_dev->dev);
> -	if (retval)
> -		goto error_register;
> -
> -	usb_acpi_register_power_resources(&port_dev->dev);
> -
> -	return 0;
> -
> -error_register:
> -	put_device(&port_dev->dev);
> -exit:
> -	return retval;
> -}
> -
>  static int hub_configure(struct usb_hub *hub,
>  	struct usb_endpoint_descriptor *endpoint)
>  {
> @@ -1548,10 +1493,24 @@ static int hub_configure(struct usb_hub *hub,
>  	if (hub->has_indicators && blinkenlights)
>  		hub->indicator [0] = INDICATOR_CYCLE;
>  
> -	for (i = 0; i < hdev->maxchild; i++)
> -		if (usb_hub_create_port_device(hub, i + 1) < 0)
> +	for (i = 0; i < hdev->maxchild; i++) {
> +		struct usb_port *port_dev = NULL;
> +
> +		port_dev = kzalloc(sizeof(*port_dev), GFP_KERNEL);
> +		if (!port_dev) {
> +			dev_err(hub->intfdev,
> +				"couldn't create port%d device due to lack mem.\n", i + 1);
> +			continue;
> +		}
> +
> +		hub->ports[i] = port_dev;
> +
> +		if (usb_hub_create_port_device(hub->intfdev, i + 1, port_dev) < 0) {
>  			dev_err(hub->intfdev,
>  				"couldn't create port%d device.\n", i + 1);
> +			hub->ports[i] = NULL;
> +		}
> +	}
>  
>  	if (!hub_is_superspeed(hdev)) {
>  		for (i = 1; i <= hdev->maxchild; i++)
> @@ -4765,46 +4724,6 @@ static int hub_thread(void *__unused)
>  	return 0;
>  }
>  
> -static ssize_t show_port_connect_type(struct device *dev,
> -	struct device_attribute *attr, char *buf)
> -{
> -	struct usb_port *port_dev = to_usb_port(dev);
> -	char *result;
> -
> -	switch (port_dev->connect_type) {
> -	case USB_PORT_CONNECT_TYPE_HOT_PLUG:
> -		result = "hotplug";
> -		break;
> -	case USB_PORT_CONNECT_TYPE_HARD_WIRED:
> -		result = "hardwired";
> -		break;
> -	case USB_PORT_NOT_USED:
> -		result = "not used";
> -		break;
> -	default:
> -		result = "unknown";
> -		break;
> -	}
> -
> -	return sprintf(buf, "%s\n", result);
> -}
> -static DEVICE_ATTR(connect_type, S_IRUGO, show_port_connect_type,
> -		NULL);
> -
> -static struct attribute *port_dev_attrs[] = {
> -	&dev_attr_connect_type.attr,
> -	NULL,
> -};
> -
> -static struct attribute_group port_dev_attr_grp = {
> -	.attrs = port_dev_attrs,
> -};
> -
> -static const struct attribute_group *port_dev_group[] = {
> -	&port_dev_attr_grp,
> -	NULL,
> -};
> -
>  static const struct usb_device_id hub_id_table[] = {
>      { .match_flags = USB_DEVICE_ID_MATCH_VENDOR
>  	           | USB_DEVICE_ID_MATCH_INT_CLASS,
> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
> new file mode 100644
> index 0000000..6523a03
> --- /dev/null
> +++ b/drivers/usb/core/port.c
> @@ -0,0 +1,82 @@
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/module.h>
> +#include <linux/usb.h>
> +
> +#include "usb.h"
> +
> +static ssize_t show_port_connect_type(struct device *dev,
> +	struct device_attribute *attr, char *buf)
> +{
> +	struct usb_port *port_dev = to_usb_port(dev);
> +	char *result;
> +
> +	switch (port_dev->connect_type) {
> +	case USB_PORT_CONNECT_TYPE_HOT_PLUG:
> +		result = "hotplug";
> +		break;
> +	case USB_PORT_CONNECT_TYPE_HARD_WIRED:
> +		result = "hardwired";
> +		break;
> +	case USB_PORT_NOT_USED:
> +		result = "not used";
> +		break;
> +	default:
> +		result = "unknown";
> +		break;
> +	}
> +
> +	return sprintf(buf, "%s\n", result);
> +}
> +static DEVICE_ATTR(connect_type, S_IRUGO, show_port_connect_type,
> +		NULL);
> +
> +static struct attribute *port_dev_attrs[] = {
> +	&dev_attr_connect_type.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group port_dev_attr_grp = {
> +	.attrs = port_dev_attrs,
> +};
> +
> +static const struct attribute_group *port_dev_group[] = {
> +	&port_dev_attr_grp,
> +	NULL,
> +};
> +
> +static void usb_port_device_release(struct device *dev)
> +{
> +	struct usb_port *port_dev = to_usb_port(dev);
> +
> +	usb_acpi_unregister_power_resources(dev);
> +	kfree(port_dev);
> +}
> +
> +struct device_type usb_port_device_type = {
> +	.name =		"usb_port",
> +	.release =	usb_port_device_release,
> +};
> +
> +int usb_hub_create_port_device(struct device *intfdev,
> +		int port1, struct usb_port *port_dev)
> +{
> +	int retval;
> +
> +	port_dev->dev.parent = intfdev;
> +	port_dev->dev.groups = port_dev_group;
> +	port_dev->dev.type = &usb_port_device_type;
> +	dev_set_name(&port_dev->dev, "port%d", port1);
> +
> +	retval = device_register(&port_dev->dev);
> +	if (retval)
> +		goto error_register;
> +
> +	usb_acpi_register_power_resources(&port_dev->dev);
> +
> +	return 0;
> +
> +error_register:
> +	put_device(&port_dev->dev);
> +	return retval;
> +}
> diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
> index 38958fb..de7434b 100644
> --- a/drivers/usb/core/usb.h
> +++ b/drivers/usb/core/usb.h
> @@ -60,6 +60,9 @@ extern void usb_hub_cleanup(void);
>  extern int usb_major_init(void);
>  extern void usb_major_cleanup(void);
>  
> +extern int usb_hub_create_port_device(struct device *intfdev,
> +		int port1, struct usb_port *port_dev);
> +
>  #ifdef	CONFIG_PM
>  
>  extern int usb_suspend(struct device *dev, pm_message_t msg);
> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index e996bb6..c1f1346 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -572,6 +572,22 @@ struct usb_device {
>  };
>  #define	to_usb_device(d) container_of(d, struct usb_device, dev)
>  
> +/**
> + * struct usb port - kernel's representation of a usb port
> + * @child: usb device attatched to the port
> + * @dev: generic device interface
> + * @port_owner: port's owner
> + * @connect_type: port's connect type
> + */
> +struct usb_port {
> +	struct usb_device *child;
> +	struct device dev;
> +	struct dev_state *port_owner;
> +	enum usb_port_connect_type connect_type;
> +};
> +#define to_usb_port(_dev) \
> +	container_of(_dev, struct usb_port, dev)
> +
>  static inline struct usb_device *interface_to_usbdev(struct usb_interface *intf)
>  {
>  	return to_usb_device(intf->dev.parent);
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH v2 3/6] usb: add runtime pm support for usb port device

[Rafael J. Wysocki]

On Tuesday, November 13, 2012 04:00:02 PM Lan Tianyu wrote:
> This patch is to add runtime pm callback for usb port device.
> Set/clear PORT_POWER feature in the resume/suspend callbak.
> Add portnum for struct usb_port to record port number.
> 
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>

This one looks reasonable to me.  From the PM side

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/usb/core/hub.c  |   14 ++++++++++++++
>  drivers/usb/core/port.c |   39 +++++++++++++++++++++++++++++++++++++++
>  drivers/usb/core/usb.h  |    2 ++
>  include/linux/usb.h     |    2 ++
>  4 files changed, 57 insertions(+)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 60746aa..2cb414e 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -765,6 +765,20 @@ static void hub_tt_work(struct work_struct *work)
>  	spin_unlock_irqrestore (&hub->tt.lock, flags);
>  }
>  
> +int usb_hub_set_port_power(struct usb_device *hdev, int port1,
> +		bool set)
> +{
> +	int ret;
> +
> +	if (set)
> +		ret = set_port_feature(hdev, port1,
> +				USB_PORT_FEAT_POWER);
> +	else
> +		ret = clear_port_feature(hdev, port1,
> +				USB_PORT_FEAT_POWER);
> +	return ret;
> +}
> +
>  /**
>   * usb_hub_clear_tt_buffer - clear control/bulk TT state in high speed hub
>   * @urb: an URB associated with the failed or incomplete split transaction
> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
> index 6523a03..1cda766 100644
> --- a/drivers/usb/core/port.c
> +++ b/drivers/usb/core/port.c
> @@ -53,9 +53,45 @@ static void usb_port_device_release(struct device *dev)
>  	kfree(port_dev);
>  }
>  
> +static int usb_port_runtime_resume(struct device *dev)
> +{
> +	struct usb_port *port_dev = to_usb_port(dev);
> +	struct usb_device *hdev =
> +		to_usb_device(dev->parent->parent);
> +
> +	return usb_hub_set_port_power(hdev, port_dev->portnum,
> +			true);
> +}
> +
> +static int usb_port_runtime_suspend(struct device *dev)
> +{
> +	struct usb_port *port_dev = to_usb_port(dev);
> +	struct usb_device *hdev =
> +		to_usb_device(dev->parent->parent);
> +
> +	return usb_hub_set_port_power(hdev, port_dev->portnum,
> +			false);
> +}
> +
> +static int usb_port_runtime_idle(struct device *dev)
> +{
> +	struct usb_port *port_dev = to_usb_port(dev);
> +
> +	return pm_runtime_suspend(&port_dev->dev);
> +}
> +
> +static const struct dev_pm_ops usb_port_pm_ops = {
> +#ifdef CONFIG_USB_SUSPEND
> +.runtime_suspend =	usb_port_runtime_suspend,
> +.runtime_resume =	usb_port_runtime_resume,
> +.runtime_idle =		usb_port_runtime_idle,
> +#endif
> +};
> +
>  struct device_type usb_port_device_type = {
>  	.name =		"usb_port",
>  	.release =	usb_port_device_release,
> +	.pm = &usb_port_pm_ops,
>  };
>  
>  int usb_hub_create_port_device(struct device *intfdev,
> @@ -63,6 +99,7 @@ int usb_hub_create_port_device(struct device *intfdev,
>  {
>  	int retval;
>  
> +	port_dev->portnum = port1;
>  	port_dev->dev.parent = intfdev;
>  	port_dev->dev.groups = port_dev_group;
>  	port_dev->dev.type = &usb_port_device_type;
> @@ -72,6 +109,8 @@ int usb_hub_create_port_device(struct device *intfdev,
>  	if (retval)
>  		goto error_register;
>  
> +	pm_runtime_set_active(&port_dev->dev);
> +	pm_runtime_enable(&port_dev->dev);
>  	usb_acpi_register_power_resources(&port_dev->dev);
>  
>  	return 0;
> diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
> index de7434b..47d77d4 100644
> --- a/drivers/usb/core/usb.h
> +++ b/drivers/usb/core/usb.h
> @@ -62,6 +62,8 @@ extern void usb_major_cleanup(void);
>  
>  extern int usb_hub_create_port_device(struct device *intfdev,
>  		int port1, struct usb_port *port_dev);
> +extern int usb_hub_set_port_power(struct usb_device *hdev,
> +		int port1, bool set);
>  
>  #ifdef	CONFIG_PM
>  
> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index c1f1346..71510bf 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -578,12 +578,14 @@ struct usb_device {
>   * @dev: generic device interface
>   * @port_owner: port's owner
>   * @connect_type: port's connect type
> + * @portnum: port index num based one
>   */
>  struct usb_port {
>  	struct usb_device *child;
>  	struct device dev;
>  	struct dev_state *port_owner;
>  	enum usb_port_connect_type connect_type;
> +	u8 portnum;
>  };
>  #define to_usb_port(_dev) \
>  	container_of(_dev, struct usb_port, dev)
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH v2 4/6] usb: add usb port auto power off mechanism

[Rafael J. Wysocki]

On Tuesday, November 13, 2012 04:00:03 PM Lan Tianyu wrote:
> This patch is to add usb port auto power off mechanism.
> When usb device is suspending, usb core will suspend usb port and
> usb port runtime pm callback will clear PORT_POWER feature to
> power off port if all conditions were met.These conditions are
> remote wakeup disable, pm qos NO_POWER_OFF flag clear and persist
> enable. When device is suspended and power off, usb core
> will ignore port's connect and enable change event to keep the device
> not being disconnected. When it resumes, power on port again.
> 
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>

Again, from the PM side:

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

although as far as the USB internals are concerned, I'm far from being an expert. :-)

Thanks,
Rafael


> ---
>  drivers/usb/core/hub.c  |   76 +++++++++++++++++++++++++++++++++++++++++++++--
>  drivers/usb/core/port.c |    1 +
>  include/linux/usb.h     |    2 ++
>  3 files changed, 76 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 2cb414e..267e9d7 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -26,6 +26,7 @@
>  #include <linux/mutex.h>
>  #include <linux/freezer.h>
>  #include <linux/random.h>
> +#include <linux/pm_qos.h>
>  
>  #include <asm/uaccess.h>
>  #include <asm/byteorder.h>
> @@ -159,11 +160,14 @@ MODULE_PARM_DESC(use_both_schemes,
>  DECLARE_RWSEM(ehci_cf_port_reset_rwsem);
>  EXPORT_SYMBOL_GPL(ehci_cf_port_reset_rwsem);
>  
> +#define HUB_PORT_RECONNECT_TRIES	20
> +
>  #define HUB_DEBOUNCE_TIMEOUT	1500
>  #define HUB_DEBOUNCE_STEP	  25
>  #define HUB_DEBOUNCE_STABLE	 100
>  
>  static int usb_reset_and_verify_device(struct usb_device *udev);
> +static int hub_port_debounce(struct usb_hub *hub, int port1);
>  
>  static inline char *portspeed(struct usb_hub *hub, int portstatus)
>  {
> @@ -855,7 +859,11 @@ static unsigned hub_power_on(struct usb_hub *hub, bool do_delay)
>  		dev_dbg(hub->intfdev, "trying to enable port power on "
>  				"non-switchable hub\n");
>  	for (port1 = 1; port1 <= hub->descriptor->bNbrPorts; port1++)
> -		set_port_feature(hub->hdev, port1, USB_PORT_FEAT_POWER);
> +		if (hub->ports[port1 - 1]->power_on)
> +			set_port_feature(hub->hdev, port1, USB_PORT_FEAT_POWER);
> +		else
> +			clear_port_feature(hub->hdev, port1,
> +						USB_PORT_FEAT_POWER);
>  
>  	/* Wait at least 100 msec for power to become stable */
>  	delay = max(pgood_delay, (unsigned) 100);
> @@ -2852,6 +2860,7 @@ EXPORT_SYMBOL_GPL(usb_enable_ltm);
>  int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
>  {
>  	struct usb_hub	*hub = hdev_to_hub(udev->parent);
> +	struct usb_port *port_dev = hub->ports[udev->portnum - 1];
>  	int		port1 = udev->portnum;
>  	int		status;
>  
> @@ -2946,6 +2955,19 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
>  		udev->port_is_suspended = 1;
>  		msleep(10);
>  	}
> +
> +	/*
> +	 * Check whether current status is meeting requirement of
> +	 * usb port power off mechanism
> +	 */
> +	if (!udev->do_remote_wakeup
> +			&& (dev_pm_qos_flags(&port_dev->dev,
> +			PM_QOS_FLAG_NO_POWER_OFF) != PM_QOS_FLAGS_ALL)
> +			&& udev->persist_enabled
> +			&& !status)
> +		if (!pm_runtime_put_sync(&port_dev->dev))
> +			port_dev->power_on = false;
> +
>  	usb_mark_last_busy(hub->hdev);
>  	return status;
>  }
> @@ -3029,6 +3051,25 @@ static int finish_port_resume(struct usb_device *udev)
>  	return status;
>  }
>  
> +static int usb_port_wait_for_connected(struct usb_hub *hub, int port1)
> +{
> +	int status, i;
> +
> +	for (i = 0; i < HUB_PORT_RECONNECT_TRIES; i++) {
> +		status = hub_port_debounce(hub, port1);
> +		if (status & USB_PORT_STAT_CONNECTION) {
> +			/*
> +			 * just clear enable-change feature since debounce
> +			 *  has cleared connect-change feature.
> +			 */
> +			clear_port_feature(hub->hdev, port1,
> +					USB_PORT_FEAT_C_ENABLE);
> +			return 0;
> +		}
> +	}
> +	return -ETIMEDOUT;
> +}
> +
>  /*
>   * usb_port_resume - re-activate a suspended usb device's upstream port
>   * @udev: device to re-activate, not a root hub
> @@ -3066,10 +3107,36 @@ static int finish_port_resume(struct usb_device *udev)
>  int usb_port_resume(struct usb_device *udev, pm_message_t msg)
>  {
>  	struct usb_hub	*hub = hdev_to_hub(udev->parent);
> +	struct usb_port *port_dev = hub->ports[udev->portnum  - 1];
>  	int		port1 = udev->portnum;
>  	int		status;
>  	u16		portchange, portstatus;
>  
> +
> +	set_bit(port1, hub->busy_bits);
> +
> +	if (!port_dev->power_on) {
> +		status = pm_runtime_get_sync(&port_dev->dev);
> +		if (status) {
> +			dev_dbg(&udev->dev, "can't resume usb port, status %d\n",
> +					status);
> +			return status;
> +		}
> +
> +		port_dev->power_on = true;
> +
> +		/*
> +		 * Wait for usb hub port to be reconnected in order to make
> +		 * the resume procedure successful.
> +		 */
> +		status = usb_port_wait_for_connected(hub, port1);
> +		if (status < 0) {
> +			dev_dbg(&udev->dev, "can't get reconnection after setting port  power on, status %d\n",
> +					status);
> +			return status;
> +		}
> +	}
> +
>  	/* Skip the initial Clear-Suspend step for a remote wakeup */
>  	status = hub_port_status(hub, port1, &portstatus, &portchange);
>  	if (status == 0 && !port_is_suspended(hub, portstatus))
> @@ -3077,8 +3144,6 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg)
>  
>  	// dev_dbg(hub->intfdev, "resume port %d\n", port1);
>  
> -	set_bit(port1, hub->busy_bits);
> -
>  	/* see 7.1.7.7; affects power usage, but not budgeting */
>  	if (hub_is_superspeed(hub->hdev))
>  		status = set_port_feature(hub->hdev,
> @@ -4256,6 +4321,11 @@ static void hub_port_connect_change(struct usb_hub *hub, int port1,
>  		}
>  	}
>  
> +	if (!hub->ports[port1 - 1]->power_on) {
> +		clear_bit(port1, hub->change_bits);
> +		return;
> +	}
> +
>  	/* Disconnect any existing devices under this port */
>  	if (udev)
>  		usb_disconnect(&hub->ports[port1 - 1]->child);
> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
> index 1cda766..b7388fd 100644
> --- a/drivers/usb/core/port.c
> +++ b/drivers/usb/core/port.c
> @@ -99,6 +99,7 @@ int usb_hub_create_port_device(struct device *intfdev,
>  {
>  	int retval;
>  
> +	port_dev->power_on = true;
>  	port_dev->portnum = port1;
>  	port_dev->dev.parent = intfdev;
>  	port_dev->dev.groups = port_dev_group;
> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index 71510bf..8002640 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -579,6 +579,7 @@ struct usb_device {
>   * @port_owner: port's owner
>   * @connect_type: port's connect type
>   * @portnum: port index num based one
> + * @power_on: port's power state
>   */
>  struct usb_port {
>  	struct usb_device *child;
> @@ -586,6 +587,7 @@ struct usb_port {
>  	struct dev_state *port_owner;
>  	enum usb_port_connect_type connect_type;
>  	u8 portnum;
> +	bool power_on;
>  };
>  #define to_usb_port(_dev) \
>  	container_of(_dev, struct usb_port, dev)
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH v2 5/6] usb: expose usb port's pm qos flags to user space

[Rafael J. Wysocki]

On Tuesday, November 13, 2012 04:00:04 PM Lan Tianyu wrote:
> This patch is to expose usb port's pm qos flags(pm_qos_no_power_off,
> pm_qos_remote_wakeup) to user space. User can set pm_qos_no_power_off
> flag to prohibit the port from being power off.
> 
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> ---
>  drivers/usb/core/port.c |   10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
> index b7388fd..5a7a833 100644
> --- a/drivers/usb/core/port.c
> +++ b/drivers/usb/core/port.c
> @@ -2,6 +2,7 @@
>  #include <linux/errno.h>
>  #include <linux/module.h>
>  #include <linux/usb.h>
> +#include <linux/pm_qos.h>
>  
>  #include "usb.h"
>  
> @@ -48,7 +49,7 @@ static const struct attribute_group *port_dev_group[] = {
>  static void usb_port_device_release(struct device *dev)
>  {
>  	struct usb_port *port_dev = to_usb_port(dev);
> -
> +	dev_pm_qos_hide_flags(dev);
>  	usb_acpi_unregister_power_resources(dev);
>  	kfree(port_dev);
>  }
> @@ -110,12 +111,19 @@ int usb_hub_create_port_device(struct device *intfdev,
>  	if (retval)
>  		goto error_register;
>  
> +	retval = dev_pm_qos_expose_flags(&port_dev->dev,
> +			PM_QOS_FLAG_NO_POWER_OFF);
> +	if (retval)
> +		goto error_expose_pm_qos;
> +
>  	pm_runtime_set_active(&port_dev->dev);
>  	pm_runtime_enable(&port_dev->dev);
>  	usb_acpi_register_power_resources(&port_dev->dev);
>  
>  	return 0;
>  
> +error_expose_pm_qos:
> +	device_del(&port_dev->dev);

That seems to be a bit drastic. :-)

Why don't you simply avoid enabling runtime PM in that case?

Rafael


>  error_register:
>  	put_device(&port_dev->dev);
>  	return retval;
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH v2 6/6] usb: add usb port's pm qos flags request to change NO_POWER_OFF flag

[Rafael J. Wysocki]

On Tuesday, November 13, 2012 04:00:05 PM Lan Tianyu wrote:
> Some usb devices can't be resumed correctly after power off. This
> patch is to add pm qos flags request to change NO_POWER_OFF and
> provide usb_device_allow_power_off() for device drivers to allow or
> prohibit usb core to power off the device.
> 
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>

This looks reasonable to me.  From the PM perspective:

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/usb/core/hub.c  |   36 ++++++++++++++++++++++++++++++++++++
>  drivers/usb/core/port.c |   12 ++++++++++++
>  include/linux/usb.h     |    3 +++
>  3 files changed, 51 insertions(+)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 267e9d7..7aaac720 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -189,6 +189,42 @@ static struct usb_hub *hdev_to_hub(struct usb_device *hdev)
>  	return usb_get_intfdata(hdev->actconfig->interface[0]);
>  }
>  
> +/**
> + * usb_device_allow_power_off - Allow or prohibit power off device.
> + * @udev: target usb device
> + * @set: choice of allow or prohibit
> + *
> + * Clearing or setting usb port's pm qos flag PM_QOS_FLAG_NO_POWER_OFF
> + * to allow or prohibit target usb device to be power off.
> + */
> +int usb_device_allow_power_off(struct usb_device *udev, bool set)
> +{
> +	struct usb_port *port_dev;
> +	struct dev_pm_qos_request *pm_qos;
> +	s32 value;
> +	int ret;
> +
> +	if (!udev->parent)
> +		return -EINVAL;
> +
> +	port_dev =
> +		hdev_to_hub(udev->parent)->ports[udev->portnum - 1];
> +	pm_qos = &port_dev->pm_qos;
> +	value = pm_qos->data.flr.flags;
> +
> +	if (set)
> +		value &= ~PM_QOS_FLAG_NO_POWER_OFF;
> +	else
> +		value |= PM_QOS_FLAG_NO_POWER_OFF;
> +
> +	pm_runtime_get_sync(&port_dev->dev);
> +	ret = dev_pm_qos_update_request(pm_qos, value);
> +	pm_runtime_put(&port_dev->dev);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(usb_device_allow_power_off);
> +
>  static int usb_device_supports_lpm(struct usb_device *udev)
>  {
>  	/* USB 2.1 (and greater) devices indicate LPM support through
> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
> index 5a7a833..077a494 100644
> --- a/drivers/usb/core/port.c
> +++ b/drivers/usb/core/port.c
> @@ -49,6 +49,11 @@ static const struct attribute_group *port_dev_group[] = {
>  static void usb_port_device_release(struct device *dev)
>  {
>  	struct usb_port *port_dev = to_usb_port(dev);
> +
> +	pm_runtime_get_sync(dev);
> +	dev_pm_qos_remove_request(&port_dev->pm_qos);
> +	pm_runtime_put(dev);
> +
>  	dev_pm_qos_hide_flags(dev);
>  	usb_acpi_unregister_power_resources(dev);
>  	kfree(port_dev);
> @@ -116,12 +121,19 @@ int usb_hub_create_port_device(struct device *intfdev,
>  	if (retval)
>  		goto error_expose_pm_qos;
>  
> +	retval = dev_pm_qos_add_request(&port_dev->dev, &port_dev->pm_qos,
> +			DEV_PM_QOS_FLAGS, 0);
> +	if (retval)
> +		goto error_add_qos_request;
> +
>  	pm_runtime_set_active(&port_dev->dev);
>  	pm_runtime_enable(&port_dev->dev);
>  	usb_acpi_register_power_resources(&port_dev->dev);
>  
>  	return 0;
>  
> +error_add_qos_request:
> +	pm_runtime_put(&port_dev->dev);
>  error_expose_pm_qos:
>  	device_del(&port_dev->dev);
>  error_register:
> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index 8002640..aa201bd 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -21,6 +21,7 @@
>  #include <linux/sched.h>	/* for current && schedule_timeout */
>  #include <linux/mutex.h>	/* for struct mutex */
>  #include <linux/pm_runtime.h>	/* for runtime PM */
> +#include <linux/pm_qos.h>	/* for PM Qos */
>  
>  struct usb_device;
>  struct usb_driver;
> @@ -577,6 +578,7 @@ struct usb_device {
>   * @child: usb device attatched to the port
>   * @dev: generic device interface
>   * @port_owner: port's owner
> + * @pm_qos: port's pm qos flags request
>   * @connect_type: port's connect type
>   * @portnum: port index num based one
>   * @power_on: port's power state
> @@ -585,6 +587,7 @@ struct usb_port {
>  	struct usb_device *child;
>  	struct device dev;
>  	struct dev_state *port_owner;
> +	struct dev_pm_qos_request pm_qos;
>  	enum usb_port_connect_type connect_type;
>  	u8 portnum;
>  	bool power_on;
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH v2 1/6] usb: Register usb port's acpi power resources

[Lan Tianyu]

On 2012年11月14日 07:56, Rafael J. Wysocki wrote:
> On Tuesday, November 13, 2012 08:36:15 PM Lan Tianyu wrote:
>> 于 2012/11/13 19:07, Sergei Shtylyov 写道:
>>> Hello.
>>>
>>> On 13-11-2012 12:00, Lan Tianyu wrote:
>>>
>>>> This patch is to register usb port's acpi power resources. Create
>>>> link between usb port device and its acpi power resource.
>>>
>>>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>>> [...]
>>>
>>>> diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
>>>> index cef4252..c58ebc0 100644
>>>> --- a/drivers/usb/core/usb-acpi.c
>>>> +++ b/drivers/usb/core/usb-acpi.c
>>>> @@ -216,6 +216,28 @@ static struct acpi_bus_type usb_acpi_bus = {
>>>>       .find_device = usb_acpi_find_device,
>>>>   };
>>>>
>>>> +int usb_acpi_register_power_resources(struct device *dev)
>>>> +{
>>>> +    acpi_handle port_handle = DEVICE_ACPI_HANDLE(dev);
>>>> +
>>>> +    if (!port_handle)
>>>> +        return -ENODEV;
>>>> +
>>>> +    if (acpi_power_resource_register_device(dev, port_handle) < 0)
>>>> +        return -ENODEV;
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +void usb_acpi_unregister_power_resources(struct device *dev)
>>>> +{
>>>> +    acpi_handle port_handle = DEVICE_ACPI_HANDLE(dev);
>>>> +
>>>> +    if (!port_handle)
>>>> +        return;
>>>> +
>>>> +    acpi_power_resource_register_device(dev, port_handle);
>>>
>>>     I thinbk you have been askied already, but shouldn't it be
>>> acpi_power_resource_unregister_device()?
>>>
>> Oh. Sorry. Too focus on the other modification. Thanks for your reminder.
> 
> Besides, it would be a bit more natural to do
> 
> if (port_handle)
> 	acpi_power_resource_unregister_device(dev, port_handle);
> 
> instead of doing that return when port_handle is NULL.
> 
Hi Rafael:
	Great thanks for your review. :)
	I get it and will update it at next version.
> Thanks,
> Rafael
> 
> 


-- 
Best regards
Tianyu Lan

Re: [RFC PATCH v2 2/6] usb: Add driver/usb/core/port.c file to fill usb port related code.

[Lan Tianyu]

On 2012年11月14日 08:04, Rafael J. Wysocki wrote:
> On Tuesday, November 13, 2012 04:00:01 PM Lan Tianyu wrote:
>> This patch is to create driver/usb/core/port.c and move usb port related
>> code into it.
> 
> It does seem to make functional changes in addition to that, however.
> 
No functional change. But change the usb_hub_create_port_device() param.
which original used struct usb_hub as param. Because struct usb_hub is
private struct in the hub.c and now move usb_hub_create_port_device() to
port.c. Will add changelog about this next version.
> If I'm not mistaken and that really is the case, can you (briefly)
> describe those changes here too?
> 
> Rafael
> 
> 
>> Signed-off-by: Lan Tianyu <tianyu.lan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> ---
>>  drivers/usb/core/Makefile |    1 +
>>  drivers/usb/core/hub.c    |  113 +++++++--------------------------------------
>>  drivers/usb/core/port.c   |   82 ++++++++++++++++++++++++++++++++
>>  drivers/usb/core/usb.h    |    3 ++
>>  include/linux/usb.h       |   16 +++++++
>>  5 files changed, 118 insertions(+), 97 deletions(-)
>>  create mode 100644 drivers/usb/core/port.c
>>
>> diff --git a/drivers/usb/core/Makefile b/drivers/usb/core/Makefile
>> index 26059b9..5e847ad 100644
>> --- a/drivers/usb/core/Makefile
>> +++ b/drivers/usb/core/Makefile
>> @@ -7,6 +7,7 @@ ccflags-$(CONFIG_USB_DEBUG) := -DDEBUG
>>  usbcore-y := usb.o hub.o hcd.o urb.o message.o driver.o
>>  usbcore-y += config.o file.o buffer.o sysfs.o endpoint.o
>>  usbcore-y += devio.o notify.o generic.o quirks.o devices.o
>> +usbcore-y += port.o
>>  
>>  usbcore-$(CONFIG_PCI)		+= hcd-pci.o
>>  usbcore-$(CONFIG_ACPI)		+= usb-acpi.o
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index 3c85fe1c..60746aa 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -42,13 +42,6 @@
>>  #define USB_VENDOR_GENESYS_LOGIC		0x05e3
>>  #define HUB_QUIRK_CHECK_PORT_AUTOSUSPEND	0x01
>>  
>> -struct usb_port {
>> -	struct usb_device *child;
>> -	struct device dev;
>> -	struct dev_state *port_owner;
>> -	enum usb_port_connect_type connect_type;
>> -};
>> -
>>  struct usb_hub {
>>  	struct device		*intfdev;	/* the "interface" device */
>>  	struct usb_device	*hdev;
>> @@ -170,9 +163,6 @@ EXPORT_SYMBOL_GPL(ehci_cf_port_reset_rwsem);
>>  #define HUB_DEBOUNCE_STEP	  25
>>  #define HUB_DEBOUNCE_STABLE	 100
>>  
>> -#define to_usb_port(_dev) \
>> -	container_of(_dev, struct usb_port, dev)
>> -
>>  static int usb_reset_and_verify_device(struct usb_device *udev);
>>  
>>  static inline char *portspeed(struct usb_hub *hub, int portstatus)
>> @@ -1237,57 +1227,12 @@ static int hub_post_reset(struct usb_interface *intf)
>>  	return 0;
>>  }
>>  
>> -static void usb_port_device_release(struct device *dev)
>> -{
>> -	struct usb_port *port_dev = to_usb_port(dev);
>> -
>> -	usb_acpi_unregister_power_resources(dev);
>> -	kfree(port_dev);
>> -}
>> -
>>  static void usb_hub_remove_port_device(struct usb_hub *hub,
>>  				       int port1)
>>  {
>>  	device_unregister(&hub->ports[port1 - 1]->dev);
>>  }
>>  
>> -struct device_type usb_port_device_type = {
>> -	.name =		"usb_port",
>> -	.release =	usb_port_device_release,
>> -};
>> -
>> -static int usb_hub_create_port_device(struct usb_hub *hub,
>> -				      int port1)
>> -{
>> -	struct usb_port *port_dev = NULL;
>> -	int retval;
>> -
>> -	port_dev = kzalloc(sizeof(*port_dev), GFP_KERNEL);
>> -	if (!port_dev) {
>> -		retval = -ENOMEM;
>> -		goto exit;
>> -	}
>> -
>> -	hub->ports[port1 - 1] = port_dev;
>> -	port_dev->dev.parent = hub->intfdev;
>> -	port_dev->dev.groups = port_dev_group;
>> -	port_dev->dev.type = &usb_port_device_type;
>> -	dev_set_name(&port_dev->dev, "port%d", port1);
>> -
>> -	retval = device_register(&port_dev->dev);
>> -	if (retval)
>> -		goto error_register;
>> -
>> -	usb_acpi_register_power_resources(&port_dev->dev);
>> -
>> -	return 0;
>> -
>> -error_register:
>> -	put_device(&port_dev->dev);
>> -exit:
>> -	return retval;
>> -}
>> -
>>  static int hub_configure(struct usb_hub *hub,
>>  	struct usb_endpoint_descriptor *endpoint)
>>  {
>> @@ -1548,10 +1493,24 @@ static int hub_configure(struct usb_hub *hub,
>>  	if (hub->has_indicators && blinkenlights)
>>  		hub->indicator [0] = INDICATOR_CYCLE;
>>  
>> -	for (i = 0; i < hdev->maxchild; i++)
>> -		if (usb_hub_create_port_device(hub, i + 1) < 0)
>> +	for (i = 0; i < hdev->maxchild; i++) {
>> +		struct usb_port *port_dev = NULL;
>> +
>> +		port_dev = kzalloc(sizeof(*port_dev), GFP_KERNEL);
>> +		if (!port_dev) {
>> +			dev_err(hub->intfdev,
>> +				"couldn't create port%d device due to lack mem.\n", i + 1);
>> +			continue;
>> +		}
>> +
>> +		hub->ports[i] = port_dev;
>> +
>> +		if (usb_hub_create_port_device(hub->intfdev, i + 1, port_dev) < 0) {
>>  			dev_err(hub->intfdev,
>>  				"couldn't create port%d device.\n", i + 1);
>> +			hub->ports[i] = NULL;
>> +		}
>> +	}
>>  
>>  	if (!hub_is_superspeed(hdev)) {
>>  		for (i = 1; i <= hdev->maxchild; i++)
>> @@ -4765,46 +4724,6 @@ static int hub_thread(void *__unused)
>>  	return 0;
>>  }
>>  
>> -static ssize_t show_port_connect_type(struct device *dev,
>> -	struct device_attribute *attr, char *buf)
>> -{
>> -	struct usb_port *port_dev = to_usb_port(dev);
>> -	char *result;
>> -
>> -	switch (port_dev->connect_type) {
>> -	case USB_PORT_CONNECT_TYPE_HOT_PLUG:
>> -		result = "hotplug";
>> -		break;
>> -	case USB_PORT_CONNECT_TYPE_HARD_WIRED:
>> -		result = "hardwired";
>> -		break;
>> -	case USB_PORT_NOT_USED:
>> -		result = "not used";
>> -		break;
>> -	default:
>> -		result = "unknown";
>> -		break;
>> -	}
>> -
>> -	return sprintf(buf, "%s\n", result);
>> -}
>> -static DEVICE_ATTR(connect_type, S_IRUGO, show_port_connect_type,
>> -		NULL);
>> -
>> -static struct attribute *port_dev_attrs[] = {
>> -	&dev_attr_connect_type.attr,
>> -	NULL,
>> -};
>> -
>> -static struct attribute_group port_dev_attr_grp = {
>> -	.attrs = port_dev_attrs,
>> -};
>> -
>> -static const struct attribute_group *port_dev_group[] = {
>> -	&port_dev_attr_grp,
>> -	NULL,
>> -};
>> -
>>  static const struct usb_device_id hub_id_table[] = {
>>      { .match_flags = USB_DEVICE_ID_MATCH_VENDOR
>>  	           | USB_DEVICE_ID_MATCH_INT_CLASS,
>> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
>> new file mode 100644
>> index 0000000..6523a03
>> --- /dev/null
>> +++ b/drivers/usb/core/port.c
>> @@ -0,0 +1,82 @@
>> +#include <linux/kernel.h>
>> +#include <linux/errno.h>
>> +#include <linux/module.h>
>> +#include <linux/usb.h>
>> +
>> +#include "usb.h"
>> +
>> +static ssize_t show_port_connect_type(struct device *dev,
>> +	struct device_attribute *attr, char *buf)
>> +{
>> +	struct usb_port *port_dev = to_usb_port(dev);
>> +	char *result;
>> +
>> +	switch (port_dev->connect_type) {
>> +	case USB_PORT_CONNECT_TYPE_HOT_PLUG:
>> +		result = "hotplug";
>> +		break;
>> +	case USB_PORT_CONNECT_TYPE_HARD_WIRED:
>> +		result = "hardwired";
>> +		break;
>> +	case USB_PORT_NOT_USED:
>> +		result = "not used";
>> +		break;
>> +	default:
>> +		result = "unknown";
>> +		break;
>> +	}
>> +
>> +	return sprintf(buf, "%s\n", result);
>> +}
>> +static DEVICE_ATTR(connect_type, S_IRUGO, show_port_connect_type,
>> +		NULL);
>> +
>> +static struct attribute *port_dev_attrs[] = {
>> +	&dev_attr_connect_type.attr,
>> +	NULL,
>> +};
>> +
>> +static struct attribute_group port_dev_attr_grp = {
>> +	.attrs = port_dev_attrs,
>> +};
>> +
>> +static const struct attribute_group *port_dev_group[] = {
>> +	&port_dev_attr_grp,
>> +	NULL,
>> +};
>> +
>> +static void usb_port_device_release(struct device *dev)
>> +{
>> +	struct usb_port *port_dev = to_usb_port(dev);
>> +
>> +	usb_acpi_unregister_power_resources(dev);
>> +	kfree(port_dev);
>> +}
>> +
>> +struct device_type usb_port_device_type = {
>> +	.name =		"usb_port",
>> +	.release =	usb_port_device_release,
>> +};
>> +
>> +int usb_hub_create_port_device(struct device *intfdev,
>> +		int port1, struct usb_port *port_dev)
>> +{
>> +	int retval;
>> +
>> +	port_dev->dev.parent = intfdev;
>> +	port_dev->dev.groups = port_dev_group;
>> +	port_dev->dev.type = &usb_port_device_type;
>> +	dev_set_name(&port_dev->dev, "port%d", port1);
>> +
>> +	retval = device_register(&port_dev->dev);
>> +	if (retval)
>> +		goto error_register;
>> +
>> +	usb_acpi_register_power_resources(&port_dev->dev);
>> +
>> +	return 0;
>> +
>> +error_register:
>> +	put_device(&port_dev->dev);
>> +	return retval;
>> +}
>> diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
>> index 38958fb..de7434b 100644
>> --- a/drivers/usb/core/usb.h
>> +++ b/drivers/usb/core/usb.h
>> @@ -60,6 +60,9 @@ extern void usb_hub_cleanup(void);
>>  extern int usb_major_init(void);
>>  extern void usb_major_cleanup(void);
>>  
>> +extern int usb_hub_create_port_device(struct device *intfdev,
>> +		int port1, struct usb_port *port_dev);
>> +
>>  #ifdef	CONFIG_PM
>>  
>>  extern int usb_suspend(struct device *dev, pm_message_t msg);
>> diff --git a/include/linux/usb.h b/include/linux/usb.h
>> index e996bb6..c1f1346 100644
>> --- a/include/linux/usb.h
>> +++ b/include/linux/usb.h
>> @@ -572,6 +572,22 @@ struct usb_device {
>>  };
>>  #define	to_usb_device(d) container_of(d, struct usb_device, dev)
>>  
>> +/**
>> + * struct usb port - kernel's representation of a usb port
>> + * @child: usb device attatched to the port
>> + * @dev: generic device interface
>> + * @port_owner: port's owner
>> + * @connect_type: port's connect type
>> + */
>> +struct usb_port {
>> +	struct usb_device *child;
>> +	struct device dev;
>> +	struct dev_state *port_owner;
>> +	enum usb_port_connect_type connect_type;
>> +};
>> +#define to_usb_port(_dev) \
>> +	container_of(_dev, struct usb_port, dev)
>> +
>>  static inline struct usb_device *interface_to_usbdev(struct usb_interface *intf)
>>  {
>>  	return to_usb_device(intf->dev.parent);
>>


-- 
Best regards
Tianyu Lan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 3/6] usb: add runtime pm support for usb port device

[Lan Tianyu]

On 2012年11月14日 08:08, Rafael J. Wysocki wrote:
> On Tuesday, November 13, 2012 04:00:02 PM Lan Tianyu wrote:
>> This patch is to add runtime pm callback for usb port device.
>> Set/clear PORT_POWER feature in the resume/suspend callbak.
>> Add portnum for struct usb_port to record port number.
>>
>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> 
> This one looks reasonable to me.  From the PM side
> 
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Hi Rafael and Alan:
	This patch has a collaboration problem with pm qos. Since pm core would
pm_runtime_get_sync/put(port_dev) if pm qos flags was changed and port's
suspend call_back() clear PORT_POWER feature without any check. This
will cause PORT_POWER feather being cleared every time after pm qos
flags being changed.

	I have an idea that add check in the port's runtime idle callback.
Check NO_POWER_OFF flag firstly. If set return. Second, for port without
device, suspend port directly and for port with device, increase child
device's runtime usage without resume and do barrier to ensure all
suspend process finish, at last check the child runtime status. If it
was suspended, suspend port and if do nothing.

static int usb_port_runtime_idle(struct device *dev)
{
	struct usb_port *port_dev = to_usb_port(dev);
	int retval = 0;

	if (dev_pm_qos_flags(&port_dev->dev, PM_QOS_FLAG_NO_POWER_OFF)
			== PM_QOS_FLAGS_ALL)
		return 0;

	if (!port_dev->child) {
		retval = pm_runtime_suspend(&port_dev->dev);
		if (!retval)
			port_dev->power_on =false;
	}
	else {
		pm_runtime_get_noresume(&port_dev->child->dev);
		pm_runtime_barrier(&port_dev->child->dev);
		if (port_dev->child->dev.power.runtime_status
				== RPM_SUSPENDED) {
			retval = pm_runtime_suspend(&port_dev->dev);
			if (!retval)
				port_dev->power_on =false;
		}	
		pm_runtime_put_noidle(&port_dev->child->dev);
	}
	
	return retval;
}

I'd like to see your opinion :) thanks.

> 
>> ---
>>  drivers/usb/core/hub.c  |   14 ++++++++++++++
>>  drivers/usb/core/port.c |   39 +++++++++++++++++++++++++++++++++++++++
>>  drivers/usb/core/usb.h  |    2 ++
>>  include/linux/usb.h     |    2 ++
>>  4 files changed, 57 insertions(+)
>>
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index 60746aa..2cb414e 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -765,6 +765,20 @@ static void hub_tt_work(struct work_struct *work)
>>  	spin_unlock_irqrestore (&hub->tt.lock, flags);
>>  }
>>  
>> +int usb_hub_set_port_power(struct usb_device *hdev, int port1,
>> +		bool set)
>> +{
>> +	int ret;
>> +
>> +	if (set)
>> +		ret = set_port_feature(hdev, port1,
>> +				USB_PORT_FEAT_POWER);
>> +	else
>> +		ret = clear_port_feature(hdev, port1,
>> +				USB_PORT_FEAT_POWER);
>> +	return ret;
>> +}
>> +
>>  /**
>>   * usb_hub_clear_tt_buffer - clear control/bulk TT state in high speed hub
>>   * @urb: an URB associated with the failed or incomplete split transaction
>> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
>> index 6523a03..1cda766 100644
>> --- a/drivers/usb/core/port.c
>> +++ b/drivers/usb/core/port.c
>> @@ -53,9 +53,45 @@ static void usb_port_device_release(struct device *dev)
>>  	kfree(port_dev);
>>  }
>>  
>> +static int usb_port_runtime_resume(struct device *dev)
>> +{
>> +	struct usb_port *port_dev = to_usb_port(dev);
>> +	struct usb_device *hdev =
>> +		to_usb_device(dev->parent->parent);
>> +
>> +	return usb_hub_set_port_power(hdev, port_dev->portnum,
>> +			true);
>> +}
>> +
>> +static int usb_port_runtime_suspend(struct device *dev)
>> +{
>> +	struct usb_port *port_dev = to_usb_port(dev);
>> +	struct usb_device *hdev =
>> +		to_usb_device(dev->parent->parent);
>> +
>> +	return usb_hub_set_port_power(hdev, port_dev->portnum,
>> +			false);
>> +}
>> +
>> +static int usb_port_runtime_idle(struct device *dev)
>> +{
>> +	struct usb_port *port_dev = to_usb_port(dev);
>> +
>> +	return pm_runtime_suspend(&port_dev->dev);
>> +}
>> +
>> +static const struct dev_pm_ops usb_port_pm_ops = {
>> +#ifdef CONFIG_USB_SUSPEND
>> +.runtime_suspend =	usb_port_runtime_suspend,
>> +.runtime_resume =	usb_port_runtime_resume,
>> +.runtime_idle =		usb_port_runtime_idle,
>> +#endif
>> +};
>> +
>>  struct device_type usb_port_device_type = {
>>  	.name =		"usb_port",
>>  	.release =	usb_port_device_release,
>> +	.pm = &usb_port_pm_ops,
>>  };
>>  
>>  int usb_hub_create_port_device(struct device *intfdev,
>> @@ -63,6 +99,7 @@ int usb_hub_create_port_device(struct device *intfdev,
>>  {
>>  	int retval;
>>  
>> +	port_dev->portnum = port1;
>>  	port_dev->dev.parent = intfdev;
>>  	port_dev->dev.groups = port_dev_group;
>>  	port_dev->dev.type = &usb_port_device_type;
>> @@ -72,6 +109,8 @@ int usb_hub_create_port_device(struct device *intfdev,
>>  	if (retval)
>>  		goto error_register;
>>  
>> +	pm_runtime_set_active(&port_dev->dev);
>> +	pm_runtime_enable(&port_dev->dev);
>>  	usb_acpi_register_power_resources(&port_dev->dev);
>>  
>>  	return 0;
>> diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
>> index de7434b..47d77d4 100644
>> --- a/drivers/usb/core/usb.h
>> +++ b/drivers/usb/core/usb.h
>> @@ -62,6 +62,8 @@ extern void usb_major_cleanup(void);
>>  
>>  extern int usb_hub_create_port_device(struct device *intfdev,
>>  		int port1, struct usb_port *port_dev);
>> +extern int usb_hub_set_port_power(struct usb_device *hdev,
>> +		int port1, bool set);
>>  
>>  #ifdef	CONFIG_PM
>>  
>> diff --git a/include/linux/usb.h b/include/linux/usb.h
>> index c1f1346..71510bf 100644
>> --- a/include/linux/usb.h
>> +++ b/include/linux/usb.h
>> @@ -578,12 +578,14 @@ struct usb_device {
>>   * @dev: generic device interface
>>   * @port_owner: port's owner
>>   * @connect_type: port's connect type
>> + * @portnum: port index num based one
>>   */
>>  struct usb_port {
>>  	struct usb_device *child;
>>  	struct device dev;
>>  	struct dev_state *port_owner;
>>  	enum usb_port_connect_type connect_type;
>> +	u8 portnum;
>>  };
>>  #define to_usb_port(_dev) \
>>  	container_of(_dev, struct usb_port, dev)
>>


-- 
Best regards
Tianyu Lan

Re: [RFC PATCH v2 3/6] usb: add runtime pm support for usb port device

[Rafael J. Wysocki]

On Wednesday, November 14, 2012 02:34:37 PM Lan Tianyu wrote:
> On 2012年11月14日 08:08, Rafael J. Wysocki wrote:
> > On Tuesday, November 13, 2012 04:00:02 PM Lan Tianyu wrote:
> >> This patch is to add runtime pm callback for usb port device.
> >> Set/clear PORT_POWER feature in the resume/suspend callbak.
> >> Add portnum for struct usb_port to record port number.
> >>
> >> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> > 
> > This one looks reasonable to me.  From the PM side
> > 
> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Hi Rafael and Alan:
> 	This patch has a collaboration problem with pm qos. Since pm core would
> pm_runtime_get_sync/put(port_dev) if pm qos flags was changed and port's
> suspend call_back() clear PORT_POWER feature without any check. This
> will cause PORT_POWER feather being cleared every time after pm qos
> flags being changed.
> 
> 	I have an idea that add check in the port's runtime idle callback.
> Check NO_POWER_OFF flag firstly. If set return. Second, for port without
> device, suspend port directly and for port with device, increase child
> device's runtime usage without resume and do barrier to ensure all
> suspend process finish, at last check the child runtime status. If it
> was suspended, suspend port and if do nothing.
> 
> static int usb_port_runtime_idle(struct device *dev)
> {
> 	struct usb_port *port_dev = to_usb_port(dev);
> 	int retval = 0;
> 
> 	if (dev_pm_qos_flags(&port_dev->dev, PM_QOS_FLAG_NO_POWER_OFF)
> 			== PM_QOS_FLAGS_ALL)
> 		return 0;
> 
> 	if (!port_dev->child) {
> 		retval = pm_runtime_suspend(&port_dev->dev);
> 		if (!retval)
> 			port_dev->power_on =false;
> 	}
> 	else {

This usually is written as "} else {" in the kernel code.

> 		pm_runtime_get_noresume(&port_dev->child->dev);
> 		pm_runtime_barrier(&port_dev->child->dev);
> 		if (port_dev->child->dev.power.runtime_status
> 				== RPM_SUSPENDED) {
> 			retval = pm_runtime_suspend(&port_dev->dev);
> 			if (!retval)
> 				port_dev->power_on =false;
> 		}	
> 		pm_runtime_put_noidle(&port_dev->child->dev);
> 	}

Hmm.  If child->dev is not suspended, then our usage_count should be
at least 1, so pm_runtime_suspend(&port_dev->dev) shouldn't actually
suspend us.  Isn't that the case?

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH v2 3/6] usb: add runtime pm support for usb port device

[Lan Tianyu]

于 2012/11/14 17:49, Rafael J. Wysocki 写道:
> On Wednesday, November 14, 2012 02:34:37 PM Lan Tianyu wrote:
>> On 2012年11月14日 08:08, Rafael J. Wysocki wrote:
>>> On Tuesday, November 13, 2012 04:00:02 PM Lan Tianyu wrote:
>>>> This patch is to add runtime pm callback for usb port device.
>>>> Set/clear PORT_POWER feature in the resume/suspend callbak.
>>>> Add portnum for struct usb_port to record port number.
>>>>
>>>> Signed-off-by: Lan Tianyu <tianyu.lan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>>
>>> This one looks reasonable to me.  From the PM side
>>>
>>> Acked-by: Rafael J. Wysocki <rafael.j.wysocki-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> Hi Rafael and Alan:
>> 	This patch has a collaboration problem with pm qos. Since pm core would
>> pm_runtime_get_sync/put(port_dev) if pm qos flags was changed and port's
>> suspend call_back() clear PORT_POWER feature without any check. This
>> will cause PORT_POWER feather being cleared every time after pm qos
>> flags being changed.
>>
>> 	I have an idea that add check in the port's runtime idle callback.
>> Check NO_POWER_OFF flag firstly. If set return. Second, for port without
>> device, suspend port directly and for port with device, increase child
>> device's runtime usage without resume and do barrier to ensure all
>> suspend process finish, at last check the child runtime status. If it
>> was suspended, suspend port and if do nothing.
>>
>> static int usb_port_runtime_idle(struct device *dev)
>> {
>> 	struct usb_port *port_dev = to_usb_port(dev);
>> 	int retval = 0;
>>
>> 	if (dev_pm_qos_flags(&port_dev->dev, PM_QOS_FLAG_NO_POWER_OFF)
>> 			== PM_QOS_FLAGS_ALL)
>> 		return 0;
>>
>> 	if (!port_dev->child) {
>> 		retval = pm_runtime_suspend(&port_dev->dev);
>> 		if (!retval)
>> 			port_dev->power_on =false;
>> 	}
>> 	else {
>
> This usually is written as "} else {" in the kernel code.
>
>> 		pm_runtime_get_noresume(&port_dev->child->dev);
>> 		pm_runtime_barrier(&port_dev->child->dev);
>> 		if (port_dev->child->dev.power.runtime_status
>> 				== RPM_SUSPENDED) {
>> 			retval = pm_runtime_suspend(&port_dev->dev);
>> 			if (!retval)
>> 				port_dev->power_on =false;
>> 		}	
>> 		pm_runtime_put_noidle(&port_dev->child->dev);
>> 	}
>
> Hmm.  If child->dev is not suspended, then our usage_count should be
> at least 1, so pm_runtime_suspend(&port_dev->dev) shouldn't actually
> suspend us.  Isn't that the case?
No, because the child device is not under port device and so even if
child->dev is not suspended, port device's usage still can be 0 and
power off the port.
>
> Rafael
>
>

-- 
Best regards
Tianyu Lan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 3/6] usb: add runtime pm support for usb port device

[Lan Tianyu]

于 2012/11/14 20:45, Lan Tianyu 写道:
> 于 2012/11/14 17:49, Rafael J. Wysocki 写道:
>> On Wednesday, November 14, 2012 02:34:37 PM Lan Tianyu wrote:
>>> On 2012年11月14日 08:08, Rafael J. Wysocki wrote:
>>>> On Tuesday, November 13, 2012 04:00:02 PM Lan Tianyu wrote:
>>>>> This patch is to add runtime pm callback for usb port device.
>>>>> Set/clear PORT_POWER feature in the resume/suspend callbak.
>>>>> Add portnum for struct usb_port to record port number.
>>>>>
>>>>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>>>>
>>>> This one looks reasonable to me.  From the PM side
>>>>
>>>> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>> Hi Rafael and Alan:
>>>     This patch has a collaboration problem with pm qos. Since pm core would
>>> pm_runtime_get_sync/put(port_dev) if pm qos flags was changed and port's
>>> suspend call_back() clear PORT_POWER feature without any check. This
>>> will cause PORT_POWER feather being cleared every time after pm qos
>>> flags being changed.
>>>
>>>     I have an idea that add check in the port's runtime idle callback.
>>> Check NO_POWER_OFF flag firstly. If set return. Second, for port without
>>> device, suspend port directly and for port with device, increase child
>>> device's runtime usage without resume and do barrier to ensure all
>>> suspend process finish, at last check the child runtime status. If it
>>> was suspended, suspend port and if do nothing.
>>>
>>> static int usb_port_runtime_idle(struct device *dev)
>>> {
>>>     struct usb_port *port_dev = to_usb_port(dev);
>>>     int retval = 0;
>>>
>>>     if (dev_pm_qos_flags(&port_dev->dev, PM_QOS_FLAG_NO_POWER_OFF)
>>>             == PM_QOS_FLAGS_ALL)
>>>         return 0;
>>>
>>>     if (!port_dev->child) {
>>>         retval = pm_runtime_suspend(&port_dev->dev);
>>>         if (!retval)
>>>             port_dev->power_on =false;
>>>     }
>>>     else {
>>
>> This usually is written as "} else {" in the kernel code.
>>
>>>         pm_runtime_get_noresume(&port_dev->child->dev);
>>>         pm_runtime_barrier(&port_dev->child->dev);
>>>         if (port_dev->child->dev.power.runtime_status
>>>                 == RPM_SUSPENDED) {
>>>             retval = pm_runtime_suspend(&port_dev->dev);
>>>             if (!retval)
>>>                 port_dev->power_on =false;
>>>         }
>>>         pm_runtime_put_noidle(&port_dev->child->dev);
>>>     }
>>
>> Hmm.  If child->dev is not suspended, then our usage_count should be
>> at least 1, so pm_runtime_suspend(&port_dev->dev) shouldn't actually
>> suspend us.  Isn't that the case?
> No, because the child device is not under port device and so even if
> child->dev is not suspended, port device's usage still can be 0 and
> power off the port.

Maybe I should add pm_runtime_get_noresume(&port_dev->dev) before enable
port's runtime pm. Just like following. Then it will work like you said.

@@ -72,6 +109,8 @@ int usb_hub_create_port_device(struct device *intfdev,
  	if (retval)
  		goto error_register;

+	pm_runtime_set_active(&port_dev->dev);
+	pm_runtime_get_noresume(&port_dev->dev); /* new add */
+	pm_runtime_enable(&port_dev->dev);
>>
>> Rafael
>>
>>
>

-- 
Best regards
Tianyu Lan

Re: [RFC PATCH v2 3/6] usb: add runtime pm support for usb port device

[Lan Tianyu]

于 2012/11/14 20:45, Lan Tianyu 写道:
> 于 2012/11/14 17:49, Rafael J. Wysocki 写道:
>> On Wednesday, November 14, 2012 02:34:37 PM Lan Tianyu wrote:
>>> On 2012年11月14日 08:08, Rafael J. Wysocki wrote:
>>>> On Tuesday, November 13, 2012 04:00:02 PM Lan Tianyu wrote:
>>>>> This patch is to add runtime pm callback for usb port device.
>>>>> Set/clear PORT_POWER feature in the resume/suspend callbak.
>>>>> Add portnum for struct usb_port to record port number.
>>>>>
>>>>> Signed-off-by: Lan Tianyu <tianyu.lan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>>>
>>>> This one looks reasonable to me.  From the PM side
>>>>
>>>> Acked-by: Rafael J. Wysocki <rafael.j.wysocki-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>> Hi Rafael and Alan:
>>>     This patch has a collaboration problem with pm qos. Since pm core would
>>> pm_runtime_get_sync/put(port_dev) if pm qos flags was changed and port's
>>> suspend call_back() clear PORT_POWER feature without any check. This
>>> will cause PORT_POWER feather being cleared every time after pm qos
>>> flags being changed.
>>>
>>>     I have an idea that add check in the port's runtime idle callback.
>>> Check NO_POWER_OFF flag firstly. If set return. Second, for port without
>>> device, suspend port directly and for port with device, increase child
>>> device's runtime usage without resume and do barrier to ensure all
>>> suspend process finish, at last check the child runtime status. If it
>>> was suspended, suspend port and if do nothing.
>>>
>>> static int usb_port_runtime_idle(struct device *dev)
>>> {
>>>     struct usb_port *port_dev = to_usb_port(dev);
>>>     int retval = 0;
>>>
>>>     if (dev_pm_qos_flags(&port_dev->dev, PM_QOS_FLAG_NO_POWER_OFF)
>>>             == PM_QOS_FLAGS_ALL)
>>>         return 0;
>>>
>>>     if (!port_dev->child) {
>>>         retval = pm_runtime_suspend(&port_dev->dev);
>>>         if (!retval)
>>>             port_dev->power_on =false;
>>>     }
>>>     else {
>>
>> This usually is written as "} else {" in the kernel code.
>>
>>>         pm_runtime_get_noresume(&port_dev->child->dev);
>>>         pm_runtime_barrier(&port_dev->child->dev);
>>>         if (port_dev->child->dev.power.runtime_status
>>>                 == RPM_SUSPENDED) {
>>>             retval = pm_runtime_suspend(&port_dev->dev);
>>>             if (!retval)
>>>                 port_dev->power_on =false;
>>>         }
>>>         pm_runtime_put_noidle(&port_dev->child->dev);
>>>     }
>>
>> Hmm.  If child->dev is not suspended, then our usage_count should be
>> at least 1, so pm_runtime_suspend(&port_dev->dev) shouldn't actually
>> suspend us.  Isn't that the case?
> No, because the child device is not under port device and so even if
> child->dev is not suspended, port device's usage still can be 0 and
> power off the port.
>>
Please ignore this reply. I may not understand your reply correctly.
You are right if the child->dev is not suspended, the usage count should be at
last 1. But how about if the child->dev is suspended.

Assume that usb device was suspended and power off, so port's usage count must be 0
since it has been suspended. If pm qos NO_POWER_OFF was set at this time, pm core
would get port resume and suspend it again. the usage change 0 - 1 - 0. So port is
power off with NO_POWER_OFF flag setting, Does this make sense?

>> Rafael
>>
>>
>

-- 
Best regards
Tianyu Lan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 3/6] usb: add runtime pm support for usb port device

[Alan Stern]

On Wed, 14 Nov 2012, Lan Tianyu wrote:

> >>> Hi Rafael and Alan:
> >>>     This patch has a collaboration problem with pm qos. Since pm core would
> >>> pm_runtime_get_sync/put(port_dev) if pm qos flags was changed and port's
> >>> suspend call_back() clear PORT_POWER feature without any check. This
> >>> will cause PORT_POWER feather being cleared every time after pm qos
> >>> flags being changed.
> >>>
> >>>     I have an idea that add check in the port's runtime idle callback.
> >>> Check NO_POWER_OFF flag firstly. If set return. Second, for port without
> >>> device, suspend port directly and for port with device, increase child
> >>> device's runtime usage without resume and do barrier to ensure all
> >>> suspend process finish, at last check the child runtime status. If it
> >>> was suspended, suspend port and if do nothing.

> >> Hmm.  If child->dev is not suspended, then our usage_count should be
> >> at least 1, so pm_runtime_suspend(&port_dev->dev) shouldn't actually
> >> suspend us.  Isn't that the case?
> > No, because the child device is not under port device and so even if
> > child->dev is not suspended, port device's usage still can be 0 and
> > power off the port.
> >>
> Please ignore this reply. I may not understand your reply correctly.
> You are right if the child->dev is not suspended, the usage count should be at
> last 1. But how about if the child->dev is suspended.
> 
> Assume that usb device was suspended and power off, so port's usage count must be 0
> since it has been suspended. If pm qos NO_POWER_OFF was set at this time, pm core
> would get port resume and suspend it again. the usage change 0 - 1 - 0. So port is
> power off with NO_POWER_OFF flag setting, Does this make sense?

Suppose, as you say, the USB device is suspended and the port is
powered off.  Now the user wants to set the PM QOS NO_POWER_OFF flag.  
When this happens, the PM core will first do a runtime resume of the
port, then it will set the flag, and then it will do a runtime suspend
of the port.  The port's runtime_suspend callback should see that the
flag is set and return -EAGAIN, leaving the port powered on.

Alan Stern