Re: [PATCH v3 1/2] PCI/PM: Skip resuming to D0 if disconnected

[Lukas Wunner <lukas@wunner.de>]

On Thu, Jun 19, 2025 at 09:55:34PM -0500, Mario Limonciello wrote:
> When a USB4 dock is unplugged the PCIe bridge it's connected to will
> remove issue a "Link Down" and "Card not detected event". The PCI core

Small nit here, the word "remove" seems superfluous.


> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1374,6 +1374,11 @@ int pci_power_up(struct pci_dev *dev)
>  		return -EIO;
>  	}
>  
> +	if (pci_dev_is_disconnected(dev)) {
> +		dev->current_state = PCI_D3cold;
> +		return -EIO;
> +	}
> +
>  	pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
>  	if (PCI_POSSIBLE_ERROR(pmcsr)) {
>  		pci_err(dev, "Unable to change power state from %s to D0, device inaccessible\n",

This hunk looks good to me.


> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -550,6 +550,7 @@ static inline bool pci_dev_set_io_state(struct pci_dev *dev,
>  
>  static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused)
>  {
> +	pci_info(dev, "Device disconnected\n");
>  	pci_dev_set_io_state(dev, pci_channel_io_perm_failure);
>  	pci_doe_disconnected(dev);

However this one (which I think was newly added in v3) seems problematic:

pci_dev_set_disconnected() needs to be fast so that devices are marked
unplugged as quickly as possible.  We want to minimize the time window
where MMIO and Config Space reads already return "all ones" and writes
go to nirvana, but pci_dev_is_disconnected() still returns false.
However printk() isn't a cheap operation, so it may unduly delay
marking the removed devices disconnected.

Second, the object of the patch is to make hot-removal less noisy,
but this change *increases* noise.  It will emit an extra message
for every removed device.

Third, the change is somewhat orthogonal to the first hunk in the patch,
so I would have put this in a separate patch.

Bottom line is, at the very least I'd recommend making this a pci_dbg(),
but I'm not even sure the extra message adds sufficient value to justify
the change.

Thanks,

Lukas