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