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

[Jose Ignacio Tornos Martinez <jtornosm@redhat.com>]

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

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



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.