[PATCH v1 0/5] MHI Misc Fixes

[PATCH v1 0/5] MHI Misc Fixes

[Hemant Kumar]

Patches are addressing MHI core driver bug fixes. Patches tested on
arm64 and x86 platforms.

Hemant Kumar (5):
  bus: mhi: core: Remove the system error worker thread
  bus: mhi: core: Handle disable transitions in state worker
  bus: mhi: core: Skip handling BHI irq if MHI reg access is not allowed
  bus: mhi: core: Do not process SYS_ERROR if RDDM is supported
  bus: mhi: core: Handle write lock properly in mhi_pm_m0_transition

 drivers/bus/mhi/core/init.c     |  3 ++-
 drivers/bus/mhi/core/internal.h |  4 +++-
 drivers/bus/mhi/core/main.c     | 38 +++++++++++++++++++----------
 drivers/bus/mhi/core/pm.c       | 53 ++++++++++++++++++++++++-----------------
 include/linux/mhi.h             |  2 --
 5 files changed, 61 insertions(+), 39 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v1 1/5] bus: mhi: core: Remove the system error worker thread

[Hemant Kumar]

Remove the system error worker thread and instead have the
execution environment worker handle that transition to serialize
processing and avoid any possible race conditions during
shutdown.

Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>
---
 drivers/bus/mhi/core/init.c     |  2 +-
 drivers/bus/mhi/core/internal.h |  3 ++-
 drivers/bus/mhi/core/main.c     |  6 +++---
 drivers/bus/mhi/core/pm.c       | 32 ++++++++++++++------------------
 include/linux/mhi.h             |  2 --
 5 files changed, 20 insertions(+), 25 deletions(-)

diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index 6882206..3a853c5 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -34,6 +34,7 @@
 	[DEV_ST_TRANSITION_READY] = "READY",
 	[DEV_ST_TRANSITION_SBL] = "SBL",
 	[DEV_ST_TRANSITION_MISSION_MODE] = "MISSION_MODE",
+	[DEV_ST_TRANSITION_SYS_ERR] = "SYS_ERR",
 };
 
 const char * const mhi_state_str[MHI_STATE_MAX] = {
@@ -834,7 +835,6 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
 	spin_lock_init(&mhi_cntrl->transition_lock);
 	spin_lock_init(&mhi_cntrl->wlock);
 	INIT_WORK(&mhi_cntrl->st_worker, mhi_pm_st_worker);
-	INIT_WORK(&mhi_cntrl->syserr_worker, mhi_pm_sys_err_worker);
 	init_waitqueue_head(&mhi_cntrl->state_event);
 
 	mhi_cmd = mhi_cntrl->mhi_cmd;
diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
index 80b32c2..f01283b 100644
--- a/drivers/bus/mhi/core/internal.h
+++ b/drivers/bus/mhi/core/internal.h
@@ -386,6 +386,7 @@ enum dev_st_transition {
 	DEV_ST_TRANSITION_READY,
 	DEV_ST_TRANSITION_SBL,
 	DEV_ST_TRANSITION_MISSION_MODE,
+	DEV_ST_TRANSITION_SYS_ERR,
 	DEV_ST_TRANSITION_MAX,
 };
 
@@ -587,7 +588,7 @@ enum mhi_pm_state __must_check mhi_tryset_pm_state(
 int mhi_queue_state_transition(struct mhi_controller *mhi_cntrl,
 			       enum dev_st_transition state);
 void mhi_pm_st_worker(struct work_struct *work);
-void mhi_pm_sys_err_worker(struct work_struct *work);
+void mhi_pm_sys_err_handler(struct mhi_controller *mhi_cntrl);
 void mhi_fw_load_worker(struct work_struct *work);
 int mhi_ready_state_transition(struct mhi_controller *mhi_cntrl);
 void mhi_ctrl_ev_task(unsigned long data);
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index 6a80666..9ec9b36 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -406,7 +406,7 @@ irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *dev)
 		if (MHI_IN_PBL(ee))
 			mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_FATAL_ERROR);
 		else
-			schedule_work(&mhi_cntrl->syserr_worker);
+			mhi_pm_sys_err_handler(mhi_cntrl);
 	}
 
 exit_intvec:
@@ -734,7 +734,7 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl,
 							MHI_PM_SYS_ERR_DETECT);
 				write_unlock_irq(&mhi_cntrl->pm_lock);
 				if (new_state == MHI_PM_SYS_ERR_DETECT)
-					schedule_work(&mhi_cntrl->syserr_worker);
+					mhi_pm_sys_err_handler(mhi_cntrl);
 				break;
 			}
 			default:
@@ -920,7 +920,7 @@ void mhi_ctrl_ev_task(unsigned long data)
 		}
 		write_unlock_irq(&mhi_cntrl->pm_lock);
 		if (pm_state == MHI_PM_SYS_ERR_DETECT)
-			schedule_work(&mhi_cntrl->syserr_worker);
+			mhi_pm_sys_err_handler(mhi_cntrl);
 	}
 }
 
diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
index 3cc238a..d9964d4 100644
--- a/drivers/bus/mhi/core/pm.c
+++ b/drivers/bus/mhi/core/pm.c
@@ -449,19 +449,8 @@ static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl,
 		to_mhi_pm_state_str(transition_state));
 
 	/* We must notify MHI control driver so it can clean up first */
-	if (transition_state == MHI_PM_SYS_ERR_PROCESS) {
-		/*
-		 * If controller supports RDDM, we do not process
-		 * SYS error state, instead we will jump directly
-		 * to RDDM state
-		 */
-		if (mhi_cntrl->rddm_image) {
-			dev_dbg(dev,
-				 "Controller supports RDDM, so skip SYS_ERR\n");
-			return;
-		}
+	if (transition_state == MHI_PM_SYS_ERR_PROCESS)
 		mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_SYS_ERROR);
-	}
 
 	mutex_lock(&mhi_cntrl->pm_mutex);
 	write_lock_irq(&mhi_cntrl->pm_lock);
@@ -527,7 +516,6 @@ static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl,
 	mutex_unlock(&mhi_cntrl->pm_mutex);
 	dev_dbg(dev, "Waiting for all pending threads to complete\n");
 	wake_up_all(&mhi_cntrl->state_event);
-	flush_work(&mhi_cntrl->st_worker);
 
 	dev_dbg(dev, "Reset all active channels and remove MHI devices\n");
 	device_for_each_child(mhi_cntrl->cntrl_dev, NULL, mhi_destroy_device);
