From: nicolas.ferre@atmel•com (Nicolas Ferre)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCHv3 2/2] USB: gadget: atmel_usba_udc: Enable/disable USB PLL on Vbus change
Date: Tue, 20 Jan 2015 10:02:06 +0100 [thread overview]
Message-ID: <54BE198E.4040808@atmel.com> (raw)
In-Reply-To: <20150119211532.0759b537@bbrezillon>
Le 19/01/2015 21:15, Boris Brezillon a ?crit :
> On Mon, 19 Jan 2015 18:46:31 +0100
> Sylvain Rochet <sylvain.rochet@finsecur•com> 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
next prev parent reply other threads:[~2015-01-20 9:02 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-01-16 22:21 [PATCH] USB: gadget: atmel_usba_udc: Enable/disable USB PLL on Vbus change Sylvain Rochet
2015-01-17 1:42 ` Alexandre Belloni
2015-01-17 9:43 ` Boris Brezillon
2015-01-17 11:07 ` Sylvain Rochet
2015-01-18 14:51 ` [PATCHv2 0/2] " Sylvain Rochet
2015-01-18 14:51 ` [PATCHv2 1/2] USB: gadget: atmel_usba_udc: Fixed vbus_prev initial state Sylvain Rochet
2015-01-18 14:51 ` [PATCH 2/2] USB: gadget: atmel_usba_udc: Enable/disable USB PLL on Vbus change Sylvain Rochet
2015-01-18 16:56 ` Boris Brezillon
2015-01-18 17:24 ` [PATCHv3 0/2] " Sylvain Rochet
2015-01-18 17:24 ` [PATCHv3 1/2] USB: gadget: atmel_usba_udc: Fixed vbus_prev initial state Sylvain Rochet
2015-01-19 14:09 ` Nicolas Ferre
2015-01-19 18:55 ` Felipe Balbi
2015-01-20 11:02 ` Sylvain Rochet
2015-01-20 11:11 ` Nicolas Ferre
2015-01-18 17:24 ` [PATCHv3 2/2] USB: gadget: atmel_usba_udc: Enable/disable USB PLL on Vbus change Sylvain Rochet
2015-01-19 16:55 ` Nicolas Ferre
2015-01-19 17:46 ` Sylvain Rochet
2015-01-19 20:15 ` Boris Brezillon
2015-01-20 9:02 ` Nicolas Ferre [this message]
2015-01-18 17:34 ` [PATCH " Boris Brezillon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=54BE198E.4040808@atmel.com \
--to=nicolas.ferre@atmel$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox