Messages in this thread | | | Subject | Re: [PATCH v2 RESEND] bus: mhi: core: Wait for ready state after reset | From | Jeffrey Hugo <> | Date | Tue, 16 Mar 2021 13:28:41 -0600 |
| |
On 3/16/2021 12:14 AM, Manivannan Sadhasivam wrote: > On Wed, Mar 10, 2021 at 01:41:58PM -0700, Jeffrey Hugo wrote: >> After the device has signaled the end of reset by clearing the reset bit, >> it will automatically reinit MHI and the internal device structures. Once >> That is done, the device will signal it has entered the ready state. >> >> Signaling the ready state involves sending an interrupt (MSI) to the host >> which might cause IOMMU faults if it occurs at the wrong time. >> >> If the controller is being powered down, and possibly removed, then the >> reset flow would only wait for the end of reset. At which point, the host >> and device would start a race. The host may complete its reset work, and >> remove the interrupt handler, which would cause the interrupt to be >> disabled in the IOMMU. If that occurs before the device signals the ready >> state, then the IOMMU will fault since it blocked an interrupt. While >> harmless, the fault would appear like a serious issue has occurred so let's >> silence it by making sure the device hits the ready state before the host >> completes its reset processing. >> >> Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org> >> --- >> drivers/bus/mhi/core/pm.c | 17 ++++++++++++++++- >> 1 file changed, 16 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c >> index adb0e80..414da4f 100644 >> --- a/drivers/bus/mhi/core/pm.c >> +++ b/drivers/bus/mhi/core/pm.c >> @@ -467,7 +467,7 @@ static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl) >> >> /* Trigger MHI RESET so that the device will not access host memory */ >> if (!MHI_PM_IN_FATAL_STATE(mhi_cntrl->pm_state)) { >> - u32 in_reset = -1; >> + u32 in_reset = -1, ready = 0; >> unsigned long timeout = msecs_to_jiffies(mhi_cntrl->timeout_ms); >> >> dev_dbg(dev, "Triggering MHI Reset in device\n"); >> @@ -490,6 +490,21 @@ static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl) >> * hence re-program it >> */ >> mhi_write_reg(mhi_cntrl, mhi_cntrl->bhi, BHI_INTVEC, 0); >> + >> + if (!MHI_IN_PBL(mhi_get_exec_env(mhi_cntrl))) { >> + /* wait for ready to be set */ >> + ret = wait_event_timeout(mhi_cntrl->state_event, >> + mhi_read_reg_field(mhi_cntrl, >> + mhi_cntrl->regs, >> + MHISTATUS, >> + MHISTATUS_READY_MASK, >> + MHISTATUS_READY_SHIFT, >> + &ready) >> + || ready, timeout); >> + if (!ret || !ready) >> + dev_warn(dev, >> + "Device failed to enter READY state\n"); > > Wouldn't dev_err be more appropriate here provided that we might get IOMMU fault > anytime soon?
I supposed. Didn't feel like a "true" error because nothing has actually failed, the chance of the IOMMU fault is low, and I couldn't enumerate what would be the expected action for the system user to take if they saw this as an error.
I don't have a particularly strong opinion one way or the other. I figured warn was the more conservative option here.
Will change.
-- Jeffrey Hugo Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
| |