On Tue, Feb 04, 2025 at 01:37:57PM +0800, Feng Tang wrote:
> According to PCIe 6.1 spec, section 6.7.3.2, software need to wait at
> least 1 second for the command-complete event, before resending the cmd
> or sending a new cmd.
>
> Currently get_port_device_capability() sends slot control cmd to disable
> PCIe hotplug interrupts without waiting for its completion and there was
> real problem reported for the lack of waiting.
>
> Add the necessary wait to comply with PCIe spec. The waiting logic refers
> existing pcie_poll_cmd().
[...]
> --- a/drivers/pci/pcie/portdrv.c
> +++ b/drivers/pci/pcie/portdrv.c
> @@ -230,8 +260,7 @@ static int get_port_device_capability(struct pci_dev *dev)
> * Disable hot-plug interrupts in case they have been enabled
> * by the BIOS and the hot-plug service driver is not loaded.
> */
> - pcie_capability_clear_word(dev, PCI_EXP_SLTCTL,
> - PCI_EXP_SLTCTL_CCIE | PCI_EXP_SLTCTL_HPIE);
> + pcie_disable_hp_interrupts_early(dev);
> }
The language of the code comment is a bit confusing in that it
says the hot-plug driver may not be "loaded". This sounds like
it could be modular. But it can't. It's always built-in.
So I think what is really meant here is that the driver may be
*disabled* in the config, i.e. CONFIG_HOTPLUG_PCI_PCIE=n.
Now if CONFIG_HOTPLUG_PCI_PCIE=n, you don't need to observe the
Command Completed delay because the hotplug driver won't touch
the Slot Control register afterwards. It's not compiled in.
On the other hand if CONFIG_HOTPLUG_PCI_PCIE=y, you don't need
to disable the CCIE and HPIE interrupt because the hotplug driver
will handle them.
So I think the proper solution here is to make the write to the
Slot Control register conditional on CONFIG_HOTPLUG_PCI_PCIE,
like this:
if (dev->is_hotplug_bridge &&
(pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
- pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM) &&
- (pcie_ports_native || host->native_pcie_hotplug)) {
- services |= PCIE_PORT_SERVICE_HP;
+ pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)) {
+ if (pcie_ports_native || host->native_pcie_hotplug)
+ services |= PCIE_PORT_SERVICE_HP;
/*
* Disable hot-plug interrupts in case they have been enabled
* by the BIOS and the hot-plug service driver is not loaded.
*/
- pcie_capability_clear_word(dev, PCI_EXP_SLTCTL,
- PCI_EXP_SLTCTL_CCIE | PCI_EXP_SLTCTL_HPIE);
+ if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE))
+ pcie_capability_clear_word(dev, PCI_EXP_SLTCTL,
+ PCI_EXP_SLTCTL_CCIE | PCI_EXP_SLTCTL_HPIE);
}
The above patch also makes sure the interrupts are quiesced if the
platform didn't grant hotplug control to OSPM.
Thanks,
Lukas
