Messages in this thread | | | Subject | Re: [PATCH 3/3] soc: qcom: rpmh: Conditionally check lockdep_assert_irqs_disabled() | From | Maulik Shah <> | Date | Thu, 7 Jan 2021 16:55:56 +0530 |
| |
Hi Stephen,
On 12/3/2020 12:31 PM, Stephen Boyd wrote: > Quoting Maulik Shah (2020-11-26 02:18:18) >> lockdep_assert_irqs_disabled() was added to check rpmh_flush() >> can only be invoked when irqs are disabled, this is true for >> APPS RSC as the last CPU going to deepest low power mode is >> writing sleep and wake TCSes. >> >> However for RSCs that support solver mode, drivers can invoke >> rpmh_write_sleep_and_wake() to immediately write cached sleep >> and wake sets to TCSes from any CPU. Conditionally check if RSC >> controller supports 'HW solver' mode then do not check for irqs >> disabled as such RSCs can write sleepand wake TCSes at any point. >> >> Signed-off-by: Maulik Shah <mkshah@codeaurora.org> >> --- >> drivers/soc/qcom/rpmh-internal.h | 5 +++++ >> drivers/soc/qcom/rpmh-rsc.c | 3 +++ >> drivers/soc/qcom/rpmh.c | 26 ++++++++++++++++++++++---- >> 3 files changed, 30 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h >> index 79486d6..39fa3c5 100644 >> --- a/drivers/soc/qcom/rpmh-internal.h >> +++ b/drivers/soc/qcom/rpmh-internal.h >> @@ -17,6 +17,9 @@ >> #define MAX_TCS_NR (MAX_TCS_PER_TYPE * TCS_TYPE_NR) >> #define MAX_TCS_SLOTS (MAX_CMDS_PER_TCS * MAX_TCS_PER_TYPE) >> >> +/* CTRLR specific flags */ >> +#define SOLVER_PRESENT 1 >> + >> struct rsc_drv; >> >> /** >> @@ -78,6 +81,7 @@ struct rpmh_request { >> * @cache_lock: synchronize access to the cache data >> * @dirty: was the cache updated since flush >> * @in_solver_mode: Controller is busy in solver mode >> + * @flags: Controller specific flags >> * @batch_cache: Cache sleep and wake requests sent as batch >> */ >> struct rpmh_ctrlr { >> @@ -85,6 +89,7 @@ struct rpmh_ctrlr { >> spinlock_t cache_lock; >> bool dirty; >> bool in_solver_mode; >> + u32 flags; > Maybe unsigned long is more appropriate? Do we rely on 32-bits vs. > 64-bits? We don't rely on 32-bits vs. 64-bits, u32 should be fine. > >> struct list_head batch_cache; >> }; >> >> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c >> index ffb4ca7..4caaddf 100644 >> --- a/drivers/soc/qcom/rpmh-rsc.c >> +++ b/drivers/soc/qcom/rpmh-rsc.c >> @@ -1031,6 +1031,9 @@ static int rpmh_rsc_probe(struct platform_device *pdev) >> if (!solver_config) { >> drv->rsc_pm.notifier_call = rpmh_rsc_cpu_pm_callback; >> cpu_pm_register_notifier(&drv->rsc_pm); >> + drv->client.flags &= ~SOLVER_PRESENT; >> + } else { >> + drv->client.flags |= SOLVER_PRESENT; > It looks like this could be tested by checking for > drv->rsc_pm.notifier_call being non-NULL? It may for now, but going forward we may have different flags to indicate various functions supported by RSC. > >> } >> >> /* Enable the active TCS to send requests immediately */ >> diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c >> index 725b8f0..604d511 100644 >> --- a/drivers/soc/qcom/rpmh.c >> +++ b/drivers/soc/qcom/rpmh.c >> @@ -83,6 +83,9 @@ static int check_ctrlr_state(struct rpmh_ctrlr *ctrlr, enum rpmh_state state) >> if (state != RPMH_ACTIVE_ONLY_STATE) >> return ret; >> >> + if (!(ctrlr->flags & SOLVER_PRESENT)) >> + return ret; >> + >> /* Do not allow sending active votes when in solver mode */ >> spin_lock(&ctrlr->cache_lock); >> if (ctrlr->in_solver_mode) >> @@ -468,12 +471,24 @@ int rpmh_flush(struct rpmh_ctrlr *ctrlr) >> struct cache_req *p; >> int ret = 0; >> >> - lockdep_assert_irqs_disabled(); >> + /* >> + * For RSC that don't have solver mode, >> + * rpmh_flush() is only called when we think we're running >> + * on the last CPU with irqs_disabled. >> + * >> + * For RSC that have solver mode, >> + * rpmh_flush() can be invoked with irqs enabled by any CPU. >> + * >> + * Conditionally check for irqs_disabled only when solver mode >> + * is not available. >> + */ >> + >> + if (!(ctrlr->flags & SOLVER_PRESENT)) >> + lockdep_assert_irqs_disabled(); > Can we have a different function that is called for the case where > solver mode is present and where solver mode isn't present? It would be > good to clearly show that rpmh_flush() thinks it is being called from > the last CPU vs. from some other random place because the code is > assuming solver vs. non-solver enabled state. It would be clearer from > the call site too. Hmm, we can. Patch 2 of this series already added different function which will be called where solver is present. Let me modify this to indicate whether called from last cpu or not.
Thanks, Maulik
> >> >> /* >> - * Currently rpmh_flush() is only called when we think we're running >> - * on the last processor. If the lock is busy it means another >> - * processor is up and it's better to abort than spin. >> + * If the lock is busy it means another transaction is on going, >> + * in such case it's better to abort than spin. >> */ >> if (!spin_trylock(&ctrlr->cache_lock)) >> return -EBUSY;
-- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
| |