Re: [PATCH v3 10/12] bus: mhi: core: Separate system error and power down handling

From: Bhaumik Bhatt
Date: Mon Nov 02 2020 - 11:53:18 EST

Next message: Vaibhav Gupta: "[PATCH v4 16/29] scsi: mpt3sas_scsih: use generic power management"
Previous message: Vaibhav Gupta: "[PATCH v4 14/29] scsi: hisi_sas_v3_hw: Remove extra function calls for runtime pm"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Mani,

On 2020-10-30 23:54, Manivannan Sadhasivam wrote:

Hi Bhaumik,

On 31 October 2020 1:04:07 AM IST, Bhaumik Bhatt <bbhatt@xxxxxxxxxxxxxx> wrote:
Hi Mani,

On 2020-10-30 07:06, Manivannan Sadhasivam wrote:

On Thu, Oct 29, 2020 at 09:10:55PM -0700, Bhaumik Bhatt wrote:

Currently, there exist a set of if...else statements in the
mhi_pm_disable_transition() function which make handling system
error and disable transitions differently complex. To make that
cleaner and facilitate differences in behavior, separate these
two transitions for MHI host.

And this results in a lot of duplicated code :/

Thanks,
Mani

I knew this was coming. Mainly, we can avoid adding confusing if...else
statements that plague the current mhi_pm_disable_transition() function

and in
return for some duplicate code, we can make handling separate use cases

easier
as they could pop-up anytime in the future.

If that happens then do it but now, please no.

Thanks,
Mani

It had to be done for 11/12 and 12/12 (last two) patches of the series. It's a
much cleaner approach and allows us to handle states better. We should be moving
the dev_state and EE to "Reset" and "Disable" states soon enough when a shutdown
is initiated and we can resolve bugs such as freeing the IRQs for a shutdown
sooner as well. Since these differences in shutdown versus SYS_ERROR processing
are already increasingly apparent and more could come, this patch had to step
in.

bus: mhi: core: Mark and maintain device states early on after power down
bus: mhi: core: Remove MHI event ring IRQ handlers when powering down

Signed-off-by: Bhaumik Bhatt <bbhatt@xxxxxxxxxxxxxx>
---
drivers/bus/mhi/core/pm.c | 159
+++++++++++++++++++++++++++++++++++++++-------
1 file changed, 137 insertions(+), 22 deletions(-)

diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
index 1d04e401..347ae7d 100644
--- a/drivers/bus/mhi/core/pm.c
+++ b/drivers/bus/mhi/core/pm.c
@@ -444,7 +444,7 @@ static int mhi_pm_mission_mode_transition(struct

mhi_controller *mhi_cntrl)
return ret;
}

-/* Handle SYS_ERR and Shutdown transitions */
+/* Handle shutdown transitions */
static void mhi_pm_disable_transition(struct mhi_controller
*mhi_cntrl,
enum mhi_pm_state transition_state)
{
@@ -460,10 +460,6 @@ static void mhi_pm_disable_transition(struct
mhi_controller *mhi_cntrl,
to_mhi_pm_state_str(mhi_cntrl->pm_state),
to_mhi_pm_state_str(transition_state));

- /* We must notify MHI control driver so it can clean up first */
- if (transition_state == MHI_PM_SYS_ERR_PROCESS)
- mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_SYS_ERROR);
-
mutex_lock(&mhi_cntrl->pm_mutex);
write_lock_irq(&mhi_cntrl->pm_lock);
prev_state = mhi_cntrl->pm_state;
@@ -502,11 +498,8 @@ static void mhi_pm_disable_transition(struct
mhi_controller *mhi_cntrl,
MHICTRL_RESET_SHIFT,
&in_reset) ||
!in_reset, timeout);
- if ((!ret || in_reset) && cur_state == MHI_PM_SYS_ERR_PROCESS) {
+ if (!ret || in_reset)
dev_err(dev, "Device failed to exit MHI Reset state\n");
- mutex_unlock(&mhi_cntrl->pm_mutex);
- return;
- }

/*
* Device will clear BHI_INTVEC as a part of RESET processing,
@@ -566,19 +559,142 @@ static void mhi_pm_disable_transition(struct
mhi_controller *mhi_cntrl,
er_ctxt->wp = er_ctxt->rbase;
}

- if (cur_state == MHI_PM_SYS_ERR_PROCESS) {
- mhi_ready_state_transition(mhi_cntrl);
- } else {
- /* Move to disable state */
- write_lock_irq(&mhi_cntrl->pm_lock);
- cur_state = mhi_tryset_pm_state(mhi_cntrl, MHI_PM_DISABLE);
- write_unlock_irq(&mhi_cntrl->pm_lock);
- if (unlikely(cur_state != MHI_PM_DISABLE))
- dev_err(dev, "Error moving from PM state: %s to: %s\n",
- to_mhi_pm_state_str(cur_state),
- to_mhi_pm_state_str(MHI_PM_DISABLE));
+ /* Move to disable state */
+ write_lock_irq(&mhi_cntrl->pm_lock);
+ cur_state = mhi_tryset_pm_state(mhi_cntrl, MHI_PM_DISABLE);
+ write_unlock_irq(&mhi_cntrl->pm_lock);
+ if (unlikely(cur_state != MHI_PM_DISABLE))
+ dev_err(dev, "Error moving from PM state: %s to: %s\n",
+ to_mhi_pm_state_str(cur_state),
+ to_mhi_pm_state_str(MHI_PM_DISABLE));
+
+ dev_dbg(dev, "Exiting with PM state: %s, MHI state: %s\n",
+ to_mhi_pm_state_str(mhi_cntrl->pm_state),
+ TO_MHI_STATE_STR(mhi_cntrl->dev_state));
+
+ mutex_unlock(&mhi_cntrl->pm_mutex);
+}
+
+/* Handle system error transitions */
+static void mhi_pm_sys_error_transition(struct mhi_controller
*mhi_cntrl)
+{
+ enum mhi_pm_state cur_state, prev_state;
+ struct mhi_event *mhi_event;
+ struct mhi_cmd_ctxt *cmd_ctxt;
+ struct mhi_cmd *mhi_cmd;
+ struct mhi_event_ctxt *er_ctxt;
+ struct device *dev = &mhi_cntrl->mhi_dev->dev;
+ int ret, i;
+
+ dev_dbg(dev, "Transitioning from PM state: %s to: %s\n",
+ to_mhi_pm_state_str(mhi_cntrl->pm_state),
+ to_mhi_pm_state_str(MHI_PM_SYS_ERR_PROCESS));
+
+ /* We must notify MHI control driver so it can clean up first */
+ mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_SYS_ERROR);
+
+ mutex_lock(&mhi_cntrl->pm_mutex);
+ write_lock_irq(&mhi_cntrl->pm_lock);
+ prev_state = mhi_cntrl->pm_state;
+ cur_state = mhi_tryset_pm_state(mhi_cntrl,

MHI_PM_SYS_ERR_PROCESS);

+ write_unlock_irq(&mhi_cntrl->pm_lock);
+
+ if (cur_state != MHI_PM_SYS_ERR_PROCESS) {
+ dev_err(dev, "Failed to transition from PM state: %s to: %s\n",
+ to_mhi_pm_state_str(cur_state),
+ to_mhi_pm_state_str(MHI_PM_SYS_ERR_PROCESS));
+ goto exit_sys_error_transition;
+ }
+
+ mhi_cntrl->ee = MHI_EE_DISABLE_TRANSITION;
+ mhi_cntrl->dev_state = MHI_STATE_RESET;
+
+ /* 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;
+ unsigned long timeout = msecs_to_jiffies(mhi_cntrl->timeout_ms);
+
+ dev_dbg(dev, "Triggering MHI Reset in device\n");
+ mhi_set_mhi_state(mhi_cntrl, MHI_STATE_RESET);
+
+ /* Wait for the reset bit to be cleared by the device */
+ ret = wait_event_timeout(mhi_cntrl->state_event,
+ mhi_read_reg_field(mhi_cntrl,
+ mhi_cntrl->regs,
+ MHICTRL,
+ MHICTRL_RESET_MASK,
+ MHICTRL_RESET_SHIFT,
+ &in_reset) ||
+ !in_reset, timeout);
+ if (!ret || in_reset) {
+ dev_err(dev, "Device failed to exit MHI Reset state\n");
+ goto exit_sys_error_transition;
+ }
+
+ /*
+ * Device will clear BHI_INTVEC as a part of RESET processing,
+ * hence re-program it
+ */
+ mhi_write_reg(mhi_cntrl, mhi_cntrl->bhi, BHI_INTVEC, 0);
+ }
+
+ dev_dbg(dev,
+ "Waiting for all pending event ring processing to complete\n");
+ mhi_event = mhi_cntrl->mhi_event;
+ for (i = 0; i < mhi_cntrl->total_ev_rings; i++, mhi_event++) {
+ if (mhi_event->offload_ev)
+ continue;
+ tasklet_kill(&mhi_event->task);
}

+ /* Release lock and wait for all pending threads to complete */
+ mutex_unlock(&mhi_cntrl->pm_mutex);
+ dev_dbg(dev, "Waiting for all pending threads to complete\n");
+ wake_up_all(&mhi_cntrl->state_event);
+
+ dev_dbg(dev, "Reset all active channels and remove MHI

devices\n");

+ device_for_each_child(mhi_cntrl->cntrl_dev, NULL,
mhi_destroy_device);
+
+ mutex_lock(&mhi_cntrl->pm_mutex);
+
+ WARN_ON(atomic_read(&mhi_cntrl->dev_wake));
+ WARN_ON(atomic_read(&mhi_cntrl->pending_pkts));
+
+ /* Reset the ev rings and cmd rings */
+ dev_dbg(dev, "Resetting EV CTXT and CMD CTXT\n");
+ mhi_cmd = mhi_cntrl->mhi_cmd;
+ cmd_ctxt = mhi_cntrl->mhi_ctxt->cmd_ctxt;
+ for (i = 0; i < NR_OF_CMD_RINGS; i++, mhi_cmd++, cmd_ctxt++) {
+ struct mhi_ring *ring = &mhi_cmd->ring;
+
+ ring->rp = ring->base;
+ ring->wp = ring->base;
+ cmd_ctxt->rp = cmd_ctxt->rbase;
+ cmd_ctxt->wp = cmd_ctxt->rbase;
+ }
+
+ mhi_event = mhi_cntrl->mhi_event;
+ er_ctxt = mhi_cntrl->mhi_ctxt->er_ctxt;
+ for (i = 0; i < mhi_cntrl->total_ev_rings; i++, er_ctxt++,
+ mhi_event++) {
+ struct mhi_ring *ring = &mhi_event->ring;
+
+ /* Skip offload events */
+ if (mhi_event->offload_ev)
+ continue;
+
+ ring->rp = ring->base;
+ ring->wp = ring->base;
+ er_ctxt->rp = er_ctxt->rbase;
+ er_ctxt->wp = er_ctxt->rbase;
+ }
+
+ mhi_ready_state_transition(mhi_cntrl);
+
+exit_sys_error_transition:
dev_dbg(dev, "Exiting with PM state: %s, MHI state: %s\n",
to_mhi_pm_state_str(mhi_cntrl->pm_state),
TO_MHI_STATE_STR(mhi_cntrl->dev_state));
@@ -666,8 +782,7 @@ void mhi_pm_st_worker(struct work_struct *work)
mhi_ready_state_transition(mhi_cntrl);
break;
case DEV_ST_TRANSITION_SYS_ERR:
- mhi_pm_disable_transition
- (mhi_cntrl, MHI_PM_SYS_ERR_PROCESS);
+ mhi_pm_sys_error_transition(mhi_cntrl);
break;
case DEV_ST_TRANSITION_DISABLE:
mhi_pm_disable_transition
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,
a Linux Foundation Collaborative Project

Thanks,
Bhaumik

Thanks,
Bhaumik
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Next message: Vaibhav Gupta: "[PATCH v4 16/29] scsi: mpt3sas_scsih: use generic power management"
Previous message: Vaibhav Gupta: "[PATCH v4 14/29] scsi: hisi_sas_v3_hw: Remove extra function calls for runtime pm"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]