| Message ID | 1600480955-16827-8-git-send-email-bbhatt@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Bug fixes and improvements for MHI power operations | expand |
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 >
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 --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; }
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(-)