[PATCH v1 00/10] Bug fixes and improvements for MHI power operations

[PATCH v1 00/10] Bug fixes and improvements for MHI power operations

[Bhaumik Bhatt]

Bug fixes and improvements for MHI powerup and shutdown handling.
Firmware load function names are updated to accurately reflect their purpose.
Closed certain design gaps where the host (MHI bus) would allow clients to
operate after a power down or error detection.
Move to an error state sooner based on different scenarios.

These patches were tested on arm64 and X86_64 architectures.

Bhaumik Bhatt (10):
  bus: mhi: core: Use appropriate names for firmware load functions
  bus: mhi: core: Move to using high priority workqueue
  bus: mhi: core: Skip device wake in error or shutdown states
  bus: mhi: core: Use the IRQF_ONESHOT flag for the BHI interrupt line
  bus: mhi: core: Disable IRQs when powering down
  bus: mhi: core: Improve shutdown handling after link down detection
  bus: mhi: core: Move to SYS_ERROR regardless of RDDM capability
  bus: mhi: core: Move to an error state on any firmware load failure
  bus: mhi: core: Move to an error state on mission mode failure
  bus: mhi: core: Mark device inactive soon after host issues a shutdown

 drivers/bus/mhi/core/boot.c     | 59 ++++++++++++++++++++++-----------------
 drivers/bus/mhi/core/init.c     | 10 ++++++-
 drivers/bus/mhi/core/internal.h |  1 +
 drivers/bus/mhi/core/main.c     | 28 +++++++++++++------
 drivers/bus/mhi/core/pm.c       | 62 ++++++++++++++++++++++++++++-------------
 include/linux/mhi.h             |  2 ++
 6 files changed, 108 insertions(+), 54 deletions(-)

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

[PATCH v1 01/10] bus: mhi: core: Use appropriate names for firmware load functions

[Bhaumik Bhatt]

mhi_fw_load_sbl() function is currently used to transfer SBL or EDL
images over BHI (Boot Host Interface). Same goes with mhi_fw_load_amss()
which uses BHIe. However, the contents of these functions do not
indicate support for a specific set of images. Since these can be used
for any image download over BHI or BHIe, rename them based on the
protocol used.

Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
---
 drivers/bus/mhi/core/boot.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/bus/mhi/core/boot.c b/drivers/bus/mhi/core/boot.c
index 24422f5..92b8dd3 100644
--- a/drivers/bus/mhi/core/boot.c
+++ b/drivers/bus/mhi/core/boot.c
@@ -171,7 +171,7 @@ int mhi_download_rddm_img(struct mhi_controller *mhi_cntrl, bool in_panic)
 }
 EXPORT_SYMBOL_GPL(mhi_download_rddm_img);
 
