[v12,8/8] PCI/ACPI: Enable EDR support

Commit Message

Kuppuswamy Sathyanarayanan Jan. 12, 2020, 10:44 p.m. UTC

From: Kuppuswamy Sathyanarayanan <[e-mail]>

As per PCI firmware specification r3.2 Downstream Port Containment
Related Enhancements ECN, sec 4.5.1, OS must implement following steps
to enable/use EDR feature.

1. OS can use bit 7 of _OSC Control Field to negotiate control over
Downstream Port Containment (DPC) configuration of PCIe port. After _OSC
negotiation, firmware will Set this bit to grant OS control over PCIe
DPC configuration and Clear it if this feature was requested and denied,
or was not requested.

2. Also, if OS supports EDR, it should expose its support to BIOS by
setting bit 7 of _OSC Support Field. And if OS sets bit 7 of _OSC
Control Field it must also expose support for EDR by setting bit 7 of
_OSC Support Field.

Cc: Bjorn Helgaas <[e-mail]>
Cc: "Rafael J. Wysocki" <[e-mail]>
Cc: Len Brown <[e-mail]>
Signed-off-by: Kuppuswamy Sathyanarayanan <[e-mail]>
Acked-by: Keith Busch <[e-mail]>
Tested-by: Huong Nguyen <[e-mail]>
Tested-by: Austin Bolen <[e-mail]>
---
 drivers/acpi/pci_root.c         | 9 +++++++++
 drivers/pci/pcie/portdrv_core.c | 7 +++++--
 drivers/pci/probe.c             | 1 +
 include/linux/acpi.h            | 6 ++++--
 include/linux/pci.h             | 3 ++-
 5 files changed, 21 insertions(+), 5 deletions(-)

Comments

Bjorn Helgaas Jan. 16, 2020, 10:10 p.m. UTC | #1

On Sun, Jan 12, 2020 at 02:44:02PM -0800, [e-mail] wrote:
> From: Kuppuswamy Sathyanarayanan <[e-mail]>
> 
> As per PCI firmware specification r3.2 Downstream Port Containment
> Related Enhancements ECN, sec 4.5.1, OS must implement following steps
> to enable/use EDR feature.
> 
> 1. OS can use bit 7 of _OSC Control Field to negotiate control over
> Downstream Port Containment (DPC) configuration of PCIe port. After _OSC
> negotiation, firmware will Set this bit to grant OS control over PCIe
> DPC configuration and Clear it if this feature was requested and denied,
> or was not requested.
> 
> 2. Also, if OS supports EDR, it should expose its support to BIOS by
> setting bit 7 of _OSC Support Field. And if OS sets bit 7 of _OSC
> Control Field it must also expose support for EDR by setting bit 7 of
> _OSC Support Field.
> 
> Cc: Bjorn Helgaas <[e-mail]>
> Cc: "Rafael J. Wysocki" <[e-mail]>
> Cc: Len Brown <[e-mail]>
> Signed-off-by: Kuppuswamy Sathyanarayanan <[e-mail]>
> Acked-by: Keith Busch <[e-mail]>
> Tested-by: Huong Nguyen <[e-mail]>
> Tested-by: Austin Bolen <[e-mail]>
> ---
>  drivers/acpi/pci_root.c         | 9 +++++++++
>  drivers/pci/pcie/portdrv_core.c | 7 +++++--
>  drivers/pci/probe.c             | 1 +
>  include/linux/acpi.h            | 6 ++++--
>  include/linux/pci.h             | 3 ++-
>  5 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index d1e666ef3fcc..134e20474dfd 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -131,6 +131,7 @@ static struct pci_osc_bit_struct pci_osc_support_bit[] = {
>  	{ OSC_PCI_CLOCK_PM_SUPPORT, "ClockPM" },
>  	{ OSC_PCI_SEGMENT_GROUPS_SUPPORT, "Segments" },
>  	{ OSC_PCI_MSI_SUPPORT, "MSI" },
> +	{ OSC_PCI_EDR_SUPPORT, "EDR" },
>  	{ OSC_PCI_HPX_TYPE_3_SUPPORT, "HPX-Type3" },
>  };
>  
> @@ -141,6 +142,7 @@ static struct pci_osc_bit_struct pci_osc_control_bit[] = {
>  	{ OSC_PCI_EXPRESS_AER_CONTROL, "AER" },
>  	{ OSC_PCI_EXPRESS_CAPABILITY_CONTROL, "PCIeCapability" },
>  	{ OSC_PCI_EXPRESS_LTR_CONTROL, "LTR" },
> +	{ OSC_PCI_EXPRESS_DPC_CONTROL, "DPC" },
>  };
>  
>  static void decode_osc_bits(struct acpi_pci_root *root, char *msg, u32 word,
> @@ -440,6 +442,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>  		support |= OSC_PCI_ASPM_SUPPORT | OSC_PCI_CLOCK_PM_SUPPORT;
>  	if (pci_msi_enabled())
>  		support |= OSC_PCI_MSI_SUPPORT;
> +	if (IS_ENABLED(CONFIG_PCIE_EDR))
> +		support |= OSC_PCI_EDR_SUPPORT;
>  
>  	decode_osc_support(root, "OS supports", support);
>  	status = acpi_pci_osc_support(root, support);
> @@ -487,6 +491,9 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>  			control |= OSC_PCI_EXPRESS_AER_CONTROL;
>  	}
>  
> +	if (IS_ENABLED(CONFIG_PCIE_DPC))
> +		control |= OSC_PCI_EXPRESS_DPC_CONTROL;

