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

[Daniel Lezcano]

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