diff mbox series

PCI/DPC: Request DPC only if also requesting AER

Message ID 20240220235520.1514548-1-helgaas@kernel.org (mailing list archive)
State Changes Requested
Delegated to: Bjorn Helgaas
Headers show
Series PCI/DPC: Request DPC only if also requesting AER | expand

Commit Message

Bjorn Helgaas Feb. 20, 2024, 11:55 p.m. UTC 
From: Bjorn Helgaas <bhelgaas@google.com>

When booting with "pci=noaer", we don't request control of AER, but we
previously *did* request control of DPC, as in the dmesg log attached at
the bugzilla below:

  Command line: ... pci=noaer
  acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI EDR HPX-Type3]
  acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug SHPCHotplug PME PCIeCapability LTR DPC]

That's illegal per PCI Firmware Spec, r3.3, sec 4.5.1, table 4-5, which
says:

  If the operating system sets this bit [OSC_PCI_EXPRESS_DPC_CONTROL], it
  must also set bit 7 of the Support field (indicating support for Error
  Disconnect Recover notifications) and bits 3 and 4 of the Control field
  (requesting control of PCI Express Advanced Error Reporting and the PCI
  Express Capability Structure).

Request DPC control only if we have also requested AER control.

Fixes: ac1c8e35a326 ("PCI/DPC: Add Error Disconnect Recover (EDR) support")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=218491#c12
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Cc: <stable@vger.kernel.org>	# v5.7+
Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Cc: Matthew W Carlis <mattc@purestorage.com>
Cc: Keith Busch <kbusch@kernel.org>
Cc: Lukas Wunner <lukas@wunner.de>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
 drivers/acpi/pci_root.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

Comments

Kuppuswamy Sathyanarayanan Feb. 21, 2024, 2:45 a.m. UTC | #1 
On 2/20/24 3:55 PM, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> When booting with "pci=noaer", we don't request control of AER, but we
> previously *did* request control of DPC, as in the dmesg log attached at
> the bugzilla below:
>
>   Command line: ... pci=noaer
>   acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI EDR HPX-Type3]
>   acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug SHPCHotplug PME PCIeCapability LTR DPC]
>
> That's illegal per PCI Firmware Spec, r3.3, sec 4.5.1, table 4-5, which
> says:
>
>   If the operating system sets this bit [OSC_PCI_EXPRESS_DPC_CONTROL], it
>   must also set bit 7 of the Support field (indicating support for Error
>   Disconnect Recover notifications) and bits 3 and 4 of the Control field
>   (requesting control of PCI Express Advanced Error Reporting and the PCI
>   Express Capability Structure).
>
> Request DPC control only if we have also requested AER control.

Can you also add similar check in calculate_support call?

        if (pci_aer_available() && IS_ENABLED(CONFIG_PCIE_EDR))
                support |= OSC_PCI_EDR_SUPPORT;


