Re: [PATCH 0/1] pwm: meson: fix scheduling while atomic issue

From: Jerome Brunet <jbrunet@baylibre.com>
To: Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
	thierry.reding@gmail.com, narmstrong@baylibre.com,
	linux-pwm@vger.kernel.org, linux-amlogic@lists.infradead.org
Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 0/1] pwm: meson: fix scheduling while atomic issue
Date: Mon, 25 Mar 2019 10:35:12 +0100	[thread overview]
Message-ID: <47e944da1c3b0a11cf46fc5ad5678ba961b9f9d3.camel@baylibre.com> (raw)
In-Reply-To: <20190324220217.15813-1-martin.blumenstingl@googlemail.com>

On Sun, 2019-03-24 at 23:02 +0100, Martin Blumenstingl wrote:
> Back in January a "BUG: scheduling while atomic" error showed up during
> boot on my Meson8b Odroid-C1 (which uses a PWM regulator as CPU supply).
> The call trace comes down to:
>   __mutex_lock
>   clk_prepare_lock
>   clk_core_get_rate
>   meson_pwm_apply
>   ..
>   dev_pm_opp_set_rate
>   ..
> 
> Jerome has also seen the same problem but from pwm-leds (instead of a
> pwm-regulator). He posted a patch which replaces the spinlock with a
> mutex. That works. I believe we can optimize this by reducing the time
> where the lock is held - that also allows to keep the spin-lock.
> 
> Analyzing this issue helped me understand the pwm-meson driver better.
> My plan is to send some cleanups (with the goal of re-using more of the
> goodies from the PWM core in the pwm-meson driver) after this single fix
> is merged (they can be found here: [1]).

Thanks for fixing this Martin.

As for the future enhancement, I'd like to know what you have in mind.
As I have told you previously, I think the clock bindings of this driver are
not great.

The global name of the input clocks are hard coded in this driver and it
sucks. CCF is evolving to rely less on these global names.

In addition, the 'clock' binding should be used to refer to the clock
'consumed' by the device, not to define a setting (as done now). 'assigned-
clock' binding can be used for that.

This would be a significant change in the binding meaning of this driver,
which probably calls for a v2.

Last, instead of specifying the parent to be used, I think we should come up
with some code to let the driver pick the most appropriate parent for the period/duty requested.

> 
> Dependencies: none
> 
> Target version: please queue this for -fixes so it makes it's way into
> v5.1-rc (so we can get it backported from there, because this issue has
> existed since the pwm-meson driver was introduced).
> 
> 
> [0] http://lists.infradead.org/pipermail/linux-amlogic/2019-January/009690.html
> [1] https://github.com/xdarklight/linux/commits/meson-pwm-for-5.2-v0
> 
> 
> Martin Blumenstingl (1):
>   pwm: meson: use the spin-lock only to protect register modifications
> 
>  drivers/pwm/pwm-meson.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 