On Wed, Feb 24, 2021 at 03:23:04PM -0800, Bhaumik Bhatt wrote:
> In current design, whenever the BHI interrupt is fired, the
> execution environment is updated. This can cause race conditions
> and impede ongoing power up/down processing. For example, if a
> power down is in progress, MHI host updates to a local "disabled"
> execution environment. If a BHI interrupt fires later, that value
> gets replaced with one from the BHI EE register. This impacts the
> controller as it does not expect multiple RDDM execution
> environment change status callbacks as an example. Another issue
> would be that the device can enter mission mode and the execution
> environment is updated, while device creation for SBL channels is
> still going on due to slower PM state worker thread run, 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 modes or RDDM EE
> changes as critical events directly from the interrupt handler.
> Simplify handling by waiting for SYS ERROR before handling RDDM.
> 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 <bbh...@codeaurora.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasi...@linaro.org>
Thanks,
Mani
> ---
> drivers/bus/mhi/core/main.c | 40 +++++++++++++++++++++-------------------
> drivers/bus/mhi/core/pm.c | 7 ++++---
> 2 files changed, 25 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> index 7a2e98c..9715f51 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -430,7 +430,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)) {
> @@ -439,8 +439,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));
> @@ -452,27 +451,30 @@ irqreturn_t mhi_intvec_threaded_handler(int irq_number,
> void *priv)
> }
> write_unlock_irq(&mhi_cntrl->pm_lock);
>
> - /* 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 (pm_state != MHI_PM_SYS_ERR_DETECT || ee == mhi_cntrl->ee)
> + goto exit_intvec;
>
> - if (mhi_cntrl->ee == MHI_EE_RDDM && mhi_cntrl->ee != ee) {
> + switch (ee) {
> + case MHI_EE_RDDM:
> + /* proceed if power down is not already in progress */
> + if (mhi_cntrl->rddm_image && mhi_is_active(mhi_cntrl)) {
> mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_EE_RDDM);
> + mhi_cntrl->ee = ee;
> wake_up_all(&mhi_cntrl->state_event);
> }
> - goto exit_intvec;
> - }
> -
> - if (pm_state == MHI_PM_SYS_ERR_DETECT) {
> + break;
> + case MHI_EE_PBL:
> + case MHI_EE_EDL:
> + case MHI_EE_PTHRU:
> + mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_FATAL_ERROR);
> + mhi_cntrl->ee = ee;
> wake_up_all(&mhi_cntrl->state_event);
> -
> - /* For fatal errors, we let controller decide next step */
> - if (MHI_IN_PBL(ee))
> - mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_FATAL_ERROR);
> - else
> - mhi_pm_sys_err_handler(mhi_cntrl);
> + mhi_pm_sys_err_handler(mhi_cntrl);
> + break;
> + default:
> + wake_up_all(&mhi_cntrl->state_event);
> + mhi_pm_sys_err_handler(mhi_cntrl);
> + break;
> }
>
> exit_intvec:
> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> index c09ec13..c870fa8 100644
> --- a/drivers/bus/mhi/core/pm.c
> +++ b/drivers/bus/mhi/core/pm.c
> @@ -377,21 +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 current_ee = mhi_cntrl->ee;
> + enum mhi_ee_type ee = MHI_EE_MAX, current_ee = mhi_cntrl->ee;
> 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
>
