From: Stephen Boyd <swboyd@chromium.org>
To: Andy Gross <agross@kernel.org>,
Bjorn Andersson <bjorn.andersson@linaro.org>,
Douglas Anderson <dianders@chromium.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
rafael.j.wysocki@intel.com
Cc: mka@chromium.org, mkshah@codeaurora.org, evgreen@chromium.org,
Douglas Anderson <dianders@chromium.org>,
linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 4/5] soc: qcom: rpmh-rsc: Simplify locking by eliminating the per-TCS lock
Date: Thu, 23 Apr 2020 19:48:30 -0700 [thread overview]
Message-ID: <158769651085.135303.5206480555792176636@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <20200422145408.v4.4.Ib8dccfdb10bf6b1fb1d600ca1c21d9c0db1ef746@changeid>
Quoting Douglas Anderson (2020-04-22 14:55:02)
> The rpmh-rsc code had both a driver-level lock (sometimes referred to
> in comments as drv->lock) and a lock per-TCS. The idea was supposed
> to be that there would be times where you could get by with just
> locking a TCS lock and therefor other RPMH users wouldn't be blocked.
>
> The above didn't work out so well.
>
> Looking at tcs_write() the bigger drv->lock was held for most of the
> function anyway. Only the __tcs_buffer_write() and
> __tcs_set_trigger() calls were called without it the drv->lock. It
without holding the drv->lock
> actually turns out that in tcs_write() we don't need to hold the
> drv->lock for those function calls anyway even if the per-TCS lock
> isn't there anymore.
Why?
> Thus, from a tcs_write() point of view, the
> per-TCS lock was useless.
>
> Looking at rpmh_rsc_write_ctrl_data(), only the per-TCS lock was held.
> It turns out, though, that this function already needs to be called
> with the equivalent of the drv->lock held anyway (we either need to
> hold drv->lock as we will in a future patch or we need to know no
> other CPUs could be running as happens today). Specifically
> rpmh_rsc_write_ctrl_data() might be writing to a TCS that has been
> borrowed for writing an active transation but it never checks this.
>
> Let's eliminate this extra overhead and avoid possible AB BA locking
> headaches.
>
> Suggested-by: Maulik Shah <mkshah@codeaurora.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index e540e49fd61c..71cebe7fd452 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -581,24 +575,19 @@ static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
> if (IS_ERR(tcs))
> return PTR_ERR(tcs);
>
> - spin_lock_irqsave(&tcs->lock, flags);
> - spin_lock(&drv->lock);
> + spin_lock_irqsave(&drv->lock, flags);
> /*
> * The h/w does not like if we send a request to the same address,
> * when one is already in-flight or being processed.
> */
> ret = check_for_req_inflight(drv, tcs, msg);
> - if (ret) {
> - spin_unlock(&drv->lock);
> - goto done_write;
> - }
> + if (ret)
> + goto err;
Nitpick: Usually 'goto err' is used for error paths, not unlock paths.
Use 'goto unlock' for that.
>
> - tcs_id = find_free_tcs(tcs);
> - if (tcs_id < 0) {
> - ret = tcs_id;
> - spin_unlock(&drv->lock);
> - goto done_write;
> - }
> + ret = find_free_tcs(tcs);
> + if (ret < 0)
> + goto err;
> + tcs_id = ret;
>
> tcs->req[tcs_id - tcs->offset] = msg;
> set_bit(tcs_id, drv->tcs_in_use);
> @@ -612,13 +601,21 @@ static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
> write_tcs_reg_sync(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, tcs_id, 0);
> enable_tcs_irq(drv, tcs_id, true);
> }
> - spin_unlock(&drv->lock);
> + spin_unlock_irqrestore(&drv->lock, flags);
>
> + /*
> + * These two can be done after the lock is released because:
> + * - We marked "tcs_in_use" under lock.
> + * - Once "tcs_in_use" has been marked nobody else could be writing
> + * to these registers until the interrupt goes off.
> + * - The interrupt can't go off until we trigger.
trigger via some function?
> + */
> __tcs_buffer_write(drv, tcs_id, 0, msg);
> __tcs_set_trigger(drv, tcs_id, true);
>
> -done_write:
> - spin_unlock_irqrestore(&tcs->lock, flags);
> + return 0;
> +err:
> + spin_unlock_irqrestore(&drv->lock, flags);
> return ret;
> }
>
next prev parent reply other threads:[~2020-04-24 2:48 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-22 21:54 [PATCH v4 1/5] soc: qcom: rpmh-rsc: Corrently ignore CPU_CLUSTER_PM notifications Douglas Anderson
2020-04-22 21:55 ` [PATCH v4 2/5] soc: qcom: rpmh-rsc: We aren't notified of our own failure w/ NOTIFY_BAD Douglas Anderson
2020-04-23 4:48 ` Maulik Shah
2020-04-24 2:38 ` Stephen Boyd
2020-04-24 2:41 ` Stephen Boyd
2020-04-22 21:55 ` [PATCH v4 4/5] soc: qcom: rpmh-rsc: Simplify locking by eliminating the per-TCS lock Douglas Anderson
2020-04-24 2:48 ` Stephen Boyd [this message]
2020-04-24 16:47 ` Doug Anderson
2020-04-22 21:55 ` [PATCH v4 5/5] soc: qcom: rpmh-rsc: Remove the pm_lock Douglas Anderson
2020-04-24 3:59 ` Stephen Boyd
2020-04-23 4:45 ` [PATCH v4 1/5] soc: qcom: rpmh-rsc: Corrently ignore CPU_CLUSTER_PM notifications Maulik Shah
2020-04-23 16:19 ` Doug Anderson
2020-04-24 2:38 ` Stephen Boyd
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=158769651085.135303.5206480555792176636@swboyd.mtv.corp.google.com \
--to=swboyd@chromium.org \
--cc=agross@kernel.org \
--cc=bjorn.andersson@linaro.org \
--cc=dianders@chromium.org \
--cc=evgreen@chromium.org \
--cc=gregkh@linuxfoundation.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mka@chromium.org \
--cc=mkshah@codeaurora.org \
--cc=rafael.j.wysocki@intel.com \
/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).