LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Lina Iyer <ilina@codeaurora.org>
To: Doug Anderson <dianders@chromium.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: Fri, 11 May 2018 09:06:11 -0600	[thread overview]
Message-ID: <20180511150611.GA30504@codeaurora.org> (raw)
In-Reply-To: <CAD=FV=X2k-g_LLuVZEbyJvc=_e68gcXREPRCOvvD=Fz566_Bgg@mail.gmail.com>

Hi Doug,

On Thu, May 10 2018 at 16:37 -0600, Doug Anderson wrote:
>Hi,
>
>On Tue, May 8, 2018 at 9:05 AM,  <ilina@codeaurora.org> wrote:
>> On 2018-05-03 14:26, Doug Anderson wrote:
>> Hi Doug,
>>
>>
>>> 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 am not sure I understand this.
>
>As I've said I haven't reviewed RPMh in any amount of detail and so
>perhaps I don't understand something.
>
>OK, I dug a little more and coded up something for you.  Basically
>you're doing a whole bunch of iteration / extra work here to try to
>come up with a way to associate an extra bit of data with each "struct
>rsc_drv".  Rather than that, just add an extra field into "struct
>rsc_drv".  Problem solved.
>
>See http://crosreview.com/1054646 for what I mean.
>
I tried to avoid such pointer references and keep it object oriented
with this approach. I agree that we run through a list of 2 (at the max)
RSC to get the drv* from the rpmh_ctrlr. It is not going to be
expensive.

Another things this helps with is that, if the driver is not a child of
the RSC nodes in DT, then the drvdata of the parent would not a RSC node
and accessing that would result in a crash. This offers a cleaner exit
path for the error case.

>
>>> 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.
>>>
>> The idea behind the locking is not to avoid the race between rpmh.c and
>> rpmh-rsc.c. From the DT, the devices that are dependent on the RSCs are
>> probed following the probe of the controller. And init is not that we are
>> worried about.
>> The condition here is to prevent the rpmh_rsc[] from being modified
>> concurrently by drivers.
>
>OK, I see the point of the locking now, but not the list still.
>Sounds like Matthias agrees with me that the list isn't useful.  Seems
>like you should squash my patch at http://crosreview.com/1042883 into
>yours.
>
I saw your approach. I am okay with it for your tree, my approach comes
out of experiences in qcom platforms and how things tend to shape up in
the future. I would want you to consider my reasoning as well, before we
go forward.

Thanks,
Lina

  reply	other threads:[~2018-05-11 15:06 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
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 [this message]
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=20180511150611.GA30504@codeaurora.org \
    --to=ilina@codeaurora.org \
    --cc=andy.gross@linaro.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=david.brown@linaro.org \
    --cc=dianders@chromium.org \
    --cc=evgreen@chromium.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).