On Tuesday 29 May 2007, Tony Lindgren wrote:
> * David Brownell <[EMAIL PROTECTED]> [070529 12:18]:
> > On Friday 25 May 2007, Tony Lindgren wrote:
> > > +
> >
> > 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 :)
Even so. Been there, done that ... in this case, changing
two things at once was not a win. :)
> > > + /* 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.
Right, but as I pointed out elsewhere: that applies to the exit
path. What you did here was remove the startup path, by
completely eliminating the essential "let the host actually
use the peripheral first" logic ... before hitting the exit path.
> > > + 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?
Leave it out here. Any HNP-aware root hub can msleep
if necessary ... that's often handled by the hardware.
> > 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.
The current Linux-USB OTG support expects to always take
advantage of HNP if it's offered, even if just to check
whether there is anything the B-default device should do
in host mode.
> I see your point though with "autoidle", but that's after
> the devices have enumerated the wrong way, right?
You want me to look at that code again? Aargh!
Originally there was no autoidle, so the logic was:
the only way Linux will *EVER* trigger suspend at
runtime was to try HNP. Devices that don't pass the
whitelist test would enumerate no further.
Now we have working auto-idle, so other logic could
be applied. I think you're suggesting that maybe
they could proceed through enumeration, fail that,
and then autosuspend to trigger HNP ... ? That might
be workable.
The behavioral difference would be that WHEN ("not 'IF'")
the whitelist (which is very easily checked against product
documentation) diverges from the list of configured drivers
(no easy way to crosscheck that and docs) things would not
act the same.
> If on
> the b_device user selects b_host mode, it can start
> right away when the devices are connected.
I don't know about this "user selects B_HOST" mode step.
Previously such a step did not exist. The goal was to
require as little user interface support as possible.
> Maybe we should offer HNP "early" and every "autoidle"?
I'm a minimalist here ... unless someone has a new use
case for HNP, avoid kicking it in. Most folk feel it's
a bit of a solution in search of problem, other than the
very basic "hooked cable up the wrong way" scenario.
Of course, an updated N800 with full OTG support sure
ought to be able to trigger some creativity in that
area ... :)
We may need to avoid kicking in HNP on autoidle, since
that seems to imply a disconnect of that session. But
that boils down to: either HNP at the start, for a
device that's not whitelisted ... or HNP way later.
A single "do HNP now" routine would be useful; it
could be used in any of those scenarios.
> > > + 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:
This is more or less what you were trying to achieve,
yes? But it leads to surprising behavior in cases
like:
* hook up non-OTG peripheral #1 ... acts just
the way you'd normally expect
* hook up OTG peripheral #2 ... surprise! it
refuses to act as a peripheral at first.
The "principle of least astonishment" argues that
the "peripheral #1" model should be followed for
as long as possible. Customer service calls would
be a lot simpler too...
> - Enable HNP "early" (later would be OK too);
> - Offer to do HNP if the peripheral wants to use it
Host doesn't know anything about "peripheral wants";
from its perspective, this is an blind offer to go
down the "peripheral #2" path.
> (Peripheral maintains _both_ b_hnp_enable set by
> a_host and user preference on b_device on using
> b_hnp_enable)
That "user preference" is problematic. What do you
end up with if that requirement for a user choice is
removed ... ?
> ... deletia ...
Glad to know the mechanisms are working. But right
now I'm puzzled why musb_hdrc is misbehaving in
host mode ... it's mangling descriptors as it reads
them in.
- Dave
-------------------------------------------------------------------------
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
