| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 7 Jan 2021 16:55:56 +0530
From: Maulik Shah <mkshah@...eaurora.org>
To: Stephen Boyd <sboyd@...nel.org>, andy.gross@...aro.org,
bjorn.andersson@...aro.org
Cc: linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
tkjos@...gle.com, dianders@...omium.org, ilina@...eaurora.org,
lsrao@...eaurora.org
Subject: Re: [PATCH 3/3] soc: qcom: rpmh: Conditionally check
lockdep_assert_irqs_disabled()
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@...eaurora.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
Powered by blists - more mailing lists