Hello Nicolas,
On Mon, Jan 19, 2015 at 05:55:18PM +0100, Nicolas Ferre wrote:
> Le 18/01/2015 18:24, Sylvain Rochet a écrit :
> > Prepare_enable on rising edge, disable_unprepare on falling edge. Reduce
>
> Please re-write which "edge" we are talking about: "... falling edge of
> the Vbus signal" for example.
>
> > power consumption if USB PLL is not already necessary for OHCI or EHCI.
>
> Is a verb missing in the previous sentence?
>
> > If USB host is not connected we can sleep with USB PLL stopped.
> >
> > This driver does not support suspend/resume yet, not suspending if we
> > are still attached to an USB host is fine for what I need, this patch
> > allow suspending with USB PLL stopped when USB device is not currently
> > used.
>
> Can you please make it more clear. Several separated sentences seem
> necessary here.
Maybe :)
Proposal:
Start clocks on rising edge of the Vbus signal, stop clocks on
falling edge of the Vbus signal.
If USB PLL is not necessary for other USB drivers (e.g. OHCI and EHCI)
it will reduce power consumption by switching off the USB PLL if no USB
Host is currently connected to this USB Device.
We are using Vbus GPIO signal to detect Host presence. If Vbus signal is
not available then the device stay continuously clocked.
Note this driver does not support suspend/resume yet, it may stay
clocked if USB Host is still connected when suspending. For what I need,
forbidding suspend from userland if we are still attached to an USB host
is fine, but we might as well add suspend/resume to this driver in the
future.
> > /* May happen if Vbus pin toggles during probe() */
> > - if (!udc->driver)
> > + spin_lock_irqsave(&udc->lock, flags);
> > + if (!udc->driver) {
> > + spin_unlock_irqrestore(&udc->lock, flags);
> > goto out;
> > + }
> > + spin_unlock_irqrestore(&udc->lock, flags);
>
> I'm not sure that the protection by spin_lock is needed above.
I'm not sure too, it was already in a spinlock area, I obviously kept it
because it was not the purpose of this patch.
This seem to be in mirror of atmel_usba_start() which does:
spin_lock_irqsave(&udc->lock, flags);
udc->devstatus = 1 << USB_DEVICE_SELF_POWERED;
udc->driver = driver;
spin_unlock_irqrestore(&udc->lock, flags);
… but vbus_pin IRQ is not yet enabled.
Same for atmel_usba_stop() which disable vbus_pin IRQ before setting
udc->driver to NULL, but without spinlock this time (why?, this should
be consistent IMHO).
I don't know if it is guaranteed that IRQ does not fire nor continue to
run after disable_irq() returns, especially for threaded IRQ.
If the following sequence might happen:
atmel_usba_stop()
disable_irq(vbus)
usba_vbus_irq_thread() called lately
check for (!udc->driver) and continue
udc->driver = NULL;
if (udc->driver->disconnect)
*CRASH*
Then the patch should be modified to protect udc->driver with the vbus
mutex.
In this case the previous implementation wasn't perfect too, the
atmel_usba_stop() does not lock around the NULLification of driver,
Moreover the spinlock is (and was) unlocked in VBUS interrupt just
before calling udc->driver->disconnect, which makes udc->driver actually
not locked anywere.
If the previous sequence possible ? If no, udc->driver does not need
locking, if yes, it is currently not locked enough.
Sylvain
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
