mhi.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* [PATCH v3] bus: mhi: host: Disable preemption while processing data events
@ 2022-11-15  2:23 Qiang Yu
  2022-11-17 11:43 ` Manivannan Sadhasivam
  2022-11-17 11:49 ` Manivannan Sadhasivam
  0 siblings, 2 replies; 3+ messages in thread
From: Qiang Yu @ 2022-11-15  2:23 UTC (permalink / raw)
  To: mani, loic.poulain
  Cc: mhi, linux-arm-msm, linux-kernel, quic_cang, mrana, Qiang Yu

If data processing of an event is scheduled out because core
is busy handling multiple irqs, this can starve the processing
of MHI M0 state change event on another core. Fix this issue by
disabling irq on the core processing data events.

Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
---
v2->v3: modify the comment
v1->v2: add comments about why we disable local irq

 drivers/bus/mhi/host/main.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
index f3aef77a..5dca0a0 100644
--- a/drivers/bus/mhi/host/main.c
+++ b/drivers/bus/mhi/host/main.c
@@ -1027,13 +1027,20 @@ int mhi_process_data_event_ring(struct mhi_controller *mhi_cntrl,
 
 void mhi_ev_task(unsigned long data)
 {
+	unsigned long flags;
 	struct mhi_event *mhi_event = (struct mhi_event *)data;
 	struct mhi_controller *mhi_cntrl = mhi_event->mhi_cntrl;
 
 	/* process all pending events */
-	spin_lock_bh(&mhi_event->lock);
+	spin_lock_irqsave(&mhi_event->lock, flags);
+	/*
+	 * When multiple IRQs come, the tasklet will be scheduled out with event ring lock
+	 * remaining acquired, causing M0 event process on another core gets stuck when it
+	 * tries to acquire the same event ring lock. Thus, let's disable local IRQs here.
+	 */
+
 	mhi_event->process_event(mhi_cntrl, mhi_event, U32_MAX);
-	spin_unlock_bh(&mhi_event->lock);
+	spin_unlock_irqrestore(&mhi_event->lock, flags);
 }
 
 void mhi_ctrl_ev_task(unsigned long data)
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH v3] bus: mhi: host: Disable preemption while processing data events
  2022-11-15  2:23 [PATCH v3] bus: mhi: host: Disable preemption while processing data events Qiang Yu
@ 2022-11-17 11:43 ` Manivannan Sadhasivam
  2022-11-17 11:49 ` Manivannan Sadhasivam
  1 sibling, 0 replies; 3+ messages in thread
From: Manivannan Sadhasivam @ 2022-11-17 11:43 UTC (permalink / raw)
  To: Qiang Yu
  Cc: mani, loic.poulain, mhi, linux-arm-msm, linux-kernel, quic_cang, mrana

On Tue, Nov 15, 2022 at 10:23:51AM +0800, Qiang Yu wrote:
> If data processing of an event is scheduled out because core
> is busy handling multiple irqs, this can starve the processing
> of MHI M0 state change event on another core. Fix this issue by
> disabling irq on the core processing data events.
> 
> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>

I'm counting on Jeff's review for this patch.

Thanks,
Mani

