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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The condition checking for PCIe link recovery evaluates to always-true due to a type promotion mismatch, completely bypassing the 5-second wait loop.
- [Medium] The `reset_qualcomm_wifi` function performs a read-modify-write on a hardware register without verifying if the initial read succeeded.
- [Medium] The polling loop for PCIe link recovery does not perform a final condition check after the timeout, potentially causing false failures if the thread is delayed by the scheduler.
- [Medium] The device-specific reset methods may trigger a SoC hardware reset that drops the PCIe physical link, which would cause the upstream port to generate unmasked hotplug events.
--

commit 632c19d8e3a5ffaea7401f14cf4ba2e70f72cb54
Author: Jose Ignacio Tornos Martinez <jtornosm@redhat.com>
Subject: PCI: Add device-specific reset for Qualcomm devices

This commit adds device-specific reset methods for certain Qualcomm PCIe
devices (WCN6855/WCN7850 WiFi cards, SDX62/SDX65 modems) that lack working
reset methods for VFIO passthrough. It implements true hardware reset
mechanisms using BAR-space hardware reset registers.

> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4227,6 +4227,120 @@ static int reset_hinic_vf_dev(struct pci_dev *pdev, bool probe)

[ ... ]

> +static int reset_qualcomm_wifi(struct pci_dev *pdev, bool probe)
> +{
> +	bool link_recovered = false;
> +	unsigned long timeout;
> +	void __iomem *bar;
> +	u32 val;
> +	u16 cmd;
> +
> +	if (probe)
> +		return 0;
> +
> +	if (pdev->current_state != PCI_D0)
> +		return -EINVAL;
> +
> +	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> +	pci_write_config_word(pdev, PCI_COMMAND, cmd | PCI_COMMAND_MEMORY);
> +
> +	bar = pci_iomap(pdev, 0, 0);
> +	if (!bar) {
> +		pci_write_config_word(pdev, PCI_COMMAND, cmd);
> +		return -ENODEV;
> +	}
> +
> +	val = ioread32(bar + QUALCOMM_WIFI_PCIE_SOC_GLOBAL_RESET);
> +	val |= QUALCOMM_WIFI_PCIE_SOC_GLOBAL_RESET_V;
> +	iowrite32(val, bar + QUALCOMM_WIFI_PCIE_SOC_GLOBAL_RESET);
> +	ioread32(bar + QUALCOMM_WIFI_PCIE_SOC_GLOBAL_RESET);

[Severity: Medium]
If the device is already hung and the initial ioread32() returns 0xFFFFFFFF
due to a completion timeout, does this read-modify-write sequence blindly
write 0xFFFFFFFF back to the hardware? Could writing all 1s to a global SoC
reset register overwrite reserved bits or place the device in an undefined
state?

> +
> +	msleep(10);
> +
> +	val &= ~QUALCOMM_WIFI_PCIE_SOC_GLOBAL_RESET_V;
> +	iowrite32(val, bar + QUALCOMM_WIFI_PCIE_SOC_GLOBAL_RESET);
> +	ioread32(bar + QUALCOMM_WIFI_PCIE_SOC_GLOBAL_RESET);
> +
> +	msleep(10);
> +
> +	timeout = jiffies + msecs_to_jiffies(5000);

[Severity: Medium]
If this SoC hardware reset physically drops the PCIe link, and this port is
hotplug-capable, will the upstream Root Port generate unmasked Surprise
Link Down events? Since pci_dev_specific_reset() runs without masking link
change events, could pciehp asynchronously remove the device from the bus
while this reset is polling, breaking the VFIO passthrough?

> +	while (time_before(jiffies, timeout)) {

[Severity: Medium]
If the executing thread is preempted or experiences a scheduling delay just
before evaluating time_before(jiffies, timeout), and the delay pushes
jiffies past the timeout, will the loop exit immediately without a final
hardware check? Could this falsely report a timeout failure even if the
device recovered during the delay window? Should this perform a final check
after the loop, or perhaps use read_poll_timeout()?

> +		val = ioread32(bar + QUALCOMM_WIFI_PCIE_SOC_GLOBAL_RESET);
> +		if (val != PCI_ERROR_RESPONSE) {

[Severity: High]
Since val is a u32 and PCI_ERROR_RESPONSE is a 64-bit value (~0ULL), will C
type promotion zero-extend val to 64 bits and cause this condition to always
be true? When the device is unresponsive and returns 0xFFFFFFFF, does this
bypass the 5-second wait loop and falsely report success? Would it be better
to use the standard !PCI_POSSIBLE_ERROR(val) macro here?

> +			link_recovered = true;
> +			break;
> +		}
> +		msleep(20);
> +	}
> +
> +	if (!link_recovered) {

[ ... ]

> +static int reset_qualcomm_modem(struct pci_dev *pdev, bool probe)
> +{
> +	void __iomem *bar;
> +	u16 cmd;
> +
> +	if (probe)
> +		return 0;
> +
> +	if (pdev->current_state != PCI_D0)
> +		return -EINVAL;
> +
> +	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> +	pci_write_config_word(pdev, PCI_COMMAND, cmd | PCI_COMMAND_MEMORY);
> +
> +	bar = pci_iomap(pdev, 0, 0);
> +	if (!bar) {
> +		pci_write_config_word(pdev, PCI_COMMAND, cmd);
> +		return -ENODEV;
> +	}
> +
> +	iowrite32(MHI_SOC_RESET_REQ, bar + MHI_SOC_RESET_REQ_OFFSET);
> +	ioread32(bar + MHI_SOC_RESET_REQ_OFFSET);
> +
> +	/* Be sure device reset has been executed */
> +	msleep(2000);

[Severity: Medium]
If this modem SoC reset also drops the PCIe physical link, does it suffer
from the same unmasked hotplug event issue described above?

> +
> +	pci_iounmap(pdev, bar);
> +	pci_write_config_word(pdev, PCI_COMMAND, cmd);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260626055023.197470-1-jtornosm@redhat.com?part=1