[RFC PATCH 1/4] usb: add struct usb_hub_port to store port related members.

[RFC PATCH 1/4] usb: add struct usb_hub_port to store port related members.

[Lan Tianyu]

Add struct usb_hub_port pointer port_data in the struct usb_hub and allocate
struct usb_hub_port perspectively for each ports to store private data.

Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 drivers/usb/core/hub.c |   28 ++++++++++++++++------------
 1 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index aeefbab..4c20c34 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -37,6 +37,10 @@
 #endif
 #endif
 
+struct usb_hub_port {
+	void			*port_owner;
+};
+
 struct usb_hub {
 	struct device		*intfdev;	/* the "interface" device */
 	struct usb_device	*hdev;
@@ -79,7 +83,7 @@ struct usb_hub {
 	u8			indicator[USB_MAXCHILDREN];
 	struct delayed_work	leds;
 	struct delayed_work	init_work;
-	void			**port_owners;
+	struct usb_hub_port	*port_data;
 };
 
 static inline int hub_is_superspeed(struct usb_device *hdev)
@@ -1021,8 +1025,9 @@ static int hub_configure(struct usb_hub *hub,
 	dev_info (hub_dev, "%d port%s detected\n", hdev->maxchild,
 		(hdev->maxchild == 1) ? "" : "s");
 
-	hub->port_owners = kzalloc(hdev->maxchild * sizeof(void *), GFP_KERNEL);
-	if (!hub->port_owners) {
+	hub->port_data = kzalloc(hdev->maxchild * sizeof(struct usb_hub_port),
+			GFP_KERNEL);
+	if (!hub->port_data) {
 		ret = -ENOMEM;
 		goto fail;
 	}
@@ -1275,7 +1280,7 @@ static void hub_disconnect(struct usb_interface *intf)
 		highspeed_hubs--;
 
 	usb_free_urb(hub->urb);
-	kfree(hub->port_owners);
+	kfree(hub->port_data);
 	kfree(hub->descriptor);
 	kfree(hub->status);
 	kfree(hub->buffer);
@@ -1413,7 +1418,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)->port_data[port1 - 1].port_owner);
 	return 0;
 }
 
@@ -1451,13 +1456,12 @@ void usb_hub_release_all_ports(struct usb_device *hdev, void *owner)
 	int n;
 	void **powner;
 
-	n = find_port_owner(hdev, 1, &powner);
-	if (n == 0) {
-		for (; n < hdev->maxchild; (++n, ++powner)) {
-			if (*powner == owner)
-				*powner = NULL;
-		}
+	for (n = 1; n <= hdev->maxchild; n++) {
+		find_port_owner(hdev, n, &powner);
+		if (*powner == owner)
+			*powner = NULL;
 	}
+
 }
 
 /* The caller must hold udev's lock */
@@ -1468,7 +1472,7 @@ 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->port_data[udev->portnum - 1].port_owner;
 }
 
 
-- 
1.7.6.rc2.8.g28eb

[RFC PATCH 2/4] usb: add platform_data in the struct usb_hub_port

[Lan Tianyu]

Add the member platform_data in the usb_hub_port to store port's
platform related data. Provide usb_set_hub_port_platform_data() and
usb_get_hub_port_platform_data() to access the member.

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

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 4c20c34..af391cb 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -39,6 +39,7 @@
 
 struct usb_hub_port {
 	void			*port_owner;
+	unsigned long		platform_data;
 };
 
 struct usb_hub {
@@ -4150,3 +4151,25 @@ void usb_queue_reset_device(struct usb_interface *iface)
 	schedule_work(&iface->reset_ws);
 }
 EXPORT_SYMBOL_GPL(usb_queue_reset_device);
