diff mbox series

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

Message ID 1600480955-16827-8-git-send-email-bbhatt@codeaurora.org
State New
Headers show
Series Bug fixes and improvements for MHI power operations | expand

Commit Message

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

Comments

Manivannan Sadhasivam Oct. 9, 2020, 4:32 p.m. UTC | #1 
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
>
Bhaumik Bhatt Oct. 14, 2020, 1:06 a.m. UTC | #2 
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
>>
diff mbox series

Patch

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;
 			}