Re: [PATCH] PM / clk: make PM clock layer compatible with clocks that must sleep

[Nicolas Pitre]

On Tue, 19 Jan 2021, Geert Uytterhoeven wrote:

> Hi Kevin, Nicolas,
> 
> On Tue, Jan 19, 2021 at 7:45 PM Kevin Hilman <khilman@xxxxxxxxxxxx> wrote:
> > [ + Geert.. renesas SoCs are the primary user of PM clk ]
> 
> Thanks!
> 
> > Nicolas Pitre <npitre@xxxxxxxxxxxx> writes:
> > > The clock API splits its interface into sleepable ant atomic contexts:
> > >
> > > - clk_prepare/clk_unprepare for stuff that might sleep
> > >
> > > - clk_enable_clk_disable for anything that may be done in atomic context
> > >
> > > The code handling runtime PM for clocks only calls clk_disable() on
> > > suspend requests, and clk_enable on resume requests. This means that
> > > runtime PM with clock providers that only have the prepare/unprepare
> > > methods implemented is basically useless.
> > >
> > > Many clock implementations can't accommodate atomic contexts. This is
> > > often the case when communication with the clock happens through another
> > > subsystem like I2C or SCMI.
> > >
> > > Let's make the clock PM code useful with such clocks by safely invoking
> > > clk_prepare/clk_unprepare upon resume/suspend requests. Of course, when
> > > such clocks are registered with the PM layer then pm_runtime_irq_safe()
> > > can't be used, and neither pm_runtime_suspend() nor pm_runtime_resume()
> > > may be invoked in atomic context.
> > >
> > > For clocks that do implement the enable and disable methods then
> > > everything just works as before.
> > >
> > > Signed-off-by: Nicolas Pitre <npitre@xxxxxxxxxxxx>
> 
> Thanks for your patch!
> 
> > > --- a/drivers/base/power/clock_ops.c
> > > +++ b/drivers/base/power/clock_ops.c
> 
> > > +/**
> > > + * pm_clk_op_lock - ensure exclusive access for performing clock operations.
> > > + * @psd: pm_subsys_data instance corresponding to the PM clock entry list
> > > + *    and clk_op_might_sleep count being used.
> > > + * @flags: stored irq flags.
> > > + * @fn: string for the caller function's name.
> > > + *
> > > + * This is used by pm_clk_suspend() and pm_clk_resume() to guard
> > > + * against concurrent modifications to the clock entry list and the
> > > + * clock_op_might_sleep count. If clock_op_might_sleep is != 0 then
> > > + * only the mutex can be locked and those functions can only be used in
> > > + * non atomic context. If clock_op_might_sleep == 0 then these functions
> > > + * may be used in any context and only the spinlock can be locked.
> > > + * Returns -EINVAL if called in atomic context when clock ops might sleep.
> > > + */
> > > +static int pm_clk_op_lock(struct pm_subsys_data *psd, unsigned long *flags,
> > > +                       const char *fn)
> > > +{
> > > +     bool atomic_context = in_atomic() || irqs_disabled();
> 
> Is this safe? Cfr.
> https://lore.kernel.org/dri-devel/20200914204209.256266093@xxxxxxxxxxxxx/

I noticed this topic is a mess. This is why I'm not relying on 
in_atomic() alone as it turned out not to be sufficient in all cases 
during testing.

What's there now is safe at least in the context from which it is called 
i.e. the runtime pm core code. If not then hopefully the might_sleep() 
that follows will catch misuses.

It should be noted that we assume an atomic context by default. However, 
if you rely on clocks that must sleep then you must not invoke runtime 
pm facilities in atomic context from your driver in the first place. The 
atomic_context variable above is there only used further down as a 
validation check to catch programming mistakes and not an operational 
parameter.


Nicolas