[PATCH V6 0/8] usb: usb port power off mechanism and expose usb port connect type

[PATCH V6 0/8] usb: usb port power off mechanism and expose usb port connect type

[Lan Tianyu]

Change since v1:
       optimize the export connect type patch and adjust the DeviceRemovalbe flag
in the rh_call_control() after GetHubDescriptor request being processed.
       move all debounce operation to usb port's runtime resume callback(). Add
did_runtime_put in the struct usb_port to call pm_runtime_get/put(portdev) paired.
       using pm_runtime_get/put(portdev) to allow or prohibit device to be power
off inside of pm qos request in the kernel side. 

Change since v2:
       Correct some link breaks. 
       Add did_runtime_put in the usb_disconnect() before calling pm_runtime_put(portdev).
       Provide two seperate functions usb_device_allow_power_off() and usb_device_prevent_power_off()
instead of just one.

Change since v3:
	Set did_runtime_put to false in usb_disconnect() when its value is true
	Add comment about "not enable port runtime pm if fail to expose port's pm qos." and call
pm_runtime_set_active(portdev) unconditionally.
	rename "usb_device_allow_prevent_power_off" with "usb_device_control_power_off"
	Modify "be power off" to "be powered off"
	Expose dev_pm_qos_flags() symbol in order to ensure usb core can compile as module. 

Resend v4:
        make patch "PM/Qos: Expose dev_pm_qos_flags symbol" as first patch to avoid compilation error during "git bisect"
        correct some comments.

Change since v4:
	use EXPORT_SYMBOL_GPL to export dev_pm_qos_flags().
	correct some unnecessary link breaks.	
	Add CONFIG_USB_SUSPEND check around usb_port_runtime_resume() and usb_port_runtime_suspend()

Change since v5:
	predefine struct usb_hub_descriptor in the /driver/usb/core/usb.h instead of including "linux/usb/ch11.h"
	move patch "PM/Qos: Expose dev_pm_qos_flags symbol" before patch "usb: add runtime pm support for usb port device"
	 where dev_pm_qos_flags() fistly is called.


This patchset is based on usb-next tree commit f4cc1834 "USB: storage: avoid scanning other targets for single target device"

This patchset is to add usb port power off mechanism and
merge with patchset "usb: expose DeviceRemovable to user space via sysfs attribute".

Patchset "usb: expose DeviceRemovable to user space via sysfs attribute".
http://marc.info/?l=linux-usb&m=135279430410171&w=2 with some link break corrects

The main discussion about usb port power off mechanism is in the following link:
http://marc.info/?l=linux-usb&m=134818340017208&w=2


USB: Set usb port's DeviceRemovable according acpi information
usb: Register usb port's acpi power resources
PM/Qos: Expose dev_pm_qos_flags symbol
usb: add runtime pm support for usb port device
usb: add usb port auto power off mechanism
usb: expose usb port's pm qos flags to user space
usb: add usb_device_allow_power_off() and usb_device_prevent_power_off() function.
usb: enable usb port device's async suspend.

[PATCH v6 2/8] usb: Register usb port's acpi power resources

[Lan Tianyu]

This patch is to register usb port's acpi power resources. Create
link between usb port device and its acpi power resource.

Acked-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 drivers/usb/core/port.c     |    3 +++
 drivers/usb/core/usb-acpi.c |   20 ++++++++++++++++++++
 drivers/usb/core/usb.h      |    6 ++++++
 3 files changed, 29 insertions(+)

diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index fe5959f..4dfa254 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -66,6 +66,7 @@ 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);
 }
 
@@ -95,6 +96,8 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1)
 	if (retval)
 		goto error_register;
 
+	usb_acpi_register_power_resources(&port_dev->dev);
+
 	return 0;
 
 error_register:
diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
index cef4252..558ab01 100644
--- a/drivers/usb/core/usb-acpi.c
+++ b/drivers/usb/core/usb-acpi.c
@@ -216,6 +216,26 @@ 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)
+		acpi_power_resource_unregister_device(dev, port_handle);
+}
+
 int usb_acpi_register(void)
 {
 	return register_acpi_bus_type(&usb_acpi_bus);
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index a7f20bd..601b044 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -191,7 +191,13 @@ extern int usb_acpi_register(void);
 extern void usb_acpi_unregister(void);
 extern acpi_handle usb_get_hub_port_acpi_handle(struct usb_device *hdev,
 	int port1);
+extern int usb_acpi_register_power_resources(struct device *dev);
+extern void usb_acpi_unregister_power_resources(struct device *dev);
 #else
 static inline int usb_acpi_register(void) { return 0; };
 static inline void usb_acpi_unregister(void) { };
+static inline int usb_acpi_register_power_resources(struct device *dev)
+	{ return 0; };
+static inline void usb_acpi_unregister_power_resources(struct device *dev)
+	{ };
 #endif
-- 
1.7.9.5

Re: [PATCH v6 2/8] usb: Register usb port's acpi power resources

[Greg KH]

On Mon, Jan 21, 2013 at 10:18:01PM +0800, 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.
> 
> Acked-by: Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org>
> Signed-off-by: Lan Tianyu <tianyu.lan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/usb/core/port.c     |    3 +++
>  drivers/usb/core/usb-acpi.c |   20 ++++++++++++++++++++
>  drivers/usb/core/usb.h      |    6 ++++++
>  3 files changed, 29 insertions(+)
> 
> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
> index fe5959f..4dfa254 100644
> --- a/drivers/usb/core/port.c
> +++ b/drivers/usb/core/port.c
> @@ -66,6 +66,7 @@ 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);
>  }
>  
> @@ -95,6 +96,8 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1)
>  	if (retval)
>  		goto error_register;
>  
> +	usb_acpi_register_power_resources(&port_dev->dev);
> +
>  	return 0;
>  
>  error_register:
> diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
> index cef4252..558ab01 100644
> --- a/drivers/usb/core/usb-acpi.c
> +++ b/drivers/usb/core/usb-acpi.c
> @@ -216,6 +216,26 @@ 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;
> +}

Why return a value if you never check it?  Either properly handle the
error in the place you call this function, or don't have a return value.

I'd prefer you to handle the error properly.

thanks,

greg k-h
--
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: [PATCH v6 2/8] usb: Register usb port's acpi power resources

[Lan Tianyu]

Hi Greg:
	Great thanks for your review. I learn a lot from your comments which I
have not noticed before.

On 2013年01月22日 05:20, Greg KH wrote:
> On Mon, Jan 21, 2013 at 10:18:01PM +0800, 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.
>>
>> Acked-by: Alan Stern <stern@rowland.harvard.edu>
>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>> ---
>>  drivers/usb/core/port.c     |    3 +++
>>  drivers/usb/core/usb-acpi.c |   20 ++++++++++++++++++++
>>  drivers/usb/core/usb.h      |    6 ++++++
>>  3 files changed, 29 insertions(+)
>>
>> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
>> index fe5959f..4dfa254 100644
>> --- a/drivers/usb/core/port.c
>> +++ b/drivers/usb/core/port.c
>> @@ -66,6 +66,7 @@ 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);
>>  }
>>  
>> @@ -95,6 +96,8 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1)
>>  	if (retval)
>>  		goto error_register;
>>  
>> +	usb_acpi_register_power_resources(&port_dev->dev);
>> +
>>  	return 0;
>>  
>>  error_register:
>> diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
>> index cef4252..558ab01 100644
>> --- a/drivers/usb/core/usb-acpi.c
>> +++ b/drivers/usb/core/usb-acpi.c
>> @@ -216,6 +216,26 @@ 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;
>> +}
> 
> Why return a value if you never check it?  Either properly handle the
> error in the place you call this function, or don't have a return value.
> 
> I'd prefer you to handle the error properly.
Not all system will provide acpi power resouces for usb port and this
will not damage usb device function. So I think I should produce some
warning if the return value is not ENODEV.
> 
> thanks,
> 
> greg k-h
> 


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

