On Thu, 12 Dec 2013, Felipe Balbi wrote:
> > Felipe:
> >
> > Is the usb_enumerate_device_otg() routine really correct? It looks
> > like it will try to enable HNP whenever CONFIG_USB_OTG is set, even if
> > the root hub isn't OTG-capable.
> >
> > The routine does this:
> >
> > /* enable HNP before suspend, it's simpler */
> > if (port1 == bus->otg_port)
> > bus->b_hnp_enable = 1;
> > err = usb_control_msg(udev,
> > usb_sndctrlpipe(udev, 0),
> > USB_REQ_SET_FEATURE, 0,
> > bus->b_hnp_enable
> > ? USB_DEVICE_B_HNP_ENABLE
> > : USB_DEVICE_A_ALT_HNP_SUPPORT,
> > 0, NULL, 0, USB_CTRL_SET_TIMEOUT);
> > if (err < 0) {
> > /* OTG MESSAGE: report errors here,
> > * customize to match your product.
> > */
> > dev_info(&udev->dev,
> > "can't set HNP mode: %d\n",
> > err);
> > bus->b_hnp_enable = 0;
> > }
> >
> > Maybe the usb_control_msg and the error check should be inside the
> > first "if" statement?
>
> I don't think so, see below:
>
> | static int usb_enumerate_device_otg(struct usb_device *udev)
> | {
> | int err = 0;
> |
> | #ifdef CONFIG_USB_OTG
>
> first, we know OTG is enabled.
>
> | /*
> | * OTG-aware devices on OTG-capable root hubs may be able to use SRP,
> | * to wake us after we've powered off VBUS; and HNP, switching roles
> | * "host" to "peripheral". The OTG descriptor helps figure this out.
> | */
> | if (!udev->bus->is_b_host
> | && udev->config
> | && udev->parent == udev->bus->root_hub) {
> | struct usb_otg_descriptor *desc = NULL;
> | struct usb_bus *bus = udev->bus;
> |
> | /* descriptor may appear anywhere in config */
> | if (__usb_get_extra_descriptor (udev->rawdescriptors[0],
> | le16_to_cpu(udev->config[0].desc.wTotalLength),
> | USB_DT_OTG, (void **) &desc) == 0) {
> | if (desc->bmAttributes & USB_OTG_HNP) {
>
> this check ensures the port supports HNP
No, it doesn't. It ensures that the _device_ supports HNP. It has
nothing to do with the host port.
>
> | unsigned port1 = udev->portnum;
> |
> | dev_info(&udev->dev,
> | "Dual-Role OTG device on %sHNP port\n",
> | (port1 == bus->otg_port)
> | ? "" : "non-");
This is where the host port is tested.
> |
> | /* enable HNP before suspend, it's simpler */
> | if (port1 == bus->otg_port)
> | bus->b_hnp_enable = 1;
And here.
> this check tells is just to help you choose between enabling HNP on a B
> device, or telling the device that another root hub port supports HNP
>
> | err = usb_control_msg(udev,
> | usb_sndctrlpipe(udev, 0),
> | USB_REQ_SET_FEATURE, 0,
> | bus->b_hnp_enable
> | ? USB_DEVICE_B_HNP_ENABLE
> | : USB_DEVICE_A_ALT_HNP_SUPPORT,
>
> so, based on this, we're just enabling HNP on the device, but HNP won't
> kick in until the root hub port is suspended.
What if bus->otg_port is not equal to port1? Either the host supports
OTG on a different port or doesn't support it at all on this bus. Is
there any reason for enabling HNP on the device in those cases?
The point is that if this message fails (for example, if err is -EPIPE)
then device enumeration fails. Even if there was no reason to send
this message in the first place.
Alan Stern
--
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
