Re: [RFC PATCH v2] PCI: Waiting command completed in get_port_device_capability()

[Yao Hongbo <yaohongbo@linux.alibaba.com>]

在 2022/1/12 上午2:55, Bjorn Helgaas 写道:
> [+cc Lukas, Rafael (in case you have any recollection of 2bd50dd800b5)]
> 
> On Fri, Jan 07, 2022 at 11:22:49AM +0800, Yao Hongbo wrote:
>> According to the PCIe specification Revision 5.0, section
>> 7.5.3.11 (slot Status Register), if Command Complete notification
>> is supported,  a write to the slot control register needs to set
>> the command completed bit, which can indicate the controller is
>> ready to receive the next command.
>>
>> However, before probing the pcie hotplug service, there needs to set
>> HPIE bit in the slot ctrl register to disable hotplug interrupts,
>> and there is no wait currently.
>>
>> The interval between the two functions get_port_device_capability() and
>> pcie_disable_notification() is not long, which may cause the latter to
>> be interfered by the former.
>>
>> The command complete event received by pcie_disable_notification() may
>> belong to the operation of get_port_device_capability().
> 
> Yes, looks like a potential problem.
> 
>> Signed-off-by: Liguang Zhang <zhangliguang@linux.alibaba.com>
>> Signed-off-by: Yao Hongbo <yaohongbo@linux.alibaba.com>
>> ---
>>  drivers/pci/pcie/portdrv_core.c | 40 ++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 38 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
>> index bda6308..ec2088b6e 100644
>> --- a/drivers/pci/pcie/portdrv_core.c
>> +++ b/drivers/pci/pcie/portdrv_core.c
>> @@ -15,6 +15,7 @@
>>  #include <linux/string.h>
>>  #include <linux/slab.h>
>>  #include <linux/aer.h>
>> +#include <linux/delay.h>
>>  
>>  #include "../pci.h"
>>  #include "portdrv.h"
>> @@ -190,6 +191,42 @@ static int pcie_init_service_irqs(struct pci_dev *dev, int *irqs, int mask)
>>  	return 0;
>>  }
>>  
>> +static void pcie_port_disable_hp_interrupt(struct pci_dev *dev)
>> +{
>> +	u16 slot_status;
>> +	u32 slot_cap;
>> +	int timeout = 1000;
>> +
>> +	pcie_capability_clear_word(dev, PCI_EXP_SLTCTL,
>> +			PCI_EXP_SLTCTL_CCIE | PCI_EXP_SLTCTL_HPIE);
>> +
>> +	/*
>> +	 * If the command completed notification is not supported,
>> +	 * we don't need to wait after writing to the slot ctrl register.
>> +	 */
>> +	pcie_capability_read_dword(dev, PCI_EXP_SLTCAP, &slot_cap);
>> +	if (slot_cap & PCI_EXP_SLTCAP_NCCS)
>> +		return;
>> +
>> +	do {
>> +		pcie_capability_read_word(dev, PCI_EXP_SLTSTA, &slot_status);
>> +		if (slot_status == (u16) ~0) {
>> +			pci_info(dev, "%s: no response from device\n",  __func__);
>> +			return;
>> +		}
>> +
>> +		if (slot_status & PCI_EXP_SLTSTA_CC) {
>> +			pcie_capability_write_word(dev, PCI_EXP_SLTSTA, PCI_EXP_SLTSTA_CC);
>> +			return;
>> +		}
>> +
>> +		msleep(10);
>> +		timeout -= 10;
>> +	} while (timeout >= 0);
>> +
>> +	pci_info(dev, "Timeout on hotplug disable interrupt!\n");
>> +}
>> +
>>  /**
>>   * get_port_device_capability - discover capabilities of a PCI Express port
>>   * @dev: PCI Express port to examine
>> @@ -213,8 +250,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_port_disable_hp_interrupt(dev);
> 
> This originally came from 2bd50dd800b5 ("PCI: PCIe: Disable PCIe port
> services during port initialization"), where we disable hotplug
> interrupts in case the hotplug driver is not available.
> 
> In general, I think the OS should not be responsible for disabling
> interrupts for feature X.  The OS may predate feature X and may not
> know anything about X at all.  The power-on default for interrupts
> related to X should be "disabled" (as it is for HPIE and CCIE), and if
> firmware enables them, it should disable them or arrange to handle
> them itself before handing off to the OS.
> 
> I don't know whether 2bd50dd800b5 was prompted by spurious hotplug
> interrupts or not.  If it was, I think we were seeing a firmware
> defect or possibly a pciehp initialization issue.
> 
> At the time of 2bd50dd800b5, we always cleared HPIE and CCIE here.
> 
> But now, on ACPI systems, we only clear HPIE and CCIE here if we *do*
> have the hotplug driver (because host->native_pcie_hotplug only
> remains set if we have been granted control via _OSC, and we only
> request control when CONFIG_HOTPLUG_PCI_PCIE is enabled).  On these
> systems, we should be able to remove this disable code because pciehp
> will do whatever it needs.
> 
> For non-ACPI systems, bridge->native_pcie_hotplug will always be set,
> so we will clear HPIE and CCIE here and then (if
> CONFIG_HOTPLUG_PCI_PCIE is enabled) initialize pciehp soon after,
> which may be a problem as you describe.
> 
> What kind of system are you seeing the problem on?  It seems like it
> should be safe to drop the HPIE and CCIE disable here for ACPI
> systems.  And *likely* we could do the same for non-ACPI systems,
> though I have no experience there.

Hi, Bjorn
Thanks for your comments.

The problem occurs on ACPI systems.

[    7.739359] acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI EDR HPX-Type3]
[    7.749238] acpi PNP0A08:00: _OSC: platform does not support [SHPCHotplug AER LTR DPC]
[    7.757609] acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug PME PCIeCapability]


We clear HPIE and CCIE here because the firmware doesn't control Hotplug via __OSC.

And on ACPI systems, we can also set pcie_ports=native, which will also encounter such problems.



> Bjorn