[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20190723192159.GA18620@codeaurora.org>
Date: Tue, 23 Jul 2019 13:21:59 -0600
From: Lina Iyer <ilina@...eaurora.org>
To: Stephen Boyd <swboyd@...omium.org>
Cc: agross@...nel.org, bjorn.andersson@...aro.org,
linux-arm-msm@...r.kernel.org, linux-soc@...r.kernel.org,
rnayak@...eaurora.org, linux-kernel@...r.kernel.org,
linux-pm@...r.kernel.org, dianders@...omium.org,
mkshah@...eaurora.org, "Raju P.L.S.S.S.N" <rplsssn@...eaurora.org>
Subject: Re: [PATCH V2 1/4] drivers: qcom: rpmh-rsc: simplify TCS locking
On Tue, Jul 23 2019 at 12:22 -0600, Stephen Boyd wrote:
>Quoting Lina Iyer (2019-07-22 14:53:37)
>> From: "Raju P.L.S.S.S.N" <rplsssn@...eaurora.org>
>>
>> The tcs->lock was introduced to serialize access with in TCS group. But,
>> drv->lock is still needed to synchronize core aspects of the
>> communication. This puts the drv->lock in the critical and high latency
>> path of sending a request. drv->lock provides the all necessary
>> synchronization. So remove locking around TCS group and simply use the
>> drv->lock instead.
>
>This doesn't talk about removing the irq saving and restoring though.
You mean for drv->lock? It was not an _irqsave/_irqrestore anyways and
we were only removing the tcs->lock.
>Can you keep irq saving and restoring in this patch and then remove that
>in the next patch with reasoning? It probably isn't safe if the lock is
>taken in interrupt context anyway.
>
Yes, the drv->lock should have been irqsave/irqrestore, but it hasn't
been changed by this patch.
>>
>> Signed-off-by: Raju P.L.S.S.S.N <rplsssn@...eaurora.org>
>> [ilina: split patch into multiple files, update commit text]
>> Signed-off-by: Lina Iyer <ilina@...eaurora.org>
>
>> diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
>> index a7bbbb67991c..969d5030860e 100644
>> --- a/drivers/soc/qcom/rpmh-internal.h
>> +++ b/drivers/soc/qcom/rpmh-internal.h
>> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
>> index e278fc11fe5c..5ede8d6de3ad 100644
>> --- a/drivers/soc/qcom/rpmh-rsc.c
>> +++ b/drivers/soc/qcom/rpmh-rsc.c
>> @@ -106,26 +106,26 @@ static int tcs_invalidate(struct rsc_drv *drv, int type)
>> {
>> int m;
>> struct tcs_group *tcs;
>> + int ret = 0;
>>
>> tcs = get_tcs_of_type(drv, type);
>>
>> - spin_lock(&tcs->lock);
>> - if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS)) {
>> - spin_unlock(&tcs->lock);
>> - return 0;
>> - }
>> + spin_lock(&drv->lock);
>> + if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS))
>> + goto done_invalidate;
>>
>> for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
>> if (!tcs_is_free(drv, m)) {
>> - spin_unlock(&tcs->lock);
>> - return -EAGAIN;
>> + ret = -EAGAIN;
>> + goto done_invalidate;
>> }
>> write_tcs_reg_sync(drv, RSC_DRV_CMD_ENABLE, m, 0);
>> write_tcs_reg_sync(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, m, 0);
>> }
>> bitmap_zero(tcs->slots, MAX_TCS_SLOTS);
>> - spin_unlock(&tcs->lock);
>>
>> +done_invalidate:
>> + spin_unlock(&drv->lock);
>> return 0;
>
>return ret now?
>
Yes, will do.
>> }
>>
>> @@ -349,41 +349,35 @@ static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
>> {
>> struct tcs_group *tcs;
>> int tcs_id;
>> - unsigned long flags;
>> int ret;
>>
>> tcs = get_tcs_for_msg(drv, msg);
>> if (IS_ERR(tcs))
>> return PTR_ERR(tcs);
>>
>> - spin_lock_irqsave(&tcs->lock, flags);
>> spin_lock(&drv->lock);
>> /*
>> * 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);
>> + if (ret)
>> goto done_write;
>> - }
>>
>> tcs_id = find_free_tcs(tcs);
>> if (tcs_id < 0) {
>> ret = tcs_id;
>> - spin_unlock(&drv->lock);
>> goto done_write;
>> }
>>
>> tcs->req[tcs_id - tcs->offset] = msg;
>> set_bit(tcs_id, drv->tcs_in_use);
>> - spin_unlock(&drv->lock);
>>
>> __tcs_buffer_write(drv, tcs_id, 0, msg);
>> __tcs_trigger(drv, tcs_id);
>>
>> done_write:
>> - spin_unlock_irqrestore(&tcs->lock, flags);
>> + spin_unlock(&drv->lock);
>> return ret;
>> }
>>
>> @@ -481,19 +475,18 @@ static int tcs_ctrl_write(struct rsc_drv *drv, const struct tcs_request *msg)
>> {
>> struct tcs_group *tcs;
>> int tcs_id = 0, cmd_id = 0;
>> - unsigned long flags;
>> int ret;
>>
>> tcs = get_tcs_for_msg(drv, msg);
>> if (IS_ERR(tcs))
>> return PTR_ERR(tcs);
>>
>> - spin_lock_irqsave(&tcs->lock, flags);
>> + spin_lock(&drv->lock);
>> /* find the TCS id and the command in the TCS to write to */
>> ret = find_slots(tcs, msg, &tcs_id, &cmd_id);
>> if (!ret)
>> __tcs_buffer_write(drv, tcs_id, cmd_id, msg);
>> - spin_unlock_irqrestore(&tcs->lock, flags);
>> + spin_unlock(&drv->lock);
>>
>
>These ones, just leave them doing the irq save restore for now?
>
drv->lock ??
--Lina
Powered by blists - more mailing lists