* David Brownell <[EMAIL PROTECTED]> [070529 12:18]:
> 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!
Yes, I tried, but the nesting was getting too deep :)
> > + /* 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).
Hmm, in the USB_OTG_1-3.pdf it says the host must offer HNP to
peripherals that support HNP during the session. In my patch
we offer it right away if the peripheral wants to use it.
Quoting few sentences from USB_OTG_1-3.pdf 6.4.1.4 a_host:
"If the SetFeature(b_hnp_enable) command is sent and accepted
during the session, then the A-device shall exit to the a_suspend
state and wait for the B-device to start a session (See Section 6.5.2)"
So the way I understand "sent and accepted" above, it's up to the
b_device to use or not use the HNP session offered.
> > + 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.
Sorry, I don't understand how you prefer the 3.125 ms
timeout handled, can you please clarify? Just leave it out?
> > +
> > + /* 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.
OK
> 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.
Yes, but as I understand it's up to the peripheral to use
the HNP or not when offered. So unless b_host is enabled
on peripheral, HNP does not get used.
I see your point though with "autoidle", but that's after
the devices have enumerated the wrong way, right? If on
the b_device user selects b_host mode, it can start
right away when the devices are connected.
Maybe we should offer HNP "early" and every "autoidle"?
> > + 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.
AFAIK, the logic should be:
- Enable HNP "early" (later would be OK too);
- Offer to do HNP if the peripheral wants to use it
(Peripheral maintains _both_ b_hnp_enable set by
a_host and user preference on b_device on using
b_hnp_enable)
- For peripheral wanting to become b_host, suspend
and let b_host take over the bus
- For peripheral not wanting to become b_host, check
whitelist
- For whitelisted devices, proceed normally
- Reject non-whitelisted devices
But this I've only verified working with two N800s and
usbcv13.exe, so I guess we'll have to see what behaves
with OPT tests.
Regards,
Tony
-------------------------------------------------------------------------
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