+
+int usb_set_hub_port_platform_data(struct usb_device *udev, int port,
+	unsigned long data)
+{
+	struct usb_hub *hub = hdev_to_hub(udev);
+
+	if (port > hub->descriptor->bNbrPorts || port < 1)
+		return -EINVAL;
+	hub->port_data[port - 1].platform_data = data;
+	return 0;
+}
+
+int usb_get_hub_port_platform_data(struct usb_device *udev, int port,
+	unsigned long *data)
+{
+	struct usb_hub *hub = hdev_to_hub(udev);
+
+	if (port > hub->descriptor->bNbrPorts || port < 1)
+		return -EINVAL;
+	*data = hub->port_data[port - 1].platform_data;
+	return 0;
+}
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index 636d98e..3e70565 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -154,6 +154,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 int usb_get_hub_port_platform_data(struct usb_device *udev,
+		int port, unsigned long *data);
+extern int usb_set_hub_port_platform_data(struct usb_device *udev,
+		int port, unsigned long data);
 
 #ifdef CONFIG_ACPI
 extern int usb_acpi_register(void);
-- 
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

Re: [RFC PATCH 2/4] usb: add platform_data in the struct usb_hub_port

[Alan Stern]

On Mon, 19 Mar 2012, Lan Tianyu wrote:

> Add the member platform_data in the usb_hub_port to store port's
> platform related data. Provide usb_set_hub_port_platform_data() and
> usb_get_hub_port_platform_data() to access the member.

> +int usb_set_hub_port_platform_data(struct usb_device *udev, int port,
> +	unsigned long data)

The argument is a 1-based port number, so it should be called "port1" 
instead of "port".

The same change needs to be made in other places in your patch series.

> +{
> +	struct usb_hub *hub = hdev_to_hub(udev);
> +
> +	if (port > hub->descriptor->bNbrPorts || port < 1)

Use udev->maxchild instead of hub->descriptor->bNbrPorts.  Same thing 
in usb_get_hub_port_platform_data().

> +		return -EINVAL;
> +	hub->port_data[port - 1].platform_data = data;
> +	return 0;
> +}

Alan Stern

[RFC PATCH 3/4] usb/acpi: add the support of usb hub ports' acpi binding without attached devices.

[Lan Tianyu]

