Re: [PATCH] PCI: pciehp: Disable link and turn off slot power while removing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Unconditional slot power-off in `pciehp_release_ctrl()` abruptly cuts power to active devices, causing kernel crashes.
- [Low] Dead code in `pciehp_power_off_slot()` due to `pciehp_link_disable()` always returning 0.
--

commit 5574c68659aab56ef2cf2ec6acc4c999a6afa1fc
Author: Mario Limonciello (AMD) <superm1@kernel.org>

PCI: pciehp: Disable link and turn off slot power while removing

This commit adds code to pciehp_power_off_slot() to disable the link
before turning off slot power. It also introduces a cleanup sequence
in pciehp_release_ctrl() to disable the link and turn off slot power
during driver removal.

> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index 4c62140a3cb44..6705dcaf1ad0c 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
[ ... ]
> @@ -558,6 +563,12 @@ int pciehp_power_on_slot(struct controller *ctrl)
>  
>  void pciehp_power_off_slot(struct controller *ctrl)
>  {
> +	int retval;
> +
> +	retval = pciehp_link_disable(ctrl);
> +	if (retval)
> +		ctrl_err(ctrl, "%s: Can not disable the link!\n", __func__);

Since pciehp_link_disable() calls __pciehp_link_set() which unconditionally
returns 0, will this error path ever be executed?

> +
>  	pcie_write_cmd(ctrl, PCI_EXP_SLTCTL_PWR_OFF, PCI_EXP_SLTCTL_PCC);
>  	ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
>  		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL,
[ ... ]
> @@ -1096,6 +1107,8 @@ struct controller *pcie_init(struct pcie_device *dev)
>  void pciehp_release_ctrl(struct controller *ctrl)
>  {
>  	cancel_delayed_work_sync(&ctrl->button_work);
> +	if (POWER_CTRL(ctrl))
> +		pciehp_power_off_slot(ctrl);
>  	kfree(ctrl);
>  }

If the pciehp driver is unbound manually or encounters an error during
probe, could this unconditionally cut power to active devices?

Since pciehp is a port service, unbinding it removes hotplug monitoring
but does not detach the active child PCIe devices (such as NVMe drives)
from the PCI core.

If pciehp_release_ctrl() is called while child drivers are still bound and
running, does this lead to unexpected link down events and unsupported
requests?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260506164353.1278571-1-mario.limonciello@amd.com?part=1