[PATCHv3 2/2] USB: gadget: atmel_usba_udc: Enable/disable USB PLL on Vbus change

[sylvain.rochet@finsecur.com (Sylvain Rochet)]

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