-static int mhi_fw_load_amss(struct mhi_controller *mhi_cntrl,
+static int mhi_fw_load_bhie(struct mhi_controller *mhi_cntrl,
 			    const struct mhi_buf *mhi_buf)
 {
 	void __iomem *base = mhi_cntrl->bhie;
@@ -187,7 +187,7 @@ static int mhi_fw_load_amss(struct mhi_controller *mhi_cntrl,
 	}
 
 	sequence_id = MHI_RANDOM_U32_NONZERO(BHIE_TXVECSTATUS_SEQNUM_BMSK);
-	dev_dbg(dev, "Starting AMSS download via BHIe. Sequence ID:%u\n",
+	dev_dbg(dev, "Starting image download via BHIe. Sequence ID:%u\n",
 		sequence_id);
 	mhi_write_reg(mhi_cntrl, base, BHIE_TXVECADDR_HIGH_OFFS,
 		      upper_32_bits(mhi_buf->dma_addr));
@@ -218,7 +218,7 @@ static int mhi_fw_load_amss(struct mhi_controller *mhi_cntrl,
 	return (!ret) ? -ETIMEDOUT : 0;
 }
 
-static int mhi_fw_load_sbl(struct mhi_controller *mhi_cntrl,
+static int mhi_fw_load_bhi(struct mhi_controller *mhi_cntrl,
 			   dma_addr_t dma_addr,
 			   size_t size)
 {
@@ -245,7 +245,7 @@ static int mhi_fw_load_sbl(struct mhi_controller *mhi_cntrl,
 	}
 
 	session_id = MHI_RANDOM_U32_NONZERO(BHI_TXDB_SEQNUM_BMSK);
-	dev_dbg(dev, "Starting SBL download via BHI. Session ID:%u\n",
+	dev_dbg(dev, "Starting image download via BHI. Session ID:%u\n",
 		session_id);
 	mhi_write_reg(mhi_cntrl, base, BHI_STATUS, 0);
 	mhi_write_reg(mhi_cntrl, base, BHI_IMGADDR_HIGH,
@@ -446,9 +446,9 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
 		return;
 	}
 
-	/* Download SBL image */
+	/* Download SBL or EDL image using BHI */
 	memcpy(buf, firmware->data, size);
-	ret = mhi_fw_load_sbl(mhi_cntrl, dma_addr, size);
+	ret = mhi_fw_load_bhi(mhi_cntrl, dma_addr, size);
 	mhi_free_coherent(mhi_cntrl, size, buf, dma_addr);
 
 	if (!mhi_cntrl->fbc_download || ret || mhi_cntrl->ee == MHI_EE_EDL)
@@ -456,7 +456,7 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
 
 	/* Error or in EDL mode, we're done */
 	if (ret) {
-		dev_err(dev, "MHI did not load SBL, ret:%d\n", ret);
+		dev_err(dev, "MHI did not load SBL/EDL image, ret:%d\n", ret);
 		return;
 	}
 
@@ -506,7 +506,7 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
 
 	/* Start full firmware image download */
 	image_info = mhi_cntrl->fbc_image;
-	ret = mhi_fw_load_amss(mhi_cntrl,
+	ret = mhi_fw_load_bhie(mhi_cntrl,
 			       /* Vector table is the last entry */
 			       &image_info->mhi_buf[image_info->entries - 1]);
 	if (ret)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v1 01/10] bus: mhi: core: Use appropriate names for firmware load functions

[Manivannan Sadhasivam]

On Fri, Sep 18, 2020 at 07:02:26PM -0700, Bhaumik Bhatt wrote:
> mhi_fw_load_sbl() function is currently used to transfer SBL or EDL
> images over BHI (Boot Host Interface). Same goes with mhi_fw_load_amss()
> which uses BHIe. However, the contents of these functions do not
> indicate support for a specific set of images. Since these can be used
> for any image download over BHI or BHIe, rename them based on the
> protocol used.
> 
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> ---
>  drivers/bus/mhi/core/boot.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/bus/mhi/core/boot.c b/drivers/bus/mhi/core/boot.c
> index 24422f5..92b8dd3 100644
> --- a/drivers/bus/mhi/core/boot.c
> +++ b/drivers/bus/mhi/core/boot.c
> @@ -171,7 +171,7 @@ int mhi_download_rddm_img(struct mhi_controller *mhi_cntrl, bool in_panic)
>  }
>  EXPORT_SYMBOL_GPL(mhi_download_rddm_img);
>  
> -static int mhi_fw_load_amss(struct mhi_controller *mhi_cntrl,
> +static int mhi_fw_load_bhie(struct mhi_controller *mhi_cntrl,
>  			    const struct mhi_buf *mhi_buf)
>  {
>  	void __iomem *base = mhi_cntrl->bhie;
> @@ -187,7 +187,7 @@ static int mhi_fw_load_amss(struct mhi_controller *mhi_cntrl,
>  	}
>  
>  	sequence_id = MHI_RANDOM_U32_NONZERO(BHIE_TXVECSTATUS_SEQNUM_BMSK);
> -	dev_dbg(dev, "Starting AMSS download via BHIe. Sequence ID:%u\n",
> +	dev_dbg(dev, "Starting image download via BHIe. Sequence ID:%u\n",
>  		sequence_id);
>  	mhi_write_reg(mhi_cntrl, base, BHIE_TXVECADDR_HIGH_OFFS,
>  		      upper_32_bits(mhi_buf->dma_addr));
> @@ -218,7 +218,7 @@ static int mhi_fw_load_amss(struct mhi_controller *mhi_cntrl,
>  	return (!ret) ? -ETIMEDOUT : 0;
>  }
>  
> -static int mhi_fw_load_sbl(struct mhi_controller *mhi_cntrl,
> +static int mhi_fw_load_bhi(struct mhi_controller *mhi_cntrl,
>  			   dma_addr_t dma_addr,
>  			   size_t size)
>  {
> @@ -245,7 +245,7 @@ static int mhi_fw_load_sbl(struct mhi_controller *mhi_cntrl,
>  	}
>  
>  	session_id = MHI_RANDOM_U32_NONZERO(BHI_TXDB_SEQNUM_BMSK);
> -	dev_dbg(dev, "Starting SBL download via BHI. Session ID:%u\n",
> +	dev_dbg(dev, "Starting image download via BHI. Session ID:%u\n",
>  		session_id);
>  	mhi_write_reg(mhi_cntrl, base, BHI_STATUS, 0);
>  	mhi_write_reg(mhi_cntrl, base, BHI_IMGADDR_HIGH,
> @@ -446,9 +446,9 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
>  		return;
>  	}
>  
> -	/* Download SBL image */
> +	/* Download SBL or EDL image using BHI */

You are mentioning "image" in the debug print but "SBL/EDL" here and below.
Please use "image" for consistency.

Thanks,
Mani

>  	memcpy(buf, firmware->data, size);
> -	ret = mhi_fw_load_sbl(mhi_cntrl, dma_addr, size);
> +	ret = mhi_fw_load_bhi(mhi_cntrl, dma_addr, size);
>  	mhi_free_coherent(mhi_cntrl, size, buf, dma_addr);
>  
>  	if (!mhi_cntrl->fbc_download || ret || mhi_cntrl->ee == MHI_EE_EDL)
> @@ -456,7 +456,7 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
>  
>  	/* Error or in EDL mode, we're done */
>  	if (ret) {
> -		dev_err(dev, "MHI did not load SBL, ret:%d\n", ret);
> +		dev_err(dev, "MHI did not load SBL/EDL image, ret:%d\n", ret);
>  		return;
>  	}
>  
> @@ -506,7 +506,7 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
>  
>  	/* Start full firmware image download */
>  	image_info = mhi_cntrl->fbc_image;
> -	ret = mhi_fw_load_amss(mhi_cntrl,
> +	ret = mhi_fw_load_bhie(mhi_cntrl,
>  			       /* Vector table is the last entry */
>  			       &image_info->mhi_buf[image_info->entries - 1]);
>  	if (ret)
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

[PATCH v1 02/10] bus: mhi: core: Move to using high priority workqueue

[Bhaumik Bhatt]

MHI work is currently scheduled on the global/system workqueue and can
encounter delays on a stressed system. To avoid those unforeseen
delays which can hamper bootup or shutdown times, use a dedicated high
priority workqueue instead of the global/system workqueue.

Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
---
 drivers/bus/mhi/core/init.c | 7 +++++++
 drivers/bus/mhi/core/pm.c   | 2 +-
 include/linux/mhi.h         | 2 ++
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index 1b4161e..ca32563 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -890,6 +890,11 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
 	INIT_WORK(&mhi_cntrl->st_worker, mhi_pm_st_worker);
 	init_waitqueue_head(&mhi_cntrl->state_event);
 
+	mhi_cntrl->hiprio_wq = alloc_ordered_workqueue
+				("mhi_hiprio_wq", WQ_MEM_RECLAIM | WQ_HIGHPRI);
+	if (!mhi_cntrl->hiprio_wq)
+		goto error_alloc_cmd;
+
 	mhi_cmd = mhi_cntrl->mhi_cmd;
 	for (i = 0; i < NR_OF_CMD_RINGS; i++, mhi_cmd++)
 		spin_lock_init(&mhi_cmd->lock);
@@ -977,10 +982,12 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
 
 error_alloc_dev:
 	kfree(mhi_cntrl->mhi_cmd);
+	destroy_workqueue(mhi_cntrl->hiprio_wq);
 
 error_alloc_cmd:
 	vfree(mhi_cntrl->mhi_chan);
 	kfree(mhi_cntrl->mhi_event);
+	destroy_workqueue(mhi_cntrl->hiprio_wq);
 
 	return ret;
 }
diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
index ce4d969..9d4789d 100644
--- a/drivers/bus/mhi/core/pm.c
+++ b/drivers/bus/mhi/core/pm.c
@@ -597,7 +597,7 @@ int mhi_queue_state_transition(struct mhi_controller *mhi_cntrl,
 	list_add_tail(&item->node, &mhi_cntrl->transition_list);
 	spin_unlock_irqrestore(&mhi_cntrl->transition_lock, flags);
 
-	schedule_work(&mhi_cntrl->st_worker);
+	queue_work(mhi_cntrl->hiprio_wq, &mhi_cntrl->st_worker);
 
 	return 0;
 }
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index fb45a0f..7677676 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -338,6 +338,7 @@ struct mhi_controller_config {
  * @wlock: Lock for protecting device wakeup
  * @mhi_link_info: Device bandwidth info
  * @st_worker: State transition worker
+ * @hiprio_wq: High priority workqueue
  * @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)
@@ -421,6 +422,7 @@ struct mhi_controller {
 	spinlock_t wlock;
 	struct mhi_link_info mhi_link_info;
 	struct work_struct st_worker;
+	struct workqueue_struct *hiprio_wq;
 	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 02/10] bus: mhi: core: Move to using high priority workqueue

[Manivannan Sadhasivam]

On Fri, Sep 18, 2020 at 07:02:27PM -0700, Bhaumik Bhatt wrote:
> MHI work is currently scheduled on the global/system workqueue and can
> encounter delays on a stressed system. To avoid those unforeseen
> delays which can hamper bootup or shutdown times, use a dedicated high
> priority workqueue instead of the global/system workqueue.
> 
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> ---
>  drivers/bus/mhi/core/init.c | 7 +++++++
>  drivers/bus/mhi/core/pm.c   | 2 +-
>  include/linux/mhi.h         | 2 ++
>  3 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index 1b4161e..ca32563 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -890,6 +890,11 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
>  	INIT_WORK(&mhi_cntrl->st_worker, mhi_pm_st_worker);
>  	init_waitqueue_head(&mhi_cntrl->state_event);
>  
> +	mhi_cntrl->hiprio_wq = alloc_ordered_workqueue
> +				("mhi_hiprio_wq", WQ_MEM_RECLAIM | WQ_HIGHPRI);
> +	if (!mhi_cntrl->hiprio_wq)

Printing an error here would be helpful.

> +		goto error_alloc_cmd;
> +
>  	mhi_cmd = mhi_cntrl->mhi_cmd;
>  	for (i = 0; i < NR_OF_CMD_RINGS; i++, mhi_cmd++)
>  		spin_lock_init(&mhi_cmd->lock);
> @@ -977,10 +982,12 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
>  
>  error_alloc_dev:
>  	kfree(mhi_cntrl->mhi_cmd);
> +	destroy_workqueue(mhi_cntrl->hiprio_wq);

So you're destroying the queue two times? You don't need it here.

>  
>  error_alloc_cmd:
>  	vfree(mhi_cntrl->mhi_chan);
>  	kfree(mhi_cntrl->mhi_event);
> +	destroy_workqueue(mhi_cntrl->hiprio_wq);
>  
>  	return ret;
>  }
> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> index ce4d969..9d4789d 100644
> --- a/drivers/bus/mhi/core/pm.c
> +++ b/drivers/bus/mhi/core/pm.c
> @@ -597,7 +597,7 @@ int mhi_queue_state_transition(struct mhi_controller *mhi_cntrl,
>  	list_add_tail(&item->node, &mhi_cntrl->transition_list);
>  	spin_unlock_irqrestore(&mhi_cntrl->transition_lock, flags);
>  
> -	schedule_work(&mhi_cntrl->st_worker);
> +	queue_work(mhi_cntrl->hiprio_wq, &mhi_cntrl->st_worker);
>  
>  	return 0;
>  }
> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> index fb45a0f..7677676 100644
> --- a/include/linux/mhi.h
> +++ b/include/linux/mhi.h
> @@ -338,6 +338,7 @@ struct mhi_controller_config {
>   * @wlock: Lock for protecting device wakeup
>   * @mhi_link_info: Device bandwidth info
>   * @st_worker: State transition worker
> + * @hiprio_wq: High priority workqueue

For what? Please state the purpose.

Thanks,
Mani

>   * @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)
> @@ -421,6 +422,7 @@ struct mhi_controller {
>  	spinlock_t wlock;
>  	struct mhi_link_info mhi_link_info;
>  	struct work_struct st_worker;
> +	struct workqueue_struct *hiprio_wq;
>  	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 03/10] bus: mhi: core: Skip device wake in error or shutdown states

[Bhaumik Bhatt]

MHI clients can request a device wake even if the device may be in an
error state or undergoing shutdown. To prevent unnecessary device wake
processing, check for the device state and bail out early so that the
clients are made aware of the device state sooner.

Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
---
 drivers/bus/mhi/core/pm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
index 9d4789d..1862960 100644
--- a/drivers/bus/mhi/core/pm.c
+++ b/drivers/bus/mhi/core/pm.c
@@ -827,6 +827,10 @@ int __mhi_device_get_sync(struct mhi_controller *mhi_cntrl)
 
 	/* Wake up the device */
 	read_lock_bh(&mhi_cntrl->pm_lock);
+	if (MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state)) {
+		read_unlock_bh(&mhi_cntrl->pm_lock);
+		return -EIO;
+	}
 	mhi_cntrl->wake_get(mhi_cntrl, true);
 	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state))
 		mhi_trigger_resume(mhi_cntrl);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v1 03/10] bus: mhi: core: Skip device wake in error or shutdown states

[Manivannan Sadhasivam]

On Fri, Sep 18, 2020 at 07:02:28PM -0700, Bhaumik Bhatt wrote:
> MHI clients can request a device wake even if the device may be in an
> error state or undergoing shutdown. To prevent unnecessary device wake
> processing, check for the device state and bail out early so that the
> clients are made aware of the device state sooner.
> 

Please use the term "client drivers" everywhere. With that,

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

Thanks,
Mani

> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> ---
>  drivers/bus/mhi/core/pm.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> index 9d4789d..1862960 100644
> --- a/drivers/bus/mhi/core/pm.c
> +++ b/drivers/bus/mhi/core/pm.c
> @@ -827,6 +827,10 @@ int __mhi_device_get_sync(struct mhi_controller *mhi_cntrl)
>  
>  	/* Wake up the device */
>  	read_lock_bh(&mhi_cntrl->pm_lock);
> +	if (MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state)) {
> +		read_unlock_bh(&mhi_cntrl->pm_lock);
> +		return -EIO;
> +	}
>  	mhi_cntrl->wake_get(mhi_cntrl, true);
>  	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state))
>  		mhi_trigger_resume(mhi_cntrl);
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

[PATCH v1 04/10] bus: mhi: core: Use the IRQF_ONESHOT flag for the BHI interrupt line

[Bhaumik Bhatt]

Remove the IRQF_SHARED flag as it is not needed for the BHI interrupt
and replace it with IRQF_ONESHOT so that host can be sure that the
next BHI hard interrupt is triggered only when the threaded interrupt
handler has returned. This is to avoid any race conditions we may run
into if BHI interrupts fire one after another.

Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
---
 drivers/bus/mhi/core/init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index ca32563..9ae4c19 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -167,7 +167,7 @@ int mhi_init_irq_setup(struct mhi_controller *mhi_cntrl)
 	/* Setup BHI_INTVEC IRQ */
 	ret = request_threaded_irq(mhi_cntrl->irq[0], mhi_intvec_handler,
 				   mhi_intvec_threaded_handler,
-				   IRQF_SHARED | IRQF_NO_SUSPEND,
+				   IRQF_ONESHOT | IRQF_NO_SUSPEND,
 				   "bhi", mhi_cntrl);
 	if (ret)
 		return ret;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v1 04/10] bus: mhi: core: Use the IRQF_ONESHOT flag for the BHI interrupt line

[Manivannan Sadhasivam]

On Fri, Sep 18, 2020 at 07:02:29PM -0700, Bhaumik Bhatt wrote:
> Remove the IRQF_SHARED flag as it is not needed for the BHI interrupt
> and replace it with IRQF_ONESHOT so that host can be sure that the
> next BHI hard interrupt is triggered only when the threaded interrupt
> handler has returned. This is to avoid any race conditions we may run
> into if BHI interrupts fire one after another.
> 
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> ---
>  drivers/bus/mhi/core/init.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index ca32563..9ae4c19 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -167,7 +167,7 @@ int mhi_init_irq_setup(struct mhi_controller *mhi_cntrl)
>  	/* Setup BHI_INTVEC IRQ */
>  	ret = request_threaded_irq(mhi_cntrl->irq[0], mhi_intvec_handler,
>  				   mhi_intvec_threaded_handler,
> -				   IRQF_SHARED | IRQF_NO_SUSPEND,
> +				   IRQF_ONESHOT | IRQF_NO_SUSPEND,

Jeff, I believe you're the one requested for shared irq during initial push.
Are you okay with this?

Thanks,
Mani

>  				   "bhi", mhi_cntrl);
>  	if (ret)
>  		return ret;
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH v1 04/10] bus: mhi: core: Use the IRQF_ONESHOT flag for the BHI interrupt line

[Jeffrey Hugo]

On 10/9/2020 9:57 AM, Manivannan Sadhasivam wrote:
> On Fri, Sep 18, 2020 at 07:02:29PM -0700, Bhaumik Bhatt wrote:
>> Remove the IRQF_SHARED flag as it is not needed for the BHI interrupt
>> and replace it with IRQF_ONESHOT so that host can be sure that the
>> next BHI hard interrupt is triggered only when the threaded interrupt
>> handler has returned. This is to avoid any race conditions we may run
>> into if BHI interrupts fire one after another.
>>
>> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
>> ---
>>   drivers/bus/mhi/core/init.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
>> index ca32563..9ae4c19 100644
>> --- a/drivers/bus/mhi/core/init.c
>> +++ b/drivers/bus/mhi/core/init.c
>> @@ -167,7 +167,7 @@ int mhi_init_irq_setup(struct mhi_controller *mhi_cntrl)
>>   	/* Setup BHI_INTVEC IRQ */
>>   	ret = request_threaded_irq(mhi_cntrl->irq[0], mhi_intvec_handler,
>>   				   mhi_intvec_threaded_handler,
>> -				   IRQF_SHARED | IRQF_NO_SUSPEND,
>> +				   IRQF_ONESHOT | IRQF_NO_SUSPEND,
> 
> Jeff, I believe you're the one requested for shared irq during initial push.
> Are you okay with this?

Nope  :)

AIC100 has a single interrupt for BHI/MHI activity, so this needs to be 
shared.


-- 
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v1 04/10] bus: mhi: core: Use the IRQF_ONESHOT flag for the BHI interrupt line

[Bhaumik Bhatt]

On 2020-10-09 09:04, Jeffrey Hugo wrote:
> On 10/9/2020 9:57 AM, Manivannan Sadhasivam wrote:
>> On Fri, Sep 18, 2020 at 07:02:29PM -0700, Bhaumik Bhatt wrote:
>>> Remove the IRQF_SHARED flag as it is not needed for the BHI interrupt
>>> and replace it with IRQF_ONESHOT so that host can be sure that the
>>> next BHI hard interrupt is triggered only when the threaded interrupt
>>> handler has returned. This is to avoid any race conditions we may run
>>> into if BHI interrupts fire one after another.
>>> 
>>> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
>>> ---
>>>   drivers/bus/mhi/core/init.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/drivers/bus/mhi/core/init.c 
>>> b/drivers/bus/mhi/core/init.c
>>> index ca32563..9ae4c19 100644
>>> --- a/drivers/bus/mhi/core/init.c
>>> +++ b/drivers/bus/mhi/core/init.c
>>> @@ -167,7 +167,7 @@ int mhi_init_irq_setup(struct mhi_controller 
>>> *mhi_cntrl)
>>>   	/* Setup BHI_INTVEC IRQ */
>>>   	ret = request_threaded_irq(mhi_cntrl->irq[0], mhi_intvec_handler,
>>>   				   mhi_intvec_threaded_handler,
>>> -				   IRQF_SHARED | IRQF_NO_SUSPEND,
>>> +				   IRQF_ONESHOT | IRQF_NO_SUSPEND,
>> 
>> Jeff, I believe you're the one requested for shared irq during initial 
>> push.
>> Are you okay with this?
> 
> Nope  :)
> 
> AIC100 has a single interrupt for BHI/MHI activity, so this needs to be 
> shared.

Did not account for this. Will drop this patch.

Thanks,
Bhaumik

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

[PATCH v1 05/10] bus: mhi: core: Disable IRQs when powering down

[Bhaumik Bhatt]

While powering down, the device may or may not acknowledge the MHI
RESET issued by host for graceful shutdown scenario which can lead
to a rogue device sending an interrupt after the clean-up has been
done. This can result in a tasklet being scheduled after it has
been killed and access already freed memory causing a NULL pointer
exception. Avoid this corner case by disabling the interrupts as a
part of host clean up.

Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
---
 drivers/bus/mhi/core/pm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
index 1862960..3462d82 100644
--- a/drivers/bus/mhi/core/pm.c
+++ b/drivers/bus/mhi/core/pm.c
@@ -517,6 +517,7 @@ static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl,
 	for (i = 0; i < mhi_cntrl->total_ev_rings; i++, mhi_event++) {
 		if (mhi_event->offload_ev)
 			continue;
+		disable_irq(mhi_cntrl->irq[mhi_event->irq]);
 		tasklet_kill(&mhi_event->task);
 	}
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v1 05/10] bus: mhi: core: Disable IRQs when powering down

[Manivannan Sadhasivam]

On Fri, Sep 18, 2020 at 07:02:30PM -0700, Bhaumik Bhatt wrote:
> While powering down, the device may or may not acknowledge the MHI
> RESET issued by host for graceful shutdown scenario which can lead
> to a rogue device sending an interrupt after the clean-up has been
> done. This can result in a tasklet being scheduled after it has
> been killed and access already freed memory causing a NULL pointer
> exception. Avoid this corner case by disabling the interrupts as a
> part of host clean up.
> 
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> ---
>  drivers/bus/mhi/core/pm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> index 1862960..3462d82 100644
> --- a/drivers/bus/mhi/core/pm.c
> +++ b/drivers/bus/mhi/core/pm.c
> @@ -517,6 +517,7 @@ static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl,
>  	for (i = 0; i < mhi_cntrl->total_ev_rings; i++, mhi_event++) {
>  		if (mhi_event->offload_ev)
>  			continue;
> +		disable_irq(mhi_cntrl->irq[mhi_event->irq]);

No need to disable irq[0]?

Thanks,
Mani

>  		tasklet_kill(&mhi_event->task);
>  	}
>  
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH v1 05/10] bus: mhi: core: Disable IRQs when powering down

[Bhaumik Bhatt]

On 2020-10-09 09:02, Manivannan Sadhasivam wrote:
> On Fri, Sep 18, 2020 at 07:02:30PM -0700, Bhaumik Bhatt wrote:
>> While powering down, the device may or may not acknowledge the MHI
>> RESET issued by host for graceful shutdown scenario which can lead
>> to a rogue device sending an interrupt after the clean-up has been
>> done. This can result in a tasklet being scheduled after it has
>> been killed and access already freed memory causing a NULL pointer
>> exception. Avoid this corner case by disabling the interrupts as a
>> part of host clean up.
>> 
>> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
>> ---
>>  drivers/bus/mhi/core/pm.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
>> index 1862960..3462d82 100644
>> --- a/drivers/bus/mhi/core/pm.c
>> +++ b/drivers/bus/mhi/core/pm.c
>> @@ -517,6 +517,7 @@ static void mhi_pm_disable_transition(struct 
>> mhi_controller *mhi_cntrl,
>>  	for (i = 0; i < mhi_cntrl->total_ev_rings; i++, mhi_event++) {
>>  		if (mhi_event->offload_ev)
>>  			continue;
>> +		disable_irq(mhi_cntrl->irq[mhi_event->irq]);
> 
> No need to disable irq[0]?
> 
> Thanks,
> Mani
> 
>>  		tasklet_kill(&mhi_event->task);
>>  	}
>> 
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum,
>> a Linux Foundation Collaborative Project
>> 

This patch would disable the IRQ line and if IRQ lines are shared 
between BHI
and MHI, we would not see handling of BHI related work happen.

Discussed this with Hemant and and as I am dropping the previous patch, 
will update
this one to make it free_irq() instead which removes the IRQ handler and 
does not
disable the interrupt line.

The function mhi_deinit_free_irq() will not be called from 
mhi_power_down() and
instead, only a free for the main IRQ handler will be called.

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

Re: [PATCH v1 05/10] bus: mhi: core: Disable IRQs when powering down

[Manu Gautam]

Hi

On 9/19/2020 7:32 AM, Bhaumik Bhatt wrote:
> While powering down, the device may or may not acknowledge the MHI
> RESET issued by host for graceful shutdown scenario which can lead
> to a rogue device sending an interrupt after the clean-up has been
> done. This can result in a tasklet being scheduled after it has
> been killed and access already freed memory causing a NULL pointer
> exception. Avoid this corner case by disabling the interrupts as a
> part of host clean up.
>
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> ---
>  drivers/bus/mhi/core/pm.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> index 1862960..3462d82 100644
> --- a/drivers/bus/mhi/core/pm.c
> +++ b/drivers/bus/mhi/core/pm.c
> @@ -517,6 +517,7 @@ static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl,
>  	for (i = 0; i < mhi_cntrl->total_ev_rings; i++, mhi_event++) {
>  		if (mhi_event->offload_ev)
>  			continue;
> +		disable_irq(mhi_cntrl->irq[mhi_event->irq]);
>  		tasklet_kill(&mhi_event->task);
>  	}
>  

What about sys_err handling? IRQ may be left disabled?


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

[PATCH v1 06/10] bus: mhi: core: Improve shutdown handling after link down detection

[Bhaumik Bhatt]

If MHI were to attempt a device shutdown following an assumption
that the device is inaccessible, the host currently moves to a state
where device register accesses are allowed when they should not be.
This would end up allowing accesses to the device register space when
the link is inaccessible and can result in bus errors observed on the
host. Improve shutdown handling to prevent these outcomes and do not
move the MHI PM state to a register accessible state after device is
assumed to be inaccessible.

Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
---
 drivers/bus/mhi/core/init.c     |  1 +
 drivers/bus/mhi/core/internal.h |  1 +
 drivers/bus/mhi/core/pm.c       | 18 +++++++++++++-----
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index 9ae4c19..fa33dde 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -37,6 +37,7 @@ const char * const dev_state_tran_str[DEV_ST_TRANSITION_MAX] = {
 	[DEV_ST_TRANSITION_MISSION_MODE] = "MISSION_MODE",
 	[DEV_ST_TRANSITION_SYS_ERR] = "SYS_ERR",
 	[DEV_ST_TRANSITION_DISABLE] = "DISABLE",
+	[DEV_ST_TRANSITION_FATAL] = "FATAL SHUTDOWN",
 };
 
 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 7989269..f3b9e5a 100644
--- a/drivers/bus/mhi/core/internal.h
+++ b/drivers/bus/mhi/core/internal.h
@@ -388,6 +388,7 @@ enum dev_st_transition {
 	DEV_ST_TRANSITION_MISSION_MODE,
 	DEV_ST_TRANSITION_SYS_ERR,
 	DEV_ST_TRANSITION_DISABLE,
+	DEV_ST_TRANSITION_FATAL,
 	DEV_ST_TRANSITION_MAX,
 };
 
diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
index 3462d82..bce1f62 100644
--- a/drivers/bus/mhi/core/pm.c
+++ b/drivers/bus/mhi/core/pm.c
@@ -37,9 +37,10 @@
  *     M0 -> FW_DL_ERR
  *     M0 -> M3_ENTER -> M3 -> M3_EXIT --> M0
  * L1: SYS_ERR_DETECT -> SYS_ERR_PROCESS --> POR
- * L2: SHUTDOWN_PROCESS -> DISABLE
+ * L2: SHUTDOWN_PROCESS -> LD_ERR_FATAL_DETECT
+ *     SHUTDOWN_PROCESS -> DISABLE
  * L3: LD_ERR_FATAL_DETECT <--> LD_ERR_FATAL_DETECT
- *     LD_ERR_FATAL_DETECT -> SHUTDOWN_PROCESS
+ *     LD_ERR_FATAL_DETECT -> DISABLE
  */
 static struct mhi_pm_transitions const dev_state_transitions[] = {
 	/* L0 States */
@@ -72,7 +73,7 @@ static struct mhi_pm_transitions const dev_state_transitions[] = {
 	{
 		MHI_PM_M3,
 		MHI_PM_M3_EXIT | MHI_PM_SYS_ERR_DETECT |
-		MHI_PM_SHUTDOWN_PROCESS | MHI_PM_LD_ERR_FATAL_DETECT
+		MHI_PM_LD_ERR_FATAL_DETECT
 	},
 	{
 		MHI_PM_M3_EXIT,
@@ -103,7 +104,7 @@ static struct mhi_pm_transitions const dev_state_transitions[] = {
 	/* L3 States */
 	{
 		MHI_PM_LD_ERR_FATAL_DETECT,
-		MHI_PM_LD_ERR_FATAL_DETECT | MHI_PM_SHUTDOWN_PROCESS
+		MHI_PM_LD_ERR_FATAL_DETECT | MHI_PM_DISABLE
 	},
 };
 
@@ -670,6 +671,10 @@ void mhi_pm_st_worker(struct work_struct *work)
 			mhi_pm_disable_transition
 				(mhi_cntrl, MHI_PM_SHUTDOWN_PROCESS);
 			break;
+		case DEV_ST_TRANSITION_FATAL:
+			mhi_pm_disable_transition
+				(mhi_cntrl, MHI_PM_LD_ERR_FATAL_DETECT);
+			break;
 		default:
 			break;
 		}
@@ -1039,6 +1044,7 @@ EXPORT_SYMBOL_GPL(mhi_async_power_up);
 void mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful)
 {
 	enum mhi_pm_state cur_state;
+	enum dev_st_transition next_state = DEV_ST_TRANSITION_DISABLE;
 	struct device *dev = &mhi_cntrl->mhi_dev->dev;
 
 	/* If it's not a graceful shutdown, force MHI to linkdown state */
@@ -1053,9 +1059,11 @@ void mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful)
 			dev_dbg(dev, "Failed to move to state: %s from: %s\n",
 				to_mhi_pm_state_str(MHI_PM_LD_ERR_FATAL_DETECT),
 				to_mhi_pm_state_str(mhi_cntrl->pm_state));
+		else
+			next_state = DEV_ST_TRANSITION_FATAL;
 	}
 
-	mhi_queue_state_transition(mhi_cntrl, DEV_ST_TRANSITION_DISABLE);
+	mhi_queue_state_transition(mhi_cntrl, next_state);
 
 	/* Wait for shutdown to complete */
 	flush_work(&mhi_cntrl->st_worker);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v1 06/10] bus: mhi: core: Improve shutdown handling after link down detection

