linux-kernel - Re: [PATCH v4 3/3] clocksource: Add Low Power STM32 timers driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 20 Feb 2020 12:05:24 +0100
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     Benjamin GAIGNARD <benjamin.gaignard@...com>,
        "lee.jones@...aro.org" <lee.jones@...aro.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "mcoquelin.stm32@...il.com" <mcoquelin.stm32@...il.com>,
        Alexandre TORGUE <alexandre.torgue@...com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        Fabrice GASNIER <fabrice.gasnier@...com>
Cc:     "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-stm32@...md-mailman.stormreply.com" 
        <linux-stm32@...md-mailman.stormreply.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Benjamin Gaignard <benjamin.gaignard@...aro.org>,
        Pascal PAILLET-LME <p.paillet@...com>
Subject: Re: [PATCH v4 3/3] clocksource: Add Low Power STM32 timers driver

On 20/02/2020 11:45, Benjamin GAIGNARD wrote:

[ ... ]

>>> +{
>>> +	struct stm32_lp_private *priv = to_priv(clkevt);
>>> +
>>> +	/* disable LPTIMER to be able to write into IER register*/
>>> +	regmap_write(priv->reg, STM32_LPTIM_CR, 0);
>>> +	/* enable ARR interrupt */
>>> +	regmap_write(priv->reg, STM32_LPTIM_IER, STM32_LPTIM_ARRMIE);
>>> +	/* enable LPTIMER to be able to write into ARR register */
>>> +	regmap_write(priv->reg, STM32_LPTIM_CR, STM32_LPTIM_ENABLE);
>>> +	/* set next event counter */
>>> +	regmap_write(priv->reg, STM32_LPTIM_ARR, evt);
>>> +
>>> +	/* start counter */
>>> +	if (is_periodic)
>>> +		regmap_write(priv->reg, STM32_LPTIM_CR,
>>> +			     STM32_LPTIM_CNTSTRT | STM32_LPTIM_ENABLE);
>>> +	else
>>> +		regmap_write(priv->reg, STM32_LPTIM_CR,
>>> +			     STM32_LPTIM_SNGSTRT | STM32_LPTIM_ENABLE);
>> The regmap config in stm32-lptimer is not defined with the fast_io flag
>> (on purpose or not?) that means we can potentially deadlock here as the
>> lock is a mutex.
>>
>> Isn't it detected with the lock validation scheme?
> It wasn't a problem with other parts of the mfd and I don't notice 
> issues so I guess it is ok.

Given your comment below, the case can't happen I agree but there is
still a heavy operation with the locking.

>>> +	return 0;
>>> +}
>>> +static int stm32_clkevent_lp_remove(struct platform_device *pdev)
>>> +{
>>> +	return -EBUSY; /* cannot unregister clockevent */
>>> +}
>> Won't be the mfd into an inconsistent state here? The other subsystems
>> will be removed but this one will prevent to unload the module leading
>> to a situation where the mfd is partially removed but still there
>> without a possible recovery, no?
> We can't enable the timer part of the mfd at the same time than the 
> other features.

Hmm, interesting case. The DT binding does not give this information,
especially in the example. You should fix the DT by giving two examples IMO.

Rob, how do you describe this situation (exclusive properties)?

> It has be exclusive and that exclude the problem you describe above.

Ok, the regmap_write is not a free operation and in this case you can
get rid of all the regmap-ish in this driver.

-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog