LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Maulik Shah <mkshah@codeaurora.org>
To: Doug Anderson <dianders@chromium.org>
Cc: Stephen Boyd <swboyd@chromium.org>,
	Matthias Kaehlcke <mka@chromium.org>,
	Evan Green <evgreen@chromium.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	LKML <linux-kernel@vger.kernel.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	Andy Gross <agross@kernel.org>,
	Rajendra Nayak <rnayak@codeaurora.org>,
	Lina Iyer <ilina@codeaurora.org>,
	lsrao@codeaurora.org
Subject: Re: [PATCH v10 3/3] soc: qcom: rpmh: Invoke rpmh_flush() for dirty caches
Date: Tue, 10 Mar 2020 16:30:52 +0530	[thread overview]
Message-ID: <a46d2857-2bd3-ea8c-2f42-751e7ff62312@codeaurora.org> (raw)
In-Reply-To: <CAD=FV=UYpO2rSOoF-OdZd3jKfSZGKnpQJPoiE5fzH+u1uafS6g@mail.gmail.com>


On 3/6/2020 3:50 AM, Doug Anderson wrote:
> Hi,
>
> On Thu, Mar 5, 2020 at 3:30 AM Maulik Shah <mkshah@codeaurora.org> wrote:
>>>> +                       spin_unlock_irqrestore(&ctrlr->cache_lock, flags);
>>>> +                       return -EINVAL;
>>> nit: why not add "int ret = 0" to the top of the function, then here:
>>>
>>> if (rpmh_flush(ctrl))
>>>   ret = -EINVAL;
>>>
>>> ...then at the end "return ret".  It avoids the 2nd copy of the unlock?
>> Done.
>>> Also: Why throw away the return value of rpmh_flush and replace it
>>> with -EINVAL?  Trying to avoid -EBUSY?  ...oh, should you handle
>>> -EBUSY?  AKA:
>>>
>>> if (!psci_has_osi_support()) {
>>>   do {
>>>     ret = rpmh_flush(ctrl);
>>>   } while (ret == -EBUSY);
>>> }
>> Done, the return value from rpmh_flush() can be -EAGAIN, not -EBUSY.
>>
>> i will update the comment accordingly and will include below change as well in next series.
>>
>> https://patchwork.kernel.org/patch/11364067/
>>
>> this should address for caller to not handle -EAGAIN.
> A few issues, I guess.
>
> 1. I _think_ it's important that you enable interrupts between
> retries.  If you're on the same CPU that the interrupt is routed to
> and you were waiting for 'tcs_in_use' to be cleared you'll be in
> trouble otherwise.  ...I think we need to audit all of the places that
> are looping based on -EAGAIN and confirm that interrupts are enabled
> between retries.  Before your patch series the only looping I see was
> in rpmh_invalidate() and the lock wasn't held.  After your series it's
> also in rpmh_flush() which is called under spin_lock_irqsave() which
> will be a problem.
I will take a look at interrupts part.
>
> 2. The RPMH code uses both -EBUSY and -EAGAIN so I looked carefully at
> this again.  You're right that -EBUSY seems to be exclusively returned
> by things only called by rpmh_rsc_send_data() and that function
> handles the retries.  ...but looking at this made me find a broken
> corner case with the "zero active tcs" case (assuming you care about
> this case as per your other thread).  Specifically if you have "zero
> active tcs" then get_tcs_for_msg() can call rpmh_rsc_invalidate()
> which can return -EAGAIN.  That will return the -EAGAIN out of
> tcs_write() into rpmh_rsc_send_data().  rpmh_rsc_send_data() only
> handles -EBUSY, not -EAGAIN.
>
> -Doug

Thanks Doug. I will have a patch to fix this.

Thanks,
Maulik

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

      reply	other threads:[~2020-03-10 11:01 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-03 12:26 [PATCH v10 0/3] Invoke rpmh_flush for non OSI targets Maulik Shah
2020-03-03 12:26 ` [PATCH v10 1/3] arm64: dts: qcom: sc7180: Add cpuidle low power states Maulik Shah
2020-03-03 12:26 ` [PATCH v10 2/3] soc: qcom: rpmh: Update dirty flag only when data changes Maulik Shah
2020-03-04 23:21   ` Doug Anderson
2020-03-05 11:10     ` Maulik Shah
2020-03-05 22:22       ` Doug Anderson
2020-03-10 11:03         ` Maulik Shah
2020-03-10 15:46           ` Doug Anderson
2020-03-11  5:40             ` Maulik Shah
2020-03-03 12:26 ` [PATCH v10 3/3] soc: qcom: rpmh: Invoke rpmh_flush() for dirty caches Maulik Shah
2020-03-04 23:22   ` Doug Anderson
2020-03-05 11:30     ` Maulik Shah
2020-03-05 22:20       ` Doug Anderson
2020-03-10 11:00         ` Maulik Shah [this message]

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=a46d2857-2bd3-ea8c-2f42-751e7ff62312@codeaurora.org \
    --to=mkshah@codeaurora.org \
    --cc=agross@kernel.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=dianders@chromium.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=rnayak@codeaurora.org \
    --cc=swboyd@chromium.org \
    --subject='Re: [PATCH v10 3/3] soc: qcom: rpmh: Invoke rpmh_flush() for dirty caches' \
    /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).