[Manivannan Sadhasivam]

On Fri, Sep 18, 2020 at 07:02:31PM -0700, Bhaumik Bhatt wrote:
> If MHI were to attempt a device shutdown following an assumption

MHI host? And is this really an assumption or it is definite that the
link is inaccessible. Please clarify!

> that the device is inaccessible, the host currently moves to a state
> where device register accesses are allowed when they should not be.
> This would end up allowing accesses to the device register space when
> the link is inaccessible and can result in bus errors observed on the
> host. Improve shutdown handling to prevent these outcomes and do not
> move the MHI PM state to a register accessible state after device is
> assumed to be inaccessible.
>

Apparently you are introducing a new device transition state but your
commit description doesn't state that :/

Thanks,
Mani
 
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> ---
>  drivers/bus/mhi/core/init.c     |  1 +
>  drivers/bus/mhi/core/internal.h |  1 +
>  drivers/bus/mhi/core/pm.c       | 18 +++++++++++++-----
>  3 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index 9ae4c19..fa33dde 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -37,6 +37,7 @@ const char * const dev_state_tran_str[DEV_ST_TRANSITION_MAX] = {
>  	[DEV_ST_TRANSITION_MISSION_MODE] = "MISSION_MODE",
>  	[DEV_ST_TRANSITION_SYS_ERR] = "SYS_ERR",
>  	[DEV_ST_TRANSITION_DISABLE] = "DISABLE",
> +	[DEV_ST_TRANSITION_FATAL] = "FATAL SHUTDOWN",
>  };
>  
>  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 7989269..f3b9e5a 100644
> --- a/drivers/bus/mhi/core/internal.h
> +++ b/drivers/bus/mhi/core/internal.h
> @@ -388,6 +388,7 @@ enum dev_st_transition {
>  	DEV_ST_TRANSITION_MISSION_MODE,
>  	DEV_ST_TRANSITION_SYS_ERR,
>  	DEV_ST_TRANSITION_DISABLE,
> +	DEV_ST_TRANSITION_FATAL,
>  	DEV_ST_TRANSITION_MAX,
>  };
>  
> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> index 3462d82..bce1f62 100644
> --- a/drivers/bus/mhi/core/pm.c
> +++ b/drivers/bus/mhi/core/pm.c
> @@ -37,9 +37,10 @@
>   *     M0 -> FW_DL_ERR
>   *     M0 -> M3_ENTER -> M3 -> M3_EXIT --> M0
>   * L1: SYS_ERR_DETECT -> SYS_ERR_PROCESS --> POR
> - * L2: SHUTDOWN_PROCESS -> DISABLE
> + * L2: SHUTDOWN_PROCESS -> LD_ERR_FATAL_DETECT
> + *     SHUTDOWN_PROCESS -> DISABLE
>   * L3: LD_ERR_FATAL_DETECT <--> LD_ERR_FATAL_DETECT
> - *     LD_ERR_FATAL_DETECT -> SHUTDOWN_PROCESS
> + *     LD_ERR_FATAL_DETECT -> DISABLE
>   */
>  static struct mhi_pm_transitions const dev_state_transitions[] = {
>  	/* L0 States */
> @@ -72,7 +73,7 @@ static struct mhi_pm_transitions const dev_state_transitions[] = {
>  	{
>  		MHI_PM_M3,
>  		MHI_PM_M3_EXIT | MHI_PM_SYS_ERR_DETECT |
> -		MHI_PM_SHUTDOWN_PROCESS | MHI_PM_LD_ERR_FATAL_DETECT
> +		MHI_PM_LD_ERR_FATAL_DETECT
>  	},
>  	{
>  		MHI_PM_M3_EXIT,
> @@ -103,7 +104,7 @@ static struct mhi_pm_transitions const dev_state_transitions[] = {
>  	/* L3 States */
>  	{
>  		MHI_PM_LD_ERR_FATAL_DETECT,
> -		MHI_PM_LD_ERR_FATAL_DETECT | MHI_PM_SHUTDOWN_PROCESS
> +		MHI_PM_LD_ERR_FATAL_DETECT | MHI_PM_DISABLE
>  	},
>  };
>  
> @@ -670,6 +671,10 @@ void mhi_pm_st_worker(struct work_struct *work)
>  			mhi_pm_disable_transition
>  				(mhi_cntrl, MHI_PM_SHUTDOWN_PROCESS);
>  			break;
> +		case DEV_ST_TRANSITION_FATAL:
> +			mhi_pm_disable_transition
> +				(mhi_cntrl, MHI_PM_LD_ERR_FATAL_DETECT);
> +			break;
>  		default:
>  			break;
>  		}
> @@ -1039,6 +1044,7 @@ EXPORT_SYMBOL_GPL(mhi_async_power_up);
>  void mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful)
>  {
>  	enum mhi_pm_state cur_state;
> +	enum dev_st_transition next_state = DEV_ST_TRANSITION_DISABLE;
>  	struct device *dev = &mhi_cntrl->mhi_dev->dev;
>  
>  	/* If it's not a graceful shutdown, force MHI to linkdown state */
> @@ -1053,9 +1059,11 @@ void mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful)
>  			dev_dbg(dev, "Failed to move to state: %s from: %s\n",
>  				to_mhi_pm_state_str(MHI_PM_LD_ERR_FATAL_DETECT),
>  				to_mhi_pm_state_str(mhi_cntrl->pm_state));
> +		else
> +			next_state = DEV_ST_TRANSITION_FATAL;
>  	}
>  
> -	mhi_queue_state_transition(mhi_cntrl, DEV_ST_TRANSITION_DISABLE);
> +	mhi_queue_state_transition(mhi_cntrl, next_state);
>  
>  	/* Wait for shutdown to complete */
>  	flush_work(&mhi_cntrl->st_worker);
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH v1 06/10] bus: mhi: core: Improve shutdown handling after link down detection

[Bhaumik Bhatt]

On 2020-10-09 09:19, Manivannan Sadhasivam wrote:
> On Fri, Sep 18, 2020 at 07:02:31PM -0700, Bhaumik Bhatt wrote:
>> If MHI were to attempt a device shutdown following an assumption
> 
> MHI host? And is this really an assumption or it is definite that the
> link is inaccessible. Please clarify!
> 
Will update it to MHI host.

I say assumption because we act based on the "graceful" flag passed by
the MHI controller driver. Link may be accessible but the controller has
instructed MHI not to do any further accesses. They may decide to set 
the
flag as "false" if they see any read/access issues, an actual link down
interrupt from the bus driver or a sideband GPIO signal declaring that a
software assert occurred on the device.

MHI host sees that power down attempt as ungraceful and assumes that the
controller has decided that it's either a link down or a fatal error.

Once an "ungraceful" power down attempt is made, MHI host moves to the
LD_ERR_FATAL_DETECT pm_state and without this patch, it moved from an
LD_ERR_FATAL_DETECT to SHUTDOWN_PROCESS state, where SHUTDOWN_PROCESS
is defined as a register accessible state by the MHI_REG_ACCESS_VALID()
macro.

If someone were to do a call that needed a register access from their
.remove() callback, for example, we could see a bus error.

Moves from MHI_PM_M3 to SHUTDOWN_PROCESS and LD_ERR_FATAL_DETECT to
SHUTDOWN_PROCESS are not allowed by use of this patch.

I'll add better wording and explanation.

>> that the device is inaccessible, the host currently moves to a state
>> where device register accesses are allowed when they should not be.
>> This would end up allowing accesses to the device register space when
>> the link is inaccessible and can result in bus errors observed on the
>> host. Improve shutdown handling to prevent these outcomes and do not
>> move the MHI PM state to a register accessible state after device is
>> assumed to be inaccessible.
>> 
> 
> Apparently you are introducing a new device transition state but your
> commit description doesn't state that :/
> 
> Thanks,
> Mani
> 
Sure. I will add that.

>> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
>> ---
>>  drivers/bus/mhi/core/init.c     |  1 +
>>  drivers/bus/mhi/core/internal.h |  1 +
>>  drivers/bus/mhi/core/pm.c       | 18 +++++++++++++-----
>>  3 files changed, 15 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
>> index 9ae4c19..fa33dde 100644
>> --- a/drivers/bus/mhi/core/init.c
>> +++ b/drivers/bus/mhi/core/init.c
>> @@ -37,6 +37,7 @@ const char * const 
>> dev_state_tran_str[DEV_ST_TRANSITION_MAX] = {
>>  	[DEV_ST_TRANSITION_MISSION_MODE] = "MISSION_MODE",
>>  	[DEV_ST_TRANSITION_SYS_ERR] = "SYS_ERR",
>>  	[DEV_ST_TRANSITION_DISABLE] = "DISABLE",
>> +	[DEV_ST_TRANSITION_FATAL] = "FATAL SHUTDOWN",
>>  };
>> 
>>  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 7989269..f3b9e5a 100644
>> --- a/drivers/bus/mhi/core/internal.h
>> +++ b/drivers/bus/mhi/core/internal.h
>> @@ -388,6 +388,7 @@ enum dev_st_transition {
>>  	DEV_ST_TRANSITION_MISSION_MODE,
>>  	DEV_ST_TRANSITION_SYS_ERR,
>>  	DEV_ST_TRANSITION_DISABLE,
>> +	DEV_ST_TRANSITION_FATAL,
>>  	DEV_ST_TRANSITION_MAX,
>>  };
>> 
>> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
>> index 3462d82..bce1f62 100644
>> --- a/drivers/bus/mhi/core/pm.c
>> +++ b/drivers/bus/mhi/core/pm.c
>> @@ -37,9 +37,10 @@
>>   *     M0 -> FW_DL_ERR
>>   *     M0 -> M3_ENTER -> M3 -> M3_EXIT --> M0
>>   * L1: SYS_ERR_DETECT -> SYS_ERR_PROCESS --> POR
>> - * L2: SHUTDOWN_PROCESS -> DISABLE
>> + * L2: SHUTDOWN_PROCESS -> LD_ERR_FATAL_DETECT
>> + *     SHUTDOWN_PROCESS -> DISABLE
>>   * L3: LD_ERR_FATAL_DETECT <--> LD_ERR_FATAL_DETECT
>> - *     LD_ERR_FATAL_DETECT -> SHUTDOWN_PROCESS
>> + *     LD_ERR_FATAL_DETECT -> DISABLE
>>   */
>>  static struct mhi_pm_transitions const dev_state_transitions[] = {
>>  	/* L0 States */
>> @@ -72,7 +73,7 @@ static struct mhi_pm_transitions const 
>> dev_state_transitions[] = {
>>  	{
>>  		MHI_PM_M3,
>>  		MHI_PM_M3_EXIT | MHI_PM_SYS_ERR_DETECT |
>> -		MHI_PM_SHUTDOWN_PROCESS | MHI_PM_LD_ERR_FATAL_DETECT
>> +		MHI_PM_LD_ERR_FATAL_DETECT
>>  	},
>>  	{
>>  		MHI_PM_M3_EXIT,
>> @@ -103,7 +104,7 @@ static struct mhi_pm_transitions const 
>> dev_state_transitions[] = {
>>  	/* L3 States */
>>  	{
>>  		MHI_PM_LD_ERR_FATAL_DETECT,
>> -		MHI_PM_LD_ERR_FATAL_DETECT | MHI_PM_SHUTDOWN_PROCESS
>> +		MHI_PM_LD_ERR_FATAL_DETECT | MHI_PM_DISABLE
>>  	},
>>  };
>> 
>> @@ -670,6 +671,10 @@ void mhi_pm_st_worker(struct work_struct *work)
>>  			mhi_pm_disable_transition
>>  				(mhi_cntrl, MHI_PM_SHUTDOWN_PROCESS);
>>  			break;
>> +		case DEV_ST_TRANSITION_FATAL:
>> +			mhi_pm_disable_transition
>> +				(mhi_cntrl, MHI_PM_LD_ERR_FATAL_DETECT);
>> +			break;
>>  		default:
>>  			break;
>>  		}
>> @@ -1039,6 +1044,7 @@ EXPORT_SYMBOL_GPL(mhi_async_power_up);
>>  void mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful)
>>  {
>>  	enum mhi_pm_state cur_state;
>> +	enum dev_st_transition next_state = DEV_ST_TRANSITION_DISABLE;
>>  	struct device *dev = &mhi_cntrl->mhi_dev->dev;
>> 
>>  	/* If it's not a graceful shutdown, force MHI to linkdown state */
>> @@ -1053,9 +1059,11 @@ void mhi_power_down(struct mhi_controller 
>> *mhi_cntrl, bool graceful)
>>  			dev_dbg(dev, "Failed to move to state: %s from: %s\n",
>>  				to_mhi_pm_state_str(MHI_PM_LD_ERR_FATAL_DETECT),
>>  				to_mhi_pm_state_str(mhi_cntrl->pm_state));
>> +		else
>> +			next_state = DEV_ST_TRANSITION_FATAL;
>>  	}
>> 
>> -	mhi_queue_state_transition(mhi_cntrl, DEV_ST_TRANSITION_DISABLE);
>> +	mhi_queue_state_transition(mhi_cntrl, next_state);
>> 
>>  	/* Wait for shutdown to complete */
>>  	flush_work(&mhi_cntrl->st_worker);
>> --
>> 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 06/10] bus: mhi: core: Improve shutdown handling after link down detection

[Manu Gautam]

On 9/19/2020 7:32 AM, Bhaumik Bhatt wrote:
> If MHI were to attempt a device shutdown following an assumption
> that the device is inaccessible, the host currently moves to a state
> where device register accesses are allowed when they should not be.
> This would end up allowing accesses to the device register space when
> the link is inaccessible and can result in bus errors observed on the
> host. Improve shutdown handling to prevent these outcomes and do not
> move the MHI PM state to a register accessible state after device is

Which state are you referring to when you say 'register accessible state'?
Would it be possible to provide more details on current handling here?


> assumed to be inaccessible.
>
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> ---
>  drivers/bus/mhi/core/init.c     |  1 +
>  drivers/bus/mhi/core/internal.h |  1 +
>  drivers/bus/mhi/core/pm.c       | 18 +++++++++++++-----
>  3 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index 9ae4c19..fa33dde 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -37,6 +37,7 @@ const char * const dev_state_tran_str[DEV_ST_TRANSITION_MAX] = {
>  	[DEV_ST_TRANSITION_MISSION_MODE] = "MISSION_MODE",
>  	[DEV_ST_TRANSITION_SYS_ERR] = "SYS_ERR",
>  	[DEV_ST_TRANSITION_DISABLE] = "DISABLE",
> +	[DEV_ST_TRANSITION_FATAL] = "FATAL SHUTDOWN",
>  };
>  
>  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 7989269..f3b9e5a 100644
> --- a/drivers/bus/mhi/core/internal.h
> +++ b/drivers/bus/mhi/core/internal.h
> @@ -388,6 +388,7 @@ enum dev_st_transition {
>  	DEV_ST_TRANSITION_MISSION_MODE,
>  	DEV_ST_TRANSITION_SYS_ERR,
>  	DEV_ST_TRANSITION_DISABLE,
> +	DEV_ST_TRANSITION_FATAL,
>  	DEV_ST_TRANSITION_MAX,
>  };
>  
> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> index 3462d82..bce1f62 100644
> --- a/drivers/bus/mhi/core/pm.c
> +++ b/drivers/bus/mhi/core/pm.c
> @@ -37,9 +37,10 @@
>   *     M0 -> FW_DL_ERR
>   *     M0 -> M3_ENTER -> M3 -> M3_EXIT --> M0
>   * L1: SYS_ERR_DETECT -> SYS_ERR_PROCESS --> POR
> - * L2: SHUTDOWN_PROCESS -> DISABLE
> + * L2: SHUTDOWN_PROCESS -> LD_ERR_FATAL_DETECT
> + *     SHUTDOWN_PROCESS -> DISABLE
>   * L3: LD_ERR_FATAL_DETECT <--> LD_ERR_FATAL_DETECT
> - *     LD_ERR_FATAL_DETECT -> SHUTDOWN_PROCESS
> + *     LD_ERR_FATAL_DETECT -> DISABLE
>   */
>  static struct mhi_pm_transitions const dev_state_transitions[] = {
>  	/* L0 States */
> @@ -72,7 +73,7 @@ static struct mhi_pm_transitions const dev_state_transitions[] = {
>  	{
>  		MHI_PM_M3,
>  		MHI_PM_M3_EXIT | MHI_PM_SYS_ERR_DETECT |
> -		MHI_PM_SHUTDOWN_PROCESS | MHI_PM_LD_ERR_FATAL_DETECT
> +		MHI_PM_LD_ERR_FATAL_DETECT
>  	},
>  	{
>  		MHI_PM_M3_EXIT,
> @@ -103,7 +104,7 @@ static struct mhi_pm_transitions const dev_state_transitions[] = {
>  	/* L3 States */
>  	{
>  		MHI_PM_LD_ERR_FATAL_DETECT,
> -		MHI_PM_LD_ERR_FATAL_DETECT | MHI_PM_SHUTDOWN_PROCESS
> +		MHI_PM_LD_ERR_FATAL_DETECT | MHI_PM_DISABLE
>  	},
>  };
>  
> @@ -670,6 +671,10 @@ void mhi_pm_st_worker(struct work_struct *work)
>  			mhi_pm_disable_transition
>  				(mhi_cntrl, MHI_PM_SHUTDOWN_PROCESS);
>  			break;
> +		case DEV_ST_TRANSITION_FATAL:
> +			mhi_pm_disable_transition
> +				(mhi_cntrl, MHI_PM_LD_ERR_FATAL_DETECT);
> +			break;
>  		default:
>  			break;
>  		}
> @@ -1039,6 +1044,7 @@ EXPORT_SYMBOL_GPL(mhi_async_power_up);
>  void mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful)
>  {
>  	enum mhi_pm_state cur_state;
> +	enum dev_st_transition next_state = DEV_ST_TRANSITION_DISABLE;
>  	struct device *dev = &mhi_cntrl->mhi_dev->dev;
>  
>  	/* If it's not a graceful shutdown, force MHI to linkdown state */
> @@ -1053,9 +1059,11 @@ void mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful)
>  			dev_dbg(dev, "Failed to move to state: %s from: %s\n",
>  				to_mhi_pm_state_str(MHI_PM_LD_ERR_FATAL_DETECT),
>  				to_mhi_pm_state_str(mhi_cntrl->pm_state));
> +		else
> +			next_state = DEV_ST_TRANSITION_FATAL;
>  	}
>  
> -	mhi_queue_state_transition(mhi_cntrl, DEV_ST_TRANSITION_DISABLE);
> +	mhi_queue_state_transition(mhi_cntrl, next_state);
>  
>  	/* Wait for shutdown to complete */
>  	flush_work(&mhi_cntrl->st_worker);

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

[PATCH v1 07/10] bus: mhi: core: Move to SYS_ERROR regardless of RDDM capability

[Bhaumik Bhatt]

In some cases, the entry of device to RDDM execution environment
can occur after a significant amount of time has elapsed after the
SYS_ERROR state change event has arrived. This can result in scenarios
where users of the MHI bus are unaware of the error state of the
device. Hence, moving the MHI bus to a SYS_ERROR detected state will
prevent further client activity and wait for the RDDM entry.

Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
---
 drivers/bus/mhi/core/main.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index 2cff5dd..1c8e332 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -376,6 +376,7 @@ irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *priv)
 	enum mhi_state state = MHI_STATE_MAX;
 	enum mhi_pm_state pm_state = 0;
 	enum mhi_ee_type ee = 0;
+	bool handle_rddm = false;
 
 	write_lock_irq(&mhi_cntrl->pm_lock);
 	if (!MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
@@ -400,6 +401,17 @@ irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *priv)
 	 /* 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) {
+			/* prevent clients from queueing any more packets */
+			write_lock_irq(&mhi_cntrl->pm_lock);
+			pm_state = mhi_tryset_pm_state(mhi_cntrl,
+						       MHI_PM_SYS_ERR_DETECT);
+			if (pm_state == MHI_PM_SYS_ERR_DETECT)
+				handle_rddm = true;
+			write_unlock_irq(&mhi_cntrl->pm_lock);
+		}
+
+		if (handle_rddm) {
+			dev_err(dev, "RDDM event occurred!\n");
 			mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_EE_RDDM);
 			wake_up_all(&mhi_cntrl->state_event);
 		}
@@ -733,19 +745,15 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl,
 				break;
 			case MHI_STATE_SYS_ERR:
 			{
-				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;
+				enum mhi_pm_state state = MHI_PM_STATE_MAX;
 
 				dev_dbg(dev, "System error detected\n");
 				write_lock_irq(&mhi_cntrl->pm_lock);
-				new_state = mhi_tryset_pm_state(mhi_cntrl,
+				if (mhi_cntrl->ee != MHI_EE_RDDM)
+					state = mhi_tryset_pm_state(mhi_cntrl,
 							MHI_PM_SYS_ERR_DETECT);
 				write_unlock_irq(&mhi_cntrl->pm_lock);
-				if (new_state == MHI_PM_SYS_ERR_DETECT)
+				if (state == MHI_PM_SYS_ERR_DETECT)
 					mhi_pm_sys_err_handler(mhi_cntrl);
 				break;
 			}
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v1 07/10] bus: mhi: core: Move to SYS_ERROR regardless of RDDM capability

[Manivannan Sadhasivam]

On Fri, Sep 18, 2020 at 07:02:32PM -0700, Bhaumik Bhatt wrote:
> In some cases, the entry of device to RDDM execution environment
> can occur after a significant amount of time has elapsed after the
> SYS_ERROR state change event has arrived. This can result in scenarios
> where users of the MHI bus are unaware of the error state of the

Who are all the users of MHI bus? Client drivers?

> device. Hence, moving the MHI bus to a SYS_ERROR detected state will
> prevent further client activity and wait for the RDDM entry.
> 
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> ---
>  drivers/bus/mhi/core/main.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> index 2cff5dd..1c8e332 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -376,6 +376,7 @@ irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *priv)
>  	enum mhi_state state = MHI_STATE_MAX;
>  	enum mhi_pm_state pm_state = 0;
>  	enum mhi_ee_type ee = 0;
> +	bool handle_rddm = false;
>  
>  	write_lock_irq(&mhi_cntrl->pm_lock);
>  	if (!MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
> @@ -400,6 +401,17 @@ irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *priv)
>  	 /* 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) {
> +			/* prevent clients from queueing any more packets */
> +			write_lock_irq(&mhi_cntrl->pm_lock);
> +			pm_state = mhi_tryset_pm_state(mhi_cntrl,
> +						       MHI_PM_SYS_ERR_DETECT);

The condition above already moves MHI to MHI_PM_SYS_ERR_DETECT if the state
is MHI_STATE_SYS_ERR. Why are you doing it here again?

Thanks,
Mani

> +			if (pm_state == MHI_PM_SYS_ERR_DETECT)
> +				handle_rddm = true;
> +			write_unlock_irq(&mhi_cntrl->pm_lock);
> +		}
> +
> +		if (handle_rddm) {
> +			dev_err(dev, "RDDM event occurred!\n");
>  			mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_EE_RDDM);
>  			wake_up_all(&mhi_cntrl->state_event);
>  		}
> @@ -733,19 +745,15 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl,
>  				break;
>  			case MHI_STATE_SYS_ERR:
>  			{
> -				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;
> +				enum mhi_pm_state state = MHI_PM_STATE_MAX;
>  
>  				dev_dbg(dev, "System error detected\n");
>  				write_lock_irq(&mhi_cntrl->pm_lock);
> -				new_state = mhi_tryset_pm_state(mhi_cntrl,
> +				if (mhi_cntrl->ee != MHI_EE_RDDM)
> +					state = mhi_tryset_pm_state(mhi_cntrl,
>  							MHI_PM_SYS_ERR_DETECT);
>  				write_unlock_irq(&mhi_cntrl->pm_lock);
> -				if (new_state == MHI_PM_SYS_ERR_DETECT)
> +				if (state == MHI_PM_SYS_ERR_DETECT)
>  					mhi_pm_sys_err_handler(mhi_cntrl);
>  				break;
>  			}
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH v1 07/10] bus: mhi: core: Move to SYS_ERROR regardless of RDDM capability

[Bhaumik Bhatt]

On 2020-10-09 09:32, Manivannan Sadhasivam wrote:
> On Fri, Sep 18, 2020 at 07:02:32PM -0700, Bhaumik Bhatt wrote:
>> In some cases, the entry of device to RDDM execution environment
>> can occur after a significant amount of time has elapsed after the
>> SYS_ERROR state change event has arrived. This can result in scenarios
>> where users of the MHI bus are unaware of the error state of the
> 
> Who are all the users of MHI bus? Client drivers?
> 
Both client and controller drivers. I will change it to that.
>> device. Hence, moving the MHI bus to a SYS_ERROR detected state will
>> prevent further client activity and wait for the RDDM entry.
>> 
>> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
>> ---
>>  drivers/bus/mhi/core/main.c | 24 ++++++++++++++++--------
>>  1 file changed, 16 insertions(+), 8 deletions(-)
>> 
>> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
>> index 2cff5dd..1c8e332 100644
>> --- a/drivers/bus/mhi/core/main.c
>> +++ b/drivers/bus/mhi/core/main.c
>> @@ -376,6 +376,7 @@ irqreturn_t mhi_intvec_threaded_handler(int 
>> irq_number, void *priv)
>>  	enum mhi_state state = MHI_STATE_MAX;
>>  	enum mhi_pm_state pm_state = 0;
>>  	enum mhi_ee_type ee = 0;
>> +	bool handle_rddm = false;
>> 
>>  	write_lock_irq(&mhi_cntrl->pm_lock);
>>  	if (!MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
>> @@ -400,6 +401,17 @@ irqreturn_t mhi_intvec_threaded_handler(int 
>> irq_number, void *priv)
>>  	 /* 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) {
>> +			/* prevent clients from queueing any more packets */
>> +			write_lock_irq(&mhi_cntrl->pm_lock);
>> +			pm_state = mhi_tryset_pm_state(mhi_cntrl,
>> +						       MHI_PM_SYS_ERR_DETECT);
> 
> The condition above already moves MHI to MHI_PM_SYS_ERR_DETECT if the 
> state
> is MHI_STATE_SYS_ERR. Why are you doing it here again?
> 
> Thanks,
> Mani
> 
I added it there because any first move to RDDM required the MHI host to 
be
inactive or in an "error" state.
However, upon further thought, I have made changes that negate this need 
and
instead make the if (mhi_cntrl->rddm_image) check dependent on the 
pm_state being
MHI_PM_SYS_ERR_DETECT.

Reason is: a first move RDDM comes after a SYS_ERROR in MHI state, since 
PM state
will already by SYS_ERROR detect by then, no client drivers or 
controllers will be able
to use the bus.
>> +			if (pm_state == MHI_PM_SYS_ERR_DETECT)
>> +				handle_rddm = true;
>> +			write_unlock_irq(&mhi_cntrl->pm_lock);
>> +		}
>> +
>> +		if (handle_rddm) {
>> +			dev_err(dev, "RDDM event occurred!\n");
>>  			mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_EE_RDDM);
>>  			wake_up_all(&mhi_cntrl->state_event);
>>  		}
>> @@ -733,19 +745,15 @@ int mhi_process_ctrl_ev_ring(struct 
>> mhi_controller *mhi_cntrl,
>>  				break;
>>  			case MHI_STATE_SYS_ERR:
>>  			{
>> -				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;
>> +				enum mhi_pm_state state = MHI_PM_STATE_MAX;
>> 
>>  				dev_dbg(dev, "System error detected\n");
>>  				write_lock_irq(&mhi_cntrl->pm_lock);
>> -				new_state = mhi_tryset_pm_state(mhi_cntrl,
>> +				if (mhi_cntrl->ee != MHI_EE_RDDM)
>> +					state = mhi_tryset_pm_state(mhi_cntrl,
>>  							MHI_PM_SYS_ERR_DETECT);
>>  				write_unlock_irq(&mhi_cntrl->pm_lock);
>> -				if (new_state == MHI_PM_SYS_ERR_DETECT)
>> +				if (state == MHI_PM_SYS_ERR_DETECT)
>>  					mhi_pm_sys_err_handler(mhi_cntrl);
>>  				break;
>>  			}
>> --
>> 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

[PATCH v1 08/10] bus: mhi: core: Move to an error state on any firmware load failure

[Bhaumik Bhatt]

Move MHI to a firmware download error state for a failure to find
the firmware files or to load SBL or EBL image using BHI/BHIe. This
helps detect an error state sooner and shortens the wait for a
synchronous power up timeout.

Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
---
 drivers/bus/mhi/core/boot.c | 43 +++++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/drivers/bus/mhi/core/boot.c b/drivers/bus/mhi/core/boot.c
index 92b8dd3..fcc71f2 100644
--- a/drivers/bus/mhi/core/boot.c
+++ b/drivers/bus/mhi/core/boot.c
@@ -425,13 +425,13 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
 						     !mhi_cntrl->seg_len))) {
 		dev_err(dev,
 			"No firmware image defined or !sbl_size || !seg_len\n");
-		return;
+		goto error_fw_load;
 	}
 
 	ret = request_firmware(&firmware, fw_name, dev);
 	if (ret) {
 		dev_err(dev, "Error loading firmware: %d\n", ret);
-		return;
+		goto error_fw_load;
 	}
 
 	size = (mhi_cntrl->fbc_download) ? mhi_cntrl->sbl_size : firmware->size;
@@ -443,7 +443,7 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
 	buf = mhi_alloc_coherent(mhi_cntrl, size, &dma_addr, GFP_KERNEL);
 	if (!buf) {
 		release_firmware(firmware);
-		return;
+		goto error_fw_load;
 	}
 
 	/* Download SBL or EDL image using BHI */
@@ -451,17 +451,17 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
 	ret = mhi_fw_load_bhi(mhi_cntrl, dma_addr, size);
 	mhi_free_coherent(mhi_cntrl, size, buf, dma_addr);
 
-	if (!mhi_cntrl->fbc_download || ret || mhi_cntrl->ee == MHI_EE_EDL)
-		release_firmware(firmware);
-
 	/* Error or in EDL mode, we're done */
 	if (ret) {
 		dev_err(dev, "MHI did not load SBL/EDL image, ret:%d\n", ret);
-		return;
+		release_firmware(firmware);
+		goto error_fw_load;
 	}
 
-	if (mhi_cntrl->ee == MHI_EE_EDL)
+	if (mhi_cntrl->ee == MHI_EE_EDL) {
+		release_firmware(firmware);
 		return;
+	}
 
 	write_lock_irq(&mhi_cntrl->pm_lock);
 	mhi_cntrl->dev_state = MHI_STATE_RESET;
@@ -474,13 +474,17 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
 	if (mhi_cntrl->fbc_download) {
 		ret = mhi_alloc_bhie_table(mhi_cntrl, &mhi_cntrl->fbc_image,
 					   firmware->size);
-		if (ret)
-			goto error_alloc_fw_table;
+		if (ret) {
+			release_firmware(firmware);
+			goto error_fw_load;
+		}
 
 		/* Load the firmware into BHIE vec table */
 		mhi_firmware_copy(mhi_cntrl, firmware, mhi_cntrl->fbc_image);
 	}
 
+	release_firmware(firmware);
+
 fw_load_ee_pthru:
 	/* Transitioning into MHI RESET->READY state */
 	ret = mhi_ready_state_transition(mhi_cntrl);
