RE: [PATCH v1 2/2] bus: mhi: core: Process execution environment changes serially

[Carl Yin(殷张成)]

Hi bbhat:

On November 17, 2020 10:58 AM, bbhatt wrote:
> In current design, whenever the BHI interrupt is fired, the execution
> environment is updated. This can cause race conditions and impede any ongoing
> power up/down processing. For example, if a power down is in progress and the
> host has updated the execution environment to a local "disabled" state, any BHI
> interrupt firing later could replace it with the value from the BHI EE register.
> Another example would be that the device can enter mission mode while device
> creation for SBL is still going on, leading to multiple attempts at opening the same
> channel.
> 
> Ensure that EE changes are handled only from appropriate places and occur one
> after another and handle only PBL or RDDM EE changes as critical events directly
> from the interrupt handler. This also makes sure that we use the correct
> execution environment to notify the controller driver when the device resets to
> one of the PBL execution environments.
> 
> Signed-off-by: Bhaumik Bhatt <bbhatt@xxxxxxxxxxxxxx>
> ---
>  drivers/bus/mhi/core/main.c | 14 ++++++++------
>  drivers/bus/mhi/core/pm.c   |  6 ++++--
>  2 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c index
> 4eb93d8..cd712f2 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -377,7 +377,7 @@ irqreturn_t mhi_intvec_threaded_handler(int
> irq_number, void *priv)
>  	struct device *dev = &mhi_cntrl->mhi_dev->dev;
>  	enum mhi_state state = MHI_STATE_MAX;
>  	enum mhi_pm_state pm_state = 0;
> -	enum mhi_ee_type ee = 0;
> +	enum mhi_ee_type ee = MHI_EE_MAX;
> 
>  	write_lock_irq(&mhi_cntrl->pm_lock);
>  	if (!MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) { @@ -386,8 +386,7
> @@ irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *priv)
>  	}
> 
>  	state = mhi_get_mhi_state(mhi_cntrl);
> -	ee = mhi_cntrl->ee;
> -	mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);
> +	ee = mhi_get_exec_env(mhi_cntrl);
>  	dev_dbg(dev, "local ee:%s device ee:%s dev_state:%s\n",
>  		TO_MHI_EXEC_STR(mhi_cntrl->ee), TO_MHI_EXEC_STR(ee),
>  		TO_MHI_STATE_STR(state));
> @@ -405,8 +404,9 @@ irqreturn_t mhi_intvec_threaded_handler(int
> irq_number, void *priv)
>  		if (!mhi_is_active(mhi_cntrl))
>  			goto exit_intvec;
> 
> -		if (mhi_cntrl->ee == MHI_EE_RDDM && mhi_cntrl->ee != ee) {
> +		if (ee == MHI_EE_RDDM && mhi_cntrl->ee != MHI_EE_RDDM) {
>  			mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_EE_RDDM);
> +			mhi_cntrl->ee = ee;
>  			wake_up_all(&mhi_cntrl->state_event);
>  		}
>  		goto exit_intvec;
> @@ -416,10 +416,12 @@ irqreturn_t mhi_intvec_threaded_handler(int
> irq_number, void *priv)
>  		wake_up_all(&mhi_cntrl->state_event);
> 
>  		/* For fatal errors, we let controller decide next step */
> -		if (MHI_IN_PBL(ee))
> +		if (MHI_IN_PBL(ee)) {
>  			mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_FATAL_ERROR);
> -		else
> +			mhi_cntrl->ee = ee;
> +		} else {
>  			mhi_pm_sys_err_handler(mhi_cntrl);
> +		}
>  	}
> 
[carl.yin] I guess this patch can fix another bug (maybe is not bug)
For hw event with brst mode enabled. If M0 event come before AMSS event, as next log:
mhi_pm_m0_transition() will call mhi_ring_cmd_db(), but ring->wp not correctly set
until the later mhi_pm_mission_mode_transition() set ring->wp.

[ 1550.901882] mhi 0000:03:00.0: Requested to power ON
[ 1550.902065] mhi 0000:03:00.0: Power on setup success
[ 1550.902256] mhi 0000:03:00.0: Handling state transition: PBL
[ 1551.751325] pci 0000:01:00.0: PCI bridge to [bus 02]
[ 1556.113314] mhi 0000:03:00.0: Device in READY State
[ 1556.113319] mhi 0000:03:00.0: Initializing MHI registers
[ 1559.391644] mhi 0000:03:00.0: local ee:AMSS device ee:PASS THRU dev_state:READY
[ 1559.410969] mhi 0000:03:00.0: State change event to state: M0
[ 1559.414807] mhi 0000:03:00.0: Received EE event: AMSS
[ 1559.414860] mhi 0000:03:00.0: Handling state transition: MISSION_MODE
[ 1559.414863] mhi 0000:03:00.0: Processing Mission Mode transition

>  exit_intvec:
> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c index
> aa156b9..c1b18d6 100644
> --- a/drivers/bus/mhi/core/pm.c
> +++ b/drivers/bus/mhi/core/pm.c
> @@ -377,20 +377,22 @@ static int mhi_pm_mission_mode_transition(struct
> mhi_controller *mhi_cntrl)  {
>  	struct mhi_event *mhi_event;
>  	struct device *dev = &mhi_cntrl->mhi_dev->dev;
> +	enum mhi_ee_type ee = MHI_EE_MAX;
>  	int i, ret;
> 
>  	dev_dbg(dev, "Processing Mission Mode transition\n");
> 
>  	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);
> +		ee = mhi_get_exec_env(mhi_cntrl);
> 
> -	if (!MHI_IN_MISSION_MODE(mhi_cntrl->ee)) {
> +	if (!MHI_IN_MISSION_MODE(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;
>  	}
> +	mhi_cntrl->ee = ee;
>  	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