[PATCH v6 3/8] PM/Qos: Expose dev_pm_qos_flags symbol

[Lan Tianyu]

The dev_pm_qos_flags() will be used in the usb core which could be
compiled as a module. This patch is to export it.

Acked-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 drivers/base/power/qos.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c
index d213495..3d4d1f8 100644
--- a/drivers/base/power/qos.c
+++ b/drivers/base/power/qos.c
@@ -91,6 +91,7 @@ enum pm_qos_flags_status dev_pm_qos_flags(struct device *dev, s32 mask)
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(dev_pm_qos_flags);
 
 /**
  * __dev_pm_qos_read_value - Get PM QoS constraint for a given device.
-- 
1.7.9.5

[PATCH v6 4/8] usb: add runtime pm support for usb port device

[Lan Tianyu]

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. Do
pm_rumtime_get_sync/put(portdev) when a device is plugged/unplugged
to prevent it from being powered off when it is active.

Acked-by: Alan Stern <stern@rowland.harvard.edu>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 drivers/usb/core/hub.c  |   18 ++++++++++++++++++
 drivers/usb/core/hub.h  |    4 ++++
 drivers/usb/core/port.c |   45 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 67 insertions(+)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 29ca6ed..8c1f9a5 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -714,6 +714,18 @@ 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
@@ -1569,6 +1581,7 @@ static void hub_disconnect(struct usb_interface *intf)
 	kfree(hub->status);
 	kfree(hub->buffer);
 
+	pm_suspend_ignore_children(&intf->dev, false);
 	kref_put(&hub->kref, hub_release);
 }
 
@@ -1671,6 +1684,7 @@ descriptor_error:
 
 	usb_set_intfdata (intf, hub);
 	intf->needs_remote_wakeup = 1;
+	pm_suspend_ignore_children(&intf->dev, true);
 
 	if (hdev->speed == USB_SPEED_HIGH)
 		highspeed_hubs++;
@@ -1997,6 +2011,8 @@ void usb_disconnect(struct usb_device **pdev)
 
 		sysfs_remove_link(&udev->dev.kobj, "port");
 		sysfs_remove_link(&port_dev->dev.kobj, "device");
+
+		pm_runtime_put(&port_dev->dev);
 	}
 
 	usb_remove_ep_devs(&udev->ep0);
@@ -2307,6 +2323,8 @@ int usb_new_device(struct usb_device *udev)
 			sysfs_remove_link(&udev->dev.kobj, "port");
 			goto fail;
 		}
+
+		pm_runtime_get_sync(&port_dev->dev);
 	}
 
 	(void) usb_create_ep_devs(&udev->dev, &udev->ep0, udev);
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index d16a7c9..8ea6bc8 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -79,12 +79,14 @@ struct usb_hub {
  * @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) \
@@ -94,4 +96,6 @@ extern int usb_hub_create_port_device(struct usb_hub *hub,
 		int port1);
 extern void usb_hub_remove_port_device(struct usb_hub *hub,
 		int port1);
+extern int usb_hub_set_port_power(struct usb_device *hdev,
+		int port1, bool set);
 
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 4dfa254..bd08f7e 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -17,6 +17,7 @@
  */
 
 #include <linux/slab.h>
+#include <linux/pm_qos.h>
 
 #include "hub.h"
 
@@ -70,9 +71,50 @@ static void usb_port_device_release(struct device *dev)
 	kfree(port_dev);
 }
 
+#ifdef CONFIG_USB_SUSPEND
+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);
+	struct usb_interface *intf = to_usb_interface(dev->parent);
+	int retval;
+
+	usb_autopm_get_interface(intf);
+	retval = usb_hub_set_port_power(hdev, port_dev->portnum, true);
+	usb_autopm_put_interface(intf);
+	return retval;
+}
+
+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);
+	struct usb_interface *intf = to_usb_interface(dev->parent);
+	int retval;
+
+	if (dev_pm_qos_flags(&port_dev->dev, PM_QOS_FLAG_NO_POWER_OFF)
+			== PM_QOS_FLAGS_ALL)
+		return -EAGAIN;
+
+	usb_autopm_get_interface(intf);
+	retval = usb_hub_set_port_power(hdev, port_dev->portnum, false);
+	usb_autopm_put_interface(intf);
+	return retval;
+}
+#endif
+
+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 =		pm_generic_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 usb_hub *hub, int port1)
@@ -87,6 +129,7 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1)
 	}
 
 	hub->ports[port1 - 1] = port_dev;
+	port_dev->portnum = port1;
 	port_dev->dev.parent = hub->intfdev;
 	port_dev->dev.groups = port_dev_group;
 	port_dev->dev.type = &usb_port_device_type;
@@ -96,6 +139,8 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1)
 	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;
-- 
1.7.9.5

Re: [PATCH v6 4/8] usb: add runtime pm support for usb port device

[Greg KH]

On Mon, Jan 21, 2013 at 10:18:03PM +0800, 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. Do
> pm_rumtime_get_sync/put(portdev) when a device is plugged/unplugged
> to prevent it from being powered off when it is active.
> 
> Acked-by: Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Lan Tianyu <tianyu.lan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/usb/core/hub.c  |   18 ++++++++++++++++++
>  drivers/usb/core/hub.h  |    4 ++++
>  drivers/usb/core/port.c |   45 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 67 insertions(+)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 29ca6ed..8c1f9a5 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -714,6 +714,18 @@ 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)

As this is a new global USB function, please provide the proper
kerneldoc comments describing what it does.

thanks,

greg k-h
--
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: [PATCH v6 4/8] usb: add runtime pm support for usb port device

[Lan Tianyu]

On 2013年01月22日 05:24, Greg KH wrote:
> On Mon, Jan 21, 2013 at 10:18:03PM +0800, 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. Do
>> pm_rumtime_get_sync/put(portdev) when a device is plugged/unplugged
>> to prevent it from being powered off when it is active.
>>
>> Acked-by: Alan Stern <stern@rowland.harvard.edu>
>> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>> ---
>>  drivers/usb/core/hub.c  |   18 ++++++++++++++++++
>>  drivers/usb/core/hub.h  |    4 ++++
>>  drivers/usb/core/port.c |   45 +++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 67 insertions(+)
>>
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index 29ca6ed..8c1f9a5 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -714,6 +714,18 @@ 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)
> 
> As this is a new global USB function, please provide the proper
> kerneldoc comments describing what it does.
> 
OK. I will add later and actually the function will be only used in the
driver/usb/core/hub.c and port.c.
> thanks,
> 
> greg k-h
> 


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

[PATCH v6 1/8] USB: Set usb port's DeviceRemovable according acpi information

[Lan Tianyu]