>
> Fixes: ac1c8e35a326 ("PCI/DPC: Add Error Disconnect Recover (EDR) support")
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218491#c12
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> Cc: <stable@vger.kernel.org>	# v5.7+
> Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Cc: Matthew W Carlis <mattc@purestorage.com>
> Cc: Keith Busch <kbusch@kernel.org>
> Cc: Lukas Wunner <lukas@wunner.de>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>
> ---
>  drivers/acpi/pci_root.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 58b89b8d950e..1c16965427b3 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -518,17 +518,19 @@ static u32 calculate_control(void)
>  	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
>  		control |= OSC_PCI_SHPC_NATIVE_HP_CONTROL;
>  
> -	if (pci_aer_available())
> +	if (pci_aer_available()) {
>  		control |= OSC_PCI_EXPRESS_AER_CONTROL;
>  
> -	/*
> -	 * Per the Downstream Port Containment Related Enhancements ECN to
> -	 * the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-5,
> -	 * OSC_PCI_EXPRESS_DPC_CONTROL indicates the OS supports both DPC
> -	 * and EDR.
> -	 */
> -	if (IS_ENABLED(CONFIG_PCIE_DPC) && IS_ENABLED(CONFIG_PCIE_EDR))
> -		control |= OSC_PCI_EXPRESS_DPC_CONTROL;
> +		/*
> +		 * Per PCI Firmware Spec, r3.3, sec 4.5.1, table 4-5, the
> +		 * OS can request DPC control only if it has advertised
> +		 * OSC_PCI_EDR_SUPPORT and requested both
> +		 * OSC_PCI_EXPRESS_CAPABILITY_CONTROL and
I think you mean OSC_PCI_EXPRESS_DPC_CONTROL.
> +		 * OSC_PCI_EXPRESS_AER_CONTROL.
> +		 */
> +		if (IS_ENABLED(CONFIG_PCIE_DPC) && IS_ENABLED(CONFIG_PCIE_EDR))
> +			control |= OSC_PCI_EXPRESS_DPC_CONTROL;

Since you are cleaning up this part, why not add a patch to remove
CONFIG_PCIE_EDR?


> +	}
>  
>  	return control;
>  }
Bjorn Helgaas Feb. 21, 2024, 11:25 p.m. UTC | #2 
On Tue, Feb 20, 2024 at 06:45:32PM -0800, Kuppuswamy Sathyanarayanan wrote:
> On 2/20/24 3:55 PM, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> >
> > When booting with "pci=noaer", we don't request control of AER, but we
> > previously *did* request control of DPC, as in the dmesg log attached at
> > the bugzilla below:
> >
> >   Command line: ... pci=noaer
> >   acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI EDR HPX-Type3]
> >   acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug SHPCHotplug PME PCIeCapability LTR DPC]
> >
> > That's illegal per PCI Firmware Spec, r3.3, sec 4.5.1, table 4-5, which
> > says:
> >
> >   If the operating system sets this bit [OSC_PCI_EXPRESS_DPC_CONTROL], it
> >   must also set bit 7 of the Support field (indicating support for Error
> >   Disconnect Recover notifications) and bits 3 and 4 of the Control field
> >   (requesting control of PCI Express Advanced Error Reporting and the PCI
> >   Express Capability Structure).
> >
> > Request DPC control only if we have also requested AER control.
> 
> Can you also add similar check in calculate_support call?
> 
>         if (pci_aer_available() && IS_ENABLED(CONFIG_PCIE_EDR))
>                 support |= OSC_PCI_EDR_SUPPORT;

That doesn't seem right to me.  The implementation note in sec 4.6.12
suggests that EDR Notifications may be used even when the firmware
maintains control of AER and DPC.  Maybe that note is wrong or
misleading, but as written, I interpret that as meaning that it may be
useful for the platform to know that the OS supports EDR even if it
AER control isn't requested or granted.

