Hi Maulik,
Thanks for spinning this so promptly.
On Fri, Feb 28, 2020 at 3:38 AM Maulik Shah <mkshah@xxxxxxxxxxxxxx> wrote:
>
> Add changes to invoke rpmh flush() from within cache_lock when the data
> in cache is dirty.
>
> This is done only if OSI is not supported in PSCI. If OSI is supported
> rpmh_flush can get invoked when the last cpu going to power collapse
> deepest low power mode.
>
> Also remove "depends on COMPILE_TEST" for Kconfig option QCOM_RPMH so the
> driver is only compiled for arm64 which supports psci_has_osi_support()
> API.
>
> Signed-off-by: Maulik Shah <mkshah@xxxxxxxxxxxxxx>
> Reviewed-by: Srinivas Rao L <lsrao@xxxxxxxxxxxxxx>
> ---
> drivers/soc/qcom/Kconfig | 2 +-
> drivers/soc/qcom/rpmh.c | 33 ++++++++++++++++++++++-----------
> 2 files changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index d0a73e7..2e581bc 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -105,7 +105,7 @@ config QCOM_RMTFS_MEM
>
> config QCOM_RPMH
> bool "Qualcomm RPM-Hardened (RPMH) Communication"
> - depends on ARCH_QCOM && ARM64 || COMPILE_TEST
> + depends on ARCH_QCOM && ARM64
> help
> Support for communication with the hardened-RPM blocks in
> Qualcomm Technologies Inc (QTI) SoCs. RPMH communication uses an
> diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
> index f28afe4..6a5a60c 100644
> --- a/drivers/soc/qcom/rpmh.c
> +++ b/drivers/soc/qcom/rpmh.c
> @@ -12,6 +12,7 @@
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/platform_device.h>
> +#include <linux/psci.h>
> #include <linux/slab.h>
> #include <linux/spinlock.h>
> #include <linux/types.h>
> @@ -158,6 +159,13 @@ static struct cache_req *cache_rpm_request(struct rpmh_ctrlr *ctrlr,
> }
>
> unlock:
> + if (ctrlr->dirty && !psci_has_osi_support()) {
> + if (rpmh_flush(ctrlr)) {
> + spin_unlock_irqrestore(&ctrlr->cache_lock, flags);
> + return ERR_PTR(-EINVAL);
> + }
> + }
> +
> spin_unlock_irqrestore(&ctrlr->cache_lock, flags);
>
> return req;
> @@ -285,26 +293,35 @@ int rpmh_write(const struct device *dev, enum rpmh_state state,
> }
> EXPORT_SYMBOL(rpmh_write);
>
> -static void cache_batch(struct rpmh_ctrlr *ctrlr, struct batch_cache_req *req)
> +static int cache_batch(struct rpmh_ctrlr *ctrlr, struct batch_cache_req *req)
> {
> unsigned long flags;
>
> spin_lock_irqsave(&ctrlr->cache_lock, flags);
> +
> list_add_tail(&req->list, &ctrlr->batch_cache);
> ctrlr->dirty = true;
> +
> + if (!psci_has_osi_support()) {
> + if (rpmh_flush(ctrlr)) {
> + spin_unlock_irqrestore(&ctrlr->cache_lock, flags);
> + return -EINVAL;
> + }
> + }
> +
> spin_unlock_irqrestore(&ctrlr->cache_lock, flags);
> +
> + return 0;
> }
>
> static int flush_batch(struct rpmh_ctrlr *ctrlr)
> {
> struct batch_cache_req *req;
> const struct rpmh_request *rpm_msg;
> - unsigned long flags;
> int ret = 0;
> int i;
>
> /* Send Sleep/Wake requests to the controller, expect no response */
> - spin_lock_irqsave(&ctrlr->cache_lock, flags);
> list_for_each_entry(req, &ctrlr->batch_cache, list) {
> for (i = 0; i < req->count; i++) {
> rpm_msg = req->rpm_msgs + i;
> @@ -314,7 +331,6 @@ static int flush_batch(struct rpmh_ctrlr *ctrlr)
> break;
> }
> }
> - spin_unlock_irqrestore(&ctrlr->cache_lock, flags);
>
> return ret;
> }
> @@ -386,10 +402,8 @@ int rpmh_write_batch(const struct device *dev, enum rpmh_state state,
> cmd += n[i];
> }
>
> - if (state != RPMH_ACTIVE_ONLY_STATE) {
> - cache_batch(ctrlr, req);
> - return 0;
> - }
> + if (state != RPMH_ACTIVE_ONLY_STATE)
> + return cache_batch(ctrlr, req);
>
> for (i = 0; i < count; i++) {
> struct completion *compl = &compls[i];
> @@ -455,9 +469,6 @@ static int send_single(struct rpmh_ctrlr *ctrlr, enum rpmh_state state,
> * Return: -EBUSY if the controller is busy, probably waiting on a response
> * to a RPMH request sent earlier.
> *
> - * This function is always called from the sleep code from the last CPU
> - * that is powering down the entire system. Since no other RPMH API would be
> - * executing at this time, it is safe to run lockless.
Oh nice, I didn't even see that comment. We should probably replace
that with a comment indicating that we assume ctrlr->cache_lock is
already held.
Please also remove this comment in rpmh_flush():
/*
* Nobody else should be calling this function other than system PM,
* hence we can run without locks.
*/
list_for_each_entry(p, &ctrlr->cache, list) {
-Evan
> */
> int rpmh_flush(struct rpmh_ctrlr *ctrlr)
> {
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
