Messages in this thread | | | Date | Tue, 23 Jul 2019 13:21:59 -0600 | From | Lina Iyer <> | 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@codeaurora.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@codeaurora.org> >> [ilina: split patch into multiple files, update commit text] >> Signed-off-by: Lina Iyer <ilina@codeaurora.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
| |