From: Doug Anderson <dianders@chromium.org>
To: Stephen Boyd <swboyd@chromium.org>
Cc: Maulik Shah <mkshah@codeaurora.org>,
Bjorn Andersson <bjorn.andersson@linaro.org>,
Evan Green <evgreen@chromium.org>,
LKML <linux-kernel@vger.kernel.org>,
linux-arm-msm <linux-arm-msm@vger.kernel.org>,
Andy Gross <agross@kernel.org>,
Matthias Kaehlcke <mka@chromium.org>,
Rajendra Nayak <rnayak@codeaurora.org>,
Lina Iyer <ilina@codeaurora.org>,
lsrao@codeaurora.org
Subject: Re: [PATCH v17 2/6] soc: qcom: rpmh: Update dirty flag only when data changes
Date: Fri, 17 Apr 2020 14:16:53 -0700 [thread overview]
Message-ID: <CAD=FV=WOwMrTNS7FK0jybJyEOvT-rHarA0m5U6B+pKoeA7RCHQ@mail.gmail.com> (raw)
In-Reply-To: <158708153779.132238.7823999141480422982@swboyd.mtv.corp.google.com>
On Thu, Apr 16, 2020 at 4:59 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Maulik Shah (2020-04-12 07:50:00)
> > diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
> > index eb0ded0..03630ae 100644
> > --- a/drivers/soc/qcom/rpmh.c
> > +++ b/drivers/soc/qcom/rpmh.c
> > @@ -133,26 +134,27 @@ static struct cache_req *cache_rpm_request(struct rpmh_ctrlr *ctrlr,
> >
> > req->addr = cmd->addr;
> > req->sleep_val = req->wake_val = UINT_MAX;
> > - INIT_LIST_HEAD(&req->list);
> > list_add_tail(&req->list, &ctrlr->cache);
> >
> > existing:
> > + old_sleep_val = req->sleep_val;
> > + old_wake_val = req->wake_val;
> > +
> > switch (state) {
> > case RPMH_ACTIVE_ONLY_STATE:
> > - if (req->sleep_val != UINT_MAX)
> > - req->wake_val = cmd->data;
> > - break;
> > case RPMH_WAKE_ONLY_STATE:
> > req->wake_val = cmd->data;
> > break;
> > case RPMH_SLEEP_STATE:
> > req->sleep_val = cmd->data;
> > break;
> > - default:
> > - break;
> > }
> >
> > - ctrlr->dirty = true;
> > + ctrlr->dirty = (req->sleep_val != old_sleep_val ||
> > + req->wake_val != old_wake_val) &&
> > + req->sleep_val != UINT_MAX &&
> > + req->wake_val != UINT_MAX;
>
> Can this change ctrl->dirty from true to false? I'm worried that we need
> to make this a saturating assignment instead of an assignment.
>
> ctrl->dirty = ctrl->dirty || (req->sleep_val != .. );
This seems like a serious problem with the current code and feels like
we need a fix sooner rather than later. I'm sorry I missed it in
review (and in fact, I probably suggested the exact code that's here
so it's even more my fault). :(
I posted:
https://lore.kernel.org/r/20200417141531.1.Ia4b74158497213eabad7c3d474c50bfccb3f342e@changeid
-Doug
next prev parent reply other threads:[~2020-04-17 21:17 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-12 14:49 [PATCH v17 0/6] Invoke rpmh_flush for non OSI targets Maulik Shah
2020-04-12 14:49 ` [PATCH v17 1/6] arm64: dts: qcom: sc7180: Add cpuidle low power states Maulik Shah
2020-04-12 14:50 ` [PATCH v17 2/6] soc: qcom: rpmh: Update dirty flag only when data changes Maulik Shah
2020-04-13 21:17 ` Stephen Boyd
2020-04-16 23:58 ` Stephen Boyd
2020-04-17 21:16 ` Doug Anderson [this message]
2020-04-12 14:50 ` [PATCH v17 3/6] soc: qcom: rpmh: Invalidate SLEEP and WAKE TCSes before flushing new data Maulik Shah
2020-04-13 21:17 ` Stephen Boyd
2020-04-12 14:50 ` [PATCH v17 4/6] soc: qcom: rpmh: Invoke rpmh_flush() for dirty caches Maulik Shah
2020-04-13 16:17 ` Doug Anderson
2020-04-13 21:18 ` Stephen Boyd
2020-04-12 14:50 ` [PATCH v17 5/6] soc: qcom: rpmh-rsc: Clear active mode configuration for wake TCS Maulik Shah
2020-04-12 14:50 ` [PATCH v17 6/6] soc: qcom: rpmh-rsc: Allow using free WAKE TCS for active request Maulik Shah
2020-04-13 21:19 ` Stephen Boyd
2020-04-14 5:34 ` [PATCH v17 0/6] Invoke rpmh_flush for non OSI targets Bjorn Andersson
2020-04-14 9:19 ` Maulik Shah
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=WOwMrTNS7FK0jybJyEOvT-rHarA0m5U6B+pKoeA7RCHQ@mail.gmail.com' \
--to=dianders@chromium.org \
--cc=agross@kernel.org \
--cc=bjorn.andersson@linaro.org \
--cc=evgreen@chromium.org \
--cc=ilina@codeaurora.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=lsrao@codeaurora.org \
--cc=mka@chromium.org \
--cc=mkshah@codeaurora.org \
--cc=rnayak@codeaurora.org \
--cc=swboyd@chromium.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.