@@ -607,13 +595,17 @@ int mhi_queue_state_transition(struct mhi_controller *mhi_cntrl,
 }
 
 /* SYS_ERR worker */
-void mhi_pm_sys_err_worker(struct work_struct *work)
+void mhi_pm_sys_err_handler(struct mhi_controller *mhi_cntrl)
 {
-	struct mhi_controller *mhi_cntrl = container_of(work,
-							struct mhi_controller,
-							syserr_worker);
+	struct device *dev = &mhi_cntrl->mhi_dev->dev;
+
+	/* skip if controller supports RDDM */
+	if (mhi_cntrl->rddm_image) {
+		dev_dbg(dev, "Controller supports RDDM, skip SYS_ERROR\n");
+		return;
+	}
 
-	mhi_pm_disable_transition(mhi_cntrl, MHI_PM_SYS_ERR_PROCESS);
+	mhi_queue_state_transition(mhi_cntrl, DEV_ST_TRANSITION_SYS_ERR);
 }
 
 /* Device State Transition worker */
@@ -661,6 +653,10 @@ void mhi_pm_st_worker(struct work_struct *work)
 		case DEV_ST_TRANSITION_READY:
 			mhi_ready_state_transition(mhi_cntrl);
 			break;
+		case DEV_ST_TRANSITION_SYS_ERR:
+			mhi_pm_disable_transition
+				(mhi_cntrl, MHI_PM_SYS_ERR_PROCESS);
+			break;
 		default:
 			break;
 		}
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index 2b20b9c..b008914 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -331,7 +331,6 @@ struct mhi_controller_config {
  * @wlock: Lock for protecting device wakeup
  * @mhi_link_info: Device bandwidth info
  * @st_worker: State transition worker
- * @syserr_worker: System error worker
  * @state_event: State change event
  * @status_cb: CB function to notify power states of the device (required)
  * @wake_get: CB function to assert device wake (optional)
@@ -411,7 +410,6 @@ struct mhi_controller {
 	spinlock_t wlock;
 	struct mhi_link_info mhi_link_info;
 	struct work_struct st_worker;
-	struct work_struct syserr_worker;
 	wait_queue_head_t state_event;
 
 	void (*status_cb)(struct mhi_controller *mhi_cntrl,
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v1 2/5] bus: mhi: core: Handle disable transitions in state worker

[Hemant Kumar]

Mission mode transition is handled by state worker thread but
power off is not. There is a possibility while mission mode
transition is in progress which calls MHI client driver probe,
power off is issued by MHI controller. This results into client
driver probe and remove running in parallel and causes use after
free situation. By queuing disable transition work when mission
mode is in progress prevents the race condition.

Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>
---
 drivers/bus/mhi/core/init.c     |  1 +
 drivers/bus/mhi/core/internal.h |  1 +
 drivers/bus/mhi/core/pm.c       | 11 ++++++++++-
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index 3a853c5..12207cc 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -35,6 +35,7 @@
 	[DEV_ST_TRANSITION_SBL] = "SBL",
 	[DEV_ST_TRANSITION_MISSION_MODE] = "MISSION_MODE",
 	[DEV_ST_TRANSITION_SYS_ERR] = "SYS_ERR",
+	[DEV_ST_TRANSITION_DISABLE] = "DISABLE",
 };
 
 const char * const mhi_state_str[MHI_STATE_MAX] = {
diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
index f01283b..b1f640b 100644
--- a/drivers/bus/mhi/core/internal.h
+++ b/drivers/bus/mhi/core/internal.h
@@ -387,6 +387,7 @@ enum dev_st_transition {
 	DEV_ST_TRANSITION_SBL,
 	DEV_ST_TRANSITION_MISSION_MODE,
 	DEV_ST_TRANSITION_SYS_ERR,
+	DEV_ST_TRANSITION_DISABLE,
 	DEV_ST_TRANSITION_MAX,
 };
 
diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
index d9964d4..345f197 100644
--- a/drivers/bus/mhi/core/pm.c
+++ b/drivers/bus/mhi/core/pm.c
@@ -657,6 +657,10 @@ void mhi_pm_st_worker(struct work_struct *work)
 			mhi_pm_disable_transition
 				(mhi_cntrl, MHI_PM_SYS_ERR_PROCESS);
 			break;
+		case DEV_ST_TRANSITION_DISABLE:
+			mhi_pm_disable_transition
+				(mhi_cntrl, MHI_PM_SHUTDOWN_PROCESS);
+			break;
 		default:
 			break;
 		}
@@ -1011,7 +1015,12 @@ void mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful)
 				to_mhi_pm_state_str(MHI_PM_LD_ERR_FATAL_DETECT),
 				to_mhi_pm_state_str(mhi_cntrl->pm_state));
 	}
-	mhi_pm_disable_transition(mhi_cntrl, MHI_PM_SHUTDOWN_PROCESS);
+
+	mhi_queue_state_transition(mhi_cntrl, DEV_ST_TRANSITION_DISABLE);
+
+	/* Wait for shutdown to complete */
+	flush_work(&mhi_cntrl->st_worker);
+
 	mhi_deinit_free_irq(mhi_cntrl);
 
 	if (!mhi_cntrl->pre_init) {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v1 3/5] bus: mhi: core: Skip handling BHI irq if MHI reg access is not allowed

[Hemant Kumar]

Driver continues handling of BHI interrupt even if MHI register access
is not allowed. By doing so it calls the status call back and performs
early notification for the MHI client. This is not needed when MHI
register access is not allowed. Hence skip the handling in this case and
return. Also add debug log to print device state, local EE and device EE
when reg access is valid.

Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>
---
 drivers/bus/mhi/core/main.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index 9ec9b36..467c0ba 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -369,22 +369,29 @@ irqreturn_t mhi_irq_handler(int irq_number, void *dev)
 	return IRQ_HANDLED;
 }
 
-irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *dev)
+irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *priv)
 {
-	struct mhi_controller *mhi_cntrl = dev;
+	struct mhi_controller *mhi_cntrl = priv;
+	struct device *dev = &mhi_cntrl->mhi_dev->dev;
 	enum mhi_state state = MHI_STATE_MAX;
 	enum mhi_pm_state pm_state = 0;
 	enum mhi_ee_type ee = 0;
 
 	write_lock_irq(&mhi_cntrl->pm_lock);
-	if (MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
-		state = mhi_get_mhi_state(mhi_cntrl);
-		ee = mhi_cntrl->ee;
-		mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);
+	if (!MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
+		write_unlock_irq(&mhi_cntrl->pm_lock);
+		goto exit_intvec;
 	}
 
+	state = mhi_get_mhi_state(mhi_cntrl);
+	ee = mhi_cntrl->ee;
+	mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);
+	dev_dbg(dev, "local ee:%s device ee:%s dev_state:%s\n",
+		TO_MHI_EXEC_STR(mhi_cntrl->ee), TO_MHI_EXEC_STR(ee),
+		TO_MHI_STATE_STR(state));
+
 	if (state == MHI_STATE_SYS_ERR) {
-		dev_dbg(&mhi_cntrl->mhi_dev->dev, "System error detected\n");
+		dev_dbg(dev, "System error detected\n");
 		pm_state = mhi_tryset_pm_state(mhi_cntrl,
 					       MHI_PM_SYS_ERR_DETECT);
 	}
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v1 4/5] bus: mhi: core: Do not process SYS_ERROR if RDDM is supported

[Hemant Kumar]

Devices that support RDDM do not require processing SYS_ERROR as it is
deemed redundant. Avoid SYS_ERROR processing if RDDM is supported by
the device.

Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>
---
 drivers/bus/mhi/core/main.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index 467c0ba..1f622ce 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -397,9 +397,9 @@ irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *priv)
 	}
 	write_unlock_irq(&mhi_cntrl->pm_lock);
 
-	/* If device in RDDM don't bother processing SYS error */
-	if (mhi_cntrl->ee == MHI_EE_RDDM) {
-		if (mhi_cntrl->ee != ee) {
+	 /* If device supports RDDM don't bother processing SYS error */
+	if (mhi_cntrl->rddm_image) {
+		if (mhi_cntrl->ee == MHI_EE_RDDM && mhi_cntrl->ee != ee) {
 			mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_EE_RDDM);
 			wake_up_all(&mhi_cntrl->state_event);
 		}
@@ -735,6 +735,11 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl,
 			{
 				enum mhi_pm_state new_state;
 
+				/* skip SYS_ERROR handling if RDDM supported */
+				if (mhi_cntrl->ee == MHI_EE_RDDM ||
+				    mhi_cntrl->rddm_image)
+					break;
+
 				dev_dbg(dev, "System error detected\n");
 				write_lock_irq(&mhi_cntrl->pm_lock);
 				new_state = mhi_tryset_pm_state(mhi_cntrl,
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v1 5/5] bus: mhi: core: Handle write lock properly in mhi_pm_m0_transition

[Hemant Kumar]

Take write lock only to protect db_mode member of mhi channel.
This allows rest of the mhi channels to just take read lock which
fine grains the locking. It prevents channel readers to starve if
they try to enter critical section after a writer.

Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
---
 drivers/bus/mhi/core/pm.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
index 345f197..de5abb2 100644
--- a/drivers/bus/mhi/core/pm.c
+++ b/drivers/bus/mhi/core/pm.c
@@ -288,14 +288,18 @@ int mhi_pm_m0_transition(struct mhi_controller *mhi_cntrl)
 	for (i = 0; i < mhi_cntrl->max_chan; i++, mhi_chan++) {
 		struct mhi_ring *tre_ring = &mhi_chan->tre_ring;
 
-		write_lock_irq(&mhi_chan->lock);
-		if (mhi_chan->db_cfg.reset_req)
+		if (mhi_chan->db_cfg.reset_req) {
+			write_lock_irq(&mhi_chan->lock);
 			mhi_chan->db_cfg.db_mode = true;
+			write_unlock_irq(&mhi_chan->lock);
+		}
+
+		read_lock_irq(&mhi_chan->lock);
 
 		/* Only ring DB if ring is not empty */
 		if (tre_ring->base && tre_ring->wp  != tre_ring->rp)
 			mhi_ring_chan_db(mhi_cntrl, mhi_chan);
-		write_unlock_irq(&mhi_chan->lock);
+		read_unlock_irq(&mhi_chan->lock);
 	}
 
 	mhi_cntrl->wake_put(mhi_cntrl, false);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v1 1/5] bus: mhi: core: Remove the system error worker thread

[Manivannan Sadhasivam]

On Mon, May 11, 2020 at 07:03:05PM -0700, Hemant Kumar wrote:
> Remove the system error worker thread and instead have the
> execution environment worker handle that transition to serialize
> processing and avoid any possible race conditions during
> shutdown.
> 
> Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
> Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> ---
>  drivers/bus/mhi/core/init.c     |  2 +-
>  drivers/bus/mhi/core/internal.h |  3 ++-
>  drivers/bus/mhi/core/main.c     |  6 +++---
>  drivers/bus/mhi/core/pm.c       | 32 ++++++++++++++------------------
>  include/linux/mhi.h             |  2 --
>  5 files changed, 20 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index 6882206..3a853c5 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -34,6 +34,7 @@
>  	[DEV_ST_TRANSITION_READY] = "READY",
>  	[DEV_ST_TRANSITION_SBL] = "SBL",
>  	[DEV_ST_TRANSITION_MISSION_MODE] = "MISSION_MODE",
> +	[DEV_ST_TRANSITION_SYS_ERR] = "SYS_ERR",
>  };
>  
>  const char * const mhi_state_str[MHI_STATE_MAX] = {
> @@ -834,7 +835,6 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
>  	spin_lock_init(&mhi_cntrl->transition_lock);
>  	spin_lock_init(&mhi_cntrl->wlock);
>  	INIT_WORK(&mhi_cntrl->st_worker, mhi_pm_st_worker);
> -	INIT_WORK(&mhi_cntrl->syserr_worker, mhi_pm_sys_err_worker);
>  	init_waitqueue_head(&mhi_cntrl->state_event);
>  
>  	mhi_cmd = mhi_cntrl->mhi_cmd;
> diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
> index 80b32c2..f01283b 100644
> --- a/drivers/bus/mhi/core/internal.h
> +++ b/drivers/bus/mhi/core/internal.h
> @@ -386,6 +386,7 @@ enum dev_st_transition {
>  	DEV_ST_TRANSITION_READY,
>  	DEV_ST_TRANSITION_SBL,
>  	DEV_ST_TRANSITION_MISSION_MODE,
> +	DEV_ST_TRANSITION_SYS_ERR,
>  	DEV_ST_TRANSITION_MAX,
>  };
>  
> @@ -587,7 +588,7 @@ enum mhi_pm_state __must_check mhi_tryset_pm_state(
>  int mhi_queue_state_transition(struct mhi_controller *mhi_cntrl,
>  			       enum dev_st_transition state);
>  void mhi_pm_st_worker(struct work_struct *work);
> -void mhi_pm_sys_err_worker(struct work_struct *work);
> +void mhi_pm_sys_err_handler(struct mhi_controller *mhi_cntrl);
>  void mhi_fw_load_worker(struct work_struct *work);
>  int mhi_ready_state_transition(struct mhi_controller *mhi_cntrl);
>  void mhi_ctrl_ev_task(unsigned long data);
> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> index 6a80666..9ec9b36 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -406,7 +406,7 @@ irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *dev)
>  		if (MHI_IN_PBL(ee))
>  			mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_FATAL_ERROR);
>  		else
> -			schedule_work(&mhi_cntrl->syserr_worker);
> +			mhi_pm_sys_err_handler(mhi_cntrl);
>  	}
>  
>  exit_intvec:
> @@ -734,7 +734,7 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl,
>  							MHI_PM_SYS_ERR_DETECT);
>  				write_unlock_irq(&mhi_cntrl->pm_lock);
>  				if (new_state == MHI_PM_SYS_ERR_DETECT)
> -					schedule_work(&mhi_cntrl->syserr_worker);
> +					mhi_pm_sys_err_handler(mhi_cntrl);
>  				break;
>  			}
>  			default:
> @@ -920,7 +920,7 @@ void mhi_ctrl_ev_task(unsigned long data)
>  		}
>  		write_unlock_irq(&mhi_cntrl->pm_lock);
>  		if (pm_state == MHI_PM_SYS_ERR_DETECT)
> -			schedule_work(&mhi_cntrl->syserr_worker);
> +			mhi_pm_sys_err_handler(mhi_cntrl);
>  	}
>  }
>  
> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> index 3cc238a..d9964d4 100644
> --- a/drivers/bus/mhi/core/pm.c
> +++ b/drivers/bus/mhi/core/pm.c
> @@ -449,19 +449,8 @@ static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl,
>  		to_mhi_pm_state_str(transition_state));
>  
>  	/* We must notify MHI control driver so it can clean up first */
> -	if (transition_state == MHI_PM_SYS_ERR_PROCESS) {
> -		/*
> -		 * If controller supports RDDM, we do not process
> -		 * SYS error state, instead we will jump directly
> -		 * to RDDM state
> -		 */
> -		if (mhi_cntrl->rddm_image) {
> -			dev_dbg(dev,
> -				 "Controller supports RDDM, so skip SYS_ERR\n");
> -			return;
> -		}
> +	if (transition_state == MHI_PM_SYS_ERR_PROCESS)
>  		mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_SYS_ERROR);
> -	}
>  
>  	mutex_lock(&mhi_cntrl->pm_mutex);
>  	write_lock_irq(&mhi_cntrl->pm_lock);
> @@ -527,7 +516,6 @@ static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl,
>  	mutex_unlock(&mhi_cntrl->pm_mutex);
>  	dev_dbg(dev, "Waiting for all pending threads to complete\n");
>  	wake_up_all(&mhi_cntrl->state_event);
> -	flush_work(&mhi_cntrl->st_worker);
>  
>  	dev_dbg(dev, "Reset all active channels and remove MHI devices\n");
>  	device_for_each_child(mhi_cntrl->cntrl_dev, NULL, mhi_destroy_device);
> @@ -607,13 +595,17 @@ int mhi_queue_state_transition(struct mhi_controller *mhi_cntrl,
>  }
>  
>  /* SYS_ERR worker */
> -void mhi_pm_sys_err_worker(struct work_struct *work)
> +void mhi_pm_sys_err_handler(struct mhi_controller *mhi_cntrl)
>  {
> -	struct mhi_controller *mhi_cntrl = container_of(work,
> -							struct mhi_controller,
> -							syserr_worker);
> +	struct device *dev = &mhi_cntrl->mhi_dev->dev;
> +
> +	/* skip if controller supports RDDM */
> +	if (mhi_cntrl->rddm_image) {
> +		dev_dbg(dev, "Controller supports RDDM, skip SYS_ERROR\n");
> +		return;
> +	}
>  
> -	mhi_pm_disable_transition(mhi_cntrl, MHI_PM_SYS_ERR_PROCESS);
> +	mhi_queue_state_transition(mhi_cntrl, DEV_ST_TRANSITION_SYS_ERR);
>  }
>  
>  /* Device State Transition worker */
> @@ -661,6 +653,10 @@ void mhi_pm_st_worker(struct work_struct *work)
>  		case DEV_ST_TRANSITION_READY:
>  			mhi_ready_state_transition(mhi_cntrl);
>  			break;
> +		case DEV_ST_TRANSITION_SYS_ERR:
> +			mhi_pm_disable_transition
> +				(mhi_cntrl, MHI_PM_SYS_ERR_PROCESS);
> +			break;
>  		default:
>  			break;
>  		}
> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> index 2b20b9c..b008914 100644
> --- a/include/linux/mhi.h
> +++ b/include/linux/mhi.h
> @@ -331,7 +331,6 @@ struct mhi_controller_config {
>   * @wlock: Lock for protecting device wakeup
>   * @mhi_link_info: Device bandwidth info
>   * @st_worker: State transition worker
> - * @syserr_worker: System error worker
>   * @state_event: State change event
>   * @status_cb: CB function to notify power states of the device (required)
>   * @wake_get: CB function to assert device wake (optional)
> @@ -411,7 +410,6 @@ struct mhi_controller {
>  	spinlock_t wlock;
>  	struct mhi_link_info mhi_link_info;
>  	struct work_struct st_worker;
> -	struct work_struct syserr_worker;
>  	wait_queue_head_t state_event;
>  
>  	void (*status_cb)(struct mhi_controller *mhi_cntrl,
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project

Re: [PATCH v1 2/5] bus: mhi: core: Handle disable transitions in state worker

[Manivannan Sadhasivam]

On Mon, May 11, 2020 at 07:03:06PM -0700, Hemant Kumar wrote:
> Mission mode transition is handled by state worker thread but
> power off is not. There is a possibility while mission mode
> transition is in progress which calls MHI client driver probe,
> power off is issued by MHI controller. This results into client
> driver probe and remove running in parallel and causes use after
> free situation. By queuing disable transition work when mission
> mode is in progress prevents the race condition.
> 
> Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
> Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> ---
>  drivers/bus/mhi/core/init.c     |  1 +
>  drivers/bus/mhi/core/internal.h |  1 +
>  drivers/bus/mhi/core/pm.c       | 11 ++++++++++-
>  3 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index 3a853c5..12207cc 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -35,6 +35,7 @@
>  	[DEV_ST_TRANSITION_SBL] = "SBL",
>  	[DEV_ST_TRANSITION_MISSION_MODE] = "MISSION_MODE",
>  	[DEV_ST_TRANSITION_SYS_ERR] = "SYS_ERR",
> +	[DEV_ST_TRANSITION_DISABLE] = "DISABLE",
>  };
>  
>  const char * const mhi_state_str[MHI_STATE_MAX] = {
> diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
> index f01283b..b1f640b 100644
> --- a/drivers/bus/mhi/core/internal.h
> +++ b/drivers/bus/mhi/core/internal.h
> @@ -387,6 +387,7 @@ enum dev_st_transition {
>  	DEV_ST_TRANSITION_SBL,
>  	DEV_ST_TRANSITION_MISSION_MODE,
>  	DEV_ST_TRANSITION_SYS_ERR,
> +	DEV_ST_TRANSITION_DISABLE,
>  	DEV_ST_TRANSITION_MAX,
>  };
>  
> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> index d9964d4..345f197 100644
> --- a/drivers/bus/mhi/core/pm.c
> +++ b/drivers/bus/mhi/core/pm.c
> @@ -657,6 +657,10 @@ void mhi_pm_st_worker(struct work_struct *work)
>  			mhi_pm_disable_transition
>  				(mhi_cntrl, MHI_PM_SYS_ERR_PROCESS);
>  			break;
> +		case DEV_ST_TRANSITION_DISABLE:
> +			mhi_pm_disable_transition
> +				(mhi_cntrl, MHI_PM_SHUTDOWN_PROCESS);
> +			break;
>  		default:
>  			break;
>  		}
> @@ -1011,7 +1015,12 @@ void mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful)
>  				to_mhi_pm_state_str(MHI_PM_LD_ERR_FATAL_DETECT),
>  				to_mhi_pm_state_str(mhi_cntrl->pm_state));
>  	}
> -	mhi_pm_disable_transition(mhi_cntrl, MHI_PM_SHUTDOWN_PROCESS);
> +
> +	mhi_queue_state_transition(mhi_cntrl, DEV_ST_TRANSITION_DISABLE);
> +
> +	/* Wait for shutdown to complete */
> +	flush_work(&mhi_cntrl->st_worker);
> +
>  	mhi_deinit_free_irq(mhi_cntrl);
>  
>  	if (!mhi_cntrl->pre_init) {
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project

Re: [PATCH v1 3/5] bus: mhi: core: Skip handling BHI irq if MHI reg access is not allowed

[Manivannan Sadhasivam]

On Mon, May 11, 2020 at 07:03:07PM -0700, Hemant Kumar wrote:
> Driver continues handling of BHI interrupt even if MHI register access
> is not allowed. By doing so it calls the status call back and performs
> early notification for the MHI client. This is not needed when MHI
> register access is not allowed. Hence skip the handling in this case and
> return. Also add debug log to print device state, local EE and device EE
> when reg access is valid.
> 
> Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
> Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>
> ---
>  drivers/bus/mhi/core/main.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> index 9ec9b36..467c0ba 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -369,22 +369,29 @@ irqreturn_t mhi_irq_handler(int irq_number, void *dev)
>  	return IRQ_HANDLED;
>  }
>  
> -irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *dev)
> +irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *priv)
>  {
> -	struct mhi_controller *mhi_cntrl = dev;
> +	struct mhi_controller *mhi_cntrl = priv;
> +	struct device *dev = &mhi_cntrl->mhi_dev->dev;
>  	enum mhi_state state = MHI_STATE_MAX;
>  	enum mhi_pm_state pm_state = 0;
>  	enum mhi_ee_type ee = 0;
>  
>  	write_lock_irq(&mhi_cntrl->pm_lock);
> -	if (MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
> -		state = mhi_get_mhi_state(mhi_cntrl);
> -		ee = mhi_cntrl->ee;
> -		mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);
> +	if (!MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
> +		write_unlock_irq(&mhi_cntrl->pm_lock);

write_lock is only used for protecting 'mhi_cntrl->ee' but here we are not
updating it if reg access is not valid. So there is no reason to hold this lock.

> +		goto exit_intvec;
>  	}
>  
> +	state = mhi_get_mhi_state(mhi_cntrl);
> +	ee = mhi_cntrl->ee;
> +	mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);

But it is needed here.

Thanks,
Mani

> +	dev_dbg(dev, "local ee:%s device ee:%s dev_state:%s\n",
> +		TO_MHI_EXEC_STR(mhi_cntrl->ee), TO_MHI_EXEC_STR(ee),
> +		TO_MHI_STATE_STR(state));
> +
>  	if (state == MHI_STATE_SYS_ERR) {
> -		dev_dbg(&mhi_cntrl->mhi_dev->dev, "System error detected\n");
> +		dev_dbg(dev, "System error detected\n");
>  		pm_state = mhi_tryset_pm_state(mhi_cntrl,
>  					       MHI_PM_SYS_ERR_DETECT);
>  	}
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project

Re: [PATCH v1 4/5] bus: mhi: core: Do not process SYS_ERROR if RDDM is supported

[Manivannan Sadhasivam]

On Mon, May 11, 2020 at 07:03:08PM -0700, Hemant Kumar wrote:
> Devices that support RDDM do not require processing SYS_ERROR as it is
> deemed redundant. Avoid SYS_ERROR processing if RDDM is supported by
> the device.
> 
> Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
> Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> ---
>  drivers/bus/mhi/core/main.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> index 467c0ba..1f622ce 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -397,9 +397,9 @@ irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *priv)
>  	}
>  	write_unlock_irq(&mhi_cntrl->pm_lock);
>  
> -	/* If device in RDDM don't bother processing SYS error */
> -	if (mhi_cntrl->ee == MHI_EE_RDDM) {
> -		if (mhi_cntrl->ee != ee) {
> +	 /* If device supports RDDM don't bother processing SYS error */
> +	if (mhi_cntrl->rddm_image) {
> +		if (mhi_cntrl->ee == MHI_EE_RDDM && mhi_cntrl->ee != ee) {
>  			mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_EE_RDDM);
>  			wake_up_all(&mhi_cntrl->state_event);
>  		}
> @@ -735,6 +735,11 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl,
>  			{
>  				enum mhi_pm_state new_state;
>  
> +				/* skip SYS_ERROR handling if RDDM supported */
> +				if (mhi_cntrl->ee == MHI_EE_RDDM ||
> +				    mhi_cntrl->rddm_image)
> +					break;
> +
>  				dev_dbg(dev, "System error detected\n");
>  				write_lock_irq(&mhi_cntrl->pm_lock);
>  				new_state = mhi_tryset_pm_state(mhi_cntrl,
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project

Re: [PATCH v1 5/5] bus: mhi: core: Handle write lock properly in mhi_pm_m0_transition

[Manivannan Sadhasivam]

On Mon, May 11, 2020 at 07:03:09PM -0700, Hemant Kumar wrote:
> Take write lock only to protect db_mode member of mhi channel.
> This allows rest of the mhi channels to just take read lock which
> fine grains the locking. It prevents channel readers to starve if
> they try to enter critical section after a writer.
> 
> Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> ---
>  drivers/bus/mhi/core/pm.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> index 345f197..de5abb2 100644
> --- a/drivers/bus/mhi/core/pm.c
> +++ b/drivers/bus/mhi/core/pm.c
> @@ -288,14 +288,18 @@ int mhi_pm_m0_transition(struct mhi_controller *mhi_cntrl)
>  	for (i = 0; i < mhi_cntrl->max_chan; i++, mhi_chan++) {
>  		struct mhi_ring *tre_ring = &mhi_chan->tre_ring;
>  
> -		write_lock_irq(&mhi_chan->lock);
> -		if (mhi_chan->db_cfg.reset_req)
> +		if (mhi_chan->db_cfg.reset_req) {
> +			write_lock_irq(&mhi_chan->lock);
>  			mhi_chan->db_cfg.db_mode = true;
> +			write_unlock_irq(&mhi_chan->lock);
> +		}
> +
> +		read_lock_irq(&mhi_chan->lock);
>  
>  		/* Only ring DB if ring is not empty */
>  		if (tre_ring->base && tre_ring->wp  != tre_ring->rp)
>  			mhi_ring_chan_db(mhi_cntrl, mhi_chan);
> -		write_unlock_irq(&mhi_chan->lock);
> +		read_unlock_irq(&mhi_chan->lock);
>  	}
>  
>  	mhi_cntrl->wake_put(mhi_cntrl, false);
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project

Re: [PATCH v1 0/5] MHI Misc Fixes

[Manivannan Sadhasivam]

Hi Hemant,

On Mon, May 11, 2020 at 07:03:04PM -0700, Hemant Kumar wrote:
> Patches are addressing MHI core driver bug fixes. Patches tested on
> arm64 and x86 platforms.
> 

This series looks good except one comment I left. Once that gets fixed, I'll
club the v2 of this series with already applied series from Bhaumik and send
to Greg as a feature one for 5.8 (provided I get the updated series within
this week).

Thanks,
Mani

> Hemant Kumar (5):
>   bus: mhi: core: Remove the system error worker thread
>   bus: mhi: core: Handle disable transitions in state worker
>   bus: mhi: core: Skip handling BHI irq if MHI reg access is not allowed
>   bus: mhi: core: Do not process SYS_ERROR if RDDM is supported
>   bus: mhi: core: Handle write lock properly in mhi_pm_m0_transition
> 
>  drivers/bus/mhi/core/init.c     |  3 ++-
>  drivers/bus/mhi/core/internal.h |  4 +++-
>  drivers/bus/mhi/core/main.c     | 38 +++++++++++++++++++----------
>  drivers/bus/mhi/core/pm.c       | 53 ++++++++++++++++++++++++-----------------
>  include/linux/mhi.h             |  2 --
>  5 files changed, 61 insertions(+), 39 deletions(-)
> 
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project

Re: [PATCH v1 3/5] bus: mhi: core: Skip handling BHI irq if MHI reg access is not allowed

[Hemant Kumar]

Hi Mani,

On 5/11/20 11:53 PM, Manivannan Sadhasivam wrote:
> On Mon, May 11, 2020 at 07:03:07PM -0700, Hemant Kumar wrote:
>> Driver continues handling of BHI interrupt even if MHI register access
>> is not allowed. By doing so it calls the status call back and performs
>> early notification for the MHI client. This is not needed when MHI
>> register access is not allowed. Hence skip the handling in this case and
>> return. Also add debug log to print device state, local EE and device EE
>> when reg access is valid.
>>
>> Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
>> Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>
>> ---
>>   drivers/bus/mhi/core/main.c | 21 ++++++++++++++-------
>>   1 file changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
>> index 9ec9b36..467c0ba 100644
>> --- a/drivers/bus/mhi/core/main.c
>> +++ b/drivers/bus/mhi/core/main.c
>> @@ -369,22 +369,29 @@ irqreturn_t mhi_irq_handler(int irq_number, void *dev)
>>   	return IRQ_HANDLED;
>>   }
>>   
>> -irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *dev)
>> +irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *priv)
>>   {
>> -	struct mhi_controller *mhi_cntrl = dev;
>> +	struct mhi_controller *mhi_cntrl = priv;
>> +	struct device *dev = &mhi_cntrl->mhi_dev->dev;
>>   	enum mhi_state state = MHI_STATE_MAX;
>>   	enum mhi_pm_state pm_state = 0;
>>   	enum mhi_ee_type ee = 0;
>>   
>>   	write_lock_irq(&mhi_cntrl->pm_lock);
>> -	if (MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
>> -		state = mhi_get_mhi_state(mhi_cntrl);
>> -		ee = mhi_cntrl->ee;
>> -		mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);
>> +	if (!MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
>> +		write_unlock_irq(&mhi_cntrl->pm_lock);
> 
> write_lock is only used for protecting 'mhi_cntrl->ee' but here we are not
> updating it if reg access is not valid. So there is no reason to hold this lock.
Original code is using write_lock to protect pm_state as well as 
mhi_cntrl->ee. This patch is keeping the lock same as original code. 
Just if condition logic is negated here due to that write_unlock_irq is 
added under if condition.
> 
>> +		goto exit_intvec;
>>   	}
>>   
>> +	state = mhi_get_mhi_state(mhi_cntrl);
>> +	ee = mhi_cntrl->ee;
>> +	mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);
> 
> But it is needed here.
> 
> Thanks,
> Mani
> 
>> +	dev_dbg(dev, "local ee:%s device ee:%s dev_state:%s\n",
>> +		TO_MHI_EXEC_STR(mhi_cntrl->ee), TO_MHI_EXEC_STR(ee),
>> +		TO_MHI_STATE_STR(state));
>> +
>>   	if (state == MHI_STATE_SYS_ERR) {
>> -		dev_dbg(&mhi_cntrl->mhi_dev->dev, "System error detected\n");
>> +		dev_dbg(dev, "System error detected\n");
>>   		pm_state = mhi_tryset_pm_state(mhi_cntrl,
>>   					       MHI_PM_SYS_ERR_DETECT);
>>   	}
>> -- 
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v1 3/5] bus: mhi: core: Skip handling BHI irq if MHI reg access is not allowed

[Manivannan Sadhasivam]

On Tue, May 12, 2020 at 05:28:45PM -0700, Hemant Kumar wrote:
> Hi Mani,
> 
> On 5/11/20 11:53 PM, Manivannan Sadhasivam wrote:
> > On Mon, May 11, 2020 at 07:03:07PM -0700, Hemant Kumar wrote:
> > > Driver continues handling of BHI interrupt even if MHI register access
> > > is not allowed. By doing so it calls the status call back and performs
> > > early notification for the MHI client. This is not needed when MHI
> > > register access is not allowed. Hence skip the handling in this case and
> > > return. Also add debug log to print device state, local EE and device EE
> > > when reg access is valid.
> > > 
> > > Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
> > > Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>
> > > ---
> > >   drivers/bus/mhi/core/main.c | 21 ++++++++++++++-------
> > >   1 file changed, 14 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> > > index 9ec9b36..467c0ba 100644
> > > --- a/drivers/bus/mhi/core/main.c
> > > +++ b/drivers/bus/mhi/core/main.c
> > > @@ -369,22 +369,29 @@ irqreturn_t mhi_irq_handler(int irq_number, void *dev)
> > >   	return IRQ_HANDLED;
> > >   }
> > > -irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *dev)
> > > +irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *priv)
> > >   {
> > > -	struct mhi_controller *mhi_cntrl = dev;
> > > +	struct mhi_controller *mhi_cntrl = priv;
> > > +	struct device *dev = &mhi_cntrl->mhi_dev->dev;
> > >   	enum mhi_state state = MHI_STATE_MAX;
> > >   	enum mhi_pm_state pm_state = 0;
> > >   	enum mhi_ee_type ee = 0;
> > >   	write_lock_irq(&mhi_cntrl->pm_lock);
> > > -	if (MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
> > > -		state = mhi_get_mhi_state(mhi_cntrl);
> > > -		ee = mhi_cntrl->ee;
> > > -		mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);
> > > +	if (!MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
> > > +		write_unlock_irq(&mhi_cntrl->pm_lock);
> > 
> > write_lock is only used for protecting 'mhi_cntrl->ee' but here we are not
> > updating it if reg access is not valid. So there is no reason to hold this lock.
> Original code is using write_lock to protect pm_state as well as
> mhi_cntrl->ee. This patch is keeping the lock same as original code. Just if
> condition logic is negated here due to that write_unlock_irq is added under
> if condition.

'mhi_cntrl->pm_state' is not always protected by 'pm_lock' and that too
write_lock is used here but 'pm_state' is not modified. So as like in most of
the places, locks are abused here as well.

I think after 5.8, you should really think about fixing the usage of locks
throughout the MHI stack.

So I'll take this patch as it is.

Thanks,
Mani

> > 
> > > +		goto exit_intvec;
> > >   	}
> > > +	state = mhi_get_mhi_state(mhi_cntrl);
> > > +	ee = mhi_cntrl->ee;
> > > +	mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);
> > 
> > But it is needed here.
> > 
> > Thanks,
> > Mani
> > 
> > > +	dev_dbg(dev, "local ee:%s device ee:%s dev_state:%s\n",
> > > +		TO_MHI_EXEC_STR(mhi_cntrl->ee), TO_MHI_EXEC_STR(ee),
> > > +		TO_MHI_STATE_STR(state));
> > > +
> > >   	if (state == MHI_STATE_SYS_ERR) {
> > > -		dev_dbg(&mhi_cntrl->mhi_dev->dev, "System error detected\n");
> > > +		dev_dbg(dev, "System error detected\n");
> > >   		pm_state = mhi_tryset_pm_state(mhi_cntrl,
> > >   					       MHI_PM_SYS_ERR_DETECT);
> > >   	}
> > > -- 
> > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> > > a Linux Foundation Collaborative Project
> 
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project