The usb port is a device in the acpi table but it's not in the linux
usb subsystem. USB hub port doesn't have struct device. So the acpi
glue framework only can cover the usb port connected with usb device
and store the acpi handle to struct device.archdata.acpi_handle. This
patch gets the hub port's acpi_handle and store it in the port's platform_data
to resolve no attached device no binding problem. The acpi method "_UPC"
and "_PLD" can be accessed without attached device.

Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 drivers/usb/core/hub.c      |    1 +
 drivers/usb/core/usb-acpi.c |   38 +++++++++++++++++++++++++++++++++++++-
 drivers/usb/core/usb.h      |    4 ++++
 3 files changed, 42 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index af391cb..90877f0 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -1238,6 +1238,7 @@ static int hub_configure(struct usb_hub *hub,
 		hub->indicator [0] = INDICATOR_CYCLE;
 
 	hub_activate(hub, HUB_INIT);
+	usb_acpi_bind_hub_ports(hdev, hub->descriptor->bNbrPorts);
 	return 0;
 
 fail:
diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
index e49373a..6ecff04 100644
--- a/drivers/usb/core/usb-acpi.c
+++ b/drivers/usb/core/usb-acpi.c
@@ -82,7 +82,16 @@ static int usb_acpi_find_device(struct device *dev, acpi_handle *handle)
 	if (!parent_handle)
 		return -ENODEV;
 
-	*handle = acpi_get_child(parent_handle, udev->portnum);
+	/**
+	 * The root hub's acpi handle is got from acpi method.
+	 * Other device's acpi handle can be got from the usb hub
+	 * port's platform_data.
+	 */
+	if (!udev->parent)
+		*handle = acpi_get_child(parent_handle, udev->portnum);
+	else
+		usb_get_hub_port_platform_data(udev->parent, udev->portnum,
+			(unsigned long *)handle);
 
 	if (!*handle)
 		return -ENODEV;
@@ -105,6 +114,33 @@ static struct acpi_bus_type usb_acpi_bus = {
 	.find_device = usb_acpi_find_device,
 };
 
+int usb_acpi_bind_hub_ports(struct usb_device *hdev, int portnum)
+{
+	acpi_handle hub_handle = NULL;
+	acpi_handle port_handle = NULL;
+	struct device *dev = &hdev->dev;
+	int i;
+
+	hub_handle = DEVICE_ACPI_HANDLE(dev);
+	if (!hub_handle)
+		return -ENODEV;
+
+	/**
+	 * The usb hub port is not a device in the usb subsystem but it is a device
+	 * in the acpi table. Store it's acpi handle in the platform data of usb
+	 * hub port.
+	 */
+	for (i = 1; i < portnum; i++) {
+		port_handle = acpi_get_child(hub_handle, i);
+		if (!port_handle)
+			continue;
+		if (usb_set_hub_port_platform_data(hdev, i,
+				(unsigned long)port_handle) < 0)
+			return -ENODEV;
+	}
+	return 0;
+}
+
 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 3e70565..b780246 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -162,7 +162,11 @@ extern int usb_set_hub_port_platform_data(struct usb_device *udev,
 #ifdef CONFIG_ACPI
 extern int usb_acpi_register(void);
 extern void usb_acpi_unregister(void);
+extern int usb_acpi_bind_hub_ports(struct usb_device *hdev,
+	int portnum);
 #else
 static inline int usb_acpi_register(void) { return 0; };
 static inline void usb_acpi_unregister(void) { };
+static inline int usb_acpi_bind_hub_ports(struct usb_device *hdev,
+	int portnum) { return 0; };
 #endif
-- 
1.7.6.rc2.8.g28eb

Re: [RFC PATCH 3/4] usb/acpi: add the support of usb hub ports' acpi binding without attached devices.

[Alan Stern]

On Mon, 19 Mar 2012, Lan Tianyu wrote:

> The usb port is a device in the acpi table but it's not in the linux
> usb subsystem. USB hub port doesn't have struct device. So the acpi
> glue framework only can cover the usb port connected with usb device
> and store the acpi handle to struct device.archdata.acpi_handle. This
> patch gets the hub port's acpi_handle and store it in the port's platform_data
> to resolve no attached device no binding problem. The acpi method "_UPC"
> and "_PLD" can be accessed without attached device.

> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -1238,6 +1238,7 @@ static int hub_configure(struct usb_hub *hub,
>  		hub->indicator [0] = INDICATOR_CYCLE;
>  
>  	hub_activate(hub, HUB_INIT);
> +	usb_acpi_bind_hub_ports(hdev, hub->descriptor->bNbrPorts);

Use hdev->maxchild instead of hub->descriptor->bNbrPorts.

> @@ -105,6 +114,33 @@ static struct acpi_bus_type usb_acpi_bus = {
>  	.find_device = usb_acpi_find_device,
>  };
>  
> +int usb_acpi_bind_hub_ports(struct usb_device *hdev, int portnum)

Don't pass portnum as an argument; use hdev->maxchild instead.

> +{
> +	acpi_handle hub_handle = NULL;
> +	acpi_handle port_handle = NULL;
> +	struct device *dev = &hdev->dev;
> +	int i;
> +
> +	hub_handle = DEVICE_ACPI_HANDLE(dev);
> +	if (!hub_handle)
> +		return -ENODEV;
> +
> +	/**
> +	 * The usb hub port is not a device in the usb subsystem but it is a device
> +	 * in the acpi table. Store it's acpi handle in the platform data of usb

s/it's/its/

"its" is a possessive pronoun, but "it's" is a contraction of "it is" 
or "it has".

Alan Stern

[RFC PATCH 4/4] usb/acpi: add usb check for the connect type of usb port

[Lan Tianyu]

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_hub_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      |   27 +++++++++++++++++-
 drivers/usb/core/usb-acpi.c |   64 +++++++++++++++++++++----------------------
 drivers/usb/core/usb.h      |    4 +++
 include/linux/usb.h         |    7 +++++
 4 files changed, 68 insertions(+), 34 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 90877f0..5e8976d 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -40,6 +40,7 @@
 struct usb_hub_port {
 	void			*port_owner;
 	unsigned long		platform_data;
+	enum usb_port_connect_type connect_type;
 };
 
 struct usb_hub {
@@ -132,7 +133,7 @@ MODULE_PARM_DESC(initial_descriptor_timeout,
  *
  * For maximum flexibility there are two boolean parameters to control the
  * hub driver's behavior.  On the first initialization attempt, if the
- * "old_scheme_first" parameter is set then the old scheme will be used,
+.iiies * "old_scheme_first" parameter is set then the old scheme will be used,
  * otherwise the new scheme is used.  If that fails and "use_both_schemes"
  * is set, then the driver will make another attempt, using the other scheme.
  */
@@ -4174,3 +4175,27 @@ int usb_get_hub_port_platform_data(struct usb_device *udev, int port,
 	*data = hub->port_data[port - 1].platform_data;
 	return 0;
 }
+
+int usb_set_hub_port_connect_type(struct usb_device *hdev, int port,
+	enum usb_port_connect_type type)
+{
+	struct usb_hub *hub = hdev_to_hub(hdev);
+
+	if (port > hub->descriptor->bNbrPorts || port < 1)
+		return -EINVAL;
+
+	hub->port_data[port - 1].connect_type = type;
+	return 0;
+}
+
+int usb_get_hub_port_connect_type(struct usb_device *hdev, int port,
+	enum usb_port_connect_type *type)
+{
+	struct usb_hub *hub = hdev_to_hub(hdev);
+
+	if (port > hub->descriptor->bNbrPorts || port < 1)
+		return -EINVAL;
+
+	*type = hub->port_data[port - 1].connect_type;
+	return 0;
+}
diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
index 6ecff04..3a3b07b 100644
--- a/drivers/usb/core/usb-acpi.c
+++ b/drivers/usb/core/usb-acpi.c
@@ -19,58 +19,57 @@
 
 #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 port)
 {
 	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);
 	if (ACPI_FAILURE(status))
 		return -ENODEV;
 
 	upc = buffer.pointer;
-
 	if (!upc || (upc->type != ACPI_TYPE_PACKAGE) || upc->package.count != 4) {
 		ret = -EINVAL;
 		goto out;
 	}
 
-	if (upc->package.elements[0].integer.value)
-		udev->removable = USB_DEVICE_REMOVABLE;
-	else
-		udev->removable = USB_DEVICE_FIXED;
+	if (upc->package.elements[0].integer.value && pld.user_visible)
+		usb_set_hub_port_connect_type(hdev, port,
+			USB_PORT_CONNECT_TYPE_HOT_PLUG);
+	else if (upc->package.elements[0].integer.value && !pld.user_visible)
+		usb_set_hub_port_connect_type(hdev, port,
+			USB_PORT_CONNECT_TYPE_HARD_WIRED);
+	else if (!upc->package.elements[0].integer.value && !pld.user_visible)
+		usb_set_hub_port_connect_type(hdev, port, 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;
 	struct device *parent;
 	acpi_handle *parent_handle;
+	enum usb_port_connect_type type;
 
 	if (!is_usb_device(dev))
 		return -ENODEV;
@@ -96,14 +95,12 @@ static int usb_acpi_find_device(struct device *dev, acpi_handle *handle)
 	if (!*handle)
 		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);
+	usb_get_hub_port_connect_type(udev->parent, udev->portnum, &type);
+
+	if (type == USB_PORT_CONNECT_TYPE_HOT_PLUG)
+		udev->removable = USB_DEVICE_REMOVABLE;
+	else if (type == USB_PORT_CONNECT_TYPE_HARD_WIRED)
+		udev->removable = USB_DEVICE_FIXED;
 
 	return 0;
 }
@@ -137,6 +134,7 @@ int usb_acpi_bind_hub_ports(struct usb_device *hdev, int portnum)
 		if (usb_set_hub_port_platform_data(hdev, i,
 				(unsigned long)port_handle) < 0)
 			return -ENODEV;
+		usb_acpi_check_port_connect_type(hdev, port_handle, i);
 	}
 	return 0;
 }
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index b780246..23e6693 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -158,6 +158,10 @@ extern int usb_get_hub_port_platform_data(struct usb_device *udev,
 		int port, unsigned long *data);
 extern int usb_set_hub_port_platform_data(struct usb_device *udev,
 		int port, unsigned long data);
+extern int usb_get_hub_port_connect_type(struct usb_device *hdev, int port,
+	enum usb_port_connect_type *type);
+extern int usb_set_hub_port_connect_type(struct usb_device *hdev, int port,
+	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 0c51663..0741ea3 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -382,6 +382,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,
+};
+
 /**
  * struct usb_device - kernel's representation of a USB device
  * @devnum: device number; address on a USB bus
-- 
1.7.6.rc2.8.g28eb

Re: [RFC PATCH 4/4] usb/acpi: add usb check for the connect type of usb port

[Alan Stern]

On Mon, 19 Mar 2012, Lan Tianyu wrote:

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

> @@ -132,7 +133,7 @@ MODULE_PARM_DESC(initial_descriptor_timeout,
>   *
>   * For maximum flexibility there are two boolean parameters to control the
>   * hub driver's behavior.  On the first initialization attempt, if the
> - * "old_scheme_first" parameter is set then the old scheme will be used,
> +.iiies * "old_scheme_first" parameter is set then the old scheme will be used,

This is a typo.

>   * otherwise the new scheme is used.  If that fails and "use_both_schemes"
>   * is set, then the driver will make another attempt, using the other scheme.
>   */
> @@ -4174,3 +4175,27 @@ int usb_get_hub_port_platform_data(struct usb_device *udev, int port,
>  	*data = hub->port_data[port - 1].platform_data;
>  	return 0;
>  }
> +
> +int usb_set_hub_port_connect_type(struct usb_device *hdev, int port,
> +	enum usb_port_connect_type type)

The parameter should be called "port1", not "port".

> +{
> +	struct usb_hub *hub = hdev_to_hub(hdev);
> +
> +	if (port > hub->descriptor->bNbrPorts || port < 1)

Use hdev->maxchild instead of hub->descriptor->bNbrPorts.  Same in 
usb_get_hub_port_connect_type().

> --- a/drivers/usb/core/usb-acpi.c
> +++ b/drivers/usb/core/usb-acpi.c
> @@ -19,58 +19,57 @@
>  
>  #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 port)

port1, not port.

Alan Stern

Re: [RFC PATCH 1/4] usb: add struct usb_hub_port to store port related members.

[Alan Stern]

On Mon, 19 Mar 2012, Lan Tianyu wrote:

> Add struct usb_hub_port pointer port_data in the struct usb_hub and allocate
> struct usb_hub_port perspectively for each ports to store private data.

You might as well add the child device pointer into your new data 
structure.  This will require changes to at least three other files in 
addition to hub.c:

	devices.c, host/r8a66597-hcd.c,
	drivers/staging/usbip/usbip_common.c

Maybe some others; I didn't look through the entire kernel source.  It 
also means you will have to export a function to get a pointer to the 
child device, given the port number.

> @@ -1451,13 +1456,12 @@ void usb_hub_release_all_ports(struct usb_device *hdev, void *owner)
>  	int n;
>  	void **powner;
>  
> -	n = find_port_owner(hdev, 1, &powner);
> -	if (n == 0) {
> -		for (; n < hdev->maxchild; (++n, ++powner)) {
> -			if (*powner == owner)
> -				*powner = NULL;
> -		}
> +	for (n = 1; n <= hdev->maxchild; n++) {
> +		find_port_owner(hdev, n, &powner);
> +		if (*powner == owner)
> +			*powner = NULL;
>  	}

This is wrong; find_port_owner() doesn't set powner to anything if an 
error occurs.

You can just set the pointer field directly to NULL, instead of looking
it up in find_port_owner().  I don't remember why it was written that
way originally.

Alan Stern

Re: [RFC PATCH 1/4] usb: add struct usb_hub_port to store port related members.

[Lan Tianyu]

On 2012年03月20日 00:04, Alan Stern wrote:
> On Mon, 19 Mar 2012, Lan Tianyu wrote:
>
>> Add struct usb_hub_port pointer port_data in the struct usb_hub and allocate
>> struct usb_hub_port perspectively for each ports to store private data.
>
> You might as well add the child device pointer into your new data
> structure.  This will require changes to at least three other files in
> addition to hub.c:
hi alan:
	Great thanks for your review.
	But I still confuse about "You might as well add the child device pointer
into your new data structure." Could you help me to make it clear? :)
	Do you mean add struct usb_device pointer toward the device attached to the 
port in the
struct usb_hub_port? If yes,Why?
>
> 	devices.c, host/r8a66597-hcd.c,
> 	drivers/staging/usbip/usbip_common.c
>
> Maybe some others; I didn't look through the entire kernel source.  It
> also means you will have to export a function to get a pointer to the
> child device, given the port number.
	If it was necessary, could I fill the child pointer in the 
hub_port_connect_change()? just
after a new usb device being created.

static void hub_port_connect_change(struct usb_hub *hub, int port1,
					u16 portstatus, u16 portchange) {
		...
		/* Run it through the hoops (find a driver, etc) */
		if (!status) {
			status = usb_new_device(udev);
			if (status) {
				spin_lock_irq(&device_state_lock);
				hdev->children[port1-1] = NULL;
				spin_unlock_irq(&device_state_lock);
			}
			/* like here?*/
		}
		...
}

Thanks again. Look forward for your reply.
>
>> @@ -1451,13 +1456,12 @@ void usb_hub_release_all_ports(struct usb_device *hdev, void *owner)
>>   	int n;
>>   	void **powner;
>>
>> -	n = find_port_owner(hdev, 1,&powner);
>> -	if (n == 0) {
>> -		for (; n<  hdev->maxchild; (++n, ++powner)) {
>> -			if (*powner == owner)
>> -				*powner = NULL;
>> -		}
>> +	for (n = 1; n<= hdev->maxchild; n++) {
>> +		find_port_owner(hdev, n,&powner);
>> +		if (*powner == owner)
>> +			*powner = NULL;
>>   	}
>
> This is wrong; find_port_owner() doesn't set powner to anything if an
> error occurs.
>
> You can just set the pointer field directly to NULL, instead of looking
> it up in find_port_owner().  I don't remember why it was written that
> way originally.
>
> Alan Stern
>
>

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

Re: [RFC PATCH 1/4] usb: add struct usb_hub_port to store port related members.

[Alan Stern]

On Tue, 20 Mar 2012, Lan Tianyu wrote:

> On 2012年03月20日 00:04, Alan Stern wrote:
> > On Mon, 19 Mar 2012, Lan Tianyu wrote:
> >
> >> Add struct usb_hub_port pointer port_data in the struct usb_hub and allocate
> >> struct usb_hub_port perspectively for each ports to store private data.
> >
> > You might as well add the child device pointer into your new data
> > structure.  This will require changes to at least three other files in
> > addition to hub.c:
> hi alan:
> 	Great thanks for your review.
> 	But I still confuse about "You might as well add the child device pointer
> into your new data structure." Could you help me to make it clear? :)
> 	Do you mean add struct usb_device pointer toward the device attached to the 
> port in the
> struct usb_hub_port? If yes,Why?

Yes, that's what I mean.  It's a natural thing to do; the child device 
is directly associated with the port.  It's better than allocating a 
separate array for hdev->children.

> > 	devices.c, host/r8a66597-hcd.c,
> > 	drivers/staging/usbip/usbip_common.c
> >
> > Maybe some others; I didn't look through the entire kernel source.  It
> > also means you will have to export a function to get a pointer to the
> > child device, given the port number.
> 	If it was necessary, could I fill the child pointer in the 
> hub_port_connect_change()? just
> after a new usb device being created.
> 
> static void hub_port_connect_change(struct usb_hub *hub, int port1,
> 					u16 portstatus, u16 portchange) {
> 		...
> 		/* Run it through the hoops (find a driver, etc) */
> 		if (!status) {
> 			status = usb_new_device(udev);
> 			if (status) {
> 				spin_lock_irq(&device_state_lock);
> 				hdev->children[port1-1] = NULL;
> 				spin_unlock_irq(&device_state_lock);
> 			}
> 			/* like here?*/
> 		}
> 		...
> }

That's the right idea, but the wrong place.  The right place to fill 
in the pointer is a few lines higher, where the code already does

			hdev->children[port1-1] = udev;

while holding the device_state_lock.

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

Re: [RFC PATCH 1/4] usb: add struct usb_hub_port to store port related members.

[Lan Tianyu]

On 2012年03月20日 22:04, Alan Stern wrote:
> On Tue, 20 Mar 2012, Lan Tianyu wrote:
>
>> On 2012年03月20日 00:04, Alan Stern wrote:
>>> On Mon, 19 Mar 2012, Lan Tianyu wrote:
>>>
>>>> Add struct usb_hub_port pointer port_data in the struct usb_hub and allocate
>>>> struct usb_hub_port perspectively for each ports to store private data.
>>>
>>> You might as well add the child device pointer into your new data
>>> structure.  This will require changes to at least three other files in
>>> addition to hub.c:
>> hi alan:
>> 	Great thanks for your review.
>> 	But I still confuse about "You might as well add the child device pointer
>> into your new data structure." Could you help me to make it clear? :)
>> 	Do you mean add struct usb_device pointer toward the device attached to the
>> port in the
>> struct usb_hub_port? If yes,Why?
>
> Yes, that's what I mean.  It's a natural thing to do; the child device
> is directly associated with the port.  It's better than allocating a
> separate array for hdev->children.
OK. That' mean that I should replace the hdev->children with 
port_data->child_device.
In the hub.c, the struct usb_hub_port can be visited and the child_device can be 
accessed
directly. Out of hub.c, I should provide function to return child pointer. Is 
that right?
>
>>> 	devices.c, host/r8a66597-hcd.c,
>>> 	drivers/staging/usbip/usbip_common.c
>>>
>>> Maybe some others; I didn't look through the entire kernel source.  It
>>> also means you will have to export a function to get a pointer to the
>>> child device, given the port number.
>> 	If it was necessary, could I fill the child pointer in the
>> hub_port_connect_change()? just
>> after a new usb device being created.
>>
>> static void hub_port_connect_change(struct usb_hub *hub, int port1,
>> 					u16 portstatus, u16 portchange) {
>> 		...
>> 		/* Run it through the hoops (find a driver, etc) */
>> 		if (!status) {
>> 			status = usb_new_device(udev);
>> 			if (status) {
>> 				spin_lock_irq(&device_state_lock);
>> 				hdev->children[port1-1] = NULL;
>> 				spin_unlock_irq(&device_state_lock);
>> 			}
>> 			/* like here?*/
>> 		}
>> 		...
>> }
>
> That's the right idea, but the wrong place.  The right place to fill
> in the pointer is a few lines higher, where the code already does
>
> 			hdev->children[port1-1] = udev;
>
> while holding the device_state_lock.
>
> Alan Stern
>
>

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

Re: [RFC PATCH 1/4] usb: add struct usb_hub_port to store port related members.

[Alan Stern]

On Wed, 21 Mar 2012, Lan Tianyu wrote:

> >> hi alan:
> >> 	Great thanks for your review.
> >> 	But I still confuse about "You might as well add the child device pointer
> >> into your new data structure." Could you help me to make it clear? :)
> >> 	Do you mean add struct usb_device pointer toward the device attached to the
> >> port in the
> >> struct usb_hub_port? If yes,Why?
> >
> > Yes, that's what I mean.  It's a natural thing to do; the child device
> > is directly associated with the port.  It's better than allocating a
> > separate array for hdev->children.
> OK. That' mean that I should replace the hdev->children with 
> port_data->child_device.
> In the hub.c, the struct usb_hub_port can be visited and the child_device can be 
> accessed
> directly. Out of hub.c, I should provide function to return child pointer. Is 
> that right?

Right.

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