diff mbox series

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

Message ID 1600480955-16827-11-git-send-email-bbhatt@codeaurora.org (mailing list archive)
State Superseded
Headers show
Series Bug fixes and improvements for MHI power operations | expand

Commit Message

Bhaumik Bhatt Sept. 19, 2020, 2:02 a.m. UTC 
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(-)

Comments

Manivannan Sadhasivam Oct. 9, 2020, 5:18 p.m. UTC | #1 
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
>
Manu Gautam Oct. 11, 2020, 12:17 a.m. UTC | #2 
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 */
diff mbox series

Patch

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 */