The ECN [1] says:

  If this bit is set by the OS, this indicates that it supports both
  native OS control and firmware ownership models (i.e. Error
  Disconnect Recover notification) of Downstream Port Containment.

But if CONFIG_PCIE_DPC=y and CONFIG_PCIE_EDR is not set, we will set
OSC_PCI_EXPRESS_DPC_CONTROL even though we don't support EDR.  That
doesn't seem to match what the spec says.

I think this needs to be something like:

  if (IS_ENABLED(CONFIG_PCIE_DPC) && IS_ENABLED(CONFIG_PCIE_EDR))
    control |= OSC_PCI_EXPRESS_DPC_CONTROL;

[1] Downstream Port Containment related Enhancements, PCI ECN, Jan 28,
2019, Section 4.5.1, Table 4-5.

>  	requested = control;
>  	status = acpi_pci_osc_control_set(handle, &control,
>  					  OSC_PCI_EXPRESS_CAPABILITY_CONTROL);
> @@ -916,6 +923,8 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>  		host_bridge->native_pme = 0;
>  	if (!(root->osc_control_set & OSC_PCI_EXPRESS_LTR_CONTROL))
>  		host_bridge->native_ltr = 0;
> +	if (!(root->osc_control_set & OSC_PCI_EXPRESS_DPC_CONTROL))
> +		host_bridge->native_dpc = 0;
>  
>  	/*
>  	 * Evaluate the "PCI Boot Configuration" _DSM Function.  If it
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> index 5075cb9e850c..009742c865d6 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -253,10 +253,13 @@ static int get_port_device_capability(struct pci_dev *dev)
>  	/*
>  	 * With dpc-native, allow Linux to use DPC even if it doesn't have
>  	 * permission to use AER.
> +	 * If EDR support is enabled in OS, then even if AER is not handled in
> +	 * OS, DPC service can be enabled.
>  	 */
>  	if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
> -	    pci_aer_available() &&
> -	    (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER)))
> +	    ((IS_ENABLED(CONFIG_PCIE_EDR) && !host->native_dpc) ||
> +	    (pci_aer_available() &&
> +	    (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER)))))
>  		services |= PCIE_PORT_SERVICE_DPC;
>  
>  	if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 512cb4312ddd..c9a9c5b42e72 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -598,6 +598,7 @@ static void pci_init_host_bridge(struct pci_host_bridge *bridge)
>  	bridge->native_shpc_hotplug = 1;
>  	bridge->native_pme = 1;
>  	bridge->native_ltr = 1;
> +	bridge->native_dpc = 1;
>  }
>  
>  struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 0f37a7d5fa77..0a7aaa452a98 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -515,8 +515,9 @@ extern bool osc_pc_lpi_support_confirmed;
>  #define OSC_PCI_CLOCK_PM_SUPPORT		0x00000004
>  #define OSC_PCI_SEGMENT_GROUPS_SUPPORT		0x00000008
>  #define OSC_PCI_MSI_SUPPORT			0x00000010
> +#define OSC_PCI_EDR_SUPPORT			0x00000080
>  #define OSC_PCI_HPX_TYPE_3_SUPPORT		0x00000100
> -#define OSC_PCI_SUPPORT_MASKS			0x0000011f
> +#define OSC_PCI_SUPPORT_MASKS			0x0000019f
>  
>  /* PCI Host Bridge _OSC: Capabilities DWORD 3: Control Field */
>  #define OSC_PCI_EXPRESS_NATIVE_HP_CONTROL	0x00000001
> @@ -525,7 +526,8 @@ extern bool osc_pc_lpi_support_confirmed;
>  #define OSC_PCI_EXPRESS_AER_CONTROL		0x00000008
>  #define OSC_PCI_EXPRESS_CAPABILITY_CONTROL	0x00000010
>  #define OSC_PCI_EXPRESS_LTR_CONTROL		0x00000020
> -#define OSC_PCI_CONTROL_MASKS			0x0000003f
> +#define OSC_PCI_EXPRESS_DPC_CONTROL		0x00000080
> +#define OSC_PCI_CONTROL_MASKS			0x000000bf
>  
>  #define ACPI_GSB_ACCESS_ATTRIB_QUICK		0x00000002
>  #define ACPI_GSB_ACCESS_ATTRIB_SEND_RCV         0x00000004
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index c393dff2d66f..0b7c63c7888d 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -510,8 +510,9 @@ struct pci_host_bridge {
>  	unsigned int	native_shpc_hotplug:1;	/* OS may use SHPC hotplug */
>  	unsigned int	native_pme:1;		/* OS may use PCIe PME */
>  	unsigned int	native_ltr:1;		/* OS may use PCIe LTR */
> -	unsigned int	preserve_config:1;	/* Preserve FW resource setup */
> +	unsigned int	native_dpc:1;		/* OS may use PCIe DPC */
>  
> +	unsigned int	preserve_config:1;	/* Preserve FW resource setup */

The blank line should stay after the bitfields.  Then this will look
like the simple insertion of native_dpc, as it should.

>  	/* Resource alignment requirements */
>  	resource_size_t (*align_resource)(struct pci_dev *dev,
>  			const struct resource *res,
> -- 
> 2.21.0
>

Kuppuswamy Sathyanarayanan Jan. 16, 2020, 10:24 p.m. UTC | #2

On 1/16/20 2:10 PM, Bjorn Helgaas wrote:
> On Sun, Jan 12, 2020 at 02:44:02PM -0800, [e-mail] wrote:
>> From: Kuppuswamy Sathyanarayanan <[e-mail]>
>>
>> As per PCI firmware specification r3.2 Downstream Port Containment
>> Related Enhancements ECN, sec 4.5.1, OS must implement following steps
>> to enable/use EDR feature.
>>
>> 1. OS can use bit 7 of _OSC Control Field to negotiate control over
>> Downstream Port Containment (DPC) configuration of PCIe port. After _OSC
>> negotiation, firmware will Set this bit to grant OS control over PCIe
>> DPC configuration and Clear it if this feature was requested and denied,
>> or was not requested.
>>
>> 2. Also, if OS supports EDR, it should expose its support to BIOS by
>> setting bit 7 of _OSC Support Field. And if OS sets bit 7 of _OSC
>> Control Field it must also expose support for EDR by setting bit 7 of
>> _OSC Support Field.
>>
>> Cc: Bjorn Helgaas <[e-mail]>
>> Cc: "Rafael J. Wysocki" <[e-mail]>
>> Cc: Len Brown <[e-mail]>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <[e-mail]>
>> Acked-by: Keith Busch <[e-mail]>
>> Tested-by: Huong Nguyen <[e-mail]>
>> Tested-by: Austin Bolen <[e-mail]>
>> ---
>>   drivers/acpi/pci_root.c         | 9 +++++++++
>>   drivers/pci/pcie/portdrv_core.c | 7 +++++--
>>   drivers/pci/probe.c             | 1 +
>>   include/linux/acpi.h            | 6 ++++--
>>   include/linux/pci.h             | 3 ++-
>>   5 files changed, 21 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index d1e666ef3fcc..134e20474dfd 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -131,6 +131,7 @@ static struct pci_osc_bit_struct pci_osc_support_bit[] = {
>>   	{ OSC_PCI_CLOCK_PM_SUPPORT, "ClockPM" },
>>   	{ OSC_PCI_SEGMENT_GROUPS_SUPPORT, "Segments" },
>>   	{ OSC_PCI_MSI_SUPPORT, "MSI" },
>> +	{ OSC_PCI_EDR_SUPPORT, "EDR" },
>>   	{ OSC_PCI_HPX_TYPE_3_SUPPORT, "HPX-Type3" },
>>   };
>>   
>> @@ -141,6 +142,7 @@ static struct pci_osc_bit_struct pci_osc_control_bit[] = {
>>   	{ OSC_PCI_EXPRESS_AER_CONTROL, "AER" },
>>   	{ OSC_PCI_EXPRESS_CAPABILITY_CONTROL, "PCIeCapability" },
>>   	{ OSC_PCI_EXPRESS_LTR_CONTROL, "LTR" },
>> +	{ OSC_PCI_EXPRESS_DPC_CONTROL, "DPC" },
>>   };
>>   
>>   static void decode_osc_bits(struct acpi_pci_root *root, char *msg, u32 word,
>> @@ -440,6 +442,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>>   		support |= OSC_PCI_ASPM_SUPPORT | OSC_PCI_CLOCK_PM_SUPPORT;
>>   	if (pci_msi_enabled())
>>   		support |= OSC_PCI_MSI_SUPPORT;
>> +	if (IS_ENABLED(CONFIG_PCIE_EDR))
>> +		support |= OSC_PCI_EDR_SUPPORT;
>>   
>>   	decode_osc_support(root, "OS supports", support);
>>   	status = acpi_pci_osc_support(root, support);
>> @@ -487,6 +491,9 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>>   			control |= OSC_PCI_EXPRESS_AER_CONTROL;
>>   	}
>>   
>> +	if (IS_ENABLED(CONFIG_PCIE_DPC))
>> +		control |= OSC_PCI_EXPRESS_DPC_CONTROL;
> The ECN [1] says:
>
>    If this bit is set by the OS, this indicates that it supports both
>    native OS control and firmware ownership models (i.e. Error
>    Disconnect Recover notification) of Downstream Port Containment.
>
> But if CONFIG_PCIE_DPC=y and CONFIG_PCIE_EDR is not set, we will set
> OSC_PCI_EXPRESS_DPC_CONTROL even though we don't support EDR.  That
> doesn't seem to match what the spec says.
Agreed.
>
> I think this needs to be something like:
>
>    if (IS_ENABLED(CONFIG_PCIE_DPC) && IS_ENABLED(CONFIG_PCIE_EDR))
>      control |= OSC_PCI_EXPRESS_DPC_CONTROL;
Since CONFIG_PCIE_EDR has dependency on CONFIG_PCIE_DPC,
I think we can just use IS_ENABLED(CONFIG_PCIE_EDR) here.

I will fix it and send an update. You want to me to send a new
patch set of just an update to this patch ?
>
> [1] Downstream Port Containment related Enhancements, PCI ECN, Jan 28,
> 2019, Section 4.5.1, Table 4-5.
>
>>   	requested = control;
>>   	status = acpi_pci_osc_control_set(handle, &control,
>>   					  OSC_PCI_EXPRESS_CAPABILITY_CONTROL);
>> @@ -916,6 +923,8 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>>   		host_bridge->native_pme = 0;
>>   	if (!(root->osc_control_set & OSC_PCI_EXPRESS_LTR_CONTROL))
>>   		host_bridge->native_ltr = 0;
>> +	if (!(root->osc_control_set & OSC_PCI_EXPRESS_DPC_CONTROL))
>> +		host_bridge->native_dpc = 0;
>>   
>>   	/*
>>   	 * Evaluate the "PCI Boot Configuration" _DSM Function.  If it
>> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
>> index 5075cb9e850c..009742c865d6 100644
>> --- a/drivers/pci/pcie/portdrv_core.c
>> +++ b/drivers/pci/pcie/portdrv_core.c
>> @@ -253,10 +253,13 @@ static int get_port_device_capability(struct pci_dev *dev)
>>   	/*
>>   	 * With dpc-native, allow Linux to use DPC even if it doesn't have
>>   	 * permission to use AER.
>> +	 * If EDR support is enabled in OS, then even if AER is not handled in
>> +	 * OS, DPC service can be enabled.
>>   	 */
>>   	if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
>> -	    pci_aer_available() &&
>> -	    (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER)))
>> +	    ((IS_ENABLED(CONFIG_PCIE_EDR) && !host->native_dpc) ||
>> +	    (pci_aer_available() &&
>> +	    (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER)))))
>>   		services |= PCIE_PORT_SERVICE_DPC;
>>   
>>   	if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index 512cb4312ddd..c9a9c5b42e72 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -598,6 +598,7 @@ static void pci_init_host_bridge(struct pci_host_bridge *bridge)
>>   	bridge->native_shpc_hotplug = 1;
>>   	bridge->native_pme = 1;
>>   	bridge->native_ltr = 1;
>> +	bridge->native_dpc = 1;
>>   }
>>   
>>   struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index 0f37a7d5fa77..0a7aaa452a98 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -515,8 +515,9 @@ extern bool osc_pc_lpi_support_confirmed;
>>   #define OSC_PCI_CLOCK_PM_SUPPORT		0x00000004
>>   #define OSC_PCI_SEGMENT_GROUPS_SUPPORT		0x00000008
>>   #define OSC_PCI_MSI_SUPPORT			0x00000010
>> +#define OSC_PCI_EDR_SUPPORT			0x00000080
>>   #define OSC_PCI_HPX_TYPE_3_SUPPORT		0x00000100
>> -#define OSC_PCI_SUPPORT_MASKS			0x0000011f
>> +#define OSC_PCI_SUPPORT_MASKS			0x0000019f
>>   
>>   /* PCI Host Bridge _OSC: Capabilities DWORD 3: Control Field */
>>   #define OSC_PCI_EXPRESS_NATIVE_HP_CONTROL	0x00000001
>> @@ -525,7 +526,8 @@ extern bool osc_pc_lpi_support_confirmed;
>>   #define OSC_PCI_EXPRESS_AER_CONTROL		0x00000008
>>   #define OSC_PCI_EXPRESS_CAPABILITY_CONTROL	0x00000010
>>   #define OSC_PCI_EXPRESS_LTR_CONTROL		0x00000020
>> -#define OSC_PCI_CONTROL_MASKS			0x0000003f
>> +#define OSC_PCI_EXPRESS_DPC_CONTROL		0x00000080
>> +#define OSC_PCI_CONTROL_MASKS			0x000000bf
>>   
>>   #define ACPI_GSB_ACCESS_ATTRIB_QUICK		0x00000002
>>   #define ACPI_GSB_ACCESS_ATTRIB_SEND_RCV         0x00000004
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index c393dff2d66f..0b7c63c7888d 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -510,8 +510,9 @@ struct pci_host_bridge {
>>   	unsigned int	native_shpc_hotplug:1;	/* OS may use SHPC hotplug */
>>   	unsigned int	native_pme:1;		/* OS may use PCIe PME */
>>   	unsigned int	native_ltr:1;		/* OS may use PCIe LTR */
>> -	unsigned int	preserve_config:1;	/* Preserve FW resource setup */
>> +	unsigned int	native_dpc:1;		/* OS may use PCIe DPC */
>>   
>> +	unsigned int	preserve_config:1;	/* Preserve FW resource setup */
> The blank line should stay after the bitfields.  Then this will look
> like the simple insertion of native_dpc, as it should.
>
>>   	/* Resource alignment requirements */
>>   	resource_size_t (*align_resource)(struct pci_dev *dev,
>>   			const struct resource *res,
>> -- 
>> 2.21.0
>>

Bjorn Helgaas Jan. 16, 2020, 10:58 p.m. UTC | #3

On Thu, Jan 16, 2020 at 02:24:10PM -0800, Kuppuswamy Sathyanarayanan wrote:
> 
> On 1/16/20 2:10 PM, Bjorn Helgaas wrote:
> > On Sun, Jan 12, 2020 at 02:44:02PM -0800, [e-mail] wrote:
> > > From: Kuppuswamy Sathyanarayanan <[e-mail]>
> > > 
> > > As per PCI firmware specification r3.2 Downstream Port Containment
> > > Related Enhancements ECN, sec 4.5.1, OS must implement following steps
> > > to enable/use EDR feature.
> > > 
> > > 1. OS can use bit 7 of _OSC Control Field to negotiate control over
> > > Downstream Port Containment (DPC) configuration of PCIe port. After _OSC
> > > negotiation, firmware will Set this bit to grant OS control over PCIe
> > > DPC configuration and Clear it if this feature was requested and denied,
> > > or was not requested.
> > > 
> > > 2. Also, if OS supports EDR, it should expose its support to BIOS by
> > > setting bit 7 of _OSC Support Field. And if OS sets bit 7 of _OSC
> > > Control Field it must also expose support for EDR by setting bit 7 of
> > > _OSC Support Field.
> > > 
> > > Cc: Bjorn Helgaas <[e-mail]>
> > > Cc: "Rafael J. Wysocki" <[e-mail]>
> > > Cc: Len Brown <[e-mail]>
> > > Signed-off-by: Kuppuswamy Sathyanarayanan <[e-mail]>
> > > Acked-by: Keith Busch <[e-mail]>
> > > Tested-by: Huong Nguyen <[e-mail]>
> > > Tested-by: Austin Bolen <[e-mail]>
> > > ---
> > >   drivers/acpi/pci_root.c         | 9 +++++++++
> > >   drivers/pci/pcie/portdrv_core.c | 7 +++++--
> > >   drivers/pci/probe.c             | 1 +
> > >   include/linux/acpi.h            | 6 ++++--
> > >   include/linux/pci.h             | 3 ++-
> > >   5 files changed, 21 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> > > index d1e666ef3fcc..134e20474dfd 100644
> > > --- a/drivers/acpi/pci_root.c
> > > +++ b/drivers/acpi/pci_root.c
> > > @@ -131,6 +131,7 @@ static struct pci_osc_bit_struct pci_osc_support_bit[] = {
> > >   	{ OSC_PCI_CLOCK_PM_SUPPORT, "ClockPM" },
> > >   	{ OSC_PCI_SEGMENT_GROUPS_SUPPORT, "Segments" },
> > >   	{ OSC_PCI_MSI_SUPPORT, "MSI" },
> > > +	{ OSC_PCI_EDR_SUPPORT, "EDR" },
> > >   	{ OSC_PCI_HPX_TYPE_3_SUPPORT, "HPX-Type3" },
> > >   };
> > > @@ -141,6 +142,7 @@ static struct pci_osc_bit_struct pci_osc_control_bit[] = {
> > >   	{ OSC_PCI_EXPRESS_AER_CONTROL, "AER" },
> > >   	{ OSC_PCI_EXPRESS_CAPABILITY_CONTROL, "PCIeCapability" },
> > >   	{ OSC_PCI_EXPRESS_LTR_CONTROL, "LTR" },
> > > +	{ OSC_PCI_EXPRESS_DPC_CONTROL, "DPC" },
> > >   };
> > >   static void decode_osc_bits(struct acpi_pci_root *root, char *msg, u32 word,
> > > @@ -440,6 +442,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
> > >   		support |= OSC_PCI_ASPM_SUPPORT | OSC_PCI_CLOCK_PM_SUPPORT;
> > >   	if (pci_msi_enabled())
> > >   		support |= OSC_PCI_MSI_SUPPORT;
> > > +	if (IS_ENABLED(CONFIG_PCIE_EDR))
> > > +		support |= OSC_PCI_EDR_SUPPORT;
> > >   	decode_osc_support(root, "OS supports", support);
> > >   	status = acpi_pci_osc_support(root, support);
> > > @@ -487,6 +491,9 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
> > >   			control |= OSC_PCI_EXPRESS_AER_CONTROL;
> > >   	}
> > > +	if (IS_ENABLED(CONFIG_PCIE_DPC))
> > > +		control |= OSC_PCI_EXPRESS_DPC_CONTROL;
> > The ECN [1] says:
> > 
> >    If this bit is set by the OS, this indicates that it supports both
> >    native OS control and firmware ownership models (i.e. Error
> >    Disconnect Recover notification) of Downstream Port Containment.
> > 
> > But if CONFIG_PCIE_DPC=y and CONFIG_PCIE_EDR is not set, we will set
> > OSC_PCI_EXPRESS_DPC_CONTROL even though we don't support EDR.  That
> > doesn't seem to match what the spec says.
> Agreed.
> > 
> > I think this needs to be something like:
> > 
> >    if (IS_ENABLED(CONFIG_PCIE_DPC) && IS_ENABLED(CONFIG_PCIE_EDR))
> >      control |= OSC_PCI_EXPRESS_DPC_CONTROL;
> Since CONFIG_PCIE_EDR has dependency on CONFIG_PCIE_DPC,
> I think we can just use IS_ENABLED(CONFIG_PCIE_EDR) here.
> 
> I will fix it and send an update. You want to me to send a new
> patch set of just an update to this patch ?

I'm still working through the rest of these, so I'd suggest making
this change in your local copy but waiting to send it for a few days.
Unless there's more substantial stuff to change, I can fix this
myself.

Bjorn

Kuppuswamy Sathyanarayanan Jan. 16, 2020, 11:01 p.m. UTC | #4

On 1/16/20 2:58 PM, Bjorn Helgaas wrote:
>
> I'm still working through the rest of these, so I'd suggest making
> this change in your local copy but waiting to send it for a few days.
> Unless there's more substantial stuff to change, I can fix this
> myself.
Will do. Thanks.
>
> Bjorn
>

Bjorn Helgaas Jan. 17, 2020, 8:41 p.m. UTC | #5

On Sun, Jan 12, 2020 at 02:44:02PM -0800, [e-mail] wrote:
> From: Kuppuswamy Sathyanarayanan <[e-mail]>
> 
> As per PCI firmware specification r3.2 Downstream Port Containment
> Related Enhancements ECN, sec 4.5.1, OS must implement following steps
> to enable/use EDR feature.
> 
> 1. OS can use bit 7 of _OSC Control Field to negotiate control over
> Downstream Port Containment (DPC) configuration of PCIe port. After _OSC
> negotiation, firmware will Set this bit to grant OS control over PCIe
> DPC configuration and Clear it if this feature was requested and denied,
> or was not requested.
> 
> 2. Also, if OS supports EDR, it should expose its support to BIOS by
> setting bit 7 of _OSC Support Field. And if OS sets bit 7 of _OSC
> Control Field it must also expose support for EDR by setting bit 7 of
> _OSC Support Field.

> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -253,10 +253,13 @@ static int get_port_device_capability(struct pci_dev *dev)
>  	/*
>  	 * With dpc-native, allow Linux to use DPC even if it doesn't have
>  	 * permission to use AER.
> +	 * If EDR support is enabled in OS, then even if AER is not handled in
> +	 * OS, DPC service can be enabled.

Can you clarify this comment?  It talks about AER, but the code you
added:

  (IS_ENABLED(CONFIG_PCIE_EDR) && !host->native_dpc)

doesn't have anything to do with AER.

>  	 */
>  	if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
> -	    pci_aer_available() &&
> -	    (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER)))
> +	    ((IS_ENABLED(CONFIG_PCIE_EDR) && !host->native_dpc) ||
> +	    (pci_aer_available() &&
> +	    (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER)))))
>  		services |= PCIE_PORT_SERVICE_DPC;

Kuppuswamy Sathyanarayanan Jan. 17, 2020, 11:04 p.m. UTC | #6

On 1/17/20 12:41 PM, Bjorn Helgaas wrote:
> On Sun, Jan 12, 2020 at 02:44:02PM -0800, [e-mail] wrote:
>> From: Kuppuswamy Sathyanarayanan <[e-mail]>
>>
>> As per PCI firmware specification r3.2 Downstream Port Containment
>> Related Enhancements ECN, sec 4.5.1, OS must implement following steps
>> to enable/use EDR feature.
>>
>> 1. OS can use bit 7 of _OSC Control Field to negotiate control over
>> Downstream Port Containment (DPC) configuration of PCIe port. After _OSC
>> negotiation, firmware will Set this bit to grant OS control over PCIe
>> DPC configuration and Clear it if this feature was requested and denied,
>> or was not requested.
>>
>> 2. Also, if OS supports EDR, it should expose its support to BIOS by
>> setting bit 7 of _OSC Support Field. And if OS sets bit 7 of _OSC
>> Control Field it must also expose support for EDR by setting bit 7 of
>> _OSC Support Field.
>> --- a/drivers/pci/pcie/portdrv_core.c
>> +++ b/drivers/pci/pcie/portdrv_core.c
>> @@ -253,10 +253,13 @@ static int get_port_device_capability(struct pci_dev *dev)
>>   	/*
>>   	 * With dpc-native, allow Linux to use DPC even if it doesn't have
>>   	 * permission to use AER.
>> +	 * If EDR support is enabled in OS, then even if AER is not handled in
>> +	 * OS, DPC service can be enabled.
> Can you clarify this comment?  It talks about AER, but the code you
> added:
Previously with condition
(pci_aer_available() && (pcie_ports_dpc_native || (services & 
PCIE_PORT_SERVICE_AER)))
we have only checked whether AER is enabled and supported before 
enabling DPC.

But with EDR support, we enumerate DPC driver even if AER is not 
supported or handled
in OS. And the or condition (IS_ENABLED(CONFIG_PCIE_EDR) && 
!host->native_dpc) adds this
support.


>    (IS_ENABLED(CONFIG_PCIE_EDR) && !host->native_dpc)
>
> doesn't have anything to do with AER.
>
>>   	 */
>>   	if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
>> -	    pci_aer_available() &&
>> -	    (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER)))
>> +	    ((IS_ENABLED(CONFIG_PCIE_EDR) && !host->native_dpc) ||
>> +	    (pci_aer_available() &&
>> +	    (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER)))))
>>   		services |= PCIE_PORT_SERVICE_DPC;

Patch

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index d1e666ef3fcc..134e20474dfd 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -131,6 +131,7 @@  static struct pci_osc_bit_struct pci_osc_support_bit[] = {
 	{ OSC_PCI_CLOCK_PM_SUPPORT, "ClockPM" },
 	{ OSC_PCI_SEGMENT_GROUPS_SUPPORT, "Segments" },
 	{ OSC_PCI_MSI_SUPPORT, "MSI" },
+	{ OSC_PCI_EDR_SUPPORT, "EDR" },
 	{ OSC_PCI_HPX_TYPE_3_SUPPORT, "HPX-Type3" },
 };
 
@@ -141,6 +142,7 @@  static struct pci_osc_bit_struct pci_osc_control_bit[] = {
 	{ OSC_PCI_EXPRESS_AER_CONTROL, "AER" },
 	{ OSC_PCI_EXPRESS_CAPABILITY_CONTROL, "PCIeCapability" },
 	{ OSC_PCI_EXPRESS_LTR_CONTROL, "LTR" },
+	{ OSC_PCI_EXPRESS_DPC_CONTROL, "DPC" },
 };
 
 static void decode_osc_bits(struct acpi_pci_root *root, char *msg, u32 word,
@@ -440,6 +442,8 @@  static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 		support |= OSC_PCI_ASPM_SUPPORT | OSC_PCI_CLOCK_PM_SUPPORT;
 	if (pci_msi_enabled())
 		support |= OSC_PCI_MSI_SUPPORT;
+	if (IS_ENABLED(CONFIG_PCIE_EDR))
+		support |= OSC_PCI_EDR_SUPPORT;
 
 	decode_osc_support(root, "OS supports", support);
 	status = acpi_pci_osc_support(root, support);
@@ -487,6 +491,9 @@  static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 			control |= OSC_PCI_EXPRESS_AER_CONTROL;
 	}
 
