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

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]

Message-ID: <CAJZ5v0hj4VC_kjB5e_b_ho=ET_quG5zUh0Dbbdwofp-6azopsw@mail.gmail.com>
Date:   Fri, 22 Jan 2021 16:09:17 +0100
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     "Rafael J. Wysocki" <rafael@...nel.org>
Cc:     Nicolas Pitre <npitre@...libre.com>,
        Naresh Kamboju <naresh.kamboju@...aro.org>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Russell King <linux@...linux.org.uk>,
        Linux PM <linux-pm@...r.kernel.org>,
        linux-clk <linux-clk@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>,
        Mark Brown <broonie@...nel.org>, Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH v2] PM / clk: make PM clock layer compatible with clocks
 that must sleep

On Thu, Jan 21, 2021 at 8:01 PM Rafael J. Wysocki <rafael@...nel.org> wrote:
>
> On Thu, Jan 21, 2021 at 6:23 PM Nicolas Pitre <npitre@...libre.com> wrote:
> >
> > 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@...libre.com>
> >
> > ---
> >
> > On Thu, 21 Jan 2021, Rafael J. Wysocki wrote:
> >
> > > So I'm going to drop this patch from linux-next until the issue is
> > > resolved, thanks!
> >
> > Here's the fixed version.
>
> Applied instead of the v1, thanks!
>
> > Changes from v1:
> >
> > - Moved clk_is_enabled_when_prepared() declaration under
> >   CONFIG_HAVE_CLK_PREPARE and provided a dummy definition when that
> >   config option is unset.
> >
> > diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
> > index ced6863a16..a62fb0f9b1 100644
> > --- a/drivers/base/power/clock_ops.c
> > +++ b/drivers/base/power/clock_ops.c
> > @@ -23,6 +23,7 @@
> >  enum pce_status {
> >         PCE_STATUS_NONE = 0,
> >         PCE_STATUS_ACQUIRED,
> > +       PCE_STATUS_PREPARED,
> >         PCE_STATUS_ENABLED,
> >         PCE_STATUS_ERROR,
> >  };
> > @@ -32,8 +33,102 @@ struct pm_clock_entry {
> >         char *con_id;
> >         struct clk *clk;
> >         enum pce_status status;
> > +       bool enabled_when_prepared;
> >  };
> >
> > +/**
> > + * pm_clk_list_lock - ensure exclusive access for modifying the PM clock
> > + *                   entry list.
> > + * @psd: pm_subsys_data instance corresponding to the PM clock entry list
> > + *      and clk_op_might_sleep count to be modified.
> > + *
> > + * Get exclusive access before modifying the PM clock entry list and the
> > + * clock_op_might_sleep count to guard against concurrent modifications.
> > + * This also protects against a concurrent clock_op_might_sleep and PM clock
> > + * entry list usage in pm_clk_suspend()/pm_clk_resume() that may or may not
> > + * happen in atomic context, hence both the mutex and the spinlock must be
> > + * taken here.
> > + */
> > +static void pm_clk_list_lock(struct pm_subsys_data *psd)
> > +{
> > +       mutex_lock(&psd->clock_mutex);
> > +       spin_lock_irq(&psd->lock);
> > +}
> > +
> > +/**
> > + * pm_clk_list_unlock - counterpart to pm_clk_list_lock().
> > + * @psd: the same pm_subsys_data instance previously passed to
> > + *      pm_clk_list_lock().
> > + */
> > +static void pm_clk_list_unlock(struct pm_subsys_data *psd)

Locking annotations for sparse were missing here and above, so I've
added them by hand.

Please double check the result in my linux-next branch (just pushed).

Thanks!

> > +{
> > +       spin_unlock_irq(&psd->lock);
> > +       mutex_unlock(&psd->clock_mutex);
> > +}