Ok, so it looks like Alan finally acked all the port power off patches.
Greg, how are you feeling about taking these for 3.15? I hear that
Linus may have to release an -rc8 or even -rc9, so they could go in and
still have some time to marinate in linux-next, but it's really your
call.
I usually wouldn't accept patches this late, but Alan's done a good job
of ferreting out all the corner cases, so I have high confidence the
patches will work. They're also highly unlikely to have any impact on
systems that don't have the port power off mechanism.
It's up to you. Either way, I'll compile a kernel with these patches
for my non-Intel host machine (NEC 0.96 host) and make sure they don't
break anything.
Sarah Sharp
On Thu, Mar 20, 2014 at 09:21:56AM -0700, Dan Williams wrote:
> On Thu, 2014-03-20 at 11:58 -0400, Alan Stern wrote:
> > On Wed, 19 Mar 2014, Dan Williams wrote:
> >
> > > Once usb-acpi has set the port's connect type the usb_device's
> > > ->removable attribute can be set in the standard location
> > > set_usb_port_removable().
> > >
> > > This also changes behavior in the case where the firmware says that the
> > > port connect type is unknown. In that case just use the default setting
> > > determined from the hub descriptor.
> > >
> > > Note, we no longer pass udev->portnum to acpi_find_child_device() in the
> > > root hub case since:
> > > 1/ the usb-core sets this to zero
> > > 2/ acpi always expects zero
> > > ...just pass zero.
> > >
> > > Suggested-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> > > Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> >
> > This is the one patch I haven't ACKed yet from among the first ten.
> >
> > > @@ -155,40 +155,18 @@ static struct acpi_device *usb_acpi_find_companion(struct device *dev)
> > > */
> > > if (is_usb_device(dev)) {
> > > udev = to_usb_device(dev);
> > > - port1 = udev->portnum;
> > > - if (udev->parent) {
> > > - struct usb_hub *hub;
> > > -
> > > - hub = usb_hub_to_struct_hub(udev->parent);
> > > - /*
> > > - * According usb port's connect type to set usb device's
> > > - * removability.
> > > - */
> > > - switch (hub->ports[port1 - 1]->connect_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;
> > > - }
> > > -
> > > + if (udev->parent)
> > > return NULL;
> > > - }
> >
> > We should try to find the device's ACPI companion here. That can be
> > added in a later patch, though.
> >
> > >
> > > - /* root hub's parent is the usb hcd. */
> > > - return acpi_find_child_device(ACPI_COMPANION(dev->parent),
> > > - port1, false);
> > > + /* root hub is only child (_ADR=0) under it's parent, the hcd */
> >
> > s/it's/its/
> >
> > Remember, "it's" is a contraction of "it is".
>
> Not sure that extra clarification was worth the keystrokes, but typo
> fixed.
>
> >
> > s/the hcd/the HC/
> >
> > "hcd" refers either to a data structure (struct usb_hcd) or a Host
> > Controller Driver. But here you are referring to the host controller
> > itself, or the ACPI device corresponding to it.
>
> ok
>
> >
> > > + adev = ACPI_COMPANION(dev->parent);
> > > + return acpi_find_child_device(adev, 0, false);
> > > } else if (is_usb_port(dev)) {
> > > struct usb_port *port_dev = to_usb_port(dev);
> > > - struct acpi_device *adev = NULL;
> > > + int port1 = port_dev->portnum;
> > >
> > > /* Get the struct usb_device point of port's hub */
> > > udev = to_usb_device(dev->parent->parent);
> > > - port1 = port_dev->portnum;
> > >
> > > /*
> > > * The root hub ports' parent is the root hub. The non-root-hub
> >
> > The rest of this comment says "The non-root-hub ports' parent is the
> > parent hub port which the hub is connected to." I'm not sure that's
> > really true.
> >
> > In the ACPI code example shown in section D.1.1 of the xHCI spec, the
> > device path for port IP1 (which is a port on the integrated hub) is
> > USB0.RHUB.HCP2.IHUB.IP1. Thus, IP1's parent is the integrated hub,
> > IHUB, not the hub's upstream port, HCP2.
> >
> > This also should be fixed later.
>
> Indeed, I was overlooking the correctness of the existing
> implementation. It seems it does need more revision.
>
> > For now, after you fix the spelling
> > errors above, you can add
> >
> > Acked-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> >
>
> Thanks Alan.
>
> 8<-------
> Subject: usb: cleanup setting udev->removable from port_dev->connect_type
>
> From: Dan Williams <dan.j.williams@xxxxxxxxx>
>
> Once usb-acpi has set the port's connect type the usb_device's
> ->removable attribute can be set in the standard location
> set_usb_port_removable().
>
> This also changes behavior in the case where the firmware says that the
> port connect type is unknown. In that case just use the default setting
> determined from the hub descriptor.
>
> Note, we no longer pass udev->portnum to acpi_find_child_device() in the
> root hub case since:
> 1/ the usb-core sets this to zero
> 2/ acpi always expects zero
> ...just pass zero.
>
> Suggested-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> Acked-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> ---
> drivers/usb/core/hub.c | 22 +++++++++++++++++-----
> drivers/usb/core/usb-acpi.c | 34 ++++++----------------------------
> 2 files changed, 23 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index a946cf5c4222..10c9b52ca0b9 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -2291,6 +2291,22 @@ static void set_usb_port_removable(struct usb_device *udev)
> udev->removable = USB_DEVICE_REMOVABLE;
> else
> udev->removable = USB_DEVICE_FIXED;
> +
> + /*
> + * Platform firmware may have populated an alternative value for
> + * removable. If the parent port has a known connect_type use
> + * that instead.
> + */
> + switch (hub->ports[udev->portnum - 1]->connect_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: /* use what was set above */
> + break;
> + }
> }
>
> /**
> @@ -2360,11 +2376,7 @@ int usb_new_device(struct usb_device *udev)
>
> device_enable_async_suspend(&udev->dev);
>
> - /*
> - * check whether the hub marks this port as non-removable. Do it
> - * now so that platform-specific data can override it in
> - * device_add()
> - */
> + /* check whether the hub or firmware marks this port as non-removable */
> if (udev->parent)
> set_usb_port_removable(udev);
>
> diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
> index f91ef0220066..d3e7e1b4125e 100644
> --- a/drivers/usb/core/usb-acpi.c
> +++ b/drivers/usb/core/usb-acpi.c
> @@ -136,8 +136,8 @@ out:
>
> static struct acpi_device *usb_acpi_find_companion(struct device *dev)
> {
> - int port1;
> struct usb_device *udev;
> + struct acpi_device *adev;
> acpi_handle *parent_handle;
>
> /*
> @@ -155,40 +155,18 @@ static struct acpi_device *usb_acpi_find_companion(struct device *dev)
> */
> if (is_usb_device(dev)) {
> udev = to_usb_device(dev);
> - port1 = udev->portnum;
> - if (udev->parent) {
> - struct usb_hub *hub;
> -
> - hub = usb_hub_to_struct_hub(udev->parent);
> - /*
> - * According usb port's connect type to set usb device's
> - * removability.
> - */
> - switch (hub->ports[port1 - 1]->connect_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;
> - }
> -
> + if (udev->parent)
> return NULL;
> - }
>
> - /* root hub's parent is the usb hcd. */
> - return acpi_find_child_device(ACPI_COMPANION(dev->parent),
> - port1, false);
> + /* root hub is only child (_ADR=0) under its parent, the HC */
> + adev = ACPI_COMPANION(dev->parent);
> + return acpi_find_child_device(adev, 0, false);
> } else if (is_usb_port(dev)) {
> struct usb_port *port_dev = to_usb_port(dev);
> - struct acpi_device *adev = NULL;
> + int port1 = port_dev->portnum;
>
> /* Get the struct usb_device point of port's hub */
> udev = to_usb_device(dev->parent->parent);
> - port1 = port_dev->portnum;
>
> /*
> * The root hub ports' parent is the root hub. The non-root-hub
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