> > Fixes: ac1c8e35a326 ("PCI/DPC: Add Error Disconnect Recover (EDR) support")
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=218491#c12
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: <stable@vger.kernel.org>	# v5.7+
> > Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > Cc: Matthew W Carlis <mattc@purestorage.com>
> > Cc: Keith Busch <kbusch@kernel.org>
> > Cc: Lukas Wunner <lukas@wunner.de>
> > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>
> > ---
> >  drivers/acpi/pci_root.c | 20 +++++++++++---------
> >  1 file changed, 11 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> > index 58b89b8d950e..1c16965427b3 100644
> > --- a/drivers/acpi/pci_root.c
> > +++ b/drivers/acpi/pci_root.c
> > @@ -518,17 +518,19 @@ static u32 calculate_control(void)
> >  	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
> >  		control |= OSC_PCI_SHPC_NATIVE_HP_CONTROL;
> >  
> > -	if (pci_aer_available())
> > +	if (pci_aer_available()) {
> >  		control |= OSC_PCI_EXPRESS_AER_CONTROL;
> >  
> > -	/*
> > -	 * Per the Downstream Port Containment Related Enhancements ECN to
> > -	 * the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-5,
> > -	 * OSC_PCI_EXPRESS_DPC_CONTROL indicates the OS supports both DPC
> > -	 * and EDR.
> > -	 */
> > -	if (IS_ENABLED(CONFIG_PCIE_DPC) && IS_ENABLED(CONFIG_PCIE_EDR))
> > -		control |= OSC_PCI_EXPRESS_DPC_CONTROL;
> > +		/*
> > +		 * Per PCI Firmware Spec, r3.3, sec 4.5.1, table 4-5, the
> > +		 * OS can request DPC control only if it has advertised
> > +		 * OSC_PCI_EDR_SUPPORT and requested both
> > +		 * OSC_PCI_EXPRESS_CAPABILITY_CONTROL and
>
> I think you mean OSC_PCI_EXPRESS_DPC_CONTROL.

No, I just tried to rephrase the text for _OSC Control, bit 7 (quoted
in the commit log), so I think requesting control of bits 3 and 4 (AER
and PCIe Capability) is right.

> > +		 * OSC_PCI_EXPRESS_AER_CONTROL.
> > +		 */
> > +		if (IS_ENABLED(CONFIG_PCIE_DPC) && IS_ENABLED(CONFIG_PCIE_EDR))
> > +			control |= OSC_PCI_EXPRESS_DPC_CONTROL;
> 
> Since you are cleaning up this part, why not add a patch to remove
> CONFIG_PCIE_EDR?

Good idea, I'll do that, too.

Bjorn
Kuppuswamy Sathyanarayanan Feb. 25, 2024, 7:33 p.m. UTC | #3 
On 2/21/24 3:25 PM, Bjorn Helgaas wrote:
> On Tue, Feb 20, 2024 at 06:45:32PM -0800, Kuppuswamy Sathyanarayanan wrote:
>> On 2/20/24 3:55 PM, Bjorn Helgaas wrote:
>>> From: Bjorn Helgaas <bhelgaas@google.com>
>>>
>>> When booting with "pci=noaer", we don't request control of AER, but we
>>> previously *did* request control of DPC, as in the dmesg log attached at
>>> the bugzilla below:
>>>
>>>   Command line: ... pci=noaer
>>>   acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI EDR HPX-Type3]
>>>   acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug SHPCHotplug PME PCIeCapability LTR DPC]
>>>
>>> That's illegal per PCI Firmware Spec, r3.3, sec 4.5.1, table 4-5, which
>>> says:
>>>
>>>   If the operating system sets this bit [OSC_PCI_EXPRESS_DPC_CONTROL], it
>>>   must also set bit 7 of the Support field (indicating support for Error
>>>   Disconnect Recover notifications) and bits 3 and 4 of the Control field
>>>   (requesting control of PCI Express Advanced Error Reporting and the PCI
>>>   Express Capability Structure).
>>>
>>> Request DPC control only if we have also requested AER control.
>> Can you also add similar check in calculate_support call?
>>
>>         if (pci_aer_available() && IS_ENABLED(CONFIG_PCIE_EDR))
>>                 support |= OSC_PCI_EDR_SUPPORT;
> That doesn't seem right to me.  The implementation note in sec 4.6.12
> suggests that EDR Notifications may be used even when the firmware
> maintains control of AER and DPC.  Maybe that note is wrong or

It is correct. EDR notification is used when firmware retains control
of AER and DPC, but wants OS to handle the recovery action. But,
since EDR (like DPC) also touches the AER registers and depends
on OS supporting AER capability. For example, EDR driver internally
calls pci_aer_raw_clear_status(). So we need at-least ensure that AER
driver code is enabled when exposing support for EDR.

