Re: [PATCH] bus: mhi: host: Flush the posted write after writing to MHI_SOC_RESET_REQ_OFFSET

[Manivannan Sadhasivam]

On Wed, Jul 29, 2026 at 03:07:00PM -0600, Jeff Hugo wrote:
> On 6/23/2026 8:51 AM, Manivannan Sadhasivam wrote:
> > mhi_soc_reset() tries to reset the device by writing to the
> > MHI_SOC_RESET_REQ_OFFSET register. But it doesn't do a read-back to ensure
> > that the write gets flushed to the device before returning to the caller.
> > 
> > This may lead to the delay (if implemented) on the caller to be
> > insufficient, if the posted write doesn't reach the device before the
> > delay.
> 
> Interesting.  Is the delay tight enough that a few ms will possibly blow it?
> Seems like a poorly defined delay.  All the devices I'm familiar with take
> multiple seconds to boot (with some variability due to ddr training and
> thermal constraints), and if the reset triggers a crash dump, then its
> easily tens of seconds.
> 

Which delay you are referring to? RDDM delay which is just 2ms? Yeah, that seems
questionable. But this function itself doesn't implement any delay. So your
comment was somewhat confusing.

> Regardless, since this reset will either kill the pcie link, or disconnect
> the SoC from the link for a time,

No, this will not reset the PCIe link AFAIK. PERST separation logic available in
the Endpoint should make sure the PCIe link is active while the SoC is
undergoing the reset. Otherwise, if the host tries to access the device config
space before the device is ready, it will blow up.

>  I've been trying to figure out how this
> change might break, but I haven't found a scenario, so I suspect this is
> good enough.
> 

This change should not break any devices as it just ensures that the posted
write gets flushed to the device before the delay.

> > So add a read-back after writing to the MHI_SOC_RESET_REQ_OFFSET register.
> > 
> > Fixes: b5a8d233a588 ("bus: mhi: core: Add device hardware reset support")
> > Reported-by: Alex Williamson <alex@xxxxxxxxxxx>
> > Closes: https://lore.kernel.org/linux-pci/20260622160822.09350246@xxxxxxxxxxx
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxxxxxxxx>
> > ---
> >   drivers/bus/mhi/host/main.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
> > index 53c0ffe30070..4d458396233a 100644
> > --- a/drivers/bus/mhi/host/main.c
> > +++ b/drivers/bus/mhi/host/main.c
> > @@ -170,6 +170,9 @@ EXPORT_SYMBOL_GPL(mhi_get_mhi_state);
> >   void mhi_soc_reset(struct mhi_controller *mhi_cntrl)
> >   {
> > +	int __maybe_unused ret;
> > +	u32 tmp;
> > +
> >   	if (mhi_cntrl->reset) {
> >   		mhi_cntrl->reset(mhi_cntrl);
> >   		return;
> > @@ -178,6 +181,9 @@ void mhi_soc_reset(struct mhi_controller *mhi_cntrl)
> >   	/* Generic MHI SoC reset */
> >   	mhi_write_reg(mhi_cntrl, mhi_cntrl->regs, MHI_SOC_RESET_REQ_OFFSET,
> >   		      MHI_SOC_RESET_REQ);
> > +	/* Flush the posted write to the device (ignore return value) */
> > +	ret = mhi_read_reg(mhi_cntrl, mhi_cntrl->regs, MHI_SOC_RESET_REQ_OFFSET,
> > +			   &tmp);
> 
> If you wanted, you could fit this all on one line.  word wrapping for a 3
> char parameter seems a bit silly to me, but I suspect this is highly
> subjective.
> 

Entire driver is still 80 column width, so I was trying to be preserve the same
pattern.

> Reviewed-by: Jeff Hugo <jeff.hugo@xxxxxxxxxxxxxxxx>
> 

Thank you!

- Mani

-- 
மணிவண்ணன் சதாசிவம்