On Thu, Oct 24, 2013 at 12:09:03PM -0400, Alan Stern wrote: > On Thu, 24 Oct 2013, [iso-8859-2] J�nosi Zoli wrote: > > > https://bugzilla.kernel.org/show_bug.cgi?id=63611 > > � > > Bug ID: 63611 > > Summary: cant connect sony phones in mass storage mode > > Is CONFIG_USB_OTG turned on in your kernel's .config? If it is, try > building a new kernel with it turned off. Sorry for the long delay here. This got lost in my maildir for whatever reason. > 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 | unsigned port1 = udev->portnum; | | dev_info(&udev->dev, | "Dual-Role OTG device on %sHNP port\n", | (port1 == bus->otg_port) | ? "" : "non-"); | | /* enable HNP before suspend, it's simpler */ | if (port1 == bus->otg_port) | bus->b_hnp_enable = 1; 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. | 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; | } | } | } | } | | if (!is_targeted(udev)) { and, BTW, this needs to change. We shouldn't have a TPL *in* the kernel as that hinders the possibility of allowing USB Accessory manufactures to ship their driver in an App Store of some sort. Basically, is_targeted() should return false only after we know the device doesn't bind to any of our USB drivers. But that's for future, as that'd be a *really* invasive change to usbcore. hope this helps. -- balbi
Attachment:
signature.asc
Description: Digital signature
