Re: [PATCH] soc: qcom: rpmh-rsc: Sleep waiting for tcs slots to be free

Lina Iyer
2020-07-23T17:43:10Z

On Wed, Jul 22 2020 at 19:01 -0600, Stephen Boyd wrote:
>The busy loop in rpmh_rsc_send_data() is written with the assumption
>that the udelay will be preempted by the tcs_tx_done() irq handler when
>the TCS slots are all full. This doesn't hold true when the calling
>thread is an irqthread and the tcs_tx_done() irq is also an irqthread.
>That's because kernel irqthreads are SCHED_FIFO and thus need to
>voluntarily give up priority by calling into the scheduler so that other
>threads can run.
>
>I see RCU stalls when I boot with irqthreads on the kernel commandline
>because the modem remoteproc driver is trying to send an rpmh async
>message from an irqthread that needs to give up the CPU for the rpmh
>irqthread to run and clear out tcs slots.
>
Would this be not better, if we we use a threaded IRQ handler or offload
tx_done to another waitqueue instead of handling it in IRQ handler?

-- Lina

> rcu: INFO: rcu_preempt self-detected stall on CPU
> rcu: 0-....: (1 GPs behind) idle=402/1/0x4000000000000002 softirq=2108/2109 fqs=4920
> (t=21016 jiffies g=2933 q=590)
> Task dump for CPU 0:
> irq/11-smp2p R running task 0 148 2 0x00000028
> Call trace:
> dump_backtrace+0x0/0x154
> show_stack+0x20/0x2c
> sched_show_task+0xfc/0x108
> dump_cpu_task+0x44/0x50
> rcu_dump_cpu_stacks+0xa4/0xf8
> rcu_sched_clock_irq+0x7dc/0xaa8
> update_process_times+0x30/0x54
> tick_sched_handle+0x50/0x64
> tick_sched_timer+0x4c/0x8c
> __hrtimer_run_queues+0x21c/0x36c
> hrtimer_interrupt+0xf0/0x22c
> arch_timer_handler_phys+0x40/0x50
> handle_percpu_devid_irq+0x114/0x25c
> __handle_domain_irq+0x84/0xc4
> gic_handle_irq+0xd0/0x178
> el1_irq+0xbc/0x180
> save_return_addr+0x18/0x28
> return_address+0x54/0x88
> preempt_count_sub+0x40/0x88
> _raw_spin_unlock_irqrestore+0x4c/0x6c
> ___ratelimit+0xd0/0x128
> rpmh_rsc_send_data+0x24c/0x378
> __rpmh_write+0x1b0/0x208
> rpmh_write_async+0x90/0xbc
> rpmhpd_send_corner+0x60/0x8c
> rpmhpd_aggregate_corner+0x8c/0x124
> rpmhpd_set_performance_state+0x8c/0xbc
> _genpd_set_performance_state+0xdc/0x1b8
> dev_pm_genpd_set_performance_state+0xb8/0xf8
> q6v5_pds_disable+0x34/0x60 [qcom_q6v5_mss]
> qcom_msa_handover+0x38/0x44 [qcom_q6v5_mss]
> q6v5_handover_interrupt+0x24/0x3c [qcom_q6v5]
> handle_nested_irq+0xd0/0x138
> qcom_smp2p_intr+0x188/0x200
> irq_thread_fn+0x2c/0x70
> irq_thread+0xfc/0x14c
> kthread+0x11c/0x12c
> ret_from_fork+0x10/0x18
>
>This busy loop naturally lends itself to using a wait queue so that each
>thread that tries to send a message will sleep waiting on the waitqueue
>and only be woken up when a free slot is available. This should make
>things more predictable too because the scheduler will be able to sleep
>tasks that are waiting on a free tcs instead of the busy loop we
>currently have today.
>
>Cc: Douglas Anderson <dianders@chromium.org>
>Cc: Maulik Shah <mkshah@codeaurora.org>
>Cc: Lina Iyer <ilina@codeaurora.org>
>Signed-off-by: Stephen Boyd <swboyd@chromium.org>
>---
> drivers/soc/qcom/rpmh-internal.h | 2 +
> drivers/soc/qcom/rpmh-rsc.c | 101 ++++++++++++-------------------
> 2 files changed, 41 insertions(+), 62 deletions(-)
>
>diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
>index ef60e790a750..9a325bac58fe 100644
>--- a/drivers/soc/qcom/rpmh-internal.h
>+++ b/drivers/soc/qcom/rpmh-internal.h
>@@ -8,6 +8,7 @@
> #define __RPM_INTERNAL_H__
>
> #include <linux/bitmap.h>
>+#include <linux/wait.h>
> #include <soc/qcom/tcs.h>
>
> #define TCS_TYPE_NR 4
>@@ -118,6 +119,7 @@ struct rsc_drv {
> struct tcs_group tcs[TCS_TYPE_NR];
> DECLARE_BITMAP(tcs_in_use, MAX_TCS_NR);
> spinlock_t lock;
>+ wait_queue_head_t tcs_wait;
> struct rpmh_ctrlr client;
> };
>
>diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
>index 076fd27f3081..6c758b052c95 100644
>--- a/drivers/soc/qcom/rpmh-rsc.c
>+++ b/drivers/soc/qcom/rpmh-rsc.c
>@@ -19,6 +19,7 @@
> #include <linux/platform_device.h>
> #include <linux/slab.h>
> #include <linux/spinlock.h>
>+#include <linux/wait.h>
>
> #include <soc/qcom/cmd-db.h>
> #include <soc/qcom/tcs.h>
>@@ -444,6 +445,7 @@ static irqreturn_t tcs_tx_done(int irq, void *p)
> */
> if (!drv->tcs[ACTIVE_TCS].num_tcs)
> enable_tcs_irq(drv, i, false);
>+ wake_up(&drv->tcs_wait);
> spin_unlock(&drv->lock);
> if (req)
> rpmh_tx_done(req, err);
>@@ -562,44 +564,59 @@ static int find_free_tcs(struct tcs_group *tcs)
> return -EBUSY;
> }
>
>+static int claim_tcs_for_req(struct rsc_drv *drv, struct tcs_group *tcs,
>+ const struct tcs_request *msg)
>+{
>+ int ret;
>+
>+ /*
>+ * 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)
>+ return ret;
>+
>+ return find_free_tcs(tcs);
>+}
>+
> /**
>- * tcs_write() - Store messages into a TCS right now, or return -EBUSY.
>+ * rpmh_rsc_send_data() - Write / trigger active-only message.
> * @drv: The controller.
> * @msg: The data to be sent.
> *
>- * Grabs a TCS for ACTIVE_ONLY transfers and writes the messages to it.
>- *
>- * If there are no free TCSes for ACTIVE_ONLY transfers or if a command for
>- * the same address is already transferring returns -EBUSY which means the
>- * client should retry shortly.
>+ * NOTES:
>+ * - This is only used for "ACTIVE_ONLY" since the limitations of this
>+ * function don't make sense for sleep/wake cases.
>+ * - To do the transfer, we will grab a whole TCS for ourselves--we don't
>+ * try to share. If there are none available we'll wait indefinitely
>+ * for a free one.
>+ * - This function will not wait for the commands to be finished, only for
>+ * data to be programmed into the RPMh. See rpmh_tx_done() which will
>+ * be called when the transfer is fully complete.
>+ * - This function must be called with interrupts enabled. If the hardware
>+ * is busy doing someone else's transfer we need that transfer to fully
>+ * finish so that we can have the hardware, and to fully finish it needs
>+ * the interrupt handler to run. If the interrupts is set to run on the
>+ * active CPU this can never happen if interrupts are disabled.
> *
>- * Return: 0 on success, -EBUSY if client should retry, or an error.
>- * Client should have interrupts enabled for a bit before retrying.
>+ * Return: 0 on success, -EINVAL on error.
> */
>-static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
>+int rpmh_rsc_send_data(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(&drv->lock, flags);
>- /*
>- * 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)
>- goto unlock;
>
>- ret = find_free_tcs(tcs);
>- if (ret < 0)
>- goto unlock;
>- tcs_id = ret;
>+ wait_event_lock_irq(drv->tcs_wait,
>+ (tcs_id = claim_tcs_for_req(drv, tcs, msg)) >= 0,
>+ drv->lock);
>
> tcs->req[tcs_id - tcs->offset] = msg;
> set_bit(tcs_id, drv->tcs_in_use);
>@@ -627,47 +644,6 @@ static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
> __tcs_set_trigger(drv, tcs_id, true);
>
> return 0;
>-unlock:
>- spin_unlock_irqrestore(&drv->lock, flags);
>- return ret;
>-}
>-
>-/**
>- * rpmh_rsc_send_data() - Write / trigger active-only message.
>- * @drv: The controller.
>- * @msg: The data to be sent.
>- *
>- * NOTES:
>- * - This is only used for "ACTIVE_ONLY" since the limitations of this
>- * function don't make sense for sleep/wake cases.
>- * - To do the transfer, we will grab a whole TCS for ourselves--we don't
>- * try to share. If there are none available we'll wait indefinitely
>- * for a free one.
>- * - This function will not wait for the commands to be finished, only for
>- * data to be programmed into the RPMh. See rpmh_tx_done() which will
>- * be called when the transfer is fully complete.
>- * - This function must be called with interrupts enabled. If the hardware
>- * is busy doing someone else's transfer we need that transfer to fully
>- * finish so that we can have the hardware, and to fully finish it needs
>- * the interrupt handler to run. If the interrupts is set to run on the
>- * active CPU this can never happen if interrupts are disabled.
>- *
>- * Return: 0 on success, -EINVAL on error.
>- */
>-int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg)
>-{
>- int ret;
>-
>- do {
>- ret = tcs_write(drv, msg);
>- if (ret == -EBUSY) {
>- pr_info_ratelimited("TCS Busy, retrying RPMH message send: addr=%#x\n",
>- msg->cmds[0].addr);
>- udelay(10);
>- }
>- } while (ret == -EBUSY);
>-
>- return ret;
> }
>
> /**
>@@ -975,6 +951,7 @@ static int rpmh_rsc_probe(struct platform_device *pdev)
> return ret;
>
> spin_lock_init(&drv->lock);
>+ init_waitqueue_head(&drv->tcs_wait);
> bitmap_zero(drv->tcs_in_use, MAX_TCS_NR);
>
> irq = platform_get_irq(pdev, drv->id);
>
>base-commit: b3a9e3b9622ae10064826dccb4f7a52bd88c7407
>--
>Sent by a computer, using git, on the internet
>