> ---
> v2->v3: modify the comment
> v1->v2: add comments about why we disable local irq
> 
>  drivers/bus/mhi/host/main.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
> index f3aef77a..5dca0a0 100644
> --- a/drivers/bus/mhi/host/main.c
> +++ b/drivers/bus/mhi/host/main.c
> @@ -1027,13 +1027,20 @@ int mhi_process_data_event_ring(struct mhi_controller *mhi_cntrl,
>  
>  void mhi_ev_task(unsigned long data)
>  {
> +	unsigned long flags;
>  	struct mhi_event *mhi_event = (struct mhi_event *)data;
>  	struct mhi_controller *mhi_cntrl = mhi_event->mhi_cntrl;
>  
>  	/* process all pending events */
> -	spin_lock_bh(&mhi_event->lock);
> +	spin_lock_irqsave(&mhi_event->lock, flags);
> +	/*
> +	 * When multiple IRQs come, the tasklet will be scheduled out with event ring lock
> +	 * remaining acquired, causing M0 event process on another core gets stuck when it
> +	 * tries to acquire the same event ring lock. Thus, let's disable local IRQs here.
> +	 */
> +
>  	mhi_event->process_event(mhi_cntrl, mhi_event, U32_MAX);
> -	spin_unlock_bh(&mhi_event->lock);
> +	spin_unlock_irqrestore(&mhi_event->lock, flags);
>  }
>  
>  void mhi_ctrl_ev_task(unsigned long data)
> -- 
> 2.7.4
> 
> 

-- 
மணிவண்ணன் சதாசிவம்

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v3] bus: mhi: host: Disable preemption while processing data events
  2022-11-15  2:23 [PATCH v3] bus: mhi: host: Disable preemption while processing data events Qiang Yu
  2022-11-17 11:43 ` Manivannan Sadhasivam
@ 2022-11-17 11:49 ` Manivannan Sadhasivam
  1 sibling, 0 replies; 3+ messages in thread
From: Manivannan Sadhasivam @ 2022-11-17 11:49 UTC (permalink / raw)
  To: Qiang Yu
  Cc: mani, loic.poulain, mhi, linux-arm-msm, linux-kernel, quic_cang, mrana

On Tue, Nov 15, 2022 at 10:23:51AM +0800, Qiang Yu wrote:
> If data processing of an event is scheduled out because core
> is busy handling multiple irqs, this can starve the processing
> of MHI M0 state change event on another core. Fix this issue by
> disabling irq on the core processing data events.
> 
> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
> ---
> v2->v3: modify the comment
> v1->v2: add comments about why we disable local irq
> 
>  drivers/bus/mhi/host/main.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
> index f3aef77a..5dca0a0 100644
> --- a/drivers/bus/mhi/host/main.c
> +++ b/drivers/bus/mhi/host/main.c
> @@ -1027,13 +1027,20 @@ int mhi_process_data_event_ring(struct mhi_controller *mhi_cntrl,
>  
>  void mhi_ev_task(unsigned long data)
>  {
> +	unsigned long flags;

Reverse X-Mas tree order please. Move this definition to the last.

>  	struct mhi_event *mhi_event = (struct mhi_event *)data;
>  	struct mhi_controller *mhi_cntrl = mhi_event->mhi_cntrl;
>  
>  	/* process all pending events */
> -	spin_lock_bh(&mhi_event->lock);
> +	spin_lock_irqsave(&mhi_event->lock, flags);
> +	/*
> +	 * When multiple IRQs come, the tasklet will be scheduled out with event ring lock

s/come/arrive

> +	 * remaining acquired, causing M0 event process on another core gets stuck when it

s/remaining//

"causing other high priority events like M0 state transition getting stuck
while trying to acquire the same event ring lock."

> +	 * tries to acquire the same event ring lock. Thus, let's disable local IRQs here.
> +	 */

This comment should be placed above spin_lock_irqsave().

The comment "process all pending events" can be moved here.

Thanks,
Mani

> +
>  	mhi_event->process_event(mhi_cntrl, mhi_event, U32_MAX);
> -	spin_unlock_bh(&mhi_event->lock);
> +	spin_unlock_irqrestore(&mhi_event->lock, flags);
>  }
>  
>  void mhi_ctrl_ev_task(unsigned long data)
> -- 
> 2.7.4
> 
> 

-- 
மணிவண்ணன் சதாசிவம்

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2022-11-17 11:49 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-15  2:23 [PATCH v3] bus: mhi: host: Disable preemption while processing data events Qiang Yu
2022-11-17 11:43 ` Manivannan Sadhasivam
2022-11-17 11:49 ` Manivannan Sadhasivam

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).