ACPI provide "_PLD" and "_UPC" aml methods to describe usb port
visibility and connectability. This patch is to add usb_hub_adjust_DeviceRemovable()
to adjust usb hub port's DeviceRemovable according ACPI information and invoke it in
the rh_call_control(). When hub descriptor request is issued at first time,
usb port device isn't created and usb port is not bound with acpi. So first
hub descriptor request is not changed based on ACPI information. After usb
port devices being created, call usb_hub_adjust_DeviceRemovable in the hub_configure()
and then set hub port's DeviceRemovable according ACPI information and this also works
for non-root hub.

Acked-by: Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org>
Signed-off-by: Lan Tianyu <tianyu.lan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/usb/core/hcd.c |    4 ++++
 drivers/usb/core/hub.c |   43 +++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/core/usb.h |    3 +++
 3 files changed, 50 insertions(+)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 5f6da8b..2459896 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -619,6 +619,10 @@ nongeneric:
 		status = hcd->driver->hub_control (hcd,
 			typeReq, wValue, wIndex,
 			tbuf, wLength);
+
+		if (typeReq == GetHubDescriptor)
+			usb_hub_adjust_deviceremovable(hcd->self.root_hub,
+				(struct usb_hub_descriptor *)tbuf);
 		break;
 error:
 		/* "protocol stall" on error */
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index cfdd4ee..29ca6ed 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -1513,6 +1513,8 @@ static int hub_configure(struct usb_hub *hub,
 			dev_err(hub->intfdev,
 				"couldn't create port%d device.\n", i + 1);
 
+	usb_hub_adjust_deviceremovable(hdev, hub->descriptor);
+
 	hub_activate(hub, HUB_INIT);
 	return 0;
 
@@ -5219,6 +5221,47 @@ usb_get_hub_port_connect_type(struct usb_device *hdev, int port1)
 	return hub->ports[port1 - 1]->connect_type;
 }
 
+void usb_hub_adjust_deviceremovable(struct usb_device *hdev,
+		struct usb_hub_descriptor *desc)
+{
+	enum usb_port_connect_type connect_type;
+	int i;
+
+	if (!hub_is_superspeed(hdev)) {
+		for (i = 1; i <= hdev->maxchild; i++) {
+			connect_type = usb_get_hub_port_connect_type(hdev, i);
+
+			if (connect_type == USB_PORT_CONNECT_TYPE_HARD_WIRED) {
+				u8 mask = 1 << (i%8);
+
+				if (!(desc->u.hs.DeviceRemovable[i/8] & mask)) {
+					dev_dbg(&hdev->dev, "usb port%d's DeviceRemovable is changed to 1 according to platform information.\n",
+						i);
+					desc->u.hs.DeviceRemovable[i/8]	|= mask;
+				}
+			}
+		}
+	} else {
+		u16 port_removable = le16_to_cpu(desc->u.ss.DeviceRemovable);
+
+		for (i = 1; i <= hdev->maxchild; i++) {
+			connect_type = usb_get_hub_port_connect_type(hdev, i);
+
+			if (connect_type == USB_PORT_CONNECT_TYPE_HARD_WIRED) {
+				u16 mask = 1 << i;
+
+				if (!(port_removable & mask)) {
+					dev_dbg(&hdev->dev, "usb port%d's DeviceRemovable is changed to 1 according to platform information.\n",
+						i);
+					port_removable |= mask;
+				}
+			}
+		}
+
+		desc->u.ss.DeviceRemovable = cpu_to_le16(port_removable);
+	}
+}
+
 #ifdef CONFIG_ACPI
 /**
  * usb_get_hub_port_acpi_handle - Get the usb port's acpi handle
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index fb7d8fc..a7f20bd 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -1,6 +1,7 @@
 #include <linux/pm.h>
 #include <linux/acpi.h>
 
+struct usb_hub_descriptor;
 struct dev_state;
 
 /* Functions local to drivers/usb/core/ */
@@ -182,6 +183,8 @@ extern enum usb_port_connect_type
 	usb_get_hub_port_connect_type(struct usb_device *hdev, int port1);
 extern void usb_set_hub_port_connect_type(struct usb_device *hdev, int port1,
 	enum usb_port_connect_type type);
+extern void usb_hub_adjust_deviceremovable(struct usb_device *hdev,
+		struct usb_hub_descriptor *desc);
 
 #ifdef CONFIG_ACPI
 extern int usb_acpi_register(void);
-- 
1.7.9.5

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

[PATCH v6 5/8] usb: add usb port auto power off mechanism

[Lan Tianyu]

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 it resumes, power on port again. Add did_runtime_put
in the struct usb_port in order to call pm_runtime_get/put(portdev)
paired during suspending and resuming.

Acked-by: Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Lan Tianyu <tianyu.lan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/usb/core/hub.c  |   67 ++++++++++++++++++++++++++++++++++++++++-------
 drivers/usb/core/hub.h  |    9 +++++++
 drivers/usb/core/port.c |   40 ++++++++++++++++++++++++++--
 3 files changed, 105 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 8c1f9a5..786db99 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>
@@ -108,7 +109,7 @@ MODULE_PARM_DESC(use_both_schemes,
 DECLARE_RWSEM(ehci_cf_port_reset_rwsem);
 EXPORT_SYMBOL_GPL(ehci_cf_port_reset_rwsem);
 
-#define HUB_DEBOUNCE_TIMEOUT	1500
+#define HUB_DEBOUNCE_TIMEOUT	2000
 #define HUB_DEBOUNCE_STEP	  25
 #define HUB_DEBOUNCE_STABLE	 100
 
@@ -127,7 +128,7 @@ static inline char *portspeed(struct usb_hub *hub, int portstatus)
 }
 
 /* Note that hdev or one of its children must be locked! */
-static struct usb_hub *hdev_to_hub(struct usb_device *hdev)
+struct usb_hub *hdev_to_hub(struct usb_device *hdev)
 {
 	if (!hdev || !hdev->actconfig || !hdev->maxchild)
 		return NULL;
@@ -393,7 +394,7 @@ static int clear_hub_feature(struct usb_device *hdev, int feature)
 /*
  * USB 2.0 spec Section 11.24.2.2
  */
-static int clear_port_feature(struct usb_device *hdev, int port1, int feature)
+int clear_port_feature(struct usb_device *hdev, int port1, int feature)
 {
 	return usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0),
 		USB_REQ_CLEAR_FEATURE, USB_RT_PORT, feature, port1,
@@ -718,11 +719,16 @@ int usb_hub_set_port_power(struct usb_device *hdev, int port1,
 		bool set)
 {
 	int ret;
+	struct usb_hub *hub = hdev_to_hub(hdev);
+	struct usb_port *port_dev = hub->ports[port1 - 1];
 
 	if (set)
 		ret = set_port_feature(hdev, port1, USB_PORT_FEAT_POWER);
 	else
 		ret = clear_port_feature(hdev, port1, USB_PORT_FEAT_POWER);
+
+	if (!ret)
+		port_dev->power_is_on = set;
 	return ret;
 }
 
@@ -802,7 +808,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_is_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);
@@ -1134,10 +1144,16 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
 				set_bit(port1, hub->change_bits);
 
 		} else if (udev->persist_enabled) {
+			struct usb_port *port_dev = hub->ports[port1 - 1];
+
 #ifdef CONFIG_PM
 			udev->reset_resume = 1;
 #endif
-			set_bit(port1, hub->change_bits);
+			/* Don't set the change_bits when the device
+			 * was powered off.
+			 */
+			if (port_dev->power_is_on)
+				set_bit(port1, hub->change_bits);
 
 		} else {
 			/* The power session is gone; tell khubd */
@@ -2012,7 +2028,10 @@ void usb_disconnect(struct usb_device **pdev)
 		sysfs_remove_link(&udev->dev.kobj, "port");
 		sysfs_remove_link(&port_dev->dev.kobj, "device");
 
-		pm_runtime_put(&port_dev->dev);
+		if (!port_dev->did_runtime_put)
+			pm_runtime_put(&port_dev->dev);
+		else
+			port_dev->did_runtime_put = false;
 	}
 
 	usb_remove_ep_devs(&udev->ep0);
@@ -2844,6 +2863,8 @@ 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];
+	enum pm_qos_flags_status pm_qos_stat;
 	int		port1 = udev->portnum;
 	int		status;
 
@@ -2936,6 +2957,21 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
 		udev->port_is_suspended = 1;
 		msleep(10);
 	}
+
+	/*
+	 * Check whether current status meets the requirement of
+	 * usb port power off mechanism
+	 */
+	pm_qos_stat = dev_pm_qos_flags(&port_dev->dev,
+			PM_QOS_FLAG_NO_POWER_OFF);
+	if (!udev->do_remote_wakeup
+			&& pm_qos_stat != PM_QOS_FLAGS_ALL
+			&& udev->persist_enabled
+			&& !status) {
+		pm_runtime_put_sync(&port_dev->dev);
+		port_dev->did_runtime_put = true;
+	}
+
 	usb_mark_last_busy(hub->hdev);
 	return status;
 }
@@ -3062,10 +3098,21 @@ 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;
 
+	if (port_dev->did_runtime_put) {
+		status = pm_runtime_get_sync(&port_dev->dev);
+		port_dev->did_runtime_put = false;
+		if (status < 0) {
+			dev_dbg(&udev->dev, "can't resume usb port, 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))
@@ -3744,7 +3791,7 @@ EXPORT_SYMBOL_GPL(usb_enable_ltm);
  * every 25ms for transient disconnects.  When the port status has been
  * unchanged for 100ms it returns the port status.
  */
-static int hub_port_debounce(struct usb_hub *hub, int port1)
+int hub_port_debounce(struct usb_hub *hub, int port1, bool must_be_connected)
 {
 	int ret;
 	int total_time, stable_time = 0;
@@ -3758,7 +3805,9 @@ static int hub_port_debounce(struct usb_hub *hub, int port1)
 
 		if (!(portchange & USB_PORT_STAT_C_CONNECTION) &&
 		     (portstatus & USB_PORT_STAT_CONNECTION) == connection) {
-			stable_time += HUB_DEBOUNCE_STEP;
+			if (!must_be_connected || (connection
+					== USB_PORT_STAT_CONNECTION))
+				stable_time += HUB_DEBOUNCE_STEP;
 			if (stable_time >= HUB_DEBOUNCE_STABLE)
 				break;
 		} else {
@@ -4279,7 +4328,7 @@ static void hub_port_connect_change(struct usb_hub *hub, int port1,
 
 	if (portchange & (USB_PORT_STAT_C_CONNECTION |
 				USB_PORT_STAT_C_ENABLE)) {
-		status = hub_port_debounce(hub, port1);
+		status = hub_port_debounce(hub, port1, false);
 		if (status < 0) {
 			if (printk_ratelimit())
 				dev_err(hub_dev, "connect-debounce failed, "
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index 8ea6bc8..ccd4795 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -80,6 +80,8 @@ struct usb_hub {
  * @port_owner: port's owner
  * @connect_type: port's connect type
  * @portnum: port index num based one
+ * @power_is_on: port's power state
+ * @did_runtime_put: port has done pm_runtime_put().
  */
 struct usb_port {
 	struct usb_device *child;
@@ -87,6 +89,8 @@ struct usb_port {
 	struct dev_state *port_owner;
 	enum usb_port_connect_type connect_type;
 	u8 portnum;
+	unsigned power_is_on:1;
+	unsigned did_runtime_put:1;
 };
 
 #define to_usb_port(_dev) \
@@ -98,4 +102,9 @@ extern void usb_hub_remove_port_device(struct usb_hub *hub,
 		int port1);
 extern int usb_hub_set_port_power(struct usb_device *hdev,
 		int port1, bool set);
+extern struct usb_hub *hdev_to_hub(struct usb_device *hdev);
+extern int hub_port_debounce(struct usb_hub *hub, int port1,
+		bool must_be_connected);
+extern int clear_port_feature(struct usb_device *hdev,
+		int port1, int feature);
 
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index bd08f7e..dc1aaec 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -77,10 +77,36 @@ 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);
 	struct usb_interface *intf = to_usb_interface(dev->parent);
+	struct usb_hub *hub = hdev_to_hub(hdev);
+	int port1 = port_dev->portnum;
 	int retval;
 
+	if (!hub)
+		return -EINVAL;
+
 	usb_autopm_get_interface(intf);
-	retval = usb_hub_set_port_power(hdev, port_dev->portnum, true);
+	set_bit(port1, hub->busy_bits);
+
+	retval = usb_hub_set_port_power(hdev, port1, true);
+	if (port_dev->child && !retval) {
+		/*
+		 * Wait for usb hub port to be reconnected in order to make
+		 * the resume procedure successful.
+		 */
+		retval = hub_port_debounce(hub, port1, true);
+		if (retval < 0) {
+			dev_dbg(&port_dev->dev, "can't get reconnection after setting port  power on, status %d\n",
+					retval);
+			goto out;
+		}
+		clear_port_feature(hdev, port1,	USB_PORT_FEAT_C_ENABLE);
+
+		/* Set return value to 0 if debounce successful */
+		retval = 0;
+	}
+
+out:
+	clear_bit(port1, hub->busy_bits);
 	usb_autopm_put_interface(intf);
 	return retval;
 }
@@ -90,14 +116,23 @@ 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);
 	struct usb_interface *intf = to_usb_interface(dev->parent);
+	struct usb_hub *hub = hdev_to_hub(hdev);
+	int port1 = port_dev->portnum;
 	int retval;
 
+	if (!hub)
+		return -EINVAL;
+
 	if (dev_pm_qos_flags(&port_dev->dev, PM_QOS_FLAG_NO_POWER_OFF)
 			== PM_QOS_FLAGS_ALL)
 		return -EAGAIN;
 
 	usb_autopm_get_interface(intf);
-	retval = usb_hub_set_port_power(hdev, port_dev->portnum, false);
+	set_bit(port1, hub->busy_bits);
+	retval = usb_hub_set_port_power(hdev, port1, false);
+	clear_port_feature(hdev, port1, USB_PORT_FEAT_C_CONNECTION);
+	clear_port_feature(hdev, port1,	USB_PORT_FEAT_C_ENABLE);
+	clear_bit(port1, hub->busy_bits);
 	usb_autopm_put_interface(intf);
 	return retval;
 }
@@ -130,6 +165,7 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1)
 
 	hub->ports[port1 - 1] = port_dev;
 	port_dev->portnum = port1;
+	port_dev->power_is_on = true;
 	port_dev->dev.parent = hub->intfdev;
 	port_dev->dev.groups = port_dev_group;
 	port_dev->dev.type = &usb_port_device_type;
-- 
1.7.9.5

--
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: [PATCH v6 5/8] usb: add usb port auto power off mechanism

[Greg KH]

On Mon, Jan 21, 2013 at 10:18:04PM +0800, 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 it resumes, power on port again. Add did_runtime_put
> in the struct usb_port in order to call pm_runtime_get/put(portdev)
> paired during suspending and resuming.

I don't understand what did_runtime_put is supposed to mean.  Care to
explain this better?

> 
> Acked-by: Alan Stern <stern@rowland.harvard.edu>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> ---
>  drivers/usb/core/hub.c  |   67 ++++++++++++++++++++++++++++++++++++++++-------
>  drivers/usb/core/hub.h  |    9 +++++++
>  drivers/usb/core/port.c |   40 ++++++++++++++++++++++++++--
>  3 files changed, 105 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 8c1f9a5..786db99 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>
> @@ -108,7 +109,7 @@ MODULE_PARM_DESC(use_both_schemes,
>  DECLARE_RWSEM(ehci_cf_port_reset_rwsem);
>  EXPORT_SYMBOL_GPL(ehci_cf_port_reset_rwsem);
>  
> -#define HUB_DEBOUNCE_TIMEOUT	1500
> +#define HUB_DEBOUNCE_TIMEOUT	2000

Why did you increase this timeout?  You don't mention that above in the
changelog entry.

>  #define HUB_DEBOUNCE_STEP	  25
>  #define HUB_DEBOUNCE_STABLE	 100
>  
> @@ -127,7 +128,7 @@ static inline char *portspeed(struct usb_hub *hub, int portstatus)
>  }
>  
>  /* Note that hdev or one of its children must be locked! */
> -static struct usb_hub *hdev_to_hub(struct usb_device *hdev)
> +struct usb_hub *hdev_to_hub(struct usb_device *hdev)

This needs a better name now that this is a global function.  No one
knows what a "hdev" is.

>  {
>  	if (!hdev || !hdev->actconfig || !hdev->maxchild)
>  		return NULL;
> @@ -393,7 +394,7 @@ static int clear_hub_feature(struct usb_device *hdev, int feature)
>  /*
>   * USB 2.0 spec Section 11.24.2.2
>   */
> -static int clear_port_feature(struct usb_device *hdev, int port1, int feature)
> +int clear_port_feature(struct usb_device *hdev, int port1, int feature)

This is now a global function, please put usb_ infront of it.

>  {
>  	return usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0),
>  		USB_REQ_CLEAR_FEATURE, USB_RT_PORT, feature, port1,
> @@ -718,11 +719,16 @@ int usb_hub_set_port_power(struct usb_device *hdev, int port1,
>  		bool set)
>  {
>  	int ret;
> +	struct usb_hub *hub = hdev_to_hub(hdev);
> +	struct usb_port *port_dev = hub->ports[port1 - 1];
>  
>  	if (set)
>  		ret = set_port_feature(hdev, port1, USB_PORT_FEAT_POWER);
>  	else
>  		ret = clear_port_feature(hdev, port1, USB_PORT_FEAT_POWER);
> +
> +	if (!ret)
> +		port_dev->power_is_on = set;
>  	return ret;
>  }
>  
> @@ -802,7 +808,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_is_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);
> @@ -1134,10 +1144,16 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
>  				set_bit(port1, hub->change_bits);
>  
>  		} else if (udev->persist_enabled) {
> +			struct usb_port *port_dev = hub->ports[port1 - 1];
> +
>  #ifdef CONFIG_PM
>  			udev->reset_resume = 1;
>  #endif
> -			set_bit(port1, hub->change_bits);
> +			/* Don't set the change_bits when the device
> +			 * was powered off.
> +			 */
> +			if (port_dev->power_is_on)
> +				set_bit(port1, hub->change_bits);
>  
>  		} else {
>  			/* The power session is gone; tell khubd */
> @@ -2012,7 +2028,10 @@ void usb_disconnect(struct usb_device **pdev)
>  		sysfs_remove_link(&udev->dev.kobj, "port");
>  		sysfs_remove_link(&port_dev->dev.kobj, "device");
>  
> -		pm_runtime_put(&port_dev->dev);
> +		if (!port_dev->did_runtime_put)
> +			pm_runtime_put(&port_dev->dev);
> +		else
> +			port_dev->did_runtime_put = false;
>  	}
>  
>  	usb_remove_ep_devs(&udev->ep0);
> @@ -2844,6 +2863,8 @@ 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];
> +	enum pm_qos_flags_status pm_qos_stat;
>  	int		port1 = udev->portnum;
>  	int		status;
>  
> @@ -2936,6 +2957,21 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
>  		udev->port_is_suspended = 1;
>  		msleep(10);
>  	}
> +
> +	/*
> +	 * Check whether current status meets the requirement of
> +	 * usb port power off mechanism
> +	 */
> +	pm_qos_stat = dev_pm_qos_flags(&port_dev->dev,
> +			PM_QOS_FLAG_NO_POWER_OFF);
> +	if (!udev->do_remote_wakeup
> +			&& pm_qos_stat != PM_QOS_FLAGS_ALL
> +			&& udev->persist_enabled
> +			&& !status) {
> +		pm_runtime_put_sync(&port_dev->dev);
> +		port_dev->did_runtime_put = true;
> +	}
> +
>  	usb_mark_last_busy(hub->hdev);
>  	return status;
>  }
> @@ -3062,10 +3098,21 @@ 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;
>  
> +	if (port_dev->did_runtime_put) {
> +		status = pm_runtime_get_sync(&port_dev->dev);
> +		port_dev->did_runtime_put = false;
> +		if (status < 0) {
> +			dev_dbg(&udev->dev, "can't resume usb port, 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))
> @@ -3744,7 +3791,7 @@ EXPORT_SYMBOL_GPL(usb_enable_ltm);
>   * every 25ms for transient disconnects.  When the port status has been
>   * unchanged for 100ms it returns the port status.
>   */
> -static int hub_port_debounce(struct usb_hub *hub, int port1)
> +int hub_port_debounce(struct usb_hub *hub, int port1, bool must_be_connected)

I hate boolean flags in functions that do different things depending on
the value.  Can you just make two different functions here, and have
them call this private one with the proper flag set?  Otherwise, when
you see this function called, you have to go look up what the
"true/false" means and it wastes time and makes it very easy to get
wrong in the future.


>  {
>  	int ret;
>  	int total_time, stable_time = 0;
> @@ -3758,7 +3805,9 @@ static int hub_port_debounce(struct usb_hub *hub, int port1)
>  
>  		if (!(portchange & USB_PORT_STAT_C_CONNECTION) &&
>  		     (portstatus & USB_PORT_STAT_CONNECTION) == connection) {
> -			stable_time += HUB_DEBOUNCE_STEP;
> +			if (!must_be_connected || (connection
> +					== USB_PORT_STAT_CONNECTION))

Please do:
			if (!must_be_connected ||
			    (connection == USB_PORT_STAT_CONNECTION))

instead, that makes it much more readable.

thanks,

greg k-h


> +				stable_time += HUB_DEBOUNCE_STEP;
>  			if (stable_time >= HUB_DEBOUNCE_STABLE)
>  				break;
>  		} else {
> @@ -4279,7 +4328,7 @@ static void hub_port_connect_change(struct usb_hub *hub, int port1,
>  
>  	if (portchange & (USB_PORT_STAT_C_CONNECTION |
>  				USB_PORT_STAT_C_ENABLE)) {
> -		status = hub_port_debounce(hub, port1);
> +		status = hub_port_debounce(hub, port1, false);
>  		if (status < 0) {
>  			if (printk_ratelimit())
>  				dev_err(hub_dev, "connect-debounce failed, "
> diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
> index 8ea6bc8..ccd4795 100644
> --- a/drivers/usb/core/hub.h
> +++ b/drivers/usb/core/hub.h
> @@ -80,6 +80,8 @@ struct usb_hub {
>   * @port_owner: port's owner
>   * @connect_type: port's connect type
>   * @portnum: port index num based one
> + * @power_is_on: port's power state
> + * @did_runtime_put: port has done pm_runtime_put().
>   */
>  struct usb_port {
>  	struct usb_device *child;
> @@ -87,6 +89,8 @@ struct usb_port {
>  	struct dev_state *port_owner;
>  	enum usb_port_connect_type connect_type;
>  	u8 portnum;
> +	unsigned power_is_on:1;
> +	unsigned did_runtime_put:1;
>  };
>  
>  #define to_usb_port(_dev) \
> @@ -98,4 +102,9 @@ extern void usb_hub_remove_port_device(struct usb_hub *hub,
>  		int port1);
>  extern int usb_hub_set_port_power(struct usb_device *hdev,
>  		int port1, bool set);
> +extern struct usb_hub *hdev_to_hub(struct usb_device *hdev);
> +extern int hub_port_debounce(struct usb_hub *hub, int port1,
> +		bool must_be_connected);
> +extern int clear_port_feature(struct usb_device *hdev,
> +		int port1, int feature);
>  
> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
> index bd08f7e..dc1aaec 100644
> --- a/drivers/usb/core/port.c
> +++ b/drivers/usb/core/port.c
> @@ -77,10 +77,36 @@ 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);
>  	struct usb_interface *intf = to_usb_interface(dev->parent);
> +	struct usb_hub *hub = hdev_to_hub(hdev);
> +	int port1 = port_dev->portnum;
>  	int retval;
>  
> +	if (!hub)
> +		return -EINVAL;
> +
>  	usb_autopm_get_interface(intf);
> -	retval = usb_hub_set_port_power(hdev, port_dev->portnum, true);
> +	set_bit(port1, hub->busy_bits);
> +
> +	retval = usb_hub_set_port_power(hdev, port1, true);
> +	if (port_dev->child && !retval) {
> +		/*
> +		 * Wait for usb hub port to be reconnected in order to make
> +		 * the resume procedure successful.
> +		 */
> +		retval = hub_port_debounce(hub, port1, true);
> +		if (retval < 0) {
> +			dev_dbg(&port_dev->dev, "can't get reconnection after setting port  power on, status %d\n",
> +					retval);
> +			goto out;
> +		}
> +		clear_port_feature(hdev, port1,	USB_PORT_FEAT_C_ENABLE);
> +
> +		/* Set return value to 0 if debounce successful */
> +		retval = 0;
> +	}
> +
> +out:
> +	clear_bit(port1, hub->busy_bits);
>  	usb_autopm_put_interface(intf);
>  	return retval;
>  }
> @@ -90,14 +116,23 @@ 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);
>  	struct usb_interface *intf = to_usb_interface(dev->parent);
> +	struct usb_hub *hub = hdev_to_hub(hdev);
> +	int port1 = port_dev->portnum;
>  	int retval;
>  
> +	if (!hub)
> +		return -EINVAL;
> +
>  	if (dev_pm_qos_flags(&port_dev->dev, PM_QOS_FLAG_NO_POWER_OFF)
>  			== PM_QOS_FLAGS_ALL)
>  		return -EAGAIN;
>  
>  	usb_autopm_get_interface(intf);
> -	retval = usb_hub_set_port_power(hdev, port_dev->portnum, false);
> +	set_bit(port1, hub->busy_bits);
> +	retval = usb_hub_set_port_power(hdev, port1, false);
> +	clear_port_feature(hdev, port1, USB_PORT_FEAT_C_CONNECTION);
> +	clear_port_feature(hdev, port1,	USB_PORT_FEAT_C_ENABLE);
> +	clear_bit(port1, hub->busy_bits);
>  	usb_autopm_put_interface(intf);
>  	return retval;
>  }
> @@ -130,6 +165,7 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1)
>  
>  	hub->ports[port1 - 1] = port_dev;
>  	port_dev->portnum = port1;
> +	port_dev->power_is_on = true;
>  	port_dev->dev.parent = hub->intfdev;
>  	port_dev->dev.groups = port_dev_group;
>  	port_dev->dev.type = &usb_port_device_type;
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 8/8] usb: enable usb port device's async suspend.

[Lan Tianyu]

Signed-off-by: Lan Tianyu <tianyu.lan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/usb/core/port.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 0334d91..50b646e 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -215,7 +215,7 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1)
 		pm_runtime_enable(&port_dev->dev);
 
 	usb_acpi_register_power_resources(&port_dev->dev);

Re: [PATCH v6 8/8] usb: enable usb port device's async suspend.

[Greg KH]

On Mon, Jan 21, 2013 at 10:18:07PM +0800, Lan Tianyu wrote:
> Signed-off-by: Lan Tianyu <tianyu.lan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Why are you doing this?  Why is it now ok?

We need a much better changelog entry here.

thanks,

greg k-h
--
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: [PATCH v6 8/8] usb: enable usb port device's async suspend.

[Lan Tianyu]

On 2013/1/22 5:34, Greg KH wrote:
> On Mon, Jan 21, 2013 at 10:18:07PM +0800, Lan Tianyu wrote:
>> Signed-off-by: Lan Tianyu <tianyu.lan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>
> Why are you doing this?  Why is it now ok?
>
> We need a much better changelog entry here.
Ok. I will add later.
>
> thanks,
>
> greg k-h
>


-- 
Best Regards
Tianyu Lan
linux kernel enabling team
--
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

[PATCH v6 6/8] usb: expose usb port's pm qos flags to user space

[Lan Tianyu]

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 powered off.

Acked-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 drivers/usb/core/port.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index dc1aaec..0c51d24 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -67,6 +67,7 @@ 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);
 }
@@ -176,7 +177,15 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1)
 		goto error_register;
 
 	pm_runtime_set_active(&port_dev->dev);
-	pm_runtime_enable(&port_dev->dev);
+
+	/* It would be dangerous if user space couldn't
+	 * prevent usb device from being powered off. So don't
+	 * enable port runtime pm if failed to expose port's pm qos.
+	 */
+	if (!dev_pm_qos_expose_flags(&port_dev->dev,
+			PM_QOS_FLAG_NO_POWER_OFF))
+		pm_runtime_enable(&port_dev->dev);
+
 	usb_acpi_register_power_resources(&port_dev->dev);
 
 	return 0;
-- 
1.7.9.5

Re: [PATCH v6 6/8] usb: expose usb port's pm qos flags to user space

[Greg KH]

On Mon, Jan 21, 2013 at 10:18:05PM +0800, 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 powered off.

How are they now seen by userspace?  Should a new Documentation/ABI
entry be created somewhere describing this?

thanks,

greg k-h

Re: [PATCH v6 6/8] usb: expose usb port's pm qos flags to user space

[Lan Tianyu]

On 2013/1/22 5:31, Greg KH wrote:
> On Mon, Jan 21, 2013 at 10:18:05PM +0800, 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 powered off.
>
> How are they now seen by userspace?  Should a new Documentation/ABI
> entry be created somewhere describing this?
There are already some descriptors in the 
Documentation/ABI/testing/sysfs-devices-power(line 208, lin223).
>
> thanks,
>
> greg k-h
>


-- 
Best Regards
Tianyu Lan
linux kernel enabling team

[PATCH v6 7/8] usb: add usb_device_allow_power_off() and usb_device_prevent_power_off() function.

[Lan Tianyu]

Some usb devices can't be resumed correctly after power off. This
patch is to add usb_device_allow_power_off() and usb_device_prevent_power_off()
for device's driver. Call pm_runtime_get_sync(portdev) to increase port's usage
count and then port will not be suspended. The device will not be powered off.

Acked-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 drivers/usb/core/port.c |   28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 0c51d24..0334d91 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -18,11 +18,39 @@
 
 #include <linux/slab.h>
 #include <linux/pm_qos.h>
+#include <linux/module.h>
 
 #include "hub.h"
 
 static const struct attribute_group *port_dev_group[];
 
+/**
+ * usb_device_control_power_off - Allow or prohibit power off device.
+ * @udev: target usb device
+ * @allow: choice of allow or prohibit
+ *
+ * Call pm_runtime_get/put_sync(portdev) to allow or prohibit target
+ * usb device to be powered off in the kernel. The operations of setting
+ * true and false should be couple. The default status is allowed.
+ */
+int usb_device_control_power_off(struct usb_device *udev, bool allow)
+{
+	struct usb_port *port_dev;
+
+	if (!udev->parent)
+		return -EINVAL;
+
+	port_dev = hdev_to_hub(udev->parent)->ports[udev->portnum - 1];
+
+	if (allow)
+		pm_runtime_put_sync(&port_dev->dev);
+	else
+		pm_runtime_get_sync(&port_dev->dev);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(usb_device_control_power_off);
+
 static ssize_t show_port_connect_type(struct device *dev,
 	struct device_attribute *attr, char *buf)
 {
-- 
1.7.9.5

Re: [PATCH v6 7/8] usb: add usb_device_allow_power_off() and usb_device_prevent_power_off() function.

[Greg KH]

On Mon, Jan 21, 2013 at 10:18:06PM +0800, Lan Tianyu wrote:
> Some usb devices can't be resumed correctly after power off. This
> patch is to add usb_device_allow_power_off() and usb_device_prevent_power_off()
> for device's driver. Call pm_runtime_get_sync(portdev) to increase port's usage
> count and then port will not be suspended. The device will not be powered off.

Please linewrap comments at the proper level (git likes 72).

> 
> Acked-by: Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org>
> Signed-off-by: Lan Tianyu <tianyu.lan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/usb/core/port.c |   28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
> index 0c51d24..0334d91 100644
> --- a/drivers/usb/core/port.c
> +++ b/drivers/usb/core/port.c
> @@ -18,11 +18,39 @@
>  
>  #include <linux/slab.h>
>  #include <linux/pm_qos.h>
> +#include <linux/module.h>
>  
>  #include "hub.h"
>  
>  static const struct attribute_group *port_dev_group[];
>  
> +/**
> + * usb_device_control_power_off - Allow or prohibit power off device.
> + * @udev: target usb device
> + * @allow: choice of allow or prohibit
> + *
> + * Call pm_runtime_get/put_sync(portdev) to allow or prohibit target
> + * usb device to be powered off in the kernel. The operations of setting
> + * true and false should be couple. The default status is allowed.
> + */
> +int usb_device_control_power_off(struct usb_device *udev, bool allow)

Ick, again with the boolean variables.  Please don't do this, just make
two different functions:
	usb_device_allow_power_off()
	usb_device_forbid_power_off()
that makes it much easier to understand when you see the function being
called, right?

thanks,

greg k-h
--
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: [PATCH v6 7/8] usb: add usb_device_allow_power_off() and usb_device_prevent_power_off() function.

[Lan Tianyu]

On 2013/1/22 5:33, Greg KH wrote:
> On Mon, Jan 21, 2013 at 10:18:06PM +0800, Lan Tianyu wrote:
>> Some usb devices can't be resumed correctly after power off. This
>> patch is to add usb_device_allow_power_off() and usb_device_prevent_power_off()
>> for device's driver. Call pm_runtime_get_sync(portdev) to increase port's usage
>> count and then port will not be suspended. The device will not be powered off.
>
> Please linewrap comments at the proper level (git likes 72).
>
>>
>> Acked-by: Alan Stern <stern@rowland.harvard.edu>
>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>> ---
>>   drivers/usb/core/port.c |   28 ++++++++++++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>
>> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
>> index 0c51d24..0334d91 100644
>> --- a/drivers/usb/core/port.c
>> +++ b/drivers/usb/core/port.c
>> @@ -18,11 +18,39 @@
>>
>>   #include <linux/slab.h>
>>   #include <linux/pm_qos.h>
>> +#include <linux/module.h>
>>
>>   #include "hub.h"
>>
>>   static const struct attribute_group *port_dev_group[];
>>
>> +/**
>> + * usb_device_control_power_off - Allow or prohibit power off device.
>> + * @udev: target usb device
>> + * @allow: choice of allow or prohibit
>> + *
>> + * Call pm_runtime_get/put_sync(portdev) to allow or prohibit target
>> + * usb device to be powered off in the kernel. The operations of setting
>> + * true and false should be couple. The default status is allowed.
>> + */
>> +int usb_device_control_power_off(struct usb_device *udev, bool allow)
>
> Ick, again with the boolean variables.  Please don't do this, just make
> two different functions:
> 	usb_device_allow_power_off()
> 	usb_device_forbid_power_off()
> that makes it much easier to understand when you see the function being
> called, right?
Ok. I will do it.
>
> thanks,
>
> greg k-h
>


-- 
Best Regards
Tianyu Lan
linux kernel enabling team

Re: [PATCH v6 7/8] usb: add usb_device_allow_power_off() and usb_device_prevent_power_off() function.

[Alan Stern]

On Tue, 22 Jan 2013, Lan Tianyu wrote:

> >> +/**
> >> + * usb_device_control_power_off - Allow or prohibit power off device.
> >> + * @udev: target usb device
> >> + * @allow: choice of allow or prohibit
> >> + *
> >> + * Call pm_runtime_get/put_sync(portdev) to allow or prohibit target
> >> + * usb device to be powered off in the kernel. The operations of setting
> >> + * true and false should be couple. The default status is allowed.
> >> + */
> >> +int usb_device_control_power_off(struct usb_device *udev, bool allow)
> >
> > Ick, again with the boolean variables.  Please don't do this, just make
> > two different functions:
> > 	usb_device_allow_power_off()
> > 	usb_device_forbid_power_off()
> > that makes it much easier to understand when you see the function being
> > called, right?
> Ok. I will do it.

Greg and Tianyu:

You both missed the fact that Tianyu already did this.  It is in the 
submitted patch.  The two functions Greg asked for are defined as 
inline routines.

Alan Stern

Re: [PATCH v6 7/8] usb: add usb_device_allow_power_off() and usb_device_prevent_power_off() function.

[Greg KH]

On Mon, Jan 21, 2013 at 10:18:06PM +0800, Lan Tianyu wrote:
> Some usb devices can't be resumed correctly after power off. This
> patch is to add usb_device_allow_power_off() and usb_device_prevent_power_off()
> for device's driver. Call pm_runtime_get_sync(portdev) to increase port's usage
> count and then port will not be suspended. The device will not be powered off.
> 
> Acked-by: Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org>
> Signed-off-by: Lan Tianyu <tianyu.lan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/usb/core/port.c |   28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
> index 0c51d24..0334d91 100644
> --- a/drivers/usb/core/port.c
> +++ b/drivers/usb/core/port.c
> @@ -18,11 +18,39 @@
>  
>  #include <linux/slab.h>
>  #include <linux/pm_qos.h>
> +#include <linux/module.h>
>  
>  #include "hub.h"
>  
>  static const struct attribute_group *port_dev_group[];
>  
> +/**
> + * usb_device_control_power_off - Allow or prohibit power off device.
> + * @udev: target usb device
> + * @allow: choice of allow or prohibit
> + *
> + * Call pm_runtime_get/put_sync(portdev) to allow or prohibit target
> + * usb device to be powered off in the kernel. The operations of setting
> + * true and false should be couple. The default status is allowed.
> + */
> +int usb_device_control_power_off(struct usb_device *udev, bool allow)
> +{
> +	struct usb_port *port_dev;
> +
> +	if (!udev->parent)
> +		return -EINVAL;
> +
> +	port_dev = hdev_to_hub(udev->parent)->ports[udev->portnum - 1];
> +
> +	if (allow)
> +		pm_runtime_put_sync(&port_dev->dev);
> +	else
> +		pm_runtime_get_sync(&port_dev->dev);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_device_control_power_off);

Oh, I don't see any code calling this function, so why did you add it?

Who needs it?  Where is that code?

thanks,

greg k-h
--
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: [PATCH v6 7/8] usb: add usb_device_allow_power_off() and usb_device_prevent_power_off() function.

[Lan Tianyu]

On 2013/1/22 5:33, Greg KH wrote:
> On Mon, Jan 21, 2013 at 10:18:06PM +0800, Lan Tianyu wrote:
>> Some usb devices can't be resumed correctly after power off. This
>> patch is to add usb_device_allow_power_off() and usb_device_prevent_power_off()
>> for device's driver. Call pm_runtime_get_sync(portdev) to increase port's usage
>> count and then port will not be suspended. The device will not be powered off.
>>
>> Acked-by: Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org>
>> Signed-off-by: Lan Tianyu <tianyu.lan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> ---
>>   drivers/usb/core/port.c |   28 ++++++++++++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>
>> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
>> index 0c51d24..0334d91 100644
>> --- a/drivers/usb/core/port.c
>> +++ b/drivers/usb/core/port.c
>> @@ -18,11 +18,39 @@
>>
>>   #include <linux/slab.h>
>>   #include <linux/pm_qos.h>
>> +#include <linux/module.h>
>>
>>   #include "hub.h"
>>
>>   static const struct attribute_group *port_dev_group[];
>>
>> +/**
>> + * usb_device_control_power_off - Allow or prohibit power off device.
>> + * @udev: target usb device
>> + * @allow: choice of allow or prohibit
>> + *
>> + * Call pm_runtime_get/put_sync(portdev) to allow or prohibit target
>> + * usb device to be powered off in the kernel. The operations of setting
>> + * true and false should be couple. The default status is allowed.
>> + */
>> +int usb_device_control_power_off(struct usb_device *udev, bool allow)
>> +{
>> +	struct usb_port *port_dev;
>> +
>> +	if (!udev->parent)
>> +		return -EINVAL;
>> +
>> +	port_dev = hdev_to_hub(udev->parent)->ports[udev->portnum - 1];
>> +
>> +	if (allow)
>> +		pm_runtime_put_sync(&port_dev->dev);
>> +	else
>> +		pm_runtime_get_sync(&port_dev->dev);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(usb_device_control_power_off);
>
> Oh, I don't see any code calling this function, so why did you add it?
>
> Who needs it?  Where is that code?
This is provided for device driver. Some device may not be compatible 
with the power off mechanism and driver can use these function to 
forbid/allow it. But currently, we are not sure which kinds of device
they are. So just provide new interfaces.
>
> thanks,
>
> greg k-h
>


-- 
Best Regards
Tianyu Lan
linux kernel enabling team
--
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: [PATCH v6 7/8] usb: add usb_device_allow_power_off() and usb_device_prevent_power_off() function.

[Greg KH]

On Tue, Jan 22, 2013 at 10:23:12PM +0800, Lan Tianyu wrote:
> On 2013/1/22 5:33, Greg KH wrote:
> >On Mon, Jan 21, 2013 at 10:18:06PM +0800, Lan Tianyu wrote:
> >>Some usb devices can't be resumed correctly after power off. This
> >>patch is to add usb_device_allow_power_off() and usb_device_prevent_power_off()
> >>for device's driver. Call pm_runtime_get_sync(portdev) to increase port's usage
> >>count and then port will not be suspended. The device will not be powered off.
> >>
> >>Acked-by: Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org>
> >>Signed-off-by: Lan Tianyu <tianyu.lan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >>---
> >>  drivers/usb/core/port.c |   28 ++++++++++++++++++++++++++++
> >>  1 file changed, 28 insertions(+)
> >>
> >>diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
> >>index 0c51d24..0334d91 100644
> >>--- a/drivers/usb/core/port.c
> >>+++ b/drivers/usb/core/port.c
> >>@@ -18,11 +18,39 @@
> >>
> >>  #include <linux/slab.h>
> >>  #include <linux/pm_qos.h>
> >>+#include <linux/module.h>
> >>
> >>  #include "hub.h"
> >>
> >>  static const struct attribute_group *port_dev_group[];
> >>
> >>+/**
> >>+ * usb_device_control_power_off - Allow or prohibit power off device.
> >>+ * @udev: target usb device
> >>+ * @allow: choice of allow or prohibit
> >>+ *
> >>+ * Call pm_runtime_get/put_sync(portdev) to allow or prohibit target
> >>+ * usb device to be powered off in the kernel. The operations of setting
> >>+ * true and false should be couple. The default status is allowed.
> >>+ */
> >>+int usb_device_control_power_off(struct usb_device *udev, bool allow)
> >>+{
> >>+	struct usb_port *port_dev;
> >>+
> >>+	if (!udev->parent)
> >>+		return -EINVAL;
> >>+
> >>+	port_dev = hdev_to_hub(udev->parent)->ports[udev->portnum - 1];
> >>+
> >>+	if (allow)
> >>+		pm_runtime_put_sync(&port_dev->dev);
> >>+	else
> >>+		pm_runtime_get_sync(&port_dev->dev);
> >>+
> >>+	return 0;
> >>+}
> >>+EXPORT_SYMBOL_GPL(usb_device_control_power_off);
> >
> >Oh, I don't see any code calling this function, so why did you add it?
> >
> >Who needs it?  Where is that code?
> This is provided for device driver. Some device may not be
> compatible with the power off mechanism and driver can use these
> function to forbid/allow it. But currently, we are not sure which
> kinds of device
> they are. So just provide new interfaces.

We don't add new interfaces to the kernel unless they have a user.  If I
were to see this in the tree, I would expect it to be removed because of
that.  So don't add it at all, only add it when it is needed.

thanks,

greg k-h
--
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