Re: [PATCH v8 3/5] PCI: host-common: Add link down handling for Root Ports

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Hardware state desynchronization when CONFIG_PCIEAER is disabled.
- [Medium] Commit message claims error_detected() callback is triggered for the bridge, but the implementation skips it.
--

commit f07cf37e729701c6c67b90c92f65dd76b53b9fec
Author: Manivannan Sadhasivam <mani@kernel.org>

PCI: host-common: Add link down handling for Root Ports

This commit introduces a new API, pci_host_handle_link_down(), for host
bridge drivers to recover a specific Root Port when a link down event is
observed. It utilizes pcie_do_recovery() with AER handling if enabled, or
falls back to pci_bus_error_reset() otherwise.

> First, the AER error_detected() callback will be triggered for the bridge and then for the downstream devices.

Is this description accurate regarding the bridge receiving the callback?

Looking at pcie_do_recovery(), it uses pci_walk_bridge() to trigger the
recovery callbacks. However, pci_walk_bridge() handles bridges with
subordinate buses by iterating over devices on the subordinate bus.

Consequently, the Root Port (the bridge itself) appears to be excluded from
the callback walk and might not receive the error_detected() callback.

> diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c
> --- a/drivers/pci/controller/pci-host-common.c
> +++ b/drivers/pci/controller/pci-host-common.c

[ ... ]

> @@ -106,5 +108,38 @@ void pci_host_common_remove(struct platform_device *pdev)
>  }
>  EXPORT_SYMBOL_GPL(pci_host_common_remove);
>  
> +static pci_ers_result_t pci_host_reset_root_port(struct pci_dev *dev)
> +{
> +	int ret;
> +
> +	pci_lock_rescan_remove();
> +	ret = pci_bus_error_reset(dev);
> +	pci_unlock_rescan_remove();
> +	if (ret) {
> +		pci_err(dev, "Failed to reset Root Port: %d\n", ret);
> +		return PCI_ERS_RESULT_DISCONNECT;
> +	}
> +
> +	pci_info(dev, "Root Port has been reset\n");
> +
> +	return PCI_ERS_RESULT_RECOVERED;
> +}
> +
> +static void pci_host_recover_root_port(struct pci_dev *port)
> +{
> +#if IS_ENABLED(CONFIG_PCIEAER)
> +	pcie_do_recovery(port, pci_channel_io_frozen, pci_host_reset_root_port);
> +#else
> +	pci_host_reset_root_port(port);

If CONFIG_PCIEAER is disabled, this directly calls pci_host_reset_root_port()
which performs a bus reset using PCI_RESET_NO_RESTORE via
pci_bus_error_reset().

Since pcie_do_recovery() is skipped, downstream device drivers are never
notified via their error_detected(), slot_reset(), or resume() callbacks.

Does this leave the hardware configuration space of these devices wiped while
the bound drivers remain active and unaware?

If subsequent MMIO accesses by these drivers hit unmapped space and DMA
fails, could this lead to device lockups or data corruption? Would it be
safer to hot-remove and rescan the devices instead of blindly resetting the
bus when AER is unavailable?

> +#endif
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518-pci-port-reset-v8-0-eb5a7d331dfc@oss.qualcomm.com?part=3