linux-kernel - Re: [PATCH v3] bus: mhi: host: Wait for ready state after reset

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220418062645.GI7431@thinkpad>
Date:   Mon, 18 Apr 2022 11:56:45 +0530
From:   Manivannan Sadhasivam <mani@...nel.org>
To:     Jeffrey Hugo <quic_jhugo@...cinc.com>
Cc:     quic_hemantk@...cinc.com, quic_bbhatt@...cinc.com,
        mhi@...ts.linux.dev, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org, Jeffrey Hugo <jhugo@...eaurora.org>
Subject: Re: [PATCH v3] bus: mhi: host: Wait for ready state after reset

On Wed, Apr 13, 2022 at 12:52:26PM -0600, Jeffrey Hugo wrote:
> From: Jeffrey Hugo <jhugo@...eaurora.org>
> 
> 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@...eaurora.org>
> Reviewed-by: Hemant Kumar <hemantk@...eaurora.org>
> Signed-off-by: Jeffrey Hugo <quic_jhugo@...cinc.com>

I was about to apply this patch but wanted to check with you on using the
double signed-off by tags. If the patch get's handed over between multiple
developers then multiple signed-off by's make sense. But since it is you that
handled the earlier patch also, I think one tag is enough with your new domain.

One more thing is, using codeaurora domain will bounce now. So, please use the
quicinc domain for Hemant also.

Thanks,
Mani

> ---
> 
> v3:
> Rebase and use dev_err over dev_warn
> 
> v2: 
> Fix subject and remove use of cur_state
> 
>  drivers/bus/mhi/host/pm.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/bus/mhi/host/pm.c b/drivers/bus/mhi/host/pm.c
> index a0e91bd..f46158e 100644
> --- a/drivers/bus/mhi/host/pm.c
> +++ b/drivers/bus/mhi/host/pm.c
> @@ -483,6 +483,15 @@ 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 = mhi_poll_reg_field(mhi_cntrl, mhi_cntrl->regs,
> +						 MHISTATUS,
> +						 MHISTATUS_READY_MASK, 1, 25000);
> +			if (ret)
> +				dev_err(dev, "Device failed to enter READY state\n");
> +		}
>  	}
>  
>  	dev_dbg(dev,
> -- 
> 2.7.4
>