Re: [PATCH v1 3/5] bus: mhi: core: Skip handling BHI irq if MHI reg access is not allowed

[Manivannan Sadhasivam]

On Mon, May 11, 2020 at 07:03:07PM -0700, Hemant Kumar wrote:
> Driver continues handling of BHI interrupt even if MHI register access
> is not allowed. By doing so it calls the status call back and performs
> early notification for the MHI client. This is not needed when MHI
> register access is not allowed. Hence skip the handling in this case and
> return. Also add debug log to print device state, local EE and device EE
> when reg access is valid.
> 
> Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
> Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> ---
>  drivers/bus/mhi/core/main.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> index 9ec9b36..467c0ba 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -369,22 +369,29 @@ irqreturn_t mhi_irq_handler(int irq_number, void *dev)
>  	return IRQ_HANDLED;
>  }
>  
> -irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *dev)
> +irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *priv)
>  {
> -	struct mhi_controller *mhi_cntrl = dev;
> +	struct mhi_controller *mhi_cntrl = priv;
> +	struct device *dev = &mhi_cntrl->mhi_dev->dev;
>  	enum mhi_state state = MHI_STATE_MAX;
>  	enum mhi_pm_state pm_state = 0;
>  	enum mhi_ee_type ee = 0;
>  
>  	write_lock_irq(&mhi_cntrl->pm_lock);
> -	if (MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
> -		state = mhi_get_mhi_state(mhi_cntrl);
> -		ee = mhi_cntrl->ee;
> -		mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);
> +	if (!MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
> +		write_unlock_irq(&mhi_cntrl->pm_lock);
> +		goto exit_intvec;
>  	}
>  
> +	state = mhi_get_mhi_state(mhi_cntrl);
> +	ee = mhi_cntrl->ee;
> +	mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);
> +	dev_dbg(dev, "local ee:%s device ee:%s dev_state:%s\n",
> +		TO_MHI_EXEC_STR(mhi_cntrl->ee), TO_MHI_EXEC_STR(ee),
> +		TO_MHI_STATE_STR(state));
> +
>  	if (state == MHI_STATE_SYS_ERR) {
> -		dev_dbg(&mhi_cntrl->mhi_dev->dev, "System error detected\n");
> +		dev_dbg(dev, "System error detected\n");
>  		pm_state = mhi_tryset_pm_state(mhi_cntrl,
>  					       MHI_PM_SYS_ERR_DETECT);
>  	}
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project

Re: [PATCH v1 0/5] MHI Misc Fixes

[Manivannan Sadhasivam]

On Mon, May 11, 2020 at 07:03:04PM -0700, Hemant Kumar wrote:
> Patches are addressing MHI core driver bug fixes. Patches tested on
> arm64 and x86 platforms.
> 

Applied to mhi-next!

Thanks,
Mani

> Hemant Kumar (5):
>   bus: mhi: core: Remove the system error worker thread
>   bus: mhi: core: Handle disable transitions in state worker
>   bus: mhi: core: Skip handling BHI irq if MHI reg access is not allowed
>   bus: mhi: core: Do not process SYS_ERROR if RDDM is supported
>   bus: mhi: core: Handle write lock properly in mhi_pm_m0_transition
> 
>  drivers/bus/mhi/core/init.c     |  3 ++-
>  drivers/bus/mhi/core/internal.h |  4 +++-
>  drivers/bus/mhi/core/main.c     | 38 +++++++++++++++++++----------
>  drivers/bus/mhi/core/pm.c       | 53 ++++++++++++++++++++++++-----------------
>  include/linux/mhi.h             |  2 --
>  5 files changed, 61 insertions(+), 39 deletions(-)
> 
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project

Re: [PATCH v1 3/5] bus: mhi: core: Skip handling BHI irq if MHI reg access is not allowed

[Hemant Kumar]

Hi Mani,

On 5/13/20 12:04 AM, Manivannan Sadhasivam wrote:
> On Tue, May 12, 2020 at 05:28:45PM -0700, Hemant Kumar wrote:
>> Hi Mani,
>>
>> On 5/11/20 11:53 PM, Manivannan Sadhasivam wrote:
>>> On Mon, May 11, 2020 at 07:03:07PM -0700, Hemant Kumar wrote:
>>>> Driver continues handling of BHI interrupt even if MHI register access
>>>> is not allowed. By doing so it calls the status call back and performs
>>>> early notification for the MHI client. This is not needed when MHI
>>>> register access is not allowed. Hence skip the handling in this case and
>>>> return. Also add debug log to print device state, local EE and device EE
>>>> when reg access is valid.
>>>>
>>>> Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
>>>> Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>
>>>> ---
>>>>    drivers/bus/mhi/core/main.c | 21 ++++++++++++++-------
>>>>    1 file changed, 14 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
>>>> index 9ec9b36..467c0ba 100644
>>>> --- a/drivers/bus/mhi/core/main.c
>>>> +++ b/drivers/bus/mhi/core/main.c
>>>> @@ -369,22 +369,29 @@ irqreturn_t mhi_irq_handler(int irq_number, void *dev)
>>>>    	return IRQ_HANDLED;
>>>>    }
>>>> -irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *dev)
>>>> +irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *priv)
>>>>    {
>>>> -	struct mhi_controller *mhi_cntrl = dev;
>>>> +	struct mhi_controller *mhi_cntrl = priv;
>>>> +	struct device *dev = &mhi_cntrl->mhi_dev->dev;
>>>>    	enum mhi_state state = MHI_STATE_MAX;
>>>>    	enum mhi_pm_state pm_state = 0;
>>>>    	enum mhi_ee_type ee = 0;
>>>>    	write_lock_irq(&mhi_cntrl->pm_lock);
>>>> -	if (MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
>>>> -		state = mhi_get_mhi_state(mhi_cntrl);
>>>> -		ee = mhi_cntrl->ee;
>>>> -		mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);
>>>> +	if (!MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
>>>> +		write_unlock_irq(&mhi_cntrl->pm_lock);
>>>
>>> write_lock is only used for protecting 'mhi_cntrl->ee' but here we are not
>>> updating it if reg access is not valid. So there is no reason to hold this lock.
>> Original code is using write_lock to protect pm_state as well as
>> mhi_cntrl->ee. This patch is keeping the lock same as original code. Just if
>> condition logic is negated here due to that write_unlock_irq is added under
>> if condition.
> 
> 'mhi_cntrl->pm_state' is not always protected by 'pm_lock' and that too
> write_lock is used here but 'pm_state' is not modified. So as like in most of
> the places, locks are abused here as well.
> 
> I think after 5.8, you should really think about fixing the usage of locks
> throughout the MHI stack.
> 
> So I'll take this patch as it is.
> 
Thanks for accepting this patch. Will revisit and review usage of locks 
in MHI stack and identify areas of improvement.
> Thanks,
> Mani
> 
>>>
>>>> +		goto exit_intvec;
>>>>    	}
>>>> +	state = mhi_get_mhi_state(mhi_cntrl);
>>>> +	ee = mhi_cntrl->ee;
>>>> +	mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);
>>>
>>> But it is needed here.
>>>
>>> Thanks,
>>> Mani
>>>
>>>> +	dev_dbg(dev, "local ee:%s device ee:%s dev_state:%s\n",
>>>> +		TO_MHI_EXEC_STR(mhi_cntrl->ee), TO_MHI_EXEC_STR(ee),
>>>> +		TO_MHI_STATE_STR(state));
>>>> +
>>>>    	if (state == MHI_STATE_SYS_ERR) {
>>>> -		dev_dbg(&mhi_cntrl->mhi_dev->dev, "System error detected\n");
>>>> +		dev_dbg(dev, "System error detected\n");
>>>>    		pm_state = mhi_tryset_pm_state(mhi_cntrl,
>>>>    					       MHI_PM_SYS_ERR_DETECT);
>>>>    	}
>>>> -- 
>>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>>>> a Linux Foundation Collaborative Project
>>
>> -- 
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project