On Friday 25 May 2007, Tony Lindgren wrote:
> +
Mostly this patch just seems to move a block of code into a
standalone function, removing some nesting ... which is OK
as a cleanup, but makes it hard to see the substantive changes
in this routine.
So it'd be best to see a minimal patch just fixing any bugs...
Those substantive changes unfortunately include inserting
bugs ... which would have been easier to see if they were
not interspersed with cleanups!
> + /* If B-device wants to do HNP, it will do it after we suspend */
> + if (bus->b_hnp_enable) {
> + dev_err(&udev->dev, "trying HNP, suspending bus\n");
> + err = __usb_port_suspend(udev, udev->bus->otg_port);
No, it shouldn't suspend at this point. If the device is in
the targeted peripherals list, the host must continue to try
using the device ... using the normal code paths.
Here, you've changed it to immediately try HNP, even when
the host wants to handle the targeted device (that is, when
A_BUS_REQ is true).
> + if (err < 0) {
> + dev_err(&udev->dev, "suspend for HNP failed\n");
> + return -ENODEV;
> + }
> +
> + /* TB_ASE0_BRST, minimum 3.125 ms */
> + mdelay(4);
The assumption was that lower level code would manage
T(A/SE0 --> B/RST) as part of triggering HNP in the root
hub ... the root hub can always tell if it's triggering
HNP, so that timeout applies. Since that timeout tends
to have hardware support, I'd rather not see it here.
> +
> + /* Failed resume means B-device took over the bus with HNP */
There's another suspend/resume sequence to consider.
That's one triggered by autosuspend ... or in terms
of the OTG spec, when A_BUS_REQ goes false.
The original code sequence -- only trigger HNP for
unsupported devices -- handled normal use cases
correctly: hook up an OTG capable peripheral to the
OTG host, it gets used as usual until everything goes
idle, then it suspends ("autoidle"), and attempts HNP.
If HNP succeeds, work gets done the other direction
(host and peripheral roles switched), until *that* is
idlle, at which point b_host suspends; disconnect.
This modified code sequence skips that primary usage:
the peripheral can't be used as a peripheral, since
it immediately jumps into host mode.
> + err = usb_port_resume(udev);
> + if (err < 0) {
> + dev_err(&udev->dev, "HNP success\n");
Not an error. Except in the sense that it shouldn't
have been tried here at all ... not a _runtime_ error.
> + return -ENODEV;
> + }
> + }
> +
> + /* HNP failed. Reject B-devices not in our whitelist */
> + if (!is_targeted(udev))
... and here you fail for the wrong reason.
The original logic was OTG-conformant:
- Enable HNP "early" (later would be OK too);
- For non-whitelisted devices:
* trigger HNP if possible
* never proceed any further
- For whitelisted devices (*)
* proceed normally
* implicitly, autosuspend triggers HNP
This patch removes the primary (*) branch.
- Dave
> + return -ENODEV;
> +
> + return 0;
-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
