LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Doug Anderson <dianders@chromium.org>
To: Lina Iyer <ilina@codeaurora.org>
Cc: Andy Gross <andy.gross@linaro.org>,
	David Brown <david.brown@linaro.org>,
	linux-arm-msm@vger.kernel.org,
	"open list:ARM/QUALCOMM SUPPORT" <linux-soc@vger.kernel.org>,
	Rajendra Nayak <rnayak@codeaurora.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Stephen Boyd <sboyd@kernel.org>,
	Evan Green <evgreen@chromium.org>
Subject: Re: [PATCH v7 04/10] drivers: qcom: rpmh: add RPMH helper functions
Date: Thu, 3 May 2018 13:26:07 -0700	[thread overview]
Message-ID: <CAD=FV=VVrqHnVTATzbXYsZgj8gA9sO=Ky_HRvztfRmjLZoq0=Q@mail.gmail.com> (raw)
In-Reply-To: <20180502193749.31004-5-ilina@codeaurora.org>

Hi,

On Wed, May 2, 2018 at 12:37 PM, Lina Iyer <ilina@codeaurora.org> wrote:
> +static struct rpmh_ctrlr rpmh_rsc[RPMH_MAX_CTRLR];
> +static DEFINE_SPINLOCK(rpmh_rsc_lock);
> +
> +static struct rpmh_ctrlr *get_rpmh_ctrlr(const struct device *dev)
> +{
> +       int i;
> +       struct rsc_drv *p, *drv = dev_get_drvdata(dev->parent);
> +       struct rpmh_ctrlr *ctrlr = ERR_PTR(-EINVAL);
> +       unsigned long flags;
> +
> +       if (!drv)
> +               return ctrlr;
> +
> +       for (i = 0; i < RPMH_MAX_CTRLR; i++) {
> +               if (rpmh_rsc[i].drv == drv) {
> +                       ctrlr = &rpmh_rsc[i];
> +                       return ctrlr;
> +               }
> +       }
> +
> +       spin_lock_irqsave(&rpmh_rsc_lock, flags);
> +       list_for_each_entry(p, &rsc_drv_list, list) {
> +               if (drv == p) {
> +                       for (i = 0; i < RPMH_MAX_CTRLR; i++) {
> +                               if (!rpmh_rsc[i].drv)
> +                                       break;
> +                       }
> +                       if (i == RPMH_MAX_CTRLR) {
> +                               ctrlr = ERR_PTR(-ENOMEM);
> +                               break;
> +                       }
> +                       rpmh_rsc[i].drv = drv;
> +                       ctrlr = &rpmh_rsc[i];
> +                       break;
> +               }
> +       }
> +       spin_unlock_irqrestore(&rpmh_rsc_lock, flags);

I may have missed something, but to me it appears that this whole
"rsc_drv_list" is pretty pointless.  I wrote up a patch atop your
series to remove it at
<https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1042883/>
and it simplifies the code a whole bunch.  From that patch, my
justification was:

> The global rsc_drv_list was (as far as I can tell) racy and not useful
> for anything.
>
> I say it is racy because in general you need some sort of mutual
> exclusion for lists.  If someone is adding to a list while someone
> else is iterating over it then you get badness.
>
> I say it is not useful because the only user of it was
> get_rpmh_ctrlr() and the only thing it did was to verify that the
> "struct rsc_drv *" that it alrady had was in the list.  How could it
> not be?

Note that in v7 of your series you added a spinlock around your access
of "rsc_drv_list", but this doesn't actually remove the race.
Specifically I'm pretty sure that the list primitives don't support
calling list_add() while someone might be iterating over the list and
your spinlock isn't grabbed in rpmh_rsc_probe().

Note that I also say in my patch:

> NOTE: After this patch get_rpmh_ctrlr() still seems a bit fishy.  I'm
> not sure why every caller would need its own private global cache of
> stuff.  ...but I left that part alone.

I'll try to dig into this more so I could just be confused, but in
general it seems really odd to have a spinlock and something called a
"cache" at this level.  If we need some sort of mutual exclusion or
caching it seems like it should be stored in memory directly
associated with the RPMh device, not some external global.


-Doug

  reply	other threads:[~2018-05-03 20:26 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-02 19:37 [PATCH v7 00/10] drivers/qcom: add RPMH communication support Lina Iyer
2018-05-02 19:37 ` [PATCH v7 01/10] drivers: qcom: rpmh-rsc: add RPMH controller for QCOM SoCs Lina Iyer
2018-05-02 19:37 ` [PATCH v7 02/10] dt-bindings: introduce RPMH RSC bindings for Qualcomm SoCs Lina Iyer
2018-05-02 20:37   ` Stephen Boyd
2018-05-02 19:37 ` [PATCH v7 03/10] drivers: qcom: rpmh-rsc: log RPMH requests in FTRACE Lina Iyer
2018-05-02 19:45   ` Steven Rostedt
2018-05-02 19:37 ` [PATCH v7 04/10] drivers: qcom: rpmh: add RPMH helper functions Lina Iyer
2018-05-03 20:26   ` Doug Anderson [this message]
2018-05-04 20:50     ` Matthias Kaehlcke
2018-05-08 16:05     ` ilina
2018-05-10 22:37       ` Doug Anderson
2018-05-11 15:06         ` Lina Iyer
2018-05-11 20:14           ` Doug Anderson
2018-05-14 15:00             ` Lina Iyer
2018-05-02 19:37 ` [PATCH v7 05/10] drivers: qcom: rpmh-rsc: write sleep/wake requests to TCS Lina Iyer
2018-05-03 21:35   ` Matthias Kaehlcke
2018-05-08 16:16     ` ilina
2018-05-02 19:37 ` [PATCH v7 06/10] drivers: qcom: rpmh-rsc: allow invalidation of sleep/wake TCS Lina Iyer
2018-05-03 22:06   ` Matthias Kaehlcke
2018-05-08 16:14     ` ilina
2018-05-08 17:25       ` Matthias Kaehlcke
2018-05-02 19:37 ` [PATCH v7 07/10] drivers: qcom: rpmh: cache sleep/wake state requests Lina Iyer
2018-05-04 21:39   ` Matthias Kaehlcke
2018-05-02 19:37 ` [PATCH v7 08/10] drivers: qcom: rpmh: allow requests to be sent asynchronously Lina Iyer
2018-05-02 19:37 ` [PATCH v7 09/10] drivers: qcom: rpmh: add support for batch RPMH request Lina Iyer
2018-05-02 19:37 ` [PATCH v7 10/10] drivers: qcom: rpmh-rsc: allow active requests from wake TCS Lina Iyer

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='CAD=FV=VVrqHnVTATzbXYsZgj8gA9sO=Ky_HRvztfRmjLZoq0=Q@mail.gmail.com' \
    --to=dianders@chromium.org \
    --cc=andy.gross@linaro.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=david.brown@linaro.org \
    --cc=evgreen@chromium.org \
    --cc=ilina@codeaurora.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-soc@vger.kernel.org \
    --cc=rnayak@codeaurora.org \
    --cc=sboyd@kernel.org \
    --subject='Re: [PATCH v7 04/10] drivers: qcom: rpmh: add RPMH helper functions' \
    /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

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).