[PATCH V4 0/8] usb: make usb port a real device and provide control of port's power to usr space

[PATCH V4 0/8] usb: make usb port a real device and provide control of port's power to usr space

[Lan Tianyu]

This patchset merged two following patchsets and make usb port a real device.
[PATCH V3 1/4] usb: add struct usb_hub_port to store
[PATCH V3 2/4] usb: move struct usb_device->children
[PATCH V3 3/4] usb/acpi: add the support of usb hub
[PATCH V3 4/4] usb/acpi: add usb check for the connection
[PATCH 1/3] xhci: Add clear PORT_POWER feature process in the
[PATCH 2/3] USB/ACPI: Add usb port's acpi power control in the xhci
[PATCH 3/3] usb : Add sysfs files to control usb port's power

This patchset is to provide usb port power off mechanism. Add two
attributes "portX/control" and "portX/state" for each ports
under the usb hub interface sysfs directory.

"portX/control" has two options
"on" - port power must be on.
"off" - port power must be off.

"portX/state" reports usb port's power state
"on" - power on
"off" - power off
"error" - can't get power state

[PATCH V4 1/8] usb: convert port_owners type from void * to struct
[PATCH V4 2/8] usb: make usb port a real device
[PATCH V4 3/8] usb: move children to struct usb_port
[PATCH V4 4/8] usb/acpi : rebinding usb with acpi since usb port
[PATCH V4 5/8] usb/acpi: add check for the connect type of usb port
[PATCH V4 6/8] xhci: Add clear PORT_POWER feature process in the
[PATCH V4 7/8] USB/ACPI: Add usb port's acpi power control in the
[PATCH V4 8/8] usb : Add usb port's power control attributes


 git diff --stat HEAD~8
 Documentation/ABI/testing/sysfs-bus-usb |   25 ++++++++++++
 drivers/staging/usbip/usbip_common.c    |    3 +-
 drivers/usb/core/devices.c              |    7 ++--
 drivers/usb/core/hub.c                  |  349 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------
 drivers/usb/core/usb-acpi.c             |  202 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------
 drivers/usb/core/usb.h                  |   25 ++++++++++--
 drivers/usb/host/r8a66597-hcd.c         |    9 +++--
 drivers/usb/host/xhci-hub.c             |   16 ++++++++
 include/linux/usb.h                     |   33 +++++++++++++++-
 9 files changed, 562 insertions(+), 107 deletions(-)

[PATCH V4 1/8] usb: convert port_owners type from void * to struct dev_state *

[Lan Tianyu]

This patch is to convert ort_owners type from void * to struct dev_state *
in order to make code more readable.

Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 drivers/usb/core/hub.c |   21 ++++++++++++---------
 drivers/usb/core/usb.h |    9 ++++++---
 2 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 25a7422..4cc8dc9 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -81,7 +81,7 @@ struct usb_hub {
 	u8			indicator[USB_MAXCHILDREN];
 	struct delayed_work	leds;
 	struct delayed_work	init_work;
-	void			**port_owners;
+	struct dev_state	**port_owners;
 };
 
 static inline int hub_is_superspeed(struct usb_device *hdev)