+	if (IS_ENABLED(CONFIG_PCIE_DPC))
+		control |= OSC_PCI_EXPRESS_DPC_CONTROL;
+
 	requested = control;
 	status = acpi_pci_osc_control_set(handle, &control,
 					  OSC_PCI_EXPRESS_CAPABILITY_CONTROL);
@@ -916,6 +923,8 @@  struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 		host_bridge->native_pme = 0;
 	if (!(root->osc_control_set & OSC_PCI_EXPRESS_LTR_CONTROL))
 		host_bridge->native_ltr = 0;
+	if (!(root->osc_control_set & OSC_PCI_EXPRESS_DPC_CONTROL))
+		host_bridge->native_dpc = 0;
 
 	/*
 	 * Evaluate the "PCI Boot Configuration" _DSM Function.  If it
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 5075cb9e850c..009742c865d6 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -253,10 +253,13 @@  static int get_port_device_capability(struct pci_dev *dev)
 	/*
 	 * With dpc-native, allow Linux to use DPC even if it doesn't have
 	 * permission to use AER.
+	 * If EDR support is enabled in OS, then even if AER is not handled in
+	 * OS, DPC service can be enabled.
 	 */
 	if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
-	    pci_aer_available() &&
-	    (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER)))
+	    ((IS_ENABLED(CONFIG_PCIE_EDR) && !host->native_dpc) ||
+	    (pci_aer_available() &&
+	    (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER)))))
 		services |= PCIE_PORT_SERVICE_DPC;
 
 	if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 512cb4312ddd..c9a9c5b42e72 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -598,6 +598,7 @@  static void pci_init_host_bridge(struct pci_host_bridge *bridge)
 	bridge->native_shpc_hotplug = 1;
 	bridge->native_pme = 1;
 	bridge->native_ltr = 1;
