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

On 7/29/2026 11:19 PM, Manivannan Sadhasivam wrote:
> 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.

The delay at the caller of the API referenced by the commit text - "This 
may lead to the delay on the caller to be insufficient"

I looked at the thread referenced by this patch via the closes tag, and 
while I can follow the virtualization flow, I didn't get a good view on 
the specifics.

>> 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.

Reset separation (PERST separation is one part of) is not always 
enabled.  PBL won't enable it as it requires loading the reset sequences 
to the PMIC, which is outside the scope of ROM.  So, if you have a 
non-flash boot device, or a flash boot device that has a 
non-provisioned/otherwise corrupt flash, the EP may be sitting in PBL 
from cold boot without reset separation enabled.  If the EP is then 
passed to a VM per the related thread, a reset will occur, which will 
take the link down.

Also, if I recall correctly, automotive products do not enable reset 
separation at all for ASIL reasons.

If the link is down, the upstream component of the EP should complete 
the read with an error, not cause the host to blow up.  The only host 
issue I'm aware of is if flow control credits get exhausted, usually the 
host will watchdog, but I don't see how this patch or the situation it 
addresses would trigger that.

> 
>>   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 <[email protected]>
>>> Closes: https://lore.kernel.org/linux-pci/[email protected]
>>> Signed-off-by: Manivannan Sadhasivam <[email protected]>
>>> ---
>>>    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 <[email protected]>
>>
> 
> Thank you!
> 
> - Mani
>