On Sun, 28 Nov 2004, David Brownell wrote:
> Hi Alan,
Hi!
> On Sunday 28 November 2004 08:30, Alan Stern wrote:
> >
> > I suspect that root-hub polling shouldn't be controlled by flags within
> > the hcd structure. The state transitions are under the control of the
> > driver, so it would be wrong to make the core responsible for following
> > those transitions and adjusting the polling to match.
>
> It works today though ... why wouldn't a flag "use root hub timer"
> suffice?
Suppose the flag was clear so the timer wasn't running, but then the
driver decides that it wants to start polling. Setting a flag won't be
enough to start the timer...
And don't say this should never happen! Intel UHCI controllers send IRQs
for port-change events when the controller is suspended, but not when it
is running.
> > Instead we can add
> > some new core routines:
> >
> > usb_hcd_enable_rh_polling(),
> > usb_hcd_disable_rh_polling(),
> > usb_hcd_disable_rh_polling_sync().
> >
> > (The last is intended for when the HC is suspended and we need to be sure
> > no poll requests will arrive while the registers are inaccessible.)
>
> I really don't see a need for that variant. If polling is
> disabled, it's disabled -- and if registers aren't available,
> it had better be disabled! Note that those routines imply
> the existence of a flag. (Except for the "no registers" one!)
>
> Having a functional API to enable/disable the polling would
> be fine, if the calls were accessible in_irq().
Come to think of it, since the timer itself is directly accessible in the
hcd structure, we don't need these API calls at all. The driver can
simply call mod_timer, del_timer, and del_timer_sync.
> > To go along with this, the following HC states are appropriate for UHCI:
> >
> > HCD_STATE_STOPPED,
> > HCD_STATE_RUNNING,
> > HCD_STATE_RH_SUSPENDED,
> > HCD_STATE_CORE_SUSPENDED,
> > HCD_STATE_HC_SUSPENDED,
> > HCD_STATE_DEAD.
> >
> > Along with those states there needs to be an IN_TRANSIT modifier,
> > because controllers take some time to change their state (for instance, a
> > controller won't stop from a RUNNING state until the current transaction
> > has completed). There also needs to be a spinlock to protect the state.
>
> The existing model has a TRANSIENT flag, and at one time there was
> a locking framework. The (re)initialization sequences probably need
> as much work as anything else though.
>
>
> > Is this outline appropriate for other HCs as well as UHCI? Should it
> > replace the collection of states you have already set up in the hcd
> > structure? Or should it remain in the private uhci_hcd structure?
>
> If you really need them -- then internal to UHCI. But I don't see
> why you should need new states ... CORE_SUSPENDED would imply
> that root_dev->state == USB_STATE_SUSPENDED, RH_SUSPENDED should
> be invisible to usbcore, and I think RUNNING and DEAD are clear
> matches for the existing RUNNING and HALT.
It's a problem of synchronization. root_hub->state doesn't get changed to
USB_STATE_SUSPENDED at the right time (i.e., while holding the spinlock I
described above). I'm concerned about races occurring during the actual
transitions, because there are at least three independent pathways for
changing the state:
HCD root-hub autosuspend triggered by an HCD-private timer,
HCD root-hub autoresume because of non-zero status during
rh status check (triggered by poll timer or IRQ), and
usbcore suspend/resume running in process context with the
root hub locked.
You are correct that RH_SUSPENDED should be invisible to usbcore. Since
drivers will need to know about it, and thought the hcd structure might
be a good common place to store it. But never mind, I'll make all this
stuff private to uhci-hcd.
Alan Stern
-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://productguide.itmanagersjournal.com/
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