+	bridge->native_dpc = 1;
 }
 
 struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 0f37a7d5fa77..0a7aaa452a98 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -515,8 +515,9 @@  extern bool osc_pc_lpi_support_confirmed;
 #define OSC_PCI_CLOCK_PM_SUPPORT		0x00000004
 #define OSC_PCI_SEGMENT_GROUPS_SUPPORT		0x00000008
 #define OSC_PCI_MSI_SUPPORT			0x00000010
+#define OSC_PCI_EDR_SUPPORT			0x00000080
 #define OSC_PCI_HPX_TYPE_3_SUPPORT		0x00000100
-#define OSC_PCI_SUPPORT_MASKS			0x0000011f
+#define OSC_PCI_SUPPORT_MASKS			0x0000019f
 
 /* PCI Host Bridge _OSC: Capabilities DWORD 3: Control Field */
 #define OSC_PCI_EXPRESS_NATIVE_HP_CONTROL	0x00000001
@@ -525,7 +526,8 @@  extern bool osc_pc_lpi_support_confirmed;
 #define OSC_PCI_EXPRESS_AER_CONTROL		0x00000008
 #define OSC_PCI_EXPRESS_CAPABILITY_CONTROL	0x00000010
 #define OSC_PCI_EXPRESS_LTR_CONTROL		0x00000020
-#define OSC_PCI_CONTROL_MASKS			0x0000003f
+#define OSC_PCI_EXPRESS_DPC_CONTROL		0x00000080
+#define OSC_PCI_CONTROL_MASKS			0x000000bf
 
 #define ACPI_GSB_ACCESS_ATTRIB_QUICK		0x00000002
 #define ACPI_GSB_ACCESS_ATTRIB_SEND_RCV         0x00000004
diff --git a/include/linux/pci.h b/include/linux/pci.h
index c393dff2d66f..0b7c63c7888d 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -510,8 +510,9 @@  struct pci_host_bridge {
 	unsigned int	native_shpc_hotplug:1;	/* OS may use SHPC hotplug */
 	unsigned int	native_pme:1;		/* OS may use PCIe PME */
 	unsigned int	native_ltr:1;		/* OS may use PCIe LTR */
-	unsigned int	preserve_config:1;	/* Preserve FW resource setup */
+	unsigned int	native_dpc:1;		/* OS may use PCIe DPC */
 
+	unsigned int	preserve_config:1;	/* Preserve FW resource setup */
 	/* Resource alignment requirements */
 	resource_size_t (*align_resource)(struct pci_dev *dev,
 			const struct resource *res,