| Message ID | 1600480955-16827-11-git-send-email-bbhatt@codeaurora.org |
|---|---|
| State | New |
| Headers | show |
| Series | Bug fixes and improvements for MHI power operations | expand |
On Fri, Sep 18, 2020 at 07:02:35PM -0700, Bhaumik Bhatt wrote: > Clients on the host may see the device in an active state for a short Clients? Are you referring client drivers or controllers? Please don't mix the conventions. > period of time after the host detects a device error or power down. > Prevent any further host activity which could lead to race conditions > or multiple callbacks to the controller or any timeouts seen by > clients attempting to push data as they must be notified of the host > intent sooner rather than later. This also allows the host and device > states to be in sync with one another and prevents unnecessary host > operations. > How the change of dev_state is visible to the so called "client"? > Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org> > --- > drivers/bus/mhi/core/main.c | 4 ++++ > drivers/bus/mhi/core/pm.c | 31 +++++++++++++++++++------------ > 2 files changed, 23 insertions(+), 12 deletions(-) > > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c > index 1c8e332..5ec89e9 100644 > --- a/drivers/bus/mhi/core/main.c > +++ b/drivers/bus/mhi/core/main.c > @@ -400,6 +400,10 @@ 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) { > + /* host may be performing a device power down already */ > + if (!mhi_is_active(mhi_cntrl)) > + goto exit_intvec; > + Does this change belong to this patch? Thanks, Mani > 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); > diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c > index 16c04ab..4e2cb41 100644 > --- a/drivers/bus/mhi/core/pm.c > +++ b/drivers/bus/mhi/core/pm.c > @@ -469,15 +469,10 @@ static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl, > write_lock_irq(&mhi_cntrl->pm_lock); > prev_state = mhi_cntrl->pm_state; > cur_state = mhi_tryset_pm_state(mhi_cntrl, transition_state); > - if (cur_state == transition_state) { > - mhi_cntrl->ee = MHI_EE_DISABLE_TRANSITION; > + if (cur_state == MHI_PM_SYS_ERR_PROCESS) > mhi_cntrl->dev_state = MHI_STATE_RESET; > - } > write_unlock_irq(&mhi_cntrl->pm_lock); > > - /* Wake up threads waiting for state transition */ > - wake_up_all(&mhi_cntrl->state_event); > - > if (cur_state != transition_state) { > dev_err(dev, "Failed to transition to state: %s from: %s\n", > to_mhi_pm_state_str(transition_state), > @@ -486,6 +481,11 @@ static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl, > return; > } > > + mhi_cntrl->ee = MHI_EE_DISABLE_TRANSITION; > + > + /* Wake up threads waiting for state transition */ > + wake_up_all(&mhi_cntrl->state_event); > + > /* Trigger MHI RESET so that the device will not access host memory */ > if (MHI_REG_ACCESS_VALID(prev_state)) { > u32 in_reset = -1; > @@ -1051,22 +1051,29 @@ void mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful) > enum dev_st_transition next_state = DEV_ST_TRANSITION_DISABLE; > struct device *dev = &mhi_cntrl->mhi_dev->dev; > > + mutex_lock(&mhi_cntrl->pm_mutex); > + write_lock_irq(&mhi_cntrl->pm_lock); > + > /* If it's not a graceful shutdown, force MHI to linkdown state */ > if (!graceful) { > - mutex_lock(&mhi_cntrl->pm_mutex); > - write_lock_irq(&mhi_cntrl->pm_lock); > cur_state = mhi_tryset_pm_state(mhi_cntrl, > MHI_PM_LD_ERR_FATAL_DETECT); > - write_unlock_irq(&mhi_cntrl->pm_lock); > - mutex_unlock(&mhi_cntrl->pm_mutex); > - if (cur_state != MHI_PM_LD_ERR_FATAL_DETECT) > + if (cur_state != MHI_PM_LD_ERR_FATAL_DETECT) { > dev_dbg(dev, "Failed to move to state: %s from: %s\n", > to_mhi_pm_state_str(MHI_PM_LD_ERR_FATAL_DETECT), > to_mhi_pm_state_str(mhi_cntrl->pm_state)); > - else > + } else { > next_state = DEV_ST_TRANSITION_FATAL; > + wake_up_all(&mhi_cntrl->state_event); > + } > } > > + /* mark device inactive to avoid any further host processing */ > + mhi_cntrl->dev_state = MHI_STATE_RESET; > + > + write_unlock_irq(&mhi_cntrl->pm_lock); > + mutex_unlock(&mhi_cntrl->pm_mutex); > + > mhi_queue_state_transition(mhi_cntrl, next_state); > > /* Wait for shutdown to complete */ > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project >
On 9/19/2020 7:32 AM, Bhaumik Bhatt wrote: > Clients on the host may see the device in an active state for a short > period of time after the host detects a device error or power down. What scenario is referred as 'device error' here? And power down is the non-graceful power_down by controller? > Prevent any further host activity which could lead to race conditions > or multiple callbacks to the controller or any timeouts seen by > clients attempting to push data as they must be notified of the host > intent sooner rather than later. This also allows the host and device > states to be in sync with one another and prevents unnecessary host > operations. > > Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org> > --- > drivers/bus/mhi/core/main.c | 4 ++++ > drivers/bus/mhi/core/pm.c | 31 +++++++++++++++++++------------ > 2 files changed, 23 insertions(+), 12 deletions(-) > > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c > index 1c8e332..5ec89e9 100644 > --- a/drivers/bus/mhi/core/main.c > +++ b/drivers/bus/mhi/core/main.c > @@ -400,6 +400,10 @@ 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) { > + /* host may be performing a device power down already */ > + if (!mhi_is_active(mhi_cntrl)) > + goto exit_intvec; > + > 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); > diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c > index 16c04ab..4e2cb41 100644 > --- a/drivers/bus/mhi/core/pm.c > +++ b/drivers/bus/mhi/core/pm.c > @@ -469,15 +469,10 @@ static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl, > write_lock_irq(&mhi_cntrl->pm_lock); > prev_state = mhi_cntrl->pm_state; > cur_state = mhi_tryset_pm_state(mhi_cntrl, transition_state); > - if (cur_state == transition_state) { > - mhi_cntrl->ee = MHI_EE_DISABLE_TRANSITION; > + if (cur_state == MHI_PM_SYS_ERR_PROCESS) > mhi_cntrl->dev_state = MHI_STATE_RESET; > - } > write_unlock_irq(&mhi_cntrl->pm_lock); > > - /* Wake up threads waiting for state transition */ > - wake_up_all(&mhi_cntrl->state_event); > - > if (cur_state != transition_state) { > dev_err(dev, "Failed to transition to state: %s from: %s\n", > to_mhi_pm_state_str(transition_state), > @@ -486,6 +481,11 @@ static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl, > return; > } > > + mhi_cntrl->ee = MHI_EE_DISABLE_TRANSITION; > + > + /* Wake up threads waiting for state transition */ > + wake_up_all(&mhi_cntrl->state_event); > + > /* Trigger MHI RESET so that the device will not access host memory */ > if (MHI_REG_ACCESS_VALID(prev_state)) { > u32 in_reset = -1; > @@ -1051,22 +1051,29 @@ void mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful) > enum dev_st_transition next_state = DEV_ST_TRANSITION_DISABLE; > struct device *dev = &mhi_cntrl->mhi_dev->dev; > > + mutex_lock(&mhi_cntrl->pm_mutex); > + write_lock_irq(&mhi_cntrl->pm_lock); > + > /* If it's not a graceful shutdown, force MHI to linkdown state */ > if (!graceful) { > - mutex_lock(&mhi_cntrl->pm_mutex); > - write_lock_irq(&mhi_cntrl->pm_lock); > cur_state = mhi_tryset_pm_state(mhi_cntrl, > MHI_PM_LD_ERR_FATAL_DETECT); > - write_unlock_irq(&mhi_cntrl->pm_lock); > - mutex_unlock(&mhi_cntrl->pm_mutex); > - if (cur_state != MHI_PM_LD_ERR_FATAL_DETECT) > + if (cur_state != MHI_PM_LD_ERR_FATAL_DETECT) { > dev_dbg(dev, "Failed to move to state: %s from: %s\n", > to_mhi_pm_state_str(MHI_PM_LD_ERR_FATAL_DETECT), > to_mhi_pm_state_str(mhi_cntrl->pm_state)); > - else > + } else { > next_state = DEV_ST_TRANSITION_FATAL; > + wake_up_all(&mhi_cntrl->state_event); > + } > } > > + /* mark device inactive to avoid any further host processing */ > + mhi_cntrl->dev_state = MHI_STATE_RESET; > + > + write_unlock_irq(&mhi_cntrl->pm_lock); > + mutex_unlock(&mhi_cntrl->pm_mutex); > + > mhi_queue_state_transition(mhi_cntrl, next_state); > > /* Wait for shutdown to complete */ -- 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 1c8e332..5ec89e9 100644 --- a/drivers/bus/mhi/core/main.c +++ b/drivers/bus/mhi/core/main.c @@ -400,6 +400,10 @@ 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) { + /* host may be performing a device power down already */ + if (!mhi_is_active(mhi_cntrl)) + goto exit_intvec; + 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); diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c index 16c04ab..4e2cb41 100644 --- a/drivers/bus/mhi/core/pm.c +++ b/drivers/bus/mhi/core/pm.c @@ -469,15 +469,10 @@ static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl, write_lock_irq(&mhi_cntrl->pm_lock); prev_state = mhi_cntrl->pm_state; cur_state = mhi_tryset_pm_state(mhi_cntrl, transition_state); - if (cur_state == transition_state) { - mhi_cntrl->ee = MHI_EE_DISABLE_TRANSITION; + if (cur_state == MHI_PM_SYS_ERR_PROCESS) mhi_cntrl->dev_state = MHI_STATE_RESET; - } write_unlock_irq(&mhi_cntrl->pm_lock); - /* Wake up threads waiting for state transition */ - wake_up_all(&mhi_cntrl->state_event); - if (cur_state != transition_state) { dev_err(dev, "Failed to transition to state: %s from: %s\n", to_mhi_pm_state_str(transition_state), @@ -486,6 +481,11 @@ static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl, return; } + mhi_cntrl->ee = MHI_EE_DISABLE_TRANSITION; + + /* Wake up threads waiting for state transition */ + wake_up_all(&mhi_cntrl->state_event); + /* Trigger MHI RESET so that the device will not access host memory */ if (MHI_REG_ACCESS_VALID(prev_state)) { u32 in_reset = -1; @@ -1051,22 +1051,29 @@ void mhi_power_down(struct mhi_controller *mhi_cntrl, bool graceful) enum dev_st_transition next_state = DEV_ST_TRANSITION_DISABLE; struct device *dev = &mhi_cntrl->mhi_dev->dev; + mutex_lock(&mhi_cntrl->pm_mutex); + write_lock_irq(&mhi_cntrl->pm_lock); + /* If it's not a graceful shutdown, force MHI to linkdown state */ if (!graceful) { - mutex_lock(&mhi_cntrl->pm_mutex); - write_lock_irq(&mhi_cntrl->pm_lock); cur_state = mhi_tryset_pm_state(mhi_cntrl, MHI_PM_LD_ERR_FATAL_DETECT); - write_unlock_irq(&mhi_cntrl->pm_lock); - mutex_unlock(&mhi_cntrl->pm_mutex); - if (cur_state != MHI_PM_LD_ERR_FATAL_DETECT) + if (cur_state != MHI_PM_LD_ERR_FATAL_DETECT) { dev_dbg(dev, "Failed to move to state: %s from: %s\n", to_mhi_pm_state_str(MHI_PM_LD_ERR_FATAL_DETECT), to_mhi_pm_state_str(mhi_cntrl->pm_state)); - else + } else { next_state = DEV_ST_TRANSITION_FATAL; + wake_up_all(&mhi_cntrl->state_event); + } } + /* mark device inactive to avoid any further host processing */ + mhi_cntrl->dev_state = MHI_STATE_RESET; + + write_unlock_irq(&mhi_cntrl->pm_lock); + mutex_unlock(&mhi_cntrl->pm_mutex); + mhi_queue_state_transition(mhi_cntrl, next_state); /* Wait for shutdown to complete */
Clients on the host may see the device in an active state for a short period of time after the host detects a device error or power down. Prevent any further host activity which could lead to race conditions or multiple callbacks to the controller or any timeouts seen by clients attempting to push data as they must be notified of the host intent sooner rather than later. This also allows the host and device states to be in sync with one another and prevents unnecessary host operations. Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org> --- drivers/bus/mhi/core/main.c | 4 ++++ drivers/bus/mhi/core/pm.c | 31 +++++++++++++++++++------------ 2 files changed, 23 insertions(+), 12 deletions(-)