Re: [PATCH v12] PCI: Add device-specific reset for Qualcomm devices

[Alex Williamson <alex@shazbot.org>]

On Mon, 13 Jul 2026 10:52:32 +0200
Jose Ignacio Tornos Martinez <jtornosm@redhat.com> wrote:

> > [Severity: Medium]
> > If the device is hung or the link is temporarily unresponsive, could the
> > initial ioread32() return PCI_ERROR_RESPONSE (0xFFFFFFFF)?
> >
> > If it does, this read-modify-write operation would write 0xFFFFFFFF (and
> > later 0xFFFFFFFE) back to the device. Could writing these values trigger
> > unintended resets across the SoC or leave the hardware in an invalid state?
> >
> > Would it be safer to verify !PCI_POSSIBLE_ERROR(val) before modifying
> > and writing it back?  
> 
> The read-modify-write on the SOC_GLOBAL_RESET register replicates
> the existing ath11k/ath12k driver reset sequence
> (ath11k_pci_soc_global_reset()). If the device already returns
> 0xFFFFFFFF at this point, it is unresponsive and writing to it
> will not make things worse - the purpose of this function is
> precisely to reset a device in a bad state. The link recovery
> loop below already handles the case where the device remains
> unresponsive after the reset attempt.

The "will not make things worse" claim is really the best case
scenario, there are platforms that won't silently swallow the write
error and could escalate this to a platform level APEI/GHES error.
Testing the initial ioread() value is a valid improvement in robustness.

> > [Severity: Medium]
> > Is this polling loop vulnerable to false timeouts due to scheduling delays?
> >
> > If the thread is preempted during msleep(20) and delayed for more than 5
> > seconds, time_before(jiffies, timeout) will evaluate to false upon waking
> > up. The loop will exit without a final check of the register, returning
> > -ETIMEDOUT even if the link successfully recovered during the delay.
> >
> > Should there be a final condition check after the loop, or perhaps use
> > read_poll_timeout() instead to handle preemption robustly?  
> 
> The time_before()/msleep() polling pattern replicates the existing
> driver recovery logic and is standard kernel practice, used
> throughout drivers/pci/quirks.c and PCI core. A scheduling delay
> exceeding 5 seconds would indicate a severe system issue beyond
> the scope of this reset function. The 5-second timeout is generous
> for what typically recovers in under 1 second.

IMO, this is also a valid finding and the suggestion to use
read_poll_timeout() actually simplifies the code.  The entire loop just
becomes something like:

	ret = read_poll_timeout(ioread32, val, !PCI_POSSIBLE_ERROR(val),
				20 * USEC_PER_MSEC, 5 * USEC_PER_SEC, false,
				bar + QUALCOMM_WLAN_PCIE_SOC_GLOBAL_RESET);

It might indeed only be a pathological case that behaves exactly as
sashiko identifies, but in fact any exit from the loop due to timeout
returns with a stale value in val.  The similar use cases in quirks.c
might well be improved in the same way.

> Since the implementation replicates existing driver behavior,
> both points were already discussed during review with the
> subsystem maintainer, and are handled in the code, no changes
> are needed.

Likewise, maybe an opportunity to improve the driver code.  Thanks,

Alex