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

[ ... ]

quoted

quoted

+{
+	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.

quoted

quoted

+	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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel