lkml.org 
[lkml]   [2021]   [Mar]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 RESEND] bus: mhi: core: Wait for ready state after reset
From
Date
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.

\
 
 \ /
  Last update: 2021-03-16 20:31    [W:0.099 / U:0.636 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site