@@ -490,7 +494,7 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
 
 	if (ret) {
 		dev_err(dev, "MHI did not enter READY state\n");
-		goto error_read;
+		goto error_ready_state;
 	}
 
 	/* Wait for the SBL event */
@@ -501,7 +505,7 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
 
 	if (!ret || MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state)) {
 		dev_err(dev, "MHI did not enter SBL\n");
-		goto error_read;
+		goto error_ready_state;
 	}
 
 	/* Start full firmware image download */
@@ -509,17 +513,20 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
 	ret = mhi_fw_load_bhie(mhi_cntrl,
 			       /* Vector table is the last entry */
 			       &image_info->mhi_buf[image_info->entries - 1]);
-	if (ret)
+	if (ret) {
 		dev_err(dev, "MHI did not load AMSS, ret:%d\n", ret);
-
-	release_firmware(firmware);
+		goto error_fw_load;
+	}
 
 	return;
 
-error_read:
+error_ready_state:
 	mhi_free_bhie_table(mhi_cntrl, mhi_cntrl->fbc_image);
 	mhi_cntrl->fbc_image = NULL;
 
-error_alloc_fw_table:
-	release_firmware(firmware);
+error_fw_load:
+	write_lock_irq(&mhi_cntrl->pm_lock);
+	mhi_cntrl->pm_state = MHI_PM_FW_DL_ERR;
+	wake_up_all(&mhi_cntrl->state_event);
+	write_unlock_irq(&mhi_cntrl->pm_lock);
 }
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v1 08/10] bus: mhi: core: Move to an error state on any firmware load failure

[Manivannan Sadhasivam]

On Fri, Sep 18, 2020 at 07:02:33PM -0700, Bhaumik Bhatt wrote:
> Move MHI to a firmware download error state for a failure to find
> the firmware files or to load SBL or EBL image using BHI/BHIe. This
> helps detect an error state sooner and shortens the wait for a
> synchronous power up timeout.
> 
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> ---
>  drivers/bus/mhi/core/boot.c | 43 +++++++++++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/bus/mhi/core/boot.c b/drivers/bus/mhi/core/boot.c
> index 92b8dd3..fcc71f2 100644
> --- a/drivers/bus/mhi/core/boot.c
> +++ b/drivers/bus/mhi/core/boot.c

[...]

> -error_read:
> +error_ready_state:
>  	mhi_free_bhie_table(mhi_cntrl, mhi_cntrl->fbc_image);
>  	mhi_cntrl->fbc_image = NULL;
>  
> -error_alloc_fw_table:
> -	release_firmware(firmware);
> +error_fw_load:
> +	write_lock_irq(&mhi_cntrl->pm_lock);
> +	mhi_cntrl->pm_state = MHI_PM_FW_DL_ERR;
> +	wake_up_all(&mhi_cntrl->state_event);

Do you really need pm_lock for this?

Thanks,
Mani

> +	write_unlock_irq(&mhi_cntrl->pm_lock);
>  }
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH v1 08/10] bus: mhi: core: Move to an error state on any firmware load failure

[Bhaumik Bhatt]

On 2020-10-09 09:42, Manivannan Sadhasivam wrote:
> On Fri, Sep 18, 2020 at 07:02:33PM -0700, Bhaumik Bhatt wrote:
>> Move MHI to a firmware download error state for a failure to find
>> the firmware files or to load SBL or EBL image using BHI/BHIe. This
>> helps detect an error state sooner and shortens the wait for a
>> synchronous power up timeout.
>> 
>> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
>> ---
>>  drivers/bus/mhi/core/boot.c | 43 
>> +++++++++++++++++++++++++------------------
>>  1 file changed, 25 insertions(+), 18 deletions(-)
>> 
>> diff --git a/drivers/bus/mhi/core/boot.c b/drivers/bus/mhi/core/boot.c
>> index 92b8dd3..fcc71f2 100644
>> --- a/drivers/bus/mhi/core/boot.c
>> +++ b/drivers/bus/mhi/core/boot.c
> 
> [...]
> 
>> -error_read:
>> +error_ready_state:
>>  	mhi_free_bhie_table(mhi_cntrl, mhi_cntrl->fbc_image);
>>  	mhi_cntrl->fbc_image = NULL;
>> 
>> -error_alloc_fw_table:
>> -	release_firmware(firmware);
>> +error_fw_load:
>> +	write_lock_irq(&mhi_cntrl->pm_lock);
>> +	mhi_cntrl->pm_state = MHI_PM_FW_DL_ERR;
>> +	wake_up_all(&mhi_cntrl->state_event);
> 
> Do you really need pm_lock for this?
> 
> Thanks,
> Mani
> 
Not really, the underlying usage does not matter if this lock is used or
not. We just want to error out so removing it.
>> +	write_unlock_irq(&mhi_cntrl->pm_lock);
>>  }
>> --
>> 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

[PATCH v1 09/10] bus: mhi: core: Move to an error state on mission mode failure

[Bhaumik Bhatt]

If the host receives a mission mode event and by the time it can get
to processing it, the register accesses fail implying a connectivity
error, MHI should move to an error state. This helps avoid longer wait
times from a synchronous power up perspective and accurately reflects
the MHI execution environment and power management states.

Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
---
 drivers/bus/mhi/core/pm.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
index bce1f62..16c04ab 100644
--- a/drivers/bus/mhi/core/pm.c
+++ b/drivers/bus/mhi/core/pm.c
@@ -384,10 +384,14 @@ static int mhi_pm_mission_mode_transition(struct mhi_controller *mhi_cntrl)
 	write_lock_irq(&mhi_cntrl->pm_lock);
 	if (MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state))
 		mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);
-	write_unlock_irq(&mhi_cntrl->pm_lock);
 
-	if (!MHI_IN_MISSION_MODE(mhi_cntrl->ee))
+	if (!MHI_IN_MISSION_MODE(mhi_cntrl->ee)) {
+		mhi_cntrl->pm_state = MHI_PM_LD_ERR_FATAL_DETECT;
+		write_unlock_irq(&mhi_cntrl->pm_lock);
+		wake_up_all(&mhi_cntrl->state_event);
 		return -EIO;
+	}
+	write_unlock_irq(&mhi_cntrl->pm_lock);
 
 	wake_up_all(&mhi_cntrl->state_event);
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v1 09/10] bus: mhi: core: Move to an error state on mission mode failure

[Manivannan Sadhasivam]

On Fri, Sep 18, 2020 at 07:02:34PM -0700, Bhaumik Bhatt wrote:
> If the host receives a mission mode event and by the time it can get
> to processing it, the register accesses fail implying a connectivity
> error, MHI should move to an error state. This helps avoid longer wait
> times from a synchronous power up perspective and accurately reflects
> the MHI execution environment and power management states.
> 
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>

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

Thanks,
Mani

> ---
>  drivers/bus/mhi/core/pm.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> index bce1f62..16c04ab 100644
> --- a/drivers/bus/mhi/core/pm.c
> +++ b/drivers/bus/mhi/core/pm.c
> @@ -384,10 +384,14 @@ static int mhi_pm_mission_mode_transition(struct mhi_controller *mhi_cntrl)
>  	write_lock_irq(&mhi_cntrl->pm_lock);
>  	if (MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state))
>  		mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);
> -	write_unlock_irq(&mhi_cntrl->pm_lock);
>  
> -	if (!MHI_IN_MISSION_MODE(mhi_cntrl->ee))
> +	if (!MHI_IN_MISSION_MODE(mhi_cntrl->ee)) {
> +		mhi_cntrl->pm_state = MHI_PM_LD_ERR_FATAL_DETECT;
> +		write_unlock_irq(&mhi_cntrl->pm_lock);
> +		wake_up_all(&mhi_cntrl->state_event);
>  		return -EIO;
> +	}
> +	write_unlock_irq(&mhi_cntrl->pm_lock);
>  
>  	wake_up_all(&mhi_cntrl->state_event);
>  
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

[PATCH v1 10/10] bus: mhi: core: Mark device inactive soon after host issues a shutdown

[Bhaumik Bhatt]

