| Message ID | 1611555637-7688-3-git-send-email-mkshah@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/3] drivers: qcom: rpmh: Disallow active requests in solver mode | expand |
Hi, On Sun, Jan 24, 2021 at 10:21 PM Maulik Shah <mkshah@codeaurora.org> wrote: > > @@ -136,6 +136,6 @@ void rpmh_rsc_invalidate(struct rsc_drv *drv); > int rpmh_rsc_mode_solver_set(struct rsc_drv *drv, bool enable); > > void rpmh_tx_done(const struct tcs_request *msg, int r); > -int rpmh_flush(struct rpmh_ctrlr *ctrlr); > +int rpmh_flush(struct rpmh_ctrlr *ctrlr, bool from_last_cpu); Given that you're touching this code, why not do the cleanup you promised Stephen you'd do in April of 2020 [1]. Specifically rename this function to something other than rpmh_flush(). [1] https://lore.kernel.org/r/11c37c89-aa1f-7297-9ecf-4d77a20deebd@codeaurora.org/ > diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c > index 1c1f5b0..a67bcd6 100644 > --- a/drivers/soc/qcom/rpmh-rsc.c > +++ b/drivers/soc/qcom/rpmh-rsc.c > @@ -841,7 +841,8 @@ static int rpmh_rsc_cpu_pm_callback(struct notifier_block *nfb, > * CPU. > */ > if (spin_trylock(&drv->lock)) { > - if (rpmh_rsc_ctrlr_is_busy(drv) || rpmh_flush(&drv->client)) > + if (rpmh_rsc_ctrlr_is_busy(drv) || > + rpmh_flush(&drv->client, true)) I'll channel the spirit of Bjorn and say that it's better to blow over the 80 column limit here and avoid wrapping to a new line. > @@ -458,22 +458,33 @@ static int send_single(struct rpmh_ctrlr *ctrlr, enum rpmh_state state, > * rpmh_flush() - Flushes the buffered sleep and wake sets to TCSes > * > * @ctrlr: Controller making request to flush cached data > + * @from_last_cpu: Set if invoked from last cpu with irqs disabled > * > * Return: > * * 0 - Success > * * Error code - Otherwise > */ > -int rpmh_flush(struct rpmh_ctrlr *ctrlr) > +int rpmh_flush(struct rpmh_ctrlr *ctrlr, bool from_last_cpu) > { > struct cache_req *p; > int ret = 0; > > - lockdep_assert_irqs_disabled(); > + /* > + * rpmh_flush() can be called when we think we're running > + * on the last CPU with irqs_disabled or when RPMH client > + * explicitly requests to write sleep and wake data. > + * (for e.g. when in solver mode clients can requests to flush) > + * > + * Conditionally check for irqs_disabled only when called > + * from last cpu. > + */ > + > + if (from_last_cpu) > + lockdep_assert_irqs_disabled(); > > /* > - * 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; I think logically here you should only do the trylock if "from_last_cpu". If you're not called "from_last_cpu" you should do a normal spinlock. Also: if you're not "from_last_cpu" you need to use the "irq" or "irqsave" variants of the spinlock. Also: if you're not "from_last_cpu" I think you somehow confirm that we're in solver mode. The only time it's legal to call this when not "from_last_cpu" is when we've previously set solver mode, right? That's the thing that makes everything OK and fulfills all the requirements talked about in rpmh-rsc.c like in the comments for rpmh_rsc_write_ctrl_data() where we say: * The caller must ensure that no other RPMH actions are happening and the * controller is idle when this function is called since it runs lockless. I know I told you in patch #1 that we shouldn't have two copies of the "in_solver_mode" state variable and, on the surface, it seems like the check I'm requesting would be hard to do. I _think_ the right thing to do is actually to combine your rpmh_write_sleep_and_wake() and rpmh_mode_solver_set() functions. One way to do this would be to just have rpmh_write_sleep_and_wake() implicitly enter solver mode. Then you could change rpmh_mode_solver_set() to just rpmh_mode_solver_exit() and have it only used to exit solver mode. -Doug
diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h index 79486d6..f351780 100644 --- a/drivers/soc/qcom/rpmh-internal.h +++ b/drivers/soc/qcom/rpmh-internal.h @@ -136,6 +136,6 @@ void rpmh_rsc_invalidate(struct rsc_drv *drv); int rpmh_rsc_mode_solver_set(struct rsc_drv *drv, bool enable); void rpmh_tx_done(const struct tcs_request *msg, int r); -int rpmh_flush(struct rpmh_ctrlr *ctrlr); +int rpmh_flush(struct rpmh_ctrlr *ctrlr, bool from_last_cpu); #endif /* __RPM_INTERNAL_H__ */ diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c index 1c1f5b0..a67bcd6 100644 --- a/drivers/soc/qcom/rpmh-rsc.c +++ b/drivers/soc/qcom/rpmh-rsc.c @@ -841,7 +841,8 @@ static int rpmh_rsc_cpu_pm_callback(struct notifier_block *nfb, * CPU. */ if (spin_trylock(&drv->lock)) { - if (rpmh_rsc_ctrlr_is_busy(drv) || rpmh_flush(&drv->client)) + if (rpmh_rsc_ctrlr_is_busy(drv) || + rpmh_flush(&drv->client, true)) ret = NOTIFY_BAD; spin_unlock(&drv->lock); } else { diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c index 725b8f0..682c566 100644 --- a/drivers/soc/qcom/rpmh.c +++ b/drivers/soc/qcom/rpmh.c @@ -458,22 +458,33 @@ static int send_single(struct rpmh_ctrlr *ctrlr, enum rpmh_state state, * rpmh_flush() - Flushes the buffered sleep and wake sets to TCSes * * @ctrlr: Controller making request to flush cached data + * @from_last_cpu: Set if invoked from last cpu with irqs disabled * * Return: * * 0 - Success * * Error code - Otherwise */ -int rpmh_flush(struct rpmh_ctrlr *ctrlr) +int rpmh_flush(struct rpmh_ctrlr *ctrlr, bool from_last_cpu) { struct cache_req *p; int ret = 0; - lockdep_assert_irqs_disabled(); + /* + * rpmh_flush() can be called when we think we're running + * on the last CPU with irqs_disabled or when RPMH client + * explicitly requests to write sleep and wake data. + * (for e.g. when in solver mode clients can requests to flush) + * + * Conditionally check for irqs_disabled only when called + * from last cpu. + */ + + if (from_last_cpu) + lockdep_assert_irqs_disabled(); /* - * 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; @@ -526,7 +537,7 @@ int rpmh_flush(struct rpmh_ctrlr *ctrlr) */ int rpmh_write_sleep_and_wake(const struct device *dev) { - return rpmh_flush(get_rpmh_ctrlr(dev)); + return rpmh_flush(get_rpmh_ctrlr(dev), false); } EXPORT_SYMBOL(rpmh_write_sleep_and_wake);
lockdep_assert_irqs_disabled() was added to check rpmh_flush() can only be invoked when irqs are disabled from last CPU, this is true for APPS RSC as the last CPU going to deepest low power mode is writing sleep and wake TCSes. However platform drivers can invoke rpmh_write_sleep_and_wake() to immediately write cached sleep and wake sets to TCSes from any CPU. Conditionally check if rpmh_flush() is invoked from last CPU then do not check for irqs disabled as such RSCs can write sleep and wake TCSes at any point. Signed-off-by: Maulik Shah <mkshah@codeaurora.org> --- Changes in v2: - Update rpmh_flush() to show if its called from last CPU or not - Drop solver client flag as rpmh_flush() able to check if called from last CPU or not --- drivers/soc/qcom/rpmh-internal.h | 2 +- drivers/soc/qcom/rpmh-rsc.c | 3 ++- drivers/soc/qcom/rpmh.c | 23 +++++++++++++++++------ 3 files changed, 20 insertions(+), 8 deletions(-)