@@ -1271,7 +1271,8 @@ static int hub_configure(struct usb_hub *hub,
 
 	hdev->children = kzalloc(hdev->maxchild *
 				sizeof(struct usb_device *), GFP_KERNEL);
-	hub->port_owners = kzalloc(hdev->maxchild * sizeof(void *), GFP_KERNEL);
+	hub->port_owners = kzalloc(hdev->maxchild * sizeof(struct dev_state *),
+				GFP_KERNEL);
 	if (!hdev->children || !hub->port_owners) {
 		ret = -ENOMEM;
 		goto fail;
@@ -1649,7 +1650,7 @@ hub_ioctl(struct usb_interface *intf, unsigned int code, void *user_data)
  * to one of these "claimed" ports, the program will "own" the device.
  */
 static int find_port_owner(struct usb_device *hdev, unsigned port1,
-		void ***ppowner)
+		struct dev_state ***ppowner)
 {
 	if (hdev->state == USB_STATE_NOTATTACHED)
 		return -ENODEV;
@@ -1664,10 +1665,11 @@ static int find_port_owner(struct usb_device *hdev, unsigned port1,
 }
 
 /* In the following three functions, the caller must hold hdev's lock */
-int usb_hub_claim_port(struct usb_device *hdev, unsigned port1, void *owner)
+int usb_hub_claim_port(struct usb_device *hdev, unsigned port1,
+		       struct dev_state *owner)
 {
 	int rc;
-	void **powner;
+	struct dev_state **powner;
 
 	rc = find_port_owner(hdev, port1, &powner);
 	if (rc)
@@ -1678,10 +1680,11 @@ int usb_hub_claim_port(struct usb_device *hdev, unsigned port1, void *owner)
 	return rc;
 }
 
-int usb_hub_release_port(struct usb_device *hdev, unsigned port1, void *owner)
+int usb_hub_release_port(struct usb_device *hdev, unsigned port1,
+			 struct dev_state *owner)
 {
 	int rc;
-	void **powner;
+	struct dev_state **powner;
 
 	rc = find_port_owner(hdev, port1, &powner);
 	if (rc)
@@ -1692,10 +1695,10 @@ int usb_hub_release_port(struct usb_device *hdev, unsigned port1, void *owner)
 	return rc;
 }
 
-void usb_hub_release_all_ports(struct usb_device *hdev, void *owner)
+void usb_hub_release_all_ports(struct usb_device *hdev, struct dev_state *owner)
 {
 	int n;
-	void **powner;
+	struct dev_state **powner;
 
 	n = find_port_owner(hdev, 1, &powner);
 	if (n == 0) {
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index 5c5c538..67875a8 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -1,5 +1,7 @@
 #include <linux/pm.h>
 
+struct dev_state;
+
 /* Functions local to drivers/usb/core/ */
 
 extern int usb_create_sysfs_dev_files(struct usb_device *dev);
@@ -41,10 +43,11 @@ extern void usb_forced_unbind_intf(struct usb_interface *intf);
 extern void usb_rebind_intf(struct usb_interface *intf);
 
 extern int usb_hub_claim_port(struct usb_device *hdev, unsigned port,
-		void *owner);
+		struct dev_state *owner);
 extern int usb_hub_release_port(struct usb_device *hdev, unsigned port,
-		void *owner);
-extern void usb_hub_release_all_ports(struct usb_device *hdev, void *owner);
+		struct dev_state *owner);
+extern void usb_hub_release_all_ports(struct usb_device *hdev,
+		struct dev_state *owner);
 extern bool usb_device_is_owned(struct usb_device *udev);
 
 extern int  usb_hub_init(void);
-- 
1.7.6.rc2.8.g28eb

[PATCH V4 2/8] usb: make usb port a real device

[Lan Tianyu]

This patch is to make usb port a real device under usb hub interface.
Move port_owner to struct usb_port.

Signed-off-by: Lan Tianyu <tianyu.lan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/usb/core/hub.c |   93 +++++++++++++++++++++++++++++++++++++++--------
 1 files changed, 77 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 4cc8dc9..c19e088 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -37,6 +37,12 @@
 #endif
 #endif
 
+struct usb_port {
+	struct usb_device *udev;
+	struct device dev;
+	struct dev_state *port_owner;
+};
+
 struct usb_hub {
 	struct device		*intfdev;	/* the "interface" device */
 	struct usb_device	*hdev;
@@ -81,7 +87,11 @@ struct usb_hub {
 	u8			indicator[USB_MAXCHILDREN];
 	struct delayed_work	leds;
 	struct delayed_work	init_work;
-	struct dev_state	**port_owners;
+	struct usb_port		**ports;
+};
+
+struct device_type usb_port_device_type = {
+	.name =		"usb_port",
 };
 
 static inline int hub_is_superspeed(struct usb_device *hdev)
@@ -154,6 +164,8 @@ 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);
 
@@ -1220,6 +1232,49 @@ 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);
+
+	kfree(port_dev);
+}
+
+static void usb_hub_remove_port_device(struct usb_hub *hub,
+				       int port1)
+{
+	device_unregister(&hub->ports[port1 - 1]->dev);
+}
+
+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->udev = hub->hdev;
+	port_dev->dev.parent = hub->intfdev;
+	port_dev->dev.type = &usb_port_device_type;
+	port_dev->dev.release = usb_port_device_release;
+	dev_set_name(&port_dev->dev, "port%d", port1);
+
+	retval = device_register(&port_dev->dev);
+	if (retval)
+		goto error_register;
+	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)
 {
@@ -1229,7 +1284,7 @@ static int hub_configure(struct usb_hub *hub,
 	u16 hubstatus, hubchange;
 	u16 wHubCharacteristics;
 	unsigned int pipe;
-	int maxp, ret;
+	int maxp, ret, i;
 	char *message = "out of memory";
 
 	hub->buffer = kmalloc(sizeof(*hub->buffer), GFP_KERNEL);
@@ -1271,9 +1326,9 @@ static int hub_configure(struct usb_hub *hub,
 
 	hdev->children = kzalloc(hdev->maxchild *
 				sizeof(struct usb_device *), GFP_KERNEL);
-	hub->port_owners = kzalloc(hdev->maxchild * sizeof(struct dev_state *),
-				GFP_KERNEL);
-	if (!hdev->children || !hub->port_owners) {
+	hub->ports = kzalloc(hdev->maxchild * sizeof(struct usb_port *),
+			     GFP_KERNEL);
+	if (!hdev->children || !hub->ports) {
 		ret = -ENOMEM;
 		goto fail;
 	}
@@ -1482,6 +1537,11 @@ 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)
+			dev_err(hub->intfdev, "couldn't create port%d "
+				"device.\n", i + 1);
+
 	hub_activate(hub, HUB_INIT);
 	return 0;
 
@@ -1506,6 +1566,10 @@ static void hub_disconnect(struct usb_interface *intf)
 {
 	struct usb_hub *hub = usb_get_intfdata(intf);
 	struct usb_device *hdev = interface_to_usbdev(intf);
+	int i;
+
+	for (i = 0; i < hdev->maxchild; i++)
+		usb_hub_remove_port_device(hub, i + 1);
 
 	/* Take the hub off the event list and don't let it be added again */
 	spin_lock_irq(&hub_event_lock);
@@ -1528,7 +1592,7 @@ static void hub_disconnect(struct usb_interface *intf)
 
 	usb_free_urb(hub->urb);
 	kfree(hdev->children);
-	kfree(hub->port_owners);
+	kfree(hub->ports);
 	kfree(hub->descriptor);
 	kfree(hub->status);
 	kfree(hub->buffer);
@@ -1660,7 +1724,7 @@ static int find_port_owner(struct usb_device *hdev, unsigned port1,
 	/* This assumes that devices not managed by the hub driver
 	 * will always have maxchild equal to 0.
 	 */
-	*ppowner = &(hdev_to_hub(hdev)->port_owners[port1 - 1]);
+	*ppowner = &(hdev_to_hub(hdev)->ports[port1 - 1]->port_owner);
 	return 0;
 }
 
@@ -1697,16 +1761,14 @@ int usb_hub_release_port(struct usb_device *hdev, unsigned port1,
 
 void usb_hub_release_all_ports(struct usb_device *hdev, struct dev_state *owner)
 {
+	struct usb_hub *hub = hdev_to_hub(hdev);
 	int n;
-	struct dev_state **powner;
 
-	n = find_port_owner(hdev, 1, &powner);
-	if (n == 0) {
-		for (; n < hdev->maxchild; (++n, ++powner)) {
-			if (*powner == owner)
-				*powner = NULL;
-		}
+	for (n = 0; n < hdev->maxchild; n++) {
+		if (hub->ports[n]->port_owner == owner)
+			hub->ports[n]->port_owner = NULL;
 	}
+
 }
 
 /* The caller must hold udev's lock */
@@ -1717,10 +1779,9 @@ bool usb_device_is_owned(struct usb_device *udev)
 	if (udev->state == USB_STATE_NOTATTACHED || !udev->parent)
 		return false;
 	hub = hdev_to_hub(udev->parent);
-	return !!hub->port_owners[udev->portnum - 1];
+	return !!hub->ports[udev->portnum - 1]->port_owner;
 }
 

[PATCH V4 3/8] usb: move children to struct usb_port

[Lan Tianyu]

Move child's pointer to the struct usb_port since the child device
is directly associated with the port. Provide usb_get_hub_child()
to get child's pointer and macrio macro usb_get_hub_each_child to
iterate all child devices on the hub.

Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 drivers/staging/usbip/usbip_common.c |    3 +-
 drivers/usb/core/devices.c           |    7 +--
 drivers/usb/core/hub.c               |   71 +++++++++++++++++++++++-----------
 drivers/usb/host/r8a66597-hcd.c      |    9 ++--
 include/linux/usb.h                  |   16 +++++++-
 5 files changed, 71 insertions(+), 35 deletions(-)

diff --git a/drivers/staging/usbip/usbip_common.c b/drivers/staging/usbip/usbip_common.c
index 70f23026..95beb76 100644
--- a/drivers/staging/usbip/usbip_common.c
+++ b/drivers/staging/usbip/usbip_common.c
@@ -157,8 +157,7 @@ static void usbip_dump_usb_device(struct usb_device *udev)
 	dev_dbg(dev, "have_langid %d, string_langid %d\n",
 		udev->have_langid, udev->string_langid);
 
-	dev_dbg(dev, "maxchild %d, children %p\n",
-		udev->maxchild, udev->children);
+	dev_dbg(dev, "maxchild %d\n", udev->maxchild);
 }
 
 static void usbip_dump_request_type(__u8 rt)
diff --git a/drivers/usb/core/devices.c b/drivers/usb/core/devices.c
index d956965..04ba6e3 100644
--- a/drivers/usb/core/devices.c
+++ b/drivers/usb/core/devices.c
@@ -496,6 +496,7 @@ static ssize_t usb_device_dump(char __user **buffer, size_t *nbytes,
 	char *pages_start, *data_end, *speed;
 	unsigned int length;
 	ssize_t total_written = 0;
+	struct usb_device *childdev = NULL;
 
 	/* don't bother with anything else if we're not writing any data */
 	if (*nbytes <= 0)
@@ -589,14 +590,12 @@ static ssize_t usb_device_dump(char __user **buffer, size_t *nbytes,
 	free_pages((unsigned long)pages_start, 1);
 
 	/* Now look at all of this device's children. */
-	for (chix = 0; chix < usbdev->maxchild; chix++) {
-		struct usb_device *childdev = usbdev->children[chix];
-
+	usb_get_hub_each_child(usbdev, chix, childdev) {
 		if (childdev) {
 			usb_lock_device(childdev);
 			ret = usb_device_dump(buffer, nbytes, skip_bytes,
 					      file_offset, childdev, bus,
-					      level + 1, chix, ++cnt);
+					      level + 1, chix - 1, ++cnt);
 			usb_unlock_device(childdev);
 			if (ret == -EFAULT)
 				return total_written;
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index c19e088..01655d4 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -39,6 +39,7 @@
 
 struct usb_port {
 	struct usb_device *udev;
+	struct usb_device *child;
 	struct device dev;
 	struct dev_state *port_owner;
 };
@@ -879,8 +880,8 @@ static int hub_port_disable(struct usb_hub *hub, int port1, int set_state)
 	struct usb_device *hdev = hub->hdev;
 	int ret = 0;
 
-	if (hdev->children[port1-1] && set_state)
-		usb_set_device_state(hdev->children[port1-1],
+	if (hub->ports[port1 - 1]->child && set_state)
+		usb_set_device_state(hub->ports[port1 - 1]->child,
 				USB_STATE_NOTATTACHED);
 	if (!hub->error && !hub_is_superspeed(hub->hdev))
 		ret = clear_port_feature(hdev, port1, USB_PORT_FEAT_ENABLE);
@@ -1036,7 +1037,7 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
 	 * which ports need attention.
 	 */
 	for (port1 = 1; port1 <= hdev->maxchild; ++port1) {
-		struct usb_device *udev = hdev->children[port1-1];
+		struct usb_device *udev = hub->ports[port1 - 1]->child;
 		u16 portstatus, portchange;
 
 		portstatus = portchange = 0;
@@ -1201,8 +1202,8 @@ static void hub_quiesce(struct usb_hub *hub, enum hub_quiescing_type type)
 	if (type != HUB_SUSPEND) {
 		/* Disconnect all the children */
 		for (i = 0; i < hdev->maxchild; ++i) {
-			if (hdev->children[i])
-				usb_disconnect(&hdev->children[i]);
+			if (hub->ports[i]->child)
+				usb_disconnect(&hub->ports[i]->child);
 		}
 	}
 
@@ -1324,11 +1325,9 @@ static int hub_configure(struct usb_hub *hub,
 	dev_info (hub_dev, "%d port%s detected\n", hdev->maxchild,
 		(hdev->maxchild == 1) ? "" : "s");
 
-	hdev->children = kzalloc(hdev->maxchild *
-				sizeof(struct usb_device *), GFP_KERNEL);
 	hub->ports = kzalloc(hdev->maxchild * sizeof(struct usb_port *),
 			     GFP_KERNEL);
-	if (!hdev->children || !hub->ports) {
+	if (!hub->ports) {
 		ret = -ENOMEM;
 		goto fail;
 	}
@@ -1591,7 +1590,6 @@ static void hub_disconnect(struct usb_interface *intf)
 		highspeed_hubs--;
 
 	usb_free_urb(hub->urb);
-	kfree(hdev->children);
 	kfree(hub->ports);
 	kfree(hub->descriptor);
 	kfree(hub->status);
@@ -1679,6 +1677,7 @@ static int
 hub_ioctl(struct usb_interface *intf, unsigned int code, void *user_data)
 {
 	struct usb_device *hdev = interface_to_usbdev (intf);
+	struct usb_hub *hub = hdev_to_hub(hdev);
 
 	/* assert ifno == 0 (part of hub spec) */
 	switch (code) {
@@ -1692,11 +1691,11 @@ hub_ioctl(struct usb_interface *intf, unsigned int code, void *user_data)
 		else {
 			info->nports = hdev->maxchild;
 			for (i = 0; i < info->nports; i++) {
-				if (hdev->children[i] == NULL)
+				if (hub->ports[i]->child == NULL)
 					info->port[i] = 0;
 				else
 					info->port[i] =
-						hdev->children[i]->devnum;
+						hub->ports[i]->child->devnum;
 			}
 		}
 		spin_unlock_irq(&device_state_lock);
@@ -1784,11 +1783,12 @@ bool usb_device_is_owned(struct usb_device *udev)
 
 static void recursively_mark_NOTATTACHED(struct usb_device *udev)
 {
+	struct usb_hub *hub = hdev_to_hub(udev);
 	int i;
 
 	for (i = 0; i < udev->maxchild; ++i) {
-		if (udev->children[i])
-			recursively_mark_NOTATTACHED(udev->children[i]);
+		if (hub->ports[i]->child)
+			recursively_mark_NOTATTACHED(hub->ports[i]->child);
 	}
 	if (udev->state == USB_STATE_SUSPENDED)
 		udev->active_duration -= jiffies;
@@ -1952,6 +1952,7 @@ static void hub_free_dev(struct usb_device *udev)
 void usb_disconnect(struct usb_device **pdev)
 {
 	struct usb_device	*udev = *pdev;
+	struct usb_hub		*hub = hdev_to_hub(udev);
 	int			i;
 
 	/* mark the device as inactive, so any further urb submissions for
@@ -1966,8 +1967,8 @@ void usb_disconnect(struct usb_device **pdev)
 
 	/* Free up all the children before we remove this device */
 	for (i = 0; i < udev->maxchild; i++) {
-		if (udev->children[i])
-			usb_disconnect(&udev->children[i]);
+		if (hub->ports[i]->child)
+			usb_disconnect(&hub->ports[i]->child);
 	}
 
 	/* deallocate hcd/hardware state ... nuking all pending urbs and
@@ -3064,7 +3065,7 @@ static int hub_suspend(struct usb_interface *intf, pm_message_t msg)
 	for (port1 = 1; port1 <= hdev->maxchild; port1++) {
 		struct usb_device	*udev;
 
-		udev = hdev->children [port1-1];
+		udev = hub->ports[port1 - 1]->child;
 		if (udev && udev->can_submit) {
 			dev_warn(&intf->dev, "port %d nyet suspended\n", port1);
 			if (PMSG_IS_AUTO(msg))
@@ -3982,7 +3983,7 @@ hub_power_remaining (struct usb_hub *hub)
 
 	remaining = hdev->bus_mA - hub->descriptor->bHubContrCurrent;
 	for (port1 = 1; port1 <= hdev->maxchild; ++port1) {
-		struct usb_device	*udev = hdev->children[port1 - 1];
+		struct usb_device	*udev = hub->ports[port1 - 1]->child;
 		int			delta;
 
 		if (!udev)
@@ -4046,7 +4047,7 @@ static void hub_port_connect_change(struct usb_hub *hub, int port1,
 #endif
 
 	/* Try to resuscitate an existing device */
-	udev = hdev->children[port1-1];
+	udev = hub->ports[port1 - 1]->child;
 	if ((portstatus & USB_PORT_STAT_CONNECTION) && udev &&
 			udev->state != USB_STATE_NOTATTACHED) {
 		usb_lock_device(udev);
@@ -4075,7 +4076,7 @@ static void hub_port_connect_change(struct usb_hub *hub, int port1,
 
 	/* Disconnect any existing devices under this port */
 	if (udev)
-		usb_disconnect(&hdev->children[port1-1]);
+		usb_disconnect(&hub->ports[port1 - 1]->child);
 	clear_bit(port1, hub->change_bits);
 
 	/* We can forget about a "removed" device when there's a physical
@@ -4204,7 +4205,7 @@ static void hub_port_connect_change(struct usb_hub *hub, int port1,
 		if (hdev->state == USB_STATE_NOTATTACHED)
 			status = -ENOTCONN;
 		else
-			hdev->children[port1-1] = udev;
+			hub->ports[port1 - 1]->child = udev;
 		spin_unlock_irq(&device_state_lock);
 
 		/* Run it through the hoops (find a driver, etc) */
@@ -4212,7 +4213,7 @@ static void hub_port_connect_change(struct usb_hub *hub, int port1,
 			status = usb_new_device(udev);
 			if (status) {
 				spin_lock_irq(&device_state_lock);
-				hdev->children[port1-1] = NULL;
+				hub->ports[port1 - 1]->child = NULL;
 				spin_unlock_irq(&device_state_lock);
 			}
 		}
@@ -4258,7 +4259,7 @@ static int hub_handle_remote_wakeup(struct usb_hub *hub, unsigned int port,
 	int ret;
 
 	hdev = hub->hdev;
-	udev = hdev->children[port-1];
+	udev = hub->ports[port - 1]->child;
 	if (!hub_is_superspeed(hdev)) {
 		if (!(portchange & USB_PORT_STAT_C_SUSPEND))
 			return 0;
@@ -4412,7 +4413,7 @@ static void hub_events(void)
 				 */
 				if (!(portstatus & USB_PORT_STAT_ENABLE)
 				    && !connect_change
-				    && hdev->children[i-1]) {
+				    && hub->ports[i - 1]->child) {
 					dev_err (hub_dev,
 					    "port %i "
 					    "disabled by hub (EMI?), "
@@ -4965,3 +4966,27 @@ void usb_queue_reset_device(struct usb_interface *iface)
 	schedule_work(&iface->reset_ws);
 }
 EXPORT_SYMBOL_GPL(usb_queue_reset_device);
+
+/**
+ * usb_get_hub_child - Get the pointer of child device
+ * attached to the port which is specified by @port1.
+ * @hdev: USB device belonging to the usb hub
+ * @port1: port num to indicate which port the child device
+ *	is attached to.
+ *
+ * USB drivers call this function to get hub's child device
+ * pointer.
+ *
+ * Return NULL if input param is invalid and
+ * child's usb_device pointer if non-NULL.
+ */
+struct usb_device *usb_get_hub_child(struct usb_device *hdev,
+		int port1)
+{
+	struct usb_hub *hub = hdev_to_hub(hdev);
+
+	if (port1 < 1 || port1 > hdev->maxchild)
+		return NULL;
+	return usb_get_dev(hub->ports[port1 - 1]->child);
+}
+EXPORT_SYMBOL_GPL(usb_get_hub_child);
diff --git a/drivers/usb/host/r8a66597-hcd.c b/drivers/usb/host/r8a66597-hcd.c
index c868be6..9a5a824 100644
--- a/drivers/usb/host/r8a66597-hcd.c
+++ b/drivers/usb/host/r8a66597-hcd.c
@@ -2033,17 +2033,18 @@ static int r8a66597_get_frame(struct usb_hcd *hcd)
 static void collect_usb_address_map(struct usb_device *udev, unsigned long *map)
 {
 	int chix;
+	struct usb_device *childdev;
 
 	if (udev->state == USB_STATE_CONFIGURED &&
 	    udev->parent && udev->parent->devnum > 1 &&
 	    udev->parent->descriptor.bDeviceClass == USB_CLASS_HUB)
 		map[udev->devnum/32] |= (1 << (udev->devnum % 32));
 
-	for (chix = 0; chix < udev->maxchild; chix++) {
-		struct usb_device *childdev = udev->children[chix];
-
-		if (childdev)
+	usb_get_hub_each_child(udev, chix, childdev) {
+		if (childdev) {
+			usb_put_dev(childdev);
 			collect_usb_address_map(childdev, map);
+		}
 	}
 }
 
diff --git a/include/linux/usb.h b/include/linux/usb.h
index f717fbd..f034fb7 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -463,7 +463,6 @@ struct usb3_lpm_parameters {
  *	access from userspace
  * @usbfs_dentry: usbfs dentry entry for the device
  * @maxchild: number of ports if hub
- * @children: child devices - USB devices that are attached to this hub
  * @quirks: quirks of the whole device
  * @urbnum: number of URBs submitted for the whole device
  * @active_duration: total time device is not suspended
@@ -537,7 +536,6 @@ struct usb_device {
 	struct list_head filelist;
 
 	int maxchild;
-	struct usb_device **children;
 
 	u32 quirks;
 	atomic_t urbnum;
@@ -567,6 +565,20 @@ static inline struct usb_device *interface_to_usbdev(struct usb_interface *intf)
 
 extern struct usb_device *usb_get_dev(struct usb_device *dev);
 extern void usb_put_dev(struct usb_device *dev);
+extern struct usb_device *usb_get_hub_child(struct usb_device *hdev,
+	int port1);
+
+/**
+ * usb_get_hub_each_child - iterate over all child devices on the hub
+ * @hdev:  USB device belonging to the usb hub
+ * @port1: portnum associated with child device
+ * @child: child device pointer
+ *
+ */
+#define usb_get_hub_each_child(hdev, port1, child) \
+	for (port1 = 1,	child =	usb_get_hub_child(hdev, port1); \
+		port1 <= hdev->maxchild; \
+		child = usb_get_hub_child(hdev, ++port1))
 
 /* USB device locking */
 #define usb_lock_device(udev)		device_lock(&(udev)->dev)
-- 
1.7.6.rc2.8.g28eb

[PATCH V4 4/8] usb/acpi : rebinding usb with acpi since usb port being made a real device

[Lan Tianyu]

In the ACPI DSDT table, only usb root hub and usb ports are acpi device nodes.
Original, binding the usb ports' acpi node with the usb device attached to the
port. The usb port is made a real device by previous patch. This patch is to
rebind the port's acpi node with port's struct device.

Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 drivers/usb/core/hub.c      |   18 +++++++++++
 drivers/usb/core/usb-acpi.c |   67 +++++++++++++++++++++++++++++++++---------
 drivers/usb/core/usb.h      |   12 ++++++++
 3 files changed, 82 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 01655d4..f2ad4c5 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -4990,3 +4990,21 @@ struct usb_device *usb_get_hub_child(struct usb_device *hdev,
 	return usb_get_dev(hub->ports[port1 - 1]->child);
 }
 EXPORT_SYMBOL_GPL(usb_get_hub_child);
+
+#ifdef CONFIG_ACPI
+/**
+ * usb_get_hub_port_acpi_handle - Get the usb port's acpi handle
+ * @hdev: USB device belonging to the usb hub
+ * @port1: port num of the port
+ *
+ * Return port's acpi handle if successful, NULL if params are
+ * invaild.
+ */
+acpi_handle usb_get_hub_port_acpi_handle(struct usb_device *hdev,
+	int port1)
+{
+	struct usb_hub *hub = hdev_to_hub(hdev);
+
+	return DEVICE_ACPI_HANDLE(&hub->ports[port1 - 1]->dev);
+}
+#endif
diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
index 8947b20..47197bf 100644
--- a/drivers/usb/core/usb-acpi.c
+++ b/drivers/usb/core/usb-acpi.c
@@ -70,22 +70,59 @@ static int usb_acpi_check_pld(struct usb_device *udev, acpi_handle handle)
 static int usb_acpi_find_device(struct device *dev, acpi_handle *handle)
 {
 	struct usb_device *udev;
-	struct device *parent;
 	acpi_handle *parent_handle;
+	int port_num;
 
-	if (!is_usb_device(dev))
-		return -ENODEV;
-
-	udev = to_usb_device(dev);
-	parent = dev->parent;
-	parent_handle = DEVICE_ACPI_HANDLE(parent);
-
-	if (!parent_handle)
-		return -ENODEV;
-
-	*handle = acpi_get_child(parent_handle, udev->portnum);
-
-	if (!*handle)
+	/*
+	 * In the ACPI DSDT table, only usb root hub and usb ports are
+	 * acpi device nodes. The hierarchy like following.
+	 * Device (EHC1)
+	 *	Device (HUBN)
+	 *		Device (PR01)
+	 *			Device (PR11)
+	 *			Device (PR12)
+	 *			Device (PR13)
+	 *			...
+	 * So all binding process is divided into two parts. binding
+	 * root hub and usb ports.
+	 */
+	if (is_usb_device(dev)) {
+		udev = to_usb_device(dev);
+		if (udev->parent)
+			return -ENODEV;
+		/* root hub's parent is the usb hcd. */
+		parent_handle = DEVICE_ACPI_HANDLE(dev->parent);
+		*handle = acpi_get_child(parent_handle, udev->portnum);
+		if (!*handle)
+			return -ENODEV;
+		return 0;
+	} else if (is_usb_port(dev)) {
+		sscanf(dev_name(dev), "port%d", &port_num);
+		/* Get the struct usb_device point of port's hub */
+		udev = to_usb_device(dev->parent->parent);
+
+		/*
+		 * The root hub ports' parent is the root hub. The non-root-hub
+		 * ports' parent is the parent hub port which the hub is
+		 * connected to.
+		 */
+		if (!udev->parent) {
+			*handle = acpi_get_child(DEVICE_ACPI_HANDLE(&udev->dev),
+				port_num);
+			if (!*handle)
+				return -ENODEV;
+		} else {
+			parent_handle =
+				usb_get_hub_port_acpi_handle(udev->parent,
+				udev->portnum);
+			if (!parent_handle)
+				return -ENODEV;
+
+			*handle = acpi_get_child(parent_handle,	port_num);
+			if (!*handle)
+				return -ENODEV;
+		}
+	} else
 		return -ENODEV;
 
 	/*
@@ -102,7 +139,7 @@ static int usb_acpi_find_device(struct device *dev, acpi_handle *handle)
 
 static struct acpi_bus_type usb_acpi_bus = {
 	.bus = &usb_bus_type,
-	.find_bridge = NULL,
+	.find_bridge = usb_acpi_find_device,
 	.find_device = usb_acpi_find_device,
 };
 
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index 67875a8..4aa20f4 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -1,5 +1,9 @@
 #include <linux/pm.h>
 
+#ifdef CONFIG_ACPI
+#include <linux/acpi.h>
+#endif
+
 struct dev_state;
 
 /* Functions local to drivers/usb/core/ */
@@ -111,6 +115,7 @@ extern struct bus_type usb_bus_type;
 extern struct device_type usb_device_type;
 extern struct device_type usb_if_device_type;
 extern struct device_type usb_ep_device_type;
+extern struct device_type usb_port_device_type;
 extern struct usb_device_driver usb_generic_driver;
 
 static inline int is_usb_device(const struct device *dev)
@@ -128,6 +133,11 @@ static inline int is_usb_endpoint(const struct device *dev)
 	return dev->type == &usb_ep_device_type;
 }
 
+static inline int is_usb_port(const struct device *dev)
+{
+	return dev->type == &usb_port_device_type;
+}
+
 /* Do the same for device drivers and interface drivers. */
 
 static inline int is_usb_device_driver(struct device_driver *drv)
@@ -162,6 +172,8 @@ extern void usb_notify_remove_bus(struct usb_bus *ubus);
 #ifdef CONFIG_ACPI
 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);
 #else
 static inline int usb_acpi_register(void) { return 0; };
 static inline void usb_acpi_unregister(void) { };
-- 
1.7.6.rc2.8.g28eb

[PATCH V4 5/8] usb/acpi: add check for the connect type of usb port

[Lan Tianyu]

Change since v3: Rebase on the patch "make usb port a real device"

Check the connect type of usb port when getting the usb port's
acpi_handle and store result into connect_type in the struct
usb_port.

Accoding to ACPI Spec 9.13. PLD indicates whether usb port is
user visible and _UPC indicates whether it is connectable. If
the port was visible and connectable, it could be freely connected
and disconnected with USB devices. If no visible and connectable,
a usb device is directly hard-wired to the port. If no visible and
no connectable, the port would be not used.

When a device was found on the port, if the connect_type was hot-plug,
then the device would be removable. If the connect_type was hard-wired,
the device would be non-removable.

Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 drivers/usb/core/hub.c      |   40 +++++++++++++++++++++
 drivers/usb/core/usb-acpi.c |   80 ++++++++++++++++++++++++-------------------
 drivers/usb/core/usb.h      |    4 ++
 include/linux/usb.h         |    7 ++++
 4 files changed, 96 insertions(+), 35 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index f2ad4c5..05cdc7b 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -42,6 +42,7 @@ 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 {
@@ -4991,6 +4992,45 @@ struct usb_device *usb_get_hub_child(struct usb_device *hdev,
 }
 EXPORT_SYMBOL_GPL(usb_get_hub_child);
 
+/**
+ * usb_set_hub_port_connect_type - set hub port connect type.
+ * @hdev: USB device belonging to the usb hub
+ * @port1: port num of the port
+ * @type: connect type of the port
+ *		USB_PORT_CONNECT_TYPE_UNKNOWN
+ *		USB_PORT_CONNECT_TYPE_HOT_PLUG
+ *		USB_PORT_CONNECT_TYPE_HARD_WIRED
+ *		USB_PORT_NOT_USED
+ */
+void usb_set_hub_port_connect_type(struct usb_device *hdev, int port1,
+	enum usb_port_connect_type type)
+{
+	struct usb_hub *hub = hdev_to_hub(hdev);
+
+	hub->ports[port1 - 1]->connect_type = type;
+}
+
+/**
+ * usb_get_hub_port_connect_type - Get the port's connect
+ * type
+ * @hdev: USB device belonging to the usb hub
+ * @port1: port num of the port
+ *
+ * Return connect type of the port annd if input params are
+ *	invalid, return USB_PORT_CONNECT_TYPE_UNKNOWN.
+ *		USB_PORT_CONNECT_TYPE_UNKNOWN
+ *		USB_PORT_CONNECT_TYPE_HOT_PLUG
+ *		USB_PORT_CONNECT_TYPE_HARD_WIRED
+ *		USB_PORT_NOT_USED
+ */
+enum usb_port_connect_type
+usb_get_hub_port_connect_type(struct usb_device *hdev,	int port1)
+{
+	struct usb_hub *hub = hdev_to_hub(hdev);
+
+	return hub->ports[port1 - 1]->connect_type;
+}
+
 #ifdef CONFIG_ACPI
 /**
  * usb_get_hub_port_acpi_handle - Get the usb port's acpi handle
diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
index 47197bf..404d86a 100644
--- a/drivers/usb/core/usb-acpi.c
+++ b/drivers/usb/core/usb-acpi.c
@@ -19,20 +19,29 @@
 
 #include "usb.h"
 
-static int usb_acpi_check_upc(struct usb_device *udev, acpi_handle handle)
+static int usb_acpi_check_port_connect_type(struct usb_device *hdev,
+	acpi_handle handle, int port1)
 {
 	acpi_status status;
 	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
 	union acpi_object *upc;
+	struct acpi_pld pld;
 	int ret = 0;
 
-	status = acpi_evaluate_object(handle, "_UPC", NULL, &buffer);
-
+	/*
+	 * Accoding to ACPI Spec 9.13. PLD indicates whether usb port is
+	 * user visible and _UPC indicates whether it is connectable. If
+	 * the port was visible and connectable, it could be freely connected
+	 * and disconnected with USB devices. If no visible and connectable,
+	 * a usb device is directly hard-wired to the port. If no visible and
+	 * no connectable, the port would be not used.
+	 */
+	status = acpi_get_physical_device_location(handle, &pld);
 	if (ACPI_FAILURE(status))
 		return -ENODEV;
 
+	status = acpi_evaluate_object(handle, "_UPC", NULL, &buffer);
 	upc = buffer.pointer;
-
 	if (!upc || (upc->type != ACPI_TYPE_PACKAGE)
 		|| upc->package.count != 4) {
 		ret = -EINVAL;
@@ -40,33 +49,20 @@ static int usb_acpi_check_upc(struct usb_device *udev, acpi_handle handle)
 	}
 
 	if (upc->package.elements[0].integer.value)
-		udev->removable = USB_DEVICE_REMOVABLE;
-	else
-		udev->removable = USB_DEVICE_FIXED;
+		if (pld.user_visible)
+			usb_set_hub_port_connect_type(hdev, port1,
+				USB_PORT_CONNECT_TYPE_HOT_PLUG);
+		else
+			usb_set_hub_port_connect_type(hdev, port1,
+				USB_PORT_CONNECT_TYPE_HARD_WIRED);
+	else if (!pld.user_visible)
+		usb_set_hub_port_connect_type(hdev, port1, USB_PORT_NOT_USED);
 
 out:
 	kfree(upc);
 	return ret;
 }
 
-static int usb_acpi_check_pld(struct usb_device *udev, acpi_handle handle)
-{
-	acpi_status status;
-	struct acpi_pld pld;
-
-	status = acpi_get_physical_device_location(handle, &pld);
-
-	if (ACPI_FAILURE(status))
-		return -ENODEV;
-
-	if (pld.user_visible)
-		udev->removable = USB_DEVICE_REMOVABLE;
-	else
-		udev->removable = USB_DEVICE_FIXED;
-
-	return 0;
-}
-
 static int usb_acpi_find_device(struct device *dev, acpi_handle *handle)
 {
 	struct usb_device *udev;
@@ -88,8 +84,30 @@ static int usb_acpi_find_device(struct device *dev, acpi_handle *handle)
 	 */
 	if (is_usb_device(dev)) {
 		udev = to_usb_device(dev);
-		if (udev->parent)
+		if (udev->parent) {
+			enum usb_port_connect_type type;
+
+			/*
+			 * According usb port's connect type to set usb device's
+			 * removability.
+			 */
+			type = usb_get_hub_port_connect_type(udev->parent,
+				udev->portnum);
+			switch (type) {
+			case USB_PORT_CONNECT_TYPE_HOT_PLUG:
+				udev->removable = USB_DEVICE_REMOVABLE;
+				break;
+			case USB_PORT_CONNECT_TYPE_HARD_WIRED:
+				udev->removable = USB_DEVICE_FIXED;
+				break;
+			default:
+				udev->removable = USB_DEVICE_REMOVABLE_UNKNOWN;
+				break;
+			}
+
 			return -ENODEV;
+		}
+
 		/* root hub's parent is the usb hcd. */
 		parent_handle = DEVICE_ACPI_HANDLE(dev->parent);
 		*handle = acpi_get_child(parent_handle, udev->portnum);
@@ -122,18 +140,10 @@ static int usb_acpi_find_device(struct device *dev, acpi_handle *handle)
 			if (!*handle)
 				return -ENODEV;
 		}
+		usb_acpi_check_port_connect_type(udev, *handle, port_num);
 	} else
 		return -ENODEV;
 
-	/*
-	 * PLD will tell us whether a port is removable to the user or
-	 * not. If we don't get an answer from PLD (it's not present
-	 * or it's malformed) then try to infer it from UPC. If a
-	 * device isn't connectable then it's probably not removable.
-	 */
-	if (usb_acpi_check_pld(udev, *handle) != 0)
-		usb_acpi_check_upc(udev, *handle);
-
 	return 0;
 }
 
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index 4aa20f4..bbd7df4 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -168,6 +168,10 @@ extern void usb_notify_add_device(struct usb_device *udev);
 extern void usb_notify_remove_device(struct usb_device *udev);
 extern void usb_notify_add_bus(struct usb_bus *ubus);
 extern void usb_notify_remove_bus(struct usb_bus *ubus);
+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);
 
 #ifdef CONFIG_ACPI
 extern int usb_acpi_register(void);
diff --git a/include/linux/usb.h b/include/linux/usb.h
index f034fb7..82e9985 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -378,6 +378,13 @@ enum usb_device_removable {
 	USB_DEVICE_FIXED,
 };
 
+enum usb_port_connect_type {
+	USB_PORT_CONNECT_TYPE_UNKNOWN = 0,
+	USB_PORT_CONNECT_TYPE_HOT_PLUG,
+	USB_PORT_CONNECT_TYPE_HARD_WIRED,
+	USB_PORT_NOT_USED,
+};
+
 /*
  * USB 3.0 Link Power Management (LPM) parameters.
  *
-- 
1.7.6.rc2.8.g28eb

[PATCH V4 6/8] xhci: Add clear PORT_POWER feature process in the xhci_hub_control()

[Lan Tianyu]

This patch is to add process of clearing PORT_POWER feature request
for xhci hub.

Signed-off-by: Lan Tianyu <tianyu.lan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/usb/host/xhci-hub.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 2732ef6..2c55fcf 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -827,6 +827,10 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 			xhci_disable_port(hcd, xhci, wIndex,
 					port_array[wIndex], temp);
 			break;
+		case USB_PORT_FEAT_POWER:
+			xhci_writel(xhci, temp & ~PORT_POWER,
+				port_array[wIndex]);
+			break;
 		default:
 			goto error;
 		}
-- 
1.7.6.rc2.8.g28eb

--
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 V4 7/8] USB/ACPI: Add usb port's acpi power control in the xhci PORT_POWER feature request process.

[Lan Tianyu]

Change since v1: use variable temp to record the return value of
usb_acpi_power_manageable().

On our developping machine, bios can provide usb port's  power control via
acpi. This patch is to provide usb port's power control way through setting
or clearing PORT_POWER feature requests. Add two functions usb_acpi_power_manageable()
and usb_acpi_set_power_state(). The first one is used to find whether the
usb port has acpi power resource and the second is to set the power state.
They are invoked in the xhci_hub_control() where clearing or setting PORT_POWER
feature requests are processed.

Signed-off-by: Lan Tianyu <tianyu.lan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/usb/core/usb-acpi.c |   59 +++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/host/xhci-hub.c |   12 ++++++++
 include/linux/usb.h         |   10 +++++++
 3 files changed, 81 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
index 404d86a..ddbf8ea 100644
--- a/drivers/usb/core/usb-acpi.c
+++ b/drivers/usb/core/usb-acpi.c
@@ -19,6 +19,65 @@
 
 #include "usb.h"
 
+/**
+ * usb_acpi_power_manageable - check whether usb port has
+ * acpi power resource.
+ * @hdev: USB device belonging to the usb hub
+ * @index: port index based zero
+ *
+ * Return true if the port has acpi power resource and false if no.
+ */
+bool usb_acpi_power_manageable(struct usb_device *hdev, int index)
+{
+	acpi_handle port_handle;
+	int port1 = index + 1;
+
+	port_handle = usb_get_hub_port_acpi_handle(hdev,
+		port1);
+	if (port_handle)
+		return acpi_bus_power_manageable(port_handle);
+	else
+		return false;
+}
+EXPORT_SYMBOL_GPL(usb_acpi_power_manageable);
+
+/**
+ * usb_acpi_set_power_state - control usb port's power via acpi power
+ * resource
+ * @hdev: USB device belonging to the usb hub
+ * @index: port index based zero
+ * @enable: power state expected to be set
+ *
+ * Notice to use usb_acpi_power_manageable() to check whether the usb port
+ * has acpi power resource before invoking this function.
+ *
+ * Returns 0 on success, else negative errno.
+ */
+int usb_acpi_set_power_state(struct usb_device *hdev, int index, bool enable)
+{
+	acpi_handle port_handle;
+	unsigned char state;
+	int port1 = index + 1;
+	int error = -EINVAL;
+
+	port_handle = (acpi_handle)usb_get_hub_port_acpi_handle(hdev,
+		port1);
+	if (!port_handle)
+		return error;
+
+	if (enable)
+		state = ACPI_STATE_D0;
+	else
+		state = ACPI_STATE_D3_COLD;
+
+	error = acpi_bus_set_power(port_handle, state);
+	if (!error)
+		dev_dbg(&hdev->dev, "The power of hub port %d was set to %d\n",
+			port1, enable);
+	return error;
+}
+EXPORT_SYMBOL_GPL(usb_acpi_set_power_state);
+
 static int usb_acpi_check_port_connect_type(struct usb_device *hdev,
 	acpi_handle handle, int port1)
 {
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 2c55fcf..b990541 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -728,6 +728,12 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 
 			temp = xhci_readl(xhci, port_array[wIndex]);
 			xhci_dbg(xhci, "set port power, actual port %d status  = 0x%x\n", wIndex, temp);
+
+			temp = usb_acpi_power_manageable(hcd->self.root_hub,
+					wIndex);
+			if (temp)
+				usb_acpi_set_power_state(hcd->self.root_hub,
+						wIndex, true);
 			break;
 		case USB_PORT_FEAT_RESET:
 			temp = (temp | PORT_RESET);
@@ -830,6 +836,12 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 		case USB_PORT_FEAT_POWER:
 			xhci_writel(xhci, temp & ~PORT_POWER,
 				port_array[wIndex]);
+
+			temp = usb_acpi_power_manageable(hcd->self.root_hub,
+					wIndex);
+			if (temp)
+				usb_acpi_set_power_state(hcd->self.root_hub,
+						wIndex, false);
 			break;
 		default:
 			goto error;
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 82e9985..0b3d0ce 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -598,6 +598,16 @@ extern int usb_lock_device_for_reset(struct usb_device *udev,
 extern int usb_reset_device(struct usb_device *dev);
 extern void usb_queue_reset_device(struct usb_interface *dev);
 
+#ifdef CONFIG_ACPI
+extern int usb_acpi_set_power_state(struct usb_device *hdev, int index,
+	bool enable);
+extern bool usb_acpi_power_manageable(struct usb_device *hdev, int index);
+#else
+static inline int usb_acpi_set_power_state(struct usb_device *hdev, int index,
+	bool enable) { return 0; }
+static inline bool usb_acpi_power_manageable(struct usb_device *hdev, int index)
+	{ return true; }
+#endif
 
 /* USB autosuspend and autoresume */
 #ifdef CONFIG_USB_SUSPEND
-- 
1.7.6.rc2.8.g28eb

--
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 V4 8/8] usb : Add usb port's power control attributes

[Lan Tianyu]

This patch is to add two attributes for each usb hub ports to control port's power.
Control port's power through setting or clearing PORT_POWER feature.

control has two options. "auto", "on" and "off"
"on" - port power must be on.
"off" - port power must be off.

state reports usb port's power state
"on" - power on
"off" - power off
"error" - can't get power state

Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 Documentation/ABI/testing/sysfs-bus-usb |   25 +++++++
 drivers/usb/core/hub.c                  |  116 ++++++++++++++++++++++++++++++-
 2 files changed, 140 insertions(+), 1 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb
index 6df4e6f..abe4ea4 100644
--- a/Documentation/ABI/testing/sysfs-bus-usb
+++ b/Documentation/ABI/testing/sysfs-bus-usb
@@ -208,3 +208,28 @@ Description:
 		such as ACPI. This file will read either "removable" or
 		"fixed" if the information is available, and "unknown"
 		otherwise.
+
+What:		/sys/bus/usb/devices/.../(hub interface)/portX/control
+Date:		June 2012
+Contact		Lan Tianyu <tianyu.lan@intel.com>
+Description:
+		The /sys/bus/usb/devices/.../(hub interface)/portX/control
+		attribute allows user space to control the power policy on
+		the usb port.
+
+		All ports have one of the following two values for control
+		"on" - port power must be on.
+		"off" - port power must be off.
+
+What:		/sys/bus/usb/devices/.../(hub interface)/portX/state
+Date:		June 2012
+Contact		Lan Tianyu <tianyu.lan@intel.com>
+Description:
+		The /sys/bus/usb/devices/.../(hub interface)/portX/state
+		attribute allows user space to check hub port's power state.
+
+		All ports have three following states
+		"on"	  -    port power on
+		"off"	  -    port power off
+		"error"	  -    can't get the hub port's power state
+
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 05cdc7b..f7cf93c 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -37,12 +37,18 @@
 #endif
 #endif
 
+enum port_power_policy {
+	USB_PORT_POWER_ON = 0,
+	USB_PORT_POWER_OFF,
+};
+
 struct usb_port {
 	struct usb_device *udev;
 	struct usb_device *child;
 	struct device dev;
 	struct dev_state *port_owner;
 	enum usb_port_connect_type connect_type;
+	enum port_power_policy port_power_policy;
 };
 
 struct usb_hub {
@@ -96,6 +102,10 @@ struct device_type usb_port_device_type = {
 	.name =		"usb_port",
 };
 
+static const char on_string[] = "on";
+static const char off_string[] = "off";
+static const struct attribute_group *port_dev_group[];
+
 static inline int hub_is_superspeed(struct usb_device *hdev)
 {
 	return (hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS);
@@ -848,7 +858,9 @@ 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]->port_power_policy
+				== USB_PORT_POWER_ON)
+			set_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);
@@ -1262,6 +1274,7 @@ static int usb_hub_create_port_device(struct usb_hub *hub,
 	hub->ports[port1 - 1] = port_dev;
 	port_dev->udev = hub->hdev;
 	port_dev->dev.parent = hub->intfdev;
+	port_dev->dev.groups = port_dev_group;
 	port_dev->dev.type = &usb_port_device_type;
 	port_dev->dev.release = usb_port_device_release;
 	dev_set_name(&port_dev->dev, "port%d", port1);
@@ -2611,6 +2624,25 @@ static int port_is_power_on(struct usb_hub *hub, unsigned portstatus)
 	return ret;
 }
 
+static int usb_get_hub_port_power_state(struct usb_device *hdev, int port1)
+{
+	struct usb_hub *hub = hdev_to_hub(hdev);
+	struct usb_port_status data;
+	u16 portstatus;
+	int ret;
+
+	ret = get_port_status(hub->hdev, port1, &data);
+	if (ret < 4) {
+		dev_err(hub->intfdev,
+			"%s failed (err = %d)\n", __func__, ret);
+		if (ret >= 0)
+			ret = -EIO;
+		return ret;
+	} else
+		portstatus = le16_to_cpu(data.wPortStatus);
+	return port_is_power_on(hub, portstatus);
+}
+
 #ifdef	CONFIG_PM
 
 /* Check if a port is suspended(USB2.0 port) or in U3 state(USB3.0 port) */
@@ -4552,6 +4584,88 @@ static int hub_thread(void *__unused)
 	return 0;
 }
 
+static ssize_t show_port_power_state(struct device *dev,
+	struct device_attribute *attr, char *buf)
+{
+	struct usb_device *udev = to_usb_device(dev->parent->parent);
+	int port1, power_state;
+	const char *result;
+
+	sscanf(dev_name(dev), "port%d", &port1);
+	power_state = usb_get_hub_port_power_state(udev, port1);
+	if (power_state == 1)
+		result = on_string;
+	else if (!power_state)
+		result = off_string;
+	else
+		result = "error";
+	return sprintf(buf, "%s\n", result);
+}
+static DEVICE_ATTR(state, S_IRUGO, show_port_power_state, NULL);
+
+static ssize_t show_port_power_control(struct device *dev,
+	struct device_attribute *attr, char *buf)
+{
+	struct usb_port *hub_port = to_usb_port(dev);
+	const char *result;
+
+	switch (hub_port->port_power_policy) {
+	case USB_PORT_POWER_ON:
+		result = on_string;
+		break;
+	case USB_PORT_POWER_OFF:
+		result = off_string;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return sprintf(buf, "%s\n", result);
+}
+
+static ssize_t
+store_port_power_control(struct device *dev, struct device_attribute *attr,
+		const char *buf, size_t count)
+{
+	struct usb_device *hdev = to_usb_device(dev->parent->parent);
+	struct usb_port *hub_port = to_usb_port(dev);
+	int port1, len = count;
+	char *cp;
+
+	sscanf(dev_name(dev), "port%d", &port1);
+	cp = memchr(buf, '\n', count);
+	if (cp)
+		len = cp - buf;
+	if (len == sizeof on_string - 1
+		&& strncmp(buf, on_string, len) == 0) {
+		hub_port->port_power_policy = USB_PORT_POWER_ON;
+		set_port_feature(hdev, port1, USB_PORT_FEAT_POWER);
+	} else if (len == sizeof off_string - 1
+		&& strncmp(buf, off_string, len) == 0) {
+		hub_port->port_power_policy = USB_PORT_POWER_OFF;
+		clear_port_feature(hdev, port1, USB_PORT_FEAT_POWER);
+	} else
+		return -EINVAL;
+
+	return count;
+}
+static DEVICE_ATTR(control, S_IWUSR | S_IRUGO, show_port_power_control,
+		store_port_power_control);
+
+static struct attribute *port_dev_attrs[] = {
+	&dev_attr_control.attr,
+	&dev_attr_state.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_DEV_CLASS,
       .bDeviceClass = USB_CLASS_HUB},
-- 
1.7.6.rc2.8.g28eb

Re: [PATCH V4 1/8] usb: convert port_owners type from void * to struct dev_state *

[Alan Stern]

On Thu, 28 Jun 2012, Lan Tianyu wrote:

> This patch is to convert ort_owners type from void * to struct dev_state *

s/ort/port/

> in order to make code more readable.

Otherwise this patch is okay.

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Re: [PATCH V4 2/8] usb: make usb port a real device

[Alan Stern]

On Thu, 28 Jun 2012, Lan Tianyu wrote:

> This patch is to make usb port a real device under usb hub interface.
> Move port_owner to struct usb_port.
> 
> Signed-off-by: Lan Tianyu <tianyu.lan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/usb/core/hub.c |   93 +++++++++++++++++++++++++++++++++++++++--------
>  1 files changed, 77 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 4cc8dc9..c19e088 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -37,6 +37,12 @@
>  #endif
>  #endif
>  
> +struct usb_port {
> +	struct usb_device *udev;

Why is this field present?  It never gets used anywhere.

If you really want to keep it, please rename it to "hdev", because it 
is a pointer to a hub device, not to a general USB device.  However I 
think it isn't needed, because the hub device is always accessible via 
usb_port->parent->parent.

Alan Stern

--
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 V4 3/8] usb: move children to struct usb_port

[Alan Stern]

On Thu, 28 Jun 2012, Lan Tianyu wrote:

> Move child's pointer to the struct usb_port since the child device
> is directly associated with the port. Provide usb_get_hub_child()
> to get child's pointer and macrio macro usb_get_hub_each_child to
> iterate all child devices on the hub.
> 
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>


> --- a/drivers/usb/core/devices.c
> +++ b/drivers/usb/core/devices.c

> @@ -589,14 +590,12 @@ static ssize_t usb_device_dump(char __user **buffer, size_t *nbytes,
>  	free_pages((unsigned long)pages_start, 1);
>  
>  	/* Now look at all of this device's children. */
> -	for (chix = 0; chix < usbdev->maxchild; chix++) {
> -		struct usb_device *childdev = usbdev->children[chix];
> -
> +	usb_get_hub_each_child(usbdev, chix, childdev) {
>  		if (childdev) {
>  			usb_lock_device(childdev);
>  			ret = usb_device_dump(buffer, nbytes, skip_bytes,
>  					      file_offset, childdev, bus,
> -					      level + 1, chix, ++cnt);
> +					      level + 1, chix - 1, ++cnt);
>  			usb_unlock_device(childdev);

You forget to add usb_put_dev(childdev).

>  			if (ret == -EFAULT)
>  				return total_written;

> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c

> @@ -1784,11 +1783,12 @@ bool usb_device_is_owned(struct usb_device *udev)
>  
>  static void recursively_mark_NOTATTACHED(struct usb_device *udev)
>  {
> +	struct usb_hub *hub = hdev_to_hub(udev);

This is dangerous, because at this point you don't know whether udev is
a hub -- it might not be -- and hdev_to_hub can crash if its argument
isn't a hub.  Instead you should do:

	struct usb_hub *hub;

	if (udev->maxchild > 0)
		hub = hdev_to_hub(udev);

Or if you prefer, add the "hdev->maxchild > 0" test into hdev_to_hub 
itself.

> @@ -1952,6 +1952,7 @@ static void hub_free_dev(struct usb_device *udev)
>  void usb_disconnect(struct usb_device **pdev)
>  {
>  	struct usb_device	*udev = *pdev;
> +	struct usb_hub		*hub = hdev_to_hub(udev);

Same here.

> @@ -4965,3 +4966,27 @@ void usb_queue_reset_device(struct usb_interface *iface)
>  	schedule_work(&iface->reset_ws);
>  }
>  EXPORT_SYMBOL_GPL(usb_queue_reset_device);
> +
> +/**
> + * usb_get_hub_child - Get the pointer of child device
> + * attached to the port which is specified by @port1.
> + * @hdev: USB device belonging to the usb hub
> + * @port1: port num to indicate which port the child device
> + *	is attached to.
> + *
> + * USB drivers call this function to get hub's child device
> + * pointer.
> + *
> + * Return NULL if input param is invalid and
> + * child's usb_device pointer if non-NULL.

The kerneldoc should mention that this routine increments the child's
reference count and the caller must call usb_put_dev() when it
is finished using the child.

Also, I think it would be better if the function were named
"usb_hub_get_child".

> + */
> +struct usb_device *usb_get_hub_child(struct usb_device *hdev,
> +		int port1)
> +{
> +	struct usb_hub *hub = hdev_to_hub(hdev);
> +
> +	if (port1 < 1 || port1 > hdev->maxchild)
> +		return NULL;
> +	return usb_get_dev(hub->ports[port1 - 1]->child);
> +}

> --- a/drivers/usb/host/r8a66597-hcd.c
> +++ b/drivers/usb/host/r8a66597-hcd.c
> @@ -2033,17 +2033,18 @@ static int r8a66597_get_frame(struct usb_hcd *hcd)
>  static void collect_usb_address_map(struct usb_device *udev, unsigned long *map)
>  {
>  	int chix;
> +	struct usb_device *childdev;
>  
>  	if (udev->state == USB_STATE_CONFIGURED &&
>  	    udev->parent && udev->parent->devnum > 1 &&
>  	    udev->parent->descriptor.bDeviceClass == USB_CLASS_HUB)
>  		map[udev->devnum/32] |= (1 << (udev->devnum % 32));
>  
> -	for (chix = 0; chix < udev->maxchild; chix++) {
> -		struct usb_device *childdev = udev->children[chix];
> -
> -		if (childdev)
> +	usb_get_hub_each_child(udev, chix, childdev) {
> +		if (childdev) {
> +			usb_put_dev(childdev);
>  			collect_usb_address_map(childdev, map);

The usb_put_dev should come _after_ the collect_usb_address_map call.
Otherwise you're passing a stale pointer.

> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h

> @@ -567,6 +565,20 @@ static inline struct usb_device *interface_to_usbdev(struct usb_interface *intf)
>  
>  extern struct usb_device *usb_get_dev(struct usb_device *dev);
>  extern void usb_put_dev(struct usb_device *dev);
> +extern struct usb_device *usb_get_hub_child(struct usb_device *hdev,
> +	int port1);
> +
> +/**
> + * usb_get_hub_each_child - iterate over all child devices on the hub
> + * @hdev:  USB device belonging to the usb hub
> + * @port1: portnum associated with child device
> + * @child: child device pointer
> + *

Mention that the caller must call usb_put_dev() when it is finished
using the child.

And change the name to "usb_hub_get_each_child".

Alan Stern

Re: [PATCH V4 5/8] usb/acpi: add check for the connect type of usb port

[Alan Stern]

On Thu, 28 Jun 2012, Lan Tianyu wrote:

> Change since v3: Rebase on the patch "make usb port a real device"
> 
> Check the connect type of usb port when getting the usb port's
> acpi_handle and store result into connect_type in the struct
> usb_port.
> 
> Accoding to ACPI Spec 9.13. PLD indicates whether usb port is
> user visible and _UPC indicates whether it is connectable. If
> the port was visible and connectable, it could be freely connected
> and disconnected with USB devices. If no visible and connectable,
> a usb device is directly hard-wired to the port. If no visible and
> no connectable, the port would be not used.
> 
> When a device was found on the port, if the connect_type was hot-plug,
> then the device would be removable. If the connect_type was hard-wired,
> the device would be non-removable.
> 
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>

> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c

> @@ -4991,6 +4992,45 @@ struct usb_device *usb_get_hub_child(struct usb_device *hdev,
>  }
>  EXPORT_SYMBOL_GPL(usb_get_hub_child);
>  
> +/**
> + * usb_set_hub_port_connect_type - set hub port connect type.
> + * @hdev: USB device belonging to the usb hub
> + * @port1: port num of the port
> + * @type: connect type of the port
> + *		USB_PORT_CONNECT_TYPE_UNKNOWN
> + *		USB_PORT_CONNECT_TYPE_HOT_PLUG
> + *		USB_PORT_CONNECT_TYPE_HARD_WIRED
> + *		USB_PORT_NOT_USED

These four lines aren't needed.  People can read the declaration of
enum usb_port_connect_type easily enough.

> +/**
> + * usb_get_hub_port_connect_type - Get the port's connect
> + * type
> + * @hdev: USB device belonging to the usb hub
> + * @port1: port num of the port
> + *
> + * Return connect type of the port annd if input params are

s/annd/and/

> + *	invalid, return USB_PORT_CONNECT_TYPE_UNKNOWN.
> + *		USB_PORT_CONNECT_TYPE_UNKNOWN
> + *		USB_PORT_CONNECT_TYPE_HOT_PLUG
> + *		USB_PORT_CONNECT_TYPE_HARD_WIRED
> + *		USB_PORT_NOT_USED

Same for these lines.

Alan Stern

Re: [PATCH V4 8/8] usb : Add usb port's power control attributes

[Alan Stern]

On Thu, 28 Jun 2012, Lan Tianyu wrote:

> This patch is to add two attributes for each usb hub ports to control port's power.
> Control port's power through setting or clearing PORT_POWER feature.
> 
> control has two options. "auto", "on" and "off"
> "on" - port power must be on.
> "off" - port power must be off.
> 
> state reports usb port's power state
> "on" - power on
> "off" - power off
> "error" - can't get power state
> 
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>

> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c

> @@ -848,7 +858,9 @@ 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]->port_power_policy
> +				== USB_PORT_POWER_ON)
> +			set_port_feature(hub->hdev, port1, USB_PORT_FEAT_POWER);

Since the last patch version, I realized that sometimes host controller
drivers will turn on port power by themselves.  To be safe, you should add

		else
			clear_port_feature(hub->hdev, port1, USB_PORT_FEAT_POWER);


> +static ssize_t
> +store_port_power_control(struct device *dev, struct device_attribute *attr,
> +		const char *buf, size_t count)

Since you have to break the line anyway, you ought to put the function's
name on the first line.

> +{
> +	struct usb_device *hdev = to_usb_device(dev->parent->parent);
> +	struct usb_port *hub_port = to_usb_port(dev);
> +	int port1, len = count;
> +	char *cp;
> +
> +	sscanf(dev_name(dev), "port%d", &port1);
> +	cp = memchr(buf, '\n', count);
> +	if (cp)
> +		len = cp - buf;
> +	if (len == sizeof on_string - 1
> +		&& strncmp(buf, on_string, len) == 0) {
> +		hub_port->port_power_policy = USB_PORT_POWER_ON;
> +		set_port_feature(hdev, port1, USB_PORT_FEAT_POWER);
> +	} else if (len == sizeof off_string - 1
> +		&& strncmp(buf, off_string, len) == 0) {
> +		hub_port->port_power_policy = USB_PORT_POWER_OFF;
> +		clear_port_feature(hdev, port1, USB_PORT_FEAT_POWER);

You should return -EIO if set_port_feature or clear_port_feature fails.

Alan Stern

Re: [PATCH V4 3/8] usb: move children to struct usb_port

[Lan Tianyu]

hi alan:
	Great thanks for your review.

On 2012年06月28日 22:42, Alan Stern wrote:
> On Thu, 28 Jun 2012, Lan Tianyu wrote:
>
>> Move child's pointer to the struct usb_port since the child device
>> is directly associated with the port. Provide usb_get_hub_child()
>> to get child's pointer and macrio macro usb_get_hub_each_child to
>> iterate all child devices on the hub.
>>
>> Signed-off-by: Lan Tianyu<tianyu.lan@intel.com>
>
>
>> --- a/drivers/usb/core/devices.c
>> +++ b/drivers/usb/core/devices.c
>
>> @@ -589,14 +590,12 @@ static ssize_t usb_device_dump(char __user **buffer, size_t *nbytes,
>>   	free_pages((unsigned long)pages_start, 1);
>>
>>   	/* Now look at all of this device's children. */
>> -	for (chix = 0; chix<  usbdev->maxchild; chix++) {
>> -		struct usb_device *childdev = usbdev->children[chix];
>> -
>> +	usb_get_hub_each_child(usbdev, chix, childdev) {
>>   		if (childdev) {
>>   			usb_lock_device(childdev);
>>   			ret = usb_device_dump(buffer, nbytes, skip_bytes,
>>   					      file_offset, childdev, bus,
>> -					      level + 1, chix, ++cnt);
>> +					      level + 1, chix - 1, ++cnt);
>>   			usb_unlock_device(childdev);
>
> You forget to add usb_put_dev(childdev).
Oh. sorry. I forget to remove usb_get_dev() in the usb_get_hub_child(). since 
last our discussion,
the increase refcount maybe not helpful. I will remove it in next version.

http://marc.info/?l=linux-usb&m=133968372704178&w=2
>
>>   			if (ret == -EFAULT)
>>   				return total_written;
>
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>
>> @@ -1784,11 +1783,12 @@ bool usb_device_is_owned(struct usb_device *udev)
>>
>>   static void recursively_mark_NOTATTACHED(struct usb_device *udev)
>>   {
>> +	struct usb_hub *hub = hdev_to_hub(udev);
>
> This is dangerous, because at this point you don't know whether udev is
> a hub -- it might not be -- and hdev_to_hub can crash if its argument
> isn't a hub.  Instead you should do:
>
> 	struct usb_hub *hub;
>
> 	if (udev->maxchild>  0)
> 		hub = hdev_to_hub(udev);
>
> Or if you prefer, add the "hdev->maxchild>  0" test into hdev_to_hub
> itself.
>
>> @@ -1952,6 +1952,7 @@ static void hub_free_dev(struct usb_device *udev)
>>   void usb_disconnect(struct usb_device **pdev)
>>   {
>>   	struct usb_device	*udev = *pdev;
>> +	struct usb_hub		*hub = hdev_to_hub(udev);
>
> Same here.
>
>> @@ -4965,3 +4966,27 @@ void usb_queue_reset_device(struct usb_interface *iface)
>>   	schedule_work(&iface->reset_ws);
>>   }
>>   EXPORT_SYMBOL_GPL(usb_queue_reset_device);
>> +
>> +/**
>> + * usb_get_hub_child - Get the pointer of child device
>> + * attached to the port which is specified by @port1.
>> + * @hdev: USB device belonging to the usb hub
>> + * @port1: port num to indicate which port the child device
>> + *	is attached to.
>> + *
>> + * USB drivers call this function to get hub's child device
>> + * pointer.
>> + *
>> + * Return NULL if input param is invalid and
>> + * child's usb_device pointer if non-NULL.
>
> The kerneldoc should mention that this routine increments the child's
> reference count and the caller must call usb_put_dev() when it
> is finished using the child.
>
> Also, I think it would be better if the function were named
> "usb_hub_get_child".
Ok. I will change it.
>
>> + */
>> +struct usb_device *usb_get_hub_child(struct usb_device *hdev,
>> +		int port1)
>> +{
>> +	struct usb_hub *hub = hdev_to_hub(hdev);
>> +
>> +	if (port1<  1 || port1>  hdev->maxchild)
>> +		return NULL;
>> +	return usb_get_dev(hub->ports[port1 - 1]->child);
>> +}
>
>> --- a/drivers/usb/host/r8a66597-hcd.c
>> +++ b/drivers/usb/host/r8a66597-hcd.c
>> @@ -2033,17 +2033,18 @@ static int r8a66597_get_frame(struct usb_hcd *hcd)
>>   static void collect_usb_address_map(struct usb_device *udev, unsigned long *map)
>>   {
>>   	int chix;
>> +	struct usb_device *childdev;
>>
>>   	if (udev->state == USB_STATE_CONFIGURED&&
>>   	udev->parent&&  udev->parent->devnum>  1&&
>>   	udev->parent->descriptor.bDeviceClass == USB_CLASS_HUB)
>>   		map[udev->devnum/32] |= (1<<  (udev->devnum % 32));
>>
>> -	for (chix = 0; chix<  udev->maxchild; chix++) {
>> -		struct usb_device *childdev = udev->children[chix];
>> -
>> -		if (childdev)
>> +	usb_get_hub_each_child(udev, chix, childdev) {
>> +		if (childdev) {
>> +			usb_put_dev(childdev);
>>   			collect_usb_address_map(childdev, map);
>
> The usb_put_dev should come _after_ the collect_usb_address_map call.
> Otherwise you're passing a stale pointer.
>
>> --- a/include/linux/usb.h
>> +++ b/include/linux/usb.h
>
>> @@ -567,6 +565,20 @@ static inline struct usb_device *interface_to_usbdev(struct usb_interface *intf)
>>
>>   extern struct usb_device *usb_get_dev(struct usb_device *dev);
>>   extern void usb_put_dev(struct usb_device *dev);
>> +extern struct usb_device *usb_get_hub_child(struct usb_device *hdev,
>> +	int port1);
>> +
>> +/**
>> + * usb_get_hub_each_child - iterate over all child devices on the hub
>> + * @hdev:  USB device belonging to the usb hub
>> + * @port1: portnum associated with child device
>> + * @child: child device pointer
>> + *
>
> Mention that the caller must call usb_put_dev() when it is finished
> using the child.
>
> And change the name to "usb_hub_get_each_child".
>
> Alan Stern
>
> --
> 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

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