Clients on the host may see the device in an active state for a short
period of time after the host detects a device error or power down.
Prevent any further host activity which could lead to race conditions
or multiple callbacks to the controller or any timeouts seen by
clients attempting to push data as they must be notified of the host
intent sooner rather than later. This also allows the host and device
states to be in sync with one another and prevents unnecessary host
operations.

Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
---
 drivers/bus/mhi/core/main.c |  4 ++++
 drivers/bus/mhi/core/pm.c   | 31 +++++++++++++++++++------------
 2 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index 1c8e332..5ec89e9 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -400,6 +400,10 @@ irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *priv)
 
 	 /* If device supports RDDM don't bother processing SYS error */
 	if (mhi_cntrl->rddm_image) {
+		/* host may be performing a device power down already */
+		if (!mhi_is_active(mhi_cntrl))
+			goto exit_intvec;
+
 		if (mhi_cntrl->ee == MHI_EE_RDDM && mhi_cntrl->ee != ee) {
 			/* prevent clients from queueing any more packets */
 			write_lock_irq(&mhi_cntrl->pm_lock);
diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
index 16c04ab..4e2cb41 100644
--- a/drivers/bus/mhi/core/pm.c
+++ b/drivers/bus/mhi/core/pm.c
@@ -469,15 +469,10 @@ static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl,
 	write_lock_irq(&mhi_cntrl->pm_lock);
 	prev_state = mhi_cntrl->pm_state;
 	cur_state = mhi_tryset_pm_state(mhi_cntrl, transition_state);
-	if (cur_state == transition_state) {
-		mhi_cntrl->ee = MHI_EE_DISABLE_TRANSITION;
+	if (cur_state == MHI_PM_SYS_ERR_PROCESS)
 		mhi_cntrl->dev_state = MHI_STATE_RESET;
-	}
 	write_unlock_irq(&mhi_cntrl->pm_lock);
 
-	/* Wake up threads waiting for state transition */
-	wake_up_all(&mhi_cntrl->state_event);
-
 	if (cur_state != transition_state) {
 		dev_err(dev, "Failed to transition to state: %s from: %s\n",
 			to_mhi_pm_state_str(transition_state),
@@ -486,6 +481,11 @@ static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl,
 		return;
 	}
 
+	mhi_cntrl->ee = MHI_EE_DISABLE_TRANSITION;
+
+	/* Wake up threads waiting for state transition */
+	wake_up_all(&mhi_cntrl->state_event);
+
 	/* Trigger MHI RESET so that the device will not access host memory */
 	if (MHI_REG_ACCESS_VALID(prev_state)) {
 		u32 in_reset = -1;
@@ -1051,22 +1051,29 @@ void mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful)
 	enum dev_st_transition next_state = DEV_ST_TRANSITION_DISABLE;
 	struct device *dev = &mhi_cntrl->mhi_dev->dev;
 
+	mutex_lock(&mhi_cntrl->pm_mutex);
+	write_lock_irq(&mhi_cntrl->pm_lock);
+
 	/* If it's not a graceful shutdown, force MHI to linkdown state */
 	if (!graceful) {
-		mutex_lock(&mhi_cntrl->pm_mutex);
-		write_lock_irq(&mhi_cntrl->pm_lock);
 		cur_state = mhi_tryset_pm_state(mhi_cntrl,
 						MHI_PM_LD_ERR_FATAL_DETECT);
-		write_unlock_irq(&mhi_cntrl->pm_lock);
-		mutex_unlock(&mhi_cntrl->pm_mutex);
-		if (cur_state != MHI_PM_LD_ERR_FATAL_DETECT)
+		if (cur_state != MHI_PM_LD_ERR_FATAL_DETECT) {
 			dev_dbg(dev, "Failed to move to state: %s from: %s\n",
 				to_mhi_pm_state_str(MHI_PM_LD_ERR_FATAL_DETECT),
 				to_mhi_pm_state_str(mhi_cntrl->pm_state));
-		else
+		} else {
 			next_state = DEV_ST_TRANSITION_FATAL;
+			wake_up_all(&mhi_cntrl->state_event);
+		}
 	}
 
+	/* mark device inactive to avoid any further host processing */
+	mhi_cntrl->dev_state = MHI_STATE_RESET;
+
+	write_unlock_irq(&mhi_cntrl->pm_lock);
+	mutex_unlock(&mhi_cntrl->pm_mutex);
+
 	mhi_queue_state_transition(mhi_cntrl, next_state);
 
 	/* Wait for shutdown to complete */
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v1 10/10] bus: mhi: core: Mark device inactive soon after host issues a shutdown

[Manivannan Sadhasivam]

On Fri, Sep 18, 2020 at 07:02:35PM -0700, Bhaumik Bhatt wrote:
> Clients on the host may see the device in an active state for a short

Clients? Are you referring client drivers or controllers? Please don't mix
the conventions.

> period of time after the host detects a device error or power down.
> Prevent any further host activity which could lead to race conditions
> or multiple callbacks to the controller or any timeouts seen by
> clients attempting to push data as they must be notified of the host
> intent sooner rather than later. This also allows the host and device
> states to be in sync with one another and prevents unnecessary host
> operations.
> 

How the change of dev_state is visible to the so called "client"?

> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> ---
>  drivers/bus/mhi/core/main.c |  4 ++++
>  drivers/bus/mhi/core/pm.c   | 31 +++++++++++++++++++------------
>  2 files changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> index 1c8e332..5ec89e9 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -400,6 +400,10 @@ irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *priv)
>  
>  	 /* If device supports RDDM don't bother processing SYS error */
>  	if (mhi_cntrl->rddm_image) {
> +		/* host may be performing a device power down already */
> +		if (!mhi_is_active(mhi_cntrl))
> +			goto exit_intvec;
> +

Does this change belong to this patch?

Thanks,
Mani

>  		if (mhi_cntrl->ee == MHI_EE_RDDM && mhi_cntrl->ee != ee) {
>  			/* prevent clients from queueing any more packets */
>  			write_lock_irq(&mhi_cntrl->pm_lock);
> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> index 16c04ab..4e2cb41 100644
> --- a/drivers/bus/mhi/core/pm.c
> +++ b/drivers/bus/mhi/core/pm.c
> @@ -469,15 +469,10 @@ static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl,
>  	write_lock_irq(&mhi_cntrl->pm_lock);
>  	prev_state = mhi_cntrl->pm_state;
>  	cur_state = mhi_tryset_pm_state(mhi_cntrl, transition_state);
> -	if (cur_state == transition_state) {
> -		mhi_cntrl->ee = MHI_EE_DISABLE_TRANSITION;
> +	if (cur_state == MHI_PM_SYS_ERR_PROCESS)
>  		mhi_cntrl->dev_state = MHI_STATE_RESET;
> -	}
>  	write_unlock_irq(&mhi_cntrl->pm_lock);
>  
> -	/* Wake up threads waiting for state transition */
> -	wake_up_all(&mhi_cntrl->state_event);
> -
>  	if (cur_state != transition_state) {
>  		dev_err(dev, "Failed to transition to state: %s from: %s\n",
>  			to_mhi_pm_state_str(transition_state),
> @@ -486,6 +481,11 @@ static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl,
>  		return;
>  	}
>  
> +	mhi_cntrl->ee = MHI_EE_DISABLE_TRANSITION;
> +
> +	/* Wake up threads waiting for state transition */
> +	wake_up_all(&mhi_cntrl->state_event);
> +
>  	/* Trigger MHI RESET so that the device will not access host memory */
>  	if (MHI_REG_ACCESS_VALID(prev_state)) {
>  		u32 in_reset = -1;
> @@ -1051,22 +1051,29 @@ void mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful)
>  	enum dev_st_transition next_state = DEV_ST_TRANSITION_DISABLE;
>  	struct device *dev = &mhi_cntrl->mhi_dev->dev;
>  
> +	mutex_lock(&mhi_cntrl->pm_mutex);
> +	write_lock_irq(&mhi_cntrl->pm_lock);
> +
>  	/* If it's not a graceful shutdown, force MHI to linkdown state */
>  	if (!graceful) {
> -		mutex_lock(&mhi_cntrl->pm_mutex);
> -		write_lock_irq(&mhi_cntrl->pm_lock);
>  		cur_state = mhi_tryset_pm_state(mhi_cntrl,
>  						MHI_PM_LD_ERR_FATAL_DETECT);
> -		write_unlock_irq(&mhi_cntrl->pm_lock);
> -		mutex_unlock(&mhi_cntrl->pm_mutex);
> -		if (cur_state != MHI_PM_LD_ERR_FATAL_DETECT)
> +		if (cur_state != MHI_PM_LD_ERR_FATAL_DETECT) {
>  			dev_dbg(dev, "Failed to move to state: %s from: %s\n",
>  				to_mhi_pm_state_str(MHI_PM_LD_ERR_FATAL_DETECT),
>  				to_mhi_pm_state_str(mhi_cntrl->pm_state));
> -		else
> +		} else {
>  			next_state = DEV_ST_TRANSITION_FATAL;
> +			wake_up_all(&mhi_cntrl->state_event);
> +		}
>  	}
>  
> +	/* mark device inactive to avoid any further host processing */
> +	mhi_cntrl->dev_state = MHI_STATE_RESET;
> +
> +	write_unlock_irq(&mhi_cntrl->pm_lock);
> +	mutex_unlock(&mhi_cntrl->pm_mutex);
> +
>  	mhi_queue_state_transition(mhi_cntrl, next_state);
>  
>  	/* Wait for shutdown to complete */
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH v1 10/10] bus: mhi: core: Mark device inactive soon after host issues a shutdown

[Manu Gautam]

On 9/19/2020 7:32 AM, Bhaumik Bhatt wrote:
> Clients on the host may see the device in an active state for a short
> period of time after the host detects a device error or power down.

What scenario is referred as 'device error' here?
And power down is the non-graceful power_down by controller?

> Prevent any further host activity which could lead to race conditions
> or multiple callbacks to the controller or any timeouts seen by
> clients attempting to push data as they must be notified of the host
> intent sooner rather than later. This also allows the host and device
> states to be in sync with one another and prevents unnecessary host
> operations.
>
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> ---
>  drivers/bus/mhi/core/main.c |  4 ++++
>  drivers/bus/mhi/core/pm.c   | 31 +++++++++++++++++++------------
>  2 files changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> index 1c8e332..5ec89e9 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -400,6 +400,10 @@ irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *priv)
>  
>  	 /* If device supports RDDM don't bother processing SYS error */
>  	if (mhi_cntrl->rddm_image) {
> +		/* host may be performing a device power down already */
> +		if (!mhi_is_active(mhi_cntrl))
> +			goto exit_intvec;
> +
>  		if (mhi_cntrl->ee == MHI_EE_RDDM && mhi_cntrl->ee != ee) {
>  			/* prevent clients from queueing any more packets */
>  			write_lock_irq(&mhi_cntrl->pm_lock);
> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> index 16c04ab..4e2cb41 100644
> --- a/drivers/bus/mhi/core/pm.c
> +++ b/drivers/bus/mhi/core/pm.c
> @@ -469,15 +469,10 @@ static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl,
>  	write_lock_irq(&mhi_cntrl->pm_lock);
>  	prev_state = mhi_cntrl->pm_state;
>  	cur_state = mhi_tryset_pm_state(mhi_cntrl, transition_state);
> -	if (cur_state == transition_state) {
> -		mhi_cntrl->ee = MHI_EE_DISABLE_TRANSITION;
> +	if (cur_state == MHI_PM_SYS_ERR_PROCESS)
>  		mhi_cntrl->dev_state = MHI_STATE_RESET;
> -	}
>  	write_unlock_irq(&mhi_cntrl->pm_lock);
>  
> -	/* Wake up threads waiting for state transition */
> -	wake_up_all(&mhi_cntrl->state_event);
> -
>  	if (cur_state != transition_state) {
>  		dev_err(dev, "Failed to transition to state: %s from: %s\n",
>  			to_mhi_pm_state_str(transition_state),
> @@ -486,6 +481,11 @@ static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl,
>  		return;
>  	}
>  
> +	mhi_cntrl->ee = MHI_EE_DISABLE_TRANSITION;
> +
> +	/* Wake up threads waiting for state transition */
> +	wake_up_all(&mhi_cntrl->state_event);
> +
>  	/* Trigger MHI RESET so that the device will not access host memory */
>  	if (MHI_REG_ACCESS_VALID(prev_state)) {
>  		u32 in_reset = -1;
> @@ -1051,22 +1051,29 @@ void mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful)
>  	enum dev_st_transition next_state = DEV_ST_TRANSITION_DISABLE;
>  	struct device *dev = &mhi_cntrl->mhi_dev->dev;
>  
> +	mutex_lock(&mhi_cntrl->pm_mutex);
> +	write_lock_irq(&mhi_cntrl->pm_lock);
> +
>  	/* If it's not a graceful shutdown, force MHI to linkdown state */
>  	if (!graceful) {
> -		mutex_lock(&mhi_cntrl->pm_mutex);
> -		write_lock_irq(&mhi_cntrl->pm_lock);
>  		cur_state = mhi_tryset_pm_state(mhi_cntrl,
>  						MHI_PM_LD_ERR_FATAL_DETECT);
> -		write_unlock_irq(&mhi_cntrl->pm_lock);
> -		mutex_unlock(&mhi_cntrl->pm_mutex);
> -		if (cur_state != MHI_PM_LD_ERR_FATAL_DETECT)
> +		if (cur_state != MHI_PM_LD_ERR_FATAL_DETECT) {
>  			dev_dbg(dev, "Failed to move to state: %s from: %s\n",
>  				to_mhi_pm_state_str(MHI_PM_LD_ERR_FATAL_DETECT),
>  				to_mhi_pm_state_str(mhi_cntrl->pm_state));
> -		else
> +		} else {
>  			next_state = DEV_ST_TRANSITION_FATAL;
> +			wake_up_all(&mhi_cntrl->state_event);
> +		}
>  	}
>  
> +	/* mark device inactive to avoid any further host processing */
> +	mhi_cntrl->dev_state = MHI_STATE_RESET;
> +
> +	write_unlock_irq(&mhi_cntrl->pm_lock);
> +	mutex_unlock(&mhi_cntrl->pm_mutex);
> +
>  	mhi_queue_state_transition(mhi_cntrl, next_state);
>  
>  	/* Wait for shutdown to complete */

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