> misleading, but as written, I interpret that as meaning that it may be
> useful for the platform to know that the OS supports EDR even if it
> AER control isn't requested or granted.
>
>>> Fixes: ac1c8e35a326 ("PCI/DPC: Add Error Disconnect Recover (EDR) support")
>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218491#c12
>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>> Cc: <stable@vger.kernel.org>	# v5.7+
>>> Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>> Cc: Matthew W Carlis <mattc@purestorage.com>
>>> Cc: Keith Busch <kbusch@kernel.org>
>>> Cc: Lukas Wunner <lukas@wunner.de>
>>> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
>>> Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>
>>> ---
>>>  drivers/acpi/pci_root.c | 20 +++++++++++---------
>>>  1 file changed, 11 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>>> index 58b89b8d950e..1c16965427b3 100644
>>> --- a/drivers/acpi/pci_root.c
>>> +++ b/drivers/acpi/pci_root.c
>>> @@ -518,17 +518,19 @@ static u32 calculate_control(void)
>>>  	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
>>>  		control |= OSC_PCI_SHPC_NATIVE_HP_CONTROL;
>>>  
>>> -	if (pci_aer_available())
>>> +	if (pci_aer_available()) {
>>>  		control |= OSC_PCI_EXPRESS_AER_CONTROL;
>>>  
>>> -	/*
>>> -	 * Per the Downstream Port Containment Related Enhancements ECN to
>>> -	 * the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-5,
>>> -	 * OSC_PCI_EXPRESS_DPC_CONTROL indicates the OS supports both DPC
>>> -	 * and EDR.
>>> -	 */
>>> -	if (IS_ENABLED(CONFIG_PCIE_DPC) && IS_ENABLED(CONFIG_PCIE_EDR))
>>> -		control |= OSC_PCI_EXPRESS_DPC_CONTROL;
>>> +		/*
>>> +		 * Per PCI Firmware Spec, r3.3, sec 4.5.1, table 4-5, the
>>> +		 * OS can request DPC control only if it has advertised
>>> +		 * OSC_PCI_EDR_SUPPORT and requested both
>>> +		 * OSC_PCI_EXPRESS_CAPABILITY_CONTROL and
>> I think you mean OSC_PCI_EXPRESS_DPC_CONTROL.
> No, I just tried to rephrase the text for _OSC Control, bit 7 (quoted
> in the commit log), so I think requesting control of bits 3 and 4 (AER
> and PCIe Capability) is right.
>
>>> +		 * OSC_PCI_EXPRESS_AER_CONTROL.
>>> +		 */
>>> +		if (IS_ENABLED(CONFIG_PCIE_DPC) && IS_ENABLED(CONFIG_PCIE_EDR))
>>> +			control |= OSC_PCI_EXPRESS_DPC_CONTROL;
>> Since you are cleaning up this part, why not add a patch to remove
>> CONFIG_PCIE_EDR?
> Good idea, I'll do that, too.
>
> Bjorn
>
diff mbox series

Patch

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 58b89b8d950e..1c16965427b3 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -518,17 +518,19 @@  static u32 calculate_control(void)
 	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
 		control |= OSC_PCI_SHPC_NATIVE_HP_CONTROL;
 
-	if (pci_aer_available())
+	if (pci_aer_available()) {
 		control |= OSC_PCI_EXPRESS_AER_CONTROL;
 
-	/*
-	 * Per the Downstream Port Containment Related Enhancements ECN to
-	 * the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-5,
-	 * OSC_PCI_EXPRESS_DPC_CONTROL indicates the OS supports both DPC
-	 * and EDR.
-	 */
-	if (IS_ENABLED(CONFIG_PCIE_DPC) && IS_ENABLED(CONFIG_PCIE_EDR))
-		control |= OSC_PCI_EXPRESS_DPC_CONTROL;
+		/*
+		 * Per PCI Firmware Spec, r3.3, sec 4.5.1, table 4-5, the
+		 * OS can request DPC control only if it has advertised
+		 * OSC_PCI_EDR_SUPPORT and requested both
+		 * OSC_PCI_EXPRESS_CAPABILITY_CONTROL and
+		 * OSC_PCI_EXPRESS_AER_CONTROL.
+		 */
+		if (IS_ENABLED(CONFIG_PCIE_DPC) && IS_ENABLED(CONFIG_PCIE_EDR))
+			control |= OSC_PCI_EXPRESS_DPC_CONTROL;
+	}
 
 	return control;
 }