Linux-ARM-MSM Archive on lore.kernel.org
 help / color / Atom feed
From: Stephen Boyd <swboyd@chromium.org>
To: Lina Iyer <ilina@codeaurora.org>,
	agross@kernel.org, bjorn.andersson@linaro.org
Cc: linux-arm-msm@vger.kernel.org, linux-soc@vger.kernel.org,
	rnayak@codeaurora.org, linux-kernel@vger.kernel.org,
	linux-pm@vger.kernel.org, dianders@chromium.org,
	mkshah@codeaurora.org, Lina Iyer <ilina@codeaurora.org>
Subject: Re: [PATCH V2 2/4] drivers: qcom: rpmh-rsc: avoid locking in the interrupt handler
Date: Tue, 23 Jul 2019 13:11:10 -0700
Message-ID: <5d3769df.1c69fb81.55d03.aa33@mx.google.com> (raw)
In-Reply-To: <20190722215340.3071-2-ilina@codeaurora.org>

Quoting Lina Iyer (2019-07-22 14:53:38)
> Avoid locking in the interrupt context to improve latency. Since we
> don't lock in the interrupt context, it is possible that we now could
> race with the DRV_CONTROL register that writes the enable register and
> cleared by the interrupt handler. For fire-n-forget requests, the
> interrupt may be raised as soon as the TCS is triggered and the IRQ
> handler may clear the enable bit before the DRV_CONTROL is read back.
> 
> Use the non-sync variant when enabling the TCS register to avoid reading
> back a value that may been cleared because the interrupt handler ran
> immediately after triggering the TCS.
> 
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---

I have to read this patch carefully. The commit text isn't convincing me
that it is actually safe to make this change. It mostly talks about the
performance improvements and how we need to fix __tcs_trigger(), which
is good, but I was hoping to be convinced that not grabbing the lock
here is safe. 

How do we ensure that drv->tcs_in_use is cleared before we call
tcs_write() and try to look for a free bit? Isn't it possible that we'll
get into a situation where the bitmap is all used up but the hardware
has just received an interrupt and is going to clear out a bit and then
an rpmh write fails with -EBUSY?

>  drivers/soc/qcom/rpmh-rsc.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index 5ede8d6de3ad..694ba881624e 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -242,9 +242,7 @@ static irqreturn_t tcs_tx_done(int irq, void *p)
>                 write_tcs_reg(drv, RSC_DRV_CMD_ENABLE, i, 0);
>                 write_tcs_reg(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, i, 0);
>                 write_tcs_reg(drv, RSC_DRV_IRQ_CLEAR, 0, BIT(i));
> -               spin_lock(&drv->lock);
>                 clear_bit(i, drv->tcs_in_use);
> -               spin_unlock(&drv->lock);
>                 if (req)
>                         rpmh_tx_done(req, err);
>         }
> @@ -304,7 +302,7 @@ static void __tcs_trigger(struct rsc_drv *drv, int tcs_id)
>         enable = TCS_AMC_MODE_ENABLE;
>         write_tcs_reg_sync(drv, RSC_DRV_CONTROL, tcs_id, enable);
>         enable |= TCS_AMC_MODE_TRIGGER;
> -       write_tcs_reg_sync(drv, RSC_DRV_CONTROL, tcs_id, enable);
> +       write_tcs_reg(drv, RSC_DRV_CONTROL, tcs_id, enable);
>  }
>  
>  static int check_for_req_inflight(struct rsc_drv *drv, struct tcs_group *tcs,

  reply index

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-22 21:53 [PATCH V2 1/4] drivers: qcom: rpmh-rsc: simplify TCS locking Lina Iyer
2019-07-22 21:53 ` [PATCH V2 2/4] drivers: qcom: rpmh-rsc: avoid locking in the interrupt handler Lina Iyer
2019-07-23 20:11   ` Stephen Boyd [this message]
2019-07-24 14:52     ` Lina Iyer
2019-07-24 19:37       ` Stephen Boyd
2019-07-24 20:36         ` Lina Iyer
2019-07-24 23:27           ` Doug Anderson
2019-07-25 15:18             ` Lina Iyer
2019-07-25 15:39               ` Doug Anderson
2019-07-29 19:01                 ` Lina Iyer
2019-07-29 20:56                   ` Stephen Boyd
2019-07-30 17:29                     ` Lina Iyer
2019-07-22 21:53 ` [PATCH V2 3/4] drivers: qcom: rpmh: switch over from spinlock irq variants Lina Iyer
2019-07-23 18:24   ` Stephen Boyd
2019-07-22 21:53 ` [PATCH V2 4/4] drivers: qcom: rpmh-rsc: remove redundant register access Lina Iyer
2019-07-23 18:22 ` [PATCH V2 1/4] drivers: qcom: rpmh-rsc: simplify TCS locking Stephen Boyd
2019-07-23 19:21   ` Lina Iyer
2019-07-23 20:18     ` Stephen Boyd
2019-07-24 14:54       ` Lina Iyer
2019-07-24 18:32         ` Stephen Boyd
2019-07-24 19:36           ` Lina Iyer

Reply instructions:

You may reply publically 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=5d3769df.1c69fb81.55d03.aa33@mx.google.com \
    --to=swboyd@chromium.org \
    --cc=agross@kernel.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=dianders@chromium.org \
    --cc=ilina@codeaurora.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=linux-soc@vger.kernel.org \
    --cc=mkshah@codeaurora.org \
    --cc=rnayak@codeaurora.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

Linux-ARM-MSM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-msm/0 linux-arm-msm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-msm linux-arm-msm/ https://lore.kernel.org/linux-arm-msm \
		linux-arm-msm@vger.kernel.org
	public-inbox-index linux-arm-msm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-arm-msm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git