linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Stephen Boyd <sboyd@kernel.org>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Nishanth Menon <nm@ti.com>, Viresh Kumar <vireshk@kernel.org>,
	linux-pm@vger.kernel.org,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Rafael Wysocki <rjw@rjwysocki.net>,
	Dmitry Osipenko <digetx@gmail.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] opp: Reinitialize the list_kref before adding the static OPPs again
Date: Mon, 28 Oct 2019 05:01:32 -0700	[thread overview]
Message-ID: <20191028120133.3E85F2086D@mail.kernel.org> (raw)
In-Reply-To: <20191021022516.gecunkpahu7okvm5@vireshk-i7>

Quoting Viresh Kumar (2019-10-20 19:25:16)
> On 18-10-19, 14:12, Stephen Boyd wrote:
> > Quoting Viresh Kumar (2019-10-18 02:28:41)
> > > The list_kref reaches a count of 0 when all the static OPPs are removed,
> > > for example when dev_pm_opp_of_cpumask_remove_table() is called, though
> > > the actual OPP table may not get freed as it may still be referenced by
> > > other parts of the kernel, like from a call to
> > > dev_pm_opp_set_supported_hw(). And if we call
> > > dev_pm_opp_of_cpumask_add_table() again at this point, we must
> > > reinitialize the list_kref otherwise the kernel will hit a WARN() in
> > > kref infrastructure for incrementing a kref with value 0.
> > > 
> > > Fixes: 11e1a1648298 ("opp: Don't decrement uninitialized list_kref")
> > > Reported-by: Dmitry Osipenko <digetx@gmail.com>
> > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > > ---
> > >  drivers/opp/of.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> > > index 6dc41faf74b5..1cbb58240b80 100644
> > > --- a/drivers/opp/of.c
> > > +++ b/drivers/opp/of.c
> > > @@ -663,6 +663,13 @@ static int _of_add_opp_table_v2(struct device *dev, struct opp_table *opp_table)
> > >                 return 0;
> > >         }
> > >  
> > > +       /*
> > > +        * Re-initialize list_kref every time we add static OPPs to the OPP
> > > +        * table as the reference count may be 0 after the last tie static OPPs
> > 
> > s/tie/time/
> > 
> > > +        * were removed.
> > > +        */
> > > +       kref_init(&opp_table->list_kref);
> > 
> > It seems racy.
> 
> I am not sure if I see a race here, but maybe I am missing something.
> Care to explain ?

Some static OPP is removed at the same time that this function is
called?

> 
> > Why are we doing this vs. making an entirely new and
> > different OPP structure? Or why is the count reaching 0 when something
> > is obviously still referencing it?
> 
> The kref for the opp table is opp_table->kref and the one here is
> different. This is list_kref which is used for freeing OPPs added
> statically from the DT. The static OPPs get added to the OPP table
> when one calls dev_pm_opp_of_cpumask_add_table() and must be removed
> on a call to dev_pm_opp_of_cpumask_remove_table(). The opp table
> structure may not get freed at this moment though as it is still
> referenced by the caller of dev_pm_opp_set_supported_hw().
> 
> And now when we try to add the static OPPs again (re-insertion of
> cpufreq module), we need to reinitialize the list_kref again as its
> count reached 0 earlier and the resources (static OPPs) were freed.
> 

Right. I don't understand why the count reaches 0 if we can still get a
pointer to something. I guess we've got this kref thing that has a
lifetime beyond the life of what it's tracking, which is weird. Usually
the kref is embedded inside the pointer that is returned by the "get"
call, but here it's outside it and used to track when we should free
static OPPs. Why are we removing static OPPs? Shouldn't they just stick
around forever until the device is deleted vs. populated over and over
again?


  reply	other threads:[~2019-10-28 12:01 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-18  9:28 [PATCH] opp: Reinitialize the list_kref before adding the static OPPs again Viresh Kumar
2019-10-18 15:35 ` Dmitry Osipenko
2019-10-21  2:18   ` Viresh Kumar
2019-10-18 21:12 ` Stephen Boyd
2019-10-21  2:25   ` Viresh Kumar
2019-10-28 12:01     ` Stephen Boyd [this message]
2019-10-30  7:50       ` Viresh Kumar
     [not found]         ` <20191030143400.1680D20656@mail.kernel.org>
2019-11-11  8:21           ` Viresh Kumar
2019-11-11 11:31             ` Viresh Kumar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191028120133.3E85F2086D@mail.kernel.org \
    --to=sboyd@kernel.org \
    --cc=digetx@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=nm@ti.com \
    --cc=rjw@rjwysocki.net \
    --cc=vincent.guittot@linaro.org \
    --cc=viresh.kumar@linaro.org \
    --cc=vireshk@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).