Re: [PATCH v15 1/4] PCI: Assume control of portdrv-related features only when portdrv enabled

[Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>]

On 11/2/2026 12:20 PM, Lukas Wunner wrote:
> On Fri, Oct 02, 2026 at 02:18:25PM -0700, Kuppuswamy Sathyanarayanan wrote:
>> Native control of PME, AER, DPC, and PCIe hotplug depends on the portdrv,
>> so default to native handling of them only when CONFIG_PCIEPORTBUS is
>> enabled.
>>
>> Native control of LTR and SHPC hotplug does not depend on portdrv, so keep
>> defaulting those to native regardless.
>>
>> [bhelgaas: commit log]
>> Link: https://lore.kernel.org/r/fcbe8a624166a1101a755edfef44a185d32ff493.1603766889.git.sathyanarayanan.kuppuswamy@linux.intel.com
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> This is now commit 6347fd9aee19 in Linus' tree and I think there's a problem:
> 
>> +++ b/drivers/pci/probe.c
>> @@ -658,6 +658,8 @@ static const struct device_type pci_host_bridge_type = {
>>  
>>  static void pci_init_host_bridge(struct pci_host_bridge *bridge)
>>  {
>> +	bool port_services = IS_ENABLED(CONFIG_PCIEPORTBUS);
>> +
>>  	INIT_LIST_HEAD(&bridge->windows);
>>  	INIT_LIST_HEAD(&bridge->dma_ranges);
>>  	INIT_LIST_HEAD(&bridge->ports);
>> @@ -668,12 +670,12 @@ static void pci_init_host_bridge(struct pci_host_bridge *bridge)
>>  	 * may implement its own AER handling and use _OSC to prevent the
>>  	 * OS from interfering.
>>  	 */
>> -	bridge->native_aer = 1;
>> -	bridge->native_pcie_hotplug = 1;
>> +	bridge->native_aer = port_services;
>> +	bridge->native_pcie_hotplug = port_services;
> 
> The commit is setting the native_pcie_hotplug bit up front based on
> CONFIG_PCIEPORTBUS.
> 
> But there's an if-clause at the top of get_port_device_capability() which
> adds PCIE_PORT_SERVICE_HP to the port services used by a given PCIe port.
> 
> Inside the if-clause is code to disable hotplug interrupts if the BIOS
> left them enable.
> 
> Since the commit, if CONFIG_PCIEPORTBUS=n, the if-clause is no longer
> entered and so the interrupts are no longer disabled.

I don't think this case changed. get_port_device_capability() is in
portdrv.c, which is only built with CONFIG_PCIEPORTBUS=y:

  obj-$(CONFIG_PCIEPORTBUS)	+= pcieportdrv.o bwctrl.o

So with CONFIG_PCIEPORTBUS=n, the if-clause was never entered, before or
after this commit.

With CONFIG_PCIEPORTBUS=y, native_pcie_hotplug still starts out as 1,
so the clause still disables the hotplug interrupts when
CONFIG_HOTPLUG_PCI_PCIE=n.

Your patch was about setting native_pcie_hotplug from CONFIG_HOTPLUG_PCI_PCIE. 
That would break the PCIEPORTBUS=y, HOTPLUG_PCI_PCIE=n case, as you said. 
This commit uses CONFIG_PCIEPORTBUS instead.

Did you see a case where the interrupts stay enabled with this commit?


> 
> I'm amazed sashiko didn't flag this.  I didn't spot it during review
> either, but noticed it when rebasing my development branch.  I still
> have a patch on that branch which I developed last year but which
> was ultimately not accepted (for good reasons).  Back then I already
> noticed that the interrupt disablement issue prevented simplification
> of native_pcie_hotplug initialization.  I forgot about it but had to
> revisit the patch when rebasing the development branch on top of your
> patches:
> 
> https://lore.kernel.org/r/5eae82776d695e589b89aad500d8208b980347e5.1752390102.git.lukas@wunner.de
> 
> Now what?  The offending patch is the first one in a series of three.
> Revert all three or can we somehow salvage this?
> 
> Thanks,
> 
> Lukas

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer