Le 19/01/2015 21:15, Boris Brezillon a écrit :
> On Mon, 19 Jan 2015 18:46:31 +0100
> Sylvain Rochet <sylvain.rochet@xxxxxxxxxxxx> wrote:
>
>> 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.
Typo: s/stay/stays/
>> 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.
Sylvain, thanks for having rephrased this part! It looks clear.
>>>> /* 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.
>
> According to the comment placed above this test it's here to prevent
> any action triggered by the vbus pin irq while the driver is not
> properly probed yet.
> You could use:
>
> set_irq_flags(vbus_irq, IRQF_NOAUTOEN);
>
> before calling devm_request_threaded_irq.
> This will keep the irq disabled instead of enabling it at request time.
> Moreover, this simplify the vbus_irq request code (which disables the
> irq just after requesting it).
>
>>
>> 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.
>
> And yes, disable_irq waits for all irq handler termination, including
> threaded irqs: see [1], [2].
>
>>
>>
>> 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.
>
> If you rework the vbus_irq request as I suggested I think you can get
> rid of this !udc->driver test, and thus drop the locking around it ;-).
>
> Best Regards,
>
> Boris
>
> [1]http://lxr.free-electrons.com/source/kernel/irq/manage.c#L432
> [2]http://lxr.free-electrons.com/source/kernel/irq/manage.c#L92
Thanks Sylvain for having described the problem lengthly and Boris for
your detailed explanation and suggestion concerning this sequence.
So, if you can re-send a new version and also add the stable tags as
suggested by Felipe, it would be really cool.
Bye,
--
Nicolas Ferre
--
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
