[PATCH v8 0/5] Simplify PCIe native ownership detection logic

[PATCH v8 0/5] Simplify PCIe native ownership detection logic

[sathyanarayanan.kuppuswamy]

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

Currently, PCIe capabilities ownership status is detected by
verifying the status of pcie_ports_native, pcie_ports_dpc_native
and _OSC negotiated results (cached in  struct pci_host_bridge
->native_* members). But this logic can be simplified, and we can
use only struct pci_host_bridge ->native_* members to detect it. 

This patchset removes the distributed checks for pcie_ports_native,
pcie_ports_dpc_native parameters.

Changes since v7:
 * Fixed "fix array_size.cocci warnings".

Changes since v6:
 * Created new patch for CONFIG_PCIEPORTBUS check in
   pci_init_host_bridge().
 * Added warning message for a case when pcie_ports_native
   overrides _OSC negotiation result.

Changes since v5:
 * Rebased on top of v5.8-rc1

Changes since v4:
 * Changed the patch set title (Original link: https://lkml.org/lkml/2020/5/26/1710)
 * Added AER/DPC dependency logic cleanup fixes.
 

Kuppuswamy Sathyanarayanan (5):
  PCI: Conditionally initialize host bridge native_* members
  ACPI/PCI: Ignore _OSC negotiation result if pcie_ports_native is set.
  ACPI/PCI: Ignore _OSC DPC negotiation result if pcie_ports_dpc_native
    is set.
  PCI/portdrv: Remove redundant pci_aer_available() check in DPC enable
    logic
  PCI/DPC: Move AER/DPC dependency checks out of DPC driver

 drivers/acpi/pci_root.c           | 61 ++++++++++++++++++++++++++-----
 drivers/pci/hotplug/pciehp_core.c |  2 +-
 drivers/pci/pci-acpi.c            |  3 --
 drivers/pci/pcie/aer.c            |  2 +-
 drivers/pci/pcie/dpc.c            |  4 +-
 drivers/pci/pcie/portdrv.h        |  2 -
 drivers/pci/pcie/portdrv_core.c   | 13 +++----
 drivers/pci/probe.c               |  4 +-
 include/linux/pci.h               |  2 +
 9 files changed, 64 insertions(+), 29 deletions(-)

-- 
2.17.1

[PATCH v8 1/5] PCI: Conditionally initialize host bridge native_* members

[sathyanarayanan.kuppuswamy]

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

If CONFIG_PCIEPORTBUS is not enabled in kernel then initialing
struct pci_host_bridge PCIe specific native_* members to "1" is
incorrect. So protect the PCIe specific member initialization
with CONFIG_PCIEPORTBUS.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/pci/probe.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 2f66988cea25..a94b97564ceb 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -588,12 +588,14 @@ 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.
 	 */
+#ifdef CONFIG_PCIEPORTBUS
 	bridge->native_aer = 1;
 	bridge->native_pcie_hotplug = 1;
-	bridge->native_shpc_hotplug = 1;
 	bridge->native_pme = 1;
 	bridge->native_ltr = 1;
 	bridge->native_dpc = 1;
+#endif
+	bridge->native_shpc_hotplug = 1;
 
 	device_initialize(&bridge->dev);
 }
-- 
2.17.1

[PATCH v8 2/5] ACPI/PCI: Ignore _OSC negotiation result if pcie_ports_native is set.

[sathyanarayanan.kuppuswamy]

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

pcie_ports_native is set only if user requests native handling
of PCIe capabilities via pcie_port_setup command line option.
User input takes precedence over _OSC based control negotiation
result. So consider the _OSC negotiated result only if
pcie_ports_native is unset.

Also, since struct pci_host_bridge ->native_* members caches the
ownership status of various PCIe capabilities, use them instead
of distributed checks for pcie_ports_native.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/acpi/pci_root.c           | 61 ++++++++++++++++++++++++++-----
 drivers/pci/hotplug/pciehp_core.c |  2 +-
 drivers/pci/pci-acpi.c            |  3 --
 drivers/pci/pcie/aer.c            |  2 +-
 drivers/pci/pcie/portdrv_core.c   |  9 ++---
 5 files changed, 56 insertions(+), 21 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index f90e841c59f5..f8981d4e044d 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -145,6 +145,17 @@ static struct pci_osc_bit_struct pci_osc_control_bit[] = {
 	{ OSC_PCI_EXPRESS_DPC_CONTROL, "DPC" },
 };
 
+static char *get_osc_desc(u32 bit)
+{
+	int i = 0;
+
+	for (i = 0; i < ARRAY_SIZE(pci_osc_control_bit); i++)
+		if (bit == pci_osc_control_bit[i].bit)
+			return pci_osc_control_bit[i].desc;
+
+	return NULL;
+}
+
 static void decode_osc_bits(struct acpi_pci_root *root, char *msg, u32 word,
 			    struct pci_osc_bit_struct *table, int size)
 {
@@ -914,18 +925,48 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 		goto out_release_info;
 
 	host_bridge = to_pci_host_bridge(bus->bridge);
-	if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
-		host_bridge->native_pcie_hotplug = 0;
+	if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL)) {
+		if (!pcie_ports_native)
+			host_bridge->native_pcie_hotplug = 0;
+		else
+			dev_warn(&bus->dev, "OS overrides %s firmware control",
+			get_osc_desc(OSC_PCI_EXPRESS_NATIVE_HP_CONTROL));
+	}
+
 	if (!(root->osc_control_set & OSC_PCI_SHPC_NATIVE_HP_CONTROL))
 		host_bridge->native_shpc_hotplug = 0;
-	if (!(root->osc_control_set & OSC_PCI_EXPRESS_AER_CONTROL))
-		host_bridge->native_aer = 0;
-	if (!(root->osc_control_set & OSC_PCI_EXPRESS_PME_CONTROL))
-		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;
+
+	if (!(root->osc_control_set & OSC_PCI_EXPRESS_AER_CONTROL)) {
+		if (!pcie_ports_native)
+			host_bridge->native_aer = 0;
+		else
+			dev_warn(&bus->dev, "OS overrides %s firmware control",
+			get_osc_desc(OSC_PCI_EXPRESS_AER_CONTROL));
+	}
+
+	if (!(root->osc_control_set & OSC_PCI_EXPRESS_PME_CONTROL)) {
+		if (!pcie_ports_native)
+			host_bridge->native_pme = 0;
+		else
+			dev_warn(&bus->dev, "OS overrides %s firmware control",
+			get_osc_desc(OSC_PCI_EXPRESS_PME_CONTROL));
+	}
+
+	if (!(root->osc_control_set & OSC_PCI_EXPRESS_LTR_CONTROL)) {
+		if (!pcie_ports_native)
+			host_bridge->native_ltr = 0;
+		else
+			dev_warn(&bus->dev, "OS overrides %s firmware control",
+			get_osc_desc(OSC_PCI_EXPRESS_LTR_CONTROL));
+	}
+
+	if (!(root->osc_control_set & OSC_PCI_EXPRESS_DPC_CONTROL)) {
+		if (!pcie_ports_native)
+			host_bridge->native_dpc = 0;
+		else
+			dev_warn(&bus->dev, "OS overrides %s firmware control",
+			get_osc_desc(OSC_PCI_EXPRESS_DPC_CONTROL));
+	}
 
 	/*
 	 * Evaluate the "PCI Boot Configuration" _DSM Function.  If it
diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index bf779f291f15..5fc999bf6f1b 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -255,7 +255,7 @@ static bool pme_is_native(struct pcie_device *dev)
 	const struct pci_host_bridge *host;
 
 	host = pci_find_host_bridge(dev->port->bus);
-	return pcie_ports_native || host->native_pme;
+	return host->native_pme;
 }
 
 static void pciehp_disable_interrupt(struct pcie_device *dev)
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 7224b1e5f2a8..e09589571a9d 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -800,9 +800,6 @@ bool pciehp_is_native(struct pci_dev *bridge)
 	if (!(slot_cap & PCI_EXP_SLTCAP_HPC))
 		return false;
 
-	if (pcie_ports_native)
-		return true;
-
 	host = pci_find_host_bridge(bridge->bus);
 	return host->native_pcie_hotplug;
 }
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 3acf56683915..d663bd9c7257 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -219,7 +219,7 @@ int pcie_aer_is_native(struct pci_dev *dev)
 	if (!dev->aer_cap)
 		return 0;
 
-	return pcie_ports_native || host->native_aer;
+	return host->native_aer;
 }
 
 int pci_enable_pcie_error_reporting(struct pci_dev *dev)
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 50a9522ab07d..ccd5e0ce5605 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -208,8 +208,7 @@ static int get_port_device_capability(struct pci_dev *dev)
 	struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
 	int services = 0;
 
-	if (dev->is_hotplug_bridge &&
-	    (pcie_ports_native || host->native_pcie_hotplug)) {
+	if (dev->is_hotplug_bridge && host->native_pcie_hotplug) {
 		services |= PCIE_PORT_SERVICE_HP;
 
 		/*
@@ -221,8 +220,7 @@ static int get_port_device_capability(struct pci_dev *dev)
 	}
 
 #ifdef CONFIG_PCIEAER
-	if (dev->aer_cap && pci_aer_available() &&
-	    (pcie_ports_native || host->native_aer)) {
+	if (dev->aer_cap && pci_aer_available() && host->native_aer) {
 		services |= PCIE_PORT_SERVICE_AER;
 
 		/*
@@ -238,8 +236,7 @@ static int get_port_device_capability(struct pci_dev *dev)
 	 * Event Collectors can also generate PMEs, but we don't handle
 	 * those yet.
 	 */
-	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT &&
-	    (pcie_ports_native || host->native_pme)) {
+	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT && host->native_pme) {
 		services |= PCIE_PORT_SERVICE_PME;
 
 		/*
-- 
2.17.1

[PATCH v8 3/5] ACPI/PCI: Ignore _OSC DPC negotiation result if pcie_ports_dpc_native is set.

[sathyanarayanan.kuppuswamy]

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

pcie_ports_dpc_native is set only if user requests native handling
of PCIe DPC capability via pcie_port_setup command line option.
User input takes precedence over _OSC based control negotiation
result. So consider the _OSC negotiated result for DPC ownership
only if pcie_ports_dpc_native is unset.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/acpi/pci_root.c         | 2 +-
 drivers/pci/pcie/dpc.c          | 3 ++-
 drivers/pci/pcie/portdrv.h      | 2 --
 drivers/pci/pcie/portdrv_core.c | 2 +-
 include/linux/pci.h             | 2 ++
 5 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index f8981d4e044d..3942bb42cb93 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -961,7 +961,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 	}
 
 	if (!(root->osc_control_set & OSC_PCI_EXPRESS_DPC_CONTROL)) {
-		if (!pcie_ports_native)
+		if (!pcie_ports_native && !pcie_ports_dpc_native)
 			host_bridge->native_dpc = 0;
 		else
 			dev_warn(&bus->dev, "OS overrides %s firmware control",
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index daa9a4153776..5b1025a2994d 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -280,11 +280,12 @@ void pci_dpc_init(struct pci_dev *pdev)
 static int dpc_probe(struct pcie_device *dev)
 {
 	struct pci_dev *pdev = dev->port;
+	struct pci_host_bridge *host = pci_find_host_bridge(pdev->bus);
 	struct device *device = &dev->device;
 	int status;
 	u16 ctl, cap;
 
-	if (!pcie_aer_is_native(pdev) && !pcie_ports_dpc_native)
+	if (!pcie_aer_is_native(pdev) && !host->native_dpc)
 		return -ENOTSUPP;
 
 	status = devm_request_threaded_irq(device, dev->irq, dpc_irq,
diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h
index af7cf237432a..0ac20feef24e 100644
--- a/drivers/pci/pcie/portdrv.h
+++ b/drivers/pci/pcie/portdrv.h
@@ -25,8 +25,6 @@
 
 #define PCIE_PORT_DEVICE_MAXSERVICES   5
 
-extern bool pcie_ports_dpc_native;
-
 #ifdef CONFIG_PCIEAER
 int pcie_aer_init(void);
 int pcie_aer_is_native(struct pci_dev *dev);
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index ccd5e0ce5605..2c0278f0fdcc 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -253,7 +253,7 @@ static int get_port_device_capability(struct pci_dev *dev)
 	 */
 	if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
 	    pci_aer_available() &&
-	    (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER)))
+	    (host->native_dpc || (services & PCIE_PORT_SERVICE_AER)))
 		services |= PCIE_PORT_SERVICE_DPC;
 
 	if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 34c1c4f45288..fe7ce06a4f40 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1556,9 +1556,11 @@ static inline int pci_irqd_intx_xlate(struct irq_domain *d,
 #ifdef CONFIG_PCIEPORTBUS
 extern bool pcie_ports_disabled;
 extern bool pcie_ports_native;
+extern bool pcie_ports_dpc_native;
 #else
 #define pcie_ports_disabled	true
 #define pcie_ports_native	false
+#define pcie_ports_dpc_native	false
 #endif
 
 #define PCIE_LINK_STATE_L0S		BIT(0)
-- 
2.17.1

[PATCH v8 4/5] PCI/portdrv: Remove redundant pci_aer_available() check in DPC enable logic

[sathyanarayanan.kuppuswamy]

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

In DPC service enable logic, check for
services & PCIE_PORT_SERVICE_AER implies pci_aer_available()
is true. So there is no need to explicitly check it again.

Also, passing pcie_ports=dpc-native in kernel command line
implies DPC needs to be enabled in native mode irrespective
of AER ownership status. So checking for pci_aer_available()
without checking for pcie_ports status is incorrect.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/pci/pcie/portdrv_core.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 2c0278f0fdcc..e257a2ca3595 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -252,7 +252,6 @@ static int get_port_device_capability(struct pci_dev *dev)
 	 * permission to use AER.
 	 */
 	if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
-	    pci_aer_available() &&
 	    (host->native_dpc || (services & PCIE_PORT_SERVICE_AER)))
 		services |= PCIE_PORT_SERVICE_DPC;
 
-- 
2.17.1

[PATCH v8 5/5] PCI/DPC: Move AER/DPC dependency checks out of DPC driver

[sathyanarayanan.kuppuswamy]

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

Currently, AER and DPC Capabilities dependency checks is
distributed between DPC and portdrv service drivers. So move
them out of DPC driver.

Also, since services & PCIE_PORT_SERVICE_AER check already
ensures AER native ownership, no need to add additional
pcie_aer_is_native() check.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/pci/pcie/dpc.c          | 3 ---
 drivers/pci/pcie/portdrv_core.c | 1 +
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 5b1025a2994d..3efbe43764f3 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -285,9 +285,6 @@ static int dpc_probe(struct pcie_device *dev)
 	int status;
 	u16 ctl, cap;
 
-	if (!pcie_aer_is_native(pdev) && !host->native_dpc)
-		return -ENOTSUPP;
-
 	status = devm_request_threaded_irq(device, dev->irq, dpc_irq,
 					   dpc_handler, IRQF_SHARED,
 					   "pcie-dpc", pdev);
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index e257a2ca3595..ffa1d9fc458e 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -252,6 +252,7 @@ static int get_port_device_capability(struct pci_dev *dev)
 	 * permission to use AER.
 	 */
 	if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
+	    host->native_dpc &&
 	    (host->native_dpc || (services & PCIE_PORT_SERVICE_AER)))
 		services |= PCIE_PORT_SERVICE_DPC;
 
-- 
2.17.1

Re: [PATCH v8 2/5] ACPI/PCI: Ignore _OSC negotiation result if pcie_ports_native is set.

[Andy Shevchenko]

On Sat, Jul 25, 2020 at 7:01 AM
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>
> pcie_ports_native is set only if user requests native handling

the user

> of PCIe capabilities via pcie_port_setup command line option.
> User input takes precedence over _OSC based control negotiation
> result. So consider the _OSC negotiated result only if
> pcie_ports_native is unset.
>
> Also, since struct pci_host_bridge ->native_* members caches the
> ownership status of various PCIe capabilities, use them instead
> of distributed checks for pcie_ports_native.

...

> +static char *get_osc_desc(u32 bit)
> +{

> +       int i = 0;

Unneeded assignment.

> +       for (i = 0; i < ARRAY_SIZE(pci_osc_control_bit); i++)
> +               if (bit == pci_osc_control_bit[i].bit)
> +                       return pci_osc_control_bit[i].desc;
> +
> +       return NULL;
> +}

...

>         host_bridge = to_pci_host_bridge(bus->bridge);
> -       if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
> -               host_bridge->native_pcie_hotplug = 0;
> +       if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL)) {
> +               if (!pcie_ports_native)
> +                       host_bridge->native_pcie_hotplug = 0;
> +               else

> +                       dev_warn(&bus->dev, "OS overrides %s firmware control",

pci_warn() ?

> +                       get_osc_desc(OSC_PCI_EXPRESS_NATIVE_HP_CONTROL));
> +       }
> +

>         if (!(root->osc_control_set & OSC_PCI_SHPC_NATIVE_HP_CONTROL))
>                 host_bridge->native_shpc_hotplug = 0;

Group them in the same way you did in the previous patch.

> -       if (!(root->osc_control_set & OSC_PCI_EXPRESS_AER_CONTROL))
> -               host_bridge->native_aer = 0;
> -       if (!(root->osc_control_set & OSC_PCI_EXPRESS_PME_CONTROL))
> -               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;
> +
> +       if (!(root->osc_control_set & OSC_PCI_EXPRESS_AER_CONTROL)) {

> +               if (!pcie_ports_native)
> +                       host_bridge->native_aer = 0;
> +               else
> +                       dev_warn(&bus->dev, "OS overrides %s firmware control",
> +                       get_osc_desc(OSC_PCI_EXPRESS_AER_CONTROL));

Looks like a lot of duplication here. Perhaps split out a helper ?

> +       }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 5/5] PCI/DPC: Move AER/DPC dependency checks out of DPC driver

[Andy Shevchenko]

On Sat, Jul 25, 2020 at 7:01 AM
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>
> Currently, AER and DPC Capabilities dependency checks is

are

> distributed between DPC and portdrv service drivers. So move
> them out of DPC driver.
>
> Also, since services & PCIE_PORT_SERVICE_AER check already
> ensures AER native ownership, no need to add additional
> pcie_aer_is_native() check.

...

>         if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
> +           host->native_dpc &&
>             (host->native_dpc || (services & PCIE_PORT_SERVICE_AER)))
>                 services |= PCIE_PORT_SERVICE_DPC;

Can you elaborate this change, please?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 0/5] Simplify PCIe native ownership detection logic

[Kuppuswamy, Sathyanarayanan]

Hi Bjorn,

On 7/24/20 8:58 PM, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
> Currently, PCIe capabilities ownership status is detected by
> verifying the status of pcie_ports_native, pcie_ports_dpc_native
> and _OSC negotiated results (cached in  struct pci_host_bridge
> ->native_* members). But this logic can be simplified, and we can
> use only struct pci_host_bridge ->native_* members to detect it.
> 

Any comments on this patch set?

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v8 1/5] PCI: Conditionally initialize host bridge native_* members

[Bjorn Helgaas]

On Fri, Jul 24, 2020 at 08:58:52PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
> If CONFIG_PCIEPORTBUS is not enabled in kernel then initialing
> struct pci_host_bridge PCIe specific native_* members to "1" is
> incorrect. So protect the PCIe specific member initialization
> with CONFIG_PCIEPORTBUS.

s/initialing/initializing/

> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  drivers/pci/probe.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 2f66988cea25..a94b97564ceb 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -588,12 +588,14 @@ 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.
>  	 */
> +#ifdef CONFIG_PCIEPORTBUS
>  	bridge->native_aer = 1;
>  	bridge->native_pcie_hotplug = 1;
> -	bridge->native_shpc_hotplug = 1;
>  	bridge->native_pme = 1;
>  	bridge->native_ltr = 1;

native_ltr isn't dependent on PCIEPORTBUS either, is it?  It's only
used for ASPM.

>  	bridge->native_dpc = 1;
> +#endif
> +	bridge->native_shpc_hotplug = 1;
>  
>  	device_initialize(&bridge->dev);
>  }
> -- 
> 2.17.1
> 

Re: [PATCH v8 2/5] ACPI/PCI: Ignore _OSC negotiation result if pcie_ports_native is set.

[Bjorn Helgaas]

On Fri, Jul 24, 2020 at 08:58:53PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
> pcie_ports_native is set only if user requests native handling
> of PCIe capabilities via pcie_port_setup command line option.
> User input takes precedence over _OSC based control negotiation
> result. So consider the _OSC negotiated result only if
> pcie_ports_native is unset.
> 
> Also, since struct pci_host_bridge ->native_* members caches the
> ownership status of various PCIe capabilities, use them instead
> of distributed checks for pcie_ports_native.
> 
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  drivers/acpi/pci_root.c           | 61 ++++++++++++++++++++++++++-----
>  drivers/pci/hotplug/pciehp_core.c |  2 +-
>  drivers/pci/pci-acpi.c            |  3 --
>  drivers/pci/pcie/aer.c            |  2 +-
>  drivers/pci/pcie/portdrv_core.c   |  9 ++---
>  5 files changed, 56 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index f90e841c59f5..f8981d4e044d 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -145,6 +145,17 @@ static struct pci_osc_bit_struct pci_osc_control_bit[] = {
>  	{ OSC_PCI_EXPRESS_DPC_CONTROL, "DPC" },
>  };
>  
> +static char *get_osc_desc(u32 bit)
> +{
> +	int i = 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(pci_osc_control_bit); i++)
> +		if (bit == pci_osc_control_bit[i].bit)
> +			return pci_osc_control_bit[i].desc;
> +
> +	return NULL;
> +}
> +
>  static void decode_osc_bits(struct acpi_pci_root *root, char *msg, u32 word,
>  			    struct pci_osc_bit_struct *table, int size)
>  {
> @@ -914,18 +925,48 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>  		goto out_release_info;
>  
>  	host_bridge = to_pci_host_bridge(bus->bridge);
> -	if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
> -		host_bridge->native_pcie_hotplug = 0;
> +	if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL)) {
> +		if (!pcie_ports_native)
> +			host_bridge->native_pcie_hotplug = 0;
> +		else
> +			dev_warn(&bus->dev, "OS overrides %s firmware control",
> +			get_osc_desc(OSC_PCI_EXPRESS_NATIVE_HP_CONTROL));

There's got to be a way to write this more concisely.  Maybe something
like this?

  #define OSC_OWNER(ctrl, bit, flag) \
    if (!(ctrl & bit)) \
      flag = 0;

  if (pcie_ports_native)
    decode_osc_control(root, "OS forcibly taking over", ~0);
  else {
    ctrl = root->osc_control_set;
    OSC_OWNER(ctrl, OSC_PCI_EXPRESS_AER_CONTROL, host_bridge->native_aer);
    OSC_OWNER(ctrl, OSC_PCI_EXPRESS_PME_CONTROL, host_bridge->native_pme);
    ...
  }

> +	}
> +
>  	if (!(root->osc_control_set & OSC_PCI_SHPC_NATIVE_HP_CONTROL))
>  		host_bridge->native_shpc_hotplug = 0;
> -	if (!(root->osc_control_set & OSC_PCI_EXPRESS_AER_CONTROL))
> -		host_bridge->native_aer = 0;
> -	if (!(root->osc_control_set & OSC_PCI_EXPRESS_PME_CONTROL))
> -		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;
> +
> +	if (!(root->osc_control_set & OSC_PCI_EXPRESS_AER_CONTROL)) {
> +		if (!pcie_ports_native)
> +			host_bridge->native_aer = 0;
> +		else
> +			dev_warn(&bus->dev, "OS overrides %s firmware control",
> +			get_osc_desc(OSC_PCI_EXPRESS_AER_CONTROL));
> +	}
> +
> +	if (!(root->osc_control_set & OSC_PCI_EXPRESS_PME_CONTROL)) {
> +		if (!pcie_ports_native)
> +			host_bridge->native_pme = 0;
> +		else
> +			dev_warn(&bus->dev, "OS overrides %s firmware control",
> +			get_osc_desc(OSC_PCI_EXPRESS_PME_CONTROL));
> +	}
> +
> +	if (!(root->osc_control_set & OSC_PCI_EXPRESS_LTR_CONTROL)) {
> +		if (!pcie_ports_native)
> +			host_bridge->native_ltr = 0;
> +		else
> +			dev_warn(&bus->dev, "OS overrides %s firmware control",
> +			get_osc_desc(OSC_PCI_EXPRESS_LTR_CONTROL));
> +	}
> +
> +	if (!(root->osc_control_set & OSC_PCI_EXPRESS_DPC_CONTROL)) {
> +		if (!pcie_ports_native)
> +			host_bridge->native_dpc = 0;
> +		else
> +			dev_warn(&bus->dev, "OS overrides %s firmware control",
> +			get_osc_desc(OSC_PCI_EXPRESS_DPC_CONTROL));
> +	}
>  
>  	/*
>  	 * Evaluate the "PCI Boot Configuration" _DSM Function.  If it
> diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
> index bf779f291f15..5fc999bf6f1b 100644
> --- a/drivers/pci/hotplug/pciehp_core.c
> +++ b/drivers/pci/hotplug/pciehp_core.c
> @@ -255,7 +255,7 @@ static bool pme_is_native(struct pcie_device *dev)
>  	const struct pci_host_bridge *host;
>  
>  	host = pci_find_host_bridge(dev->port->bus);
> -	return pcie_ports_native || host->native_pme;
> +	return host->native_pme;

I love this part!

>  }
>  
>  static void pciehp_disable_interrupt(struct pcie_device *dev)
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 7224b1e5f2a8..e09589571a9d 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -800,9 +800,6 @@ bool pciehp_is_native(struct pci_dev *bridge)
>  	if (!(slot_cap & PCI_EXP_SLTCAP_HPC))
>  		return false;
>  
> -	if (pcie_ports_native)
> -		return true;
> -
>  	host = pci_find_host_bridge(bridge->bus);
>  	return host->native_pcie_hotplug;
>  }
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index 3acf56683915..d663bd9c7257 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -219,7 +219,7 @@ int pcie_aer_is_native(struct pci_dev *dev)
>  	if (!dev->aer_cap)
>  		return 0;
>  
> -	return pcie_ports_native || host->native_aer;
> +	return host->native_aer;
>  }
>  
>  int pci_enable_pcie_error_reporting(struct pci_dev *dev)
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> index 50a9522ab07d..ccd5e0ce5605 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -208,8 +208,7 @@ static int get_port_device_capability(struct pci_dev *dev)
>  	struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
>  	int services = 0;
>  
> -	if (dev->is_hotplug_bridge &&
> -	    (pcie_ports_native || host->native_pcie_hotplug)) {
> +	if (dev->is_hotplug_bridge && host->native_pcie_hotplug) {
>  		services |= PCIE_PORT_SERVICE_HP;
>  
>  		/*
> @@ -221,8 +220,7 @@ static int get_port_device_capability(struct pci_dev *dev)
>  	}
>  
>  #ifdef CONFIG_PCIEAER
> -	if (dev->aer_cap && pci_aer_available() &&
> -	    (pcie_ports_native || host->native_aer)) {
> +	if (dev->aer_cap && pci_aer_available() && host->native_aer) {
>  		services |= PCIE_PORT_SERVICE_AER;
>  
>  		/*
> @@ -238,8 +236,7 @@ static int get_port_device_capability(struct pci_dev *dev)
>  	 * Event Collectors can also generate PMEs, but we don't handle
>  	 * those yet.
>  	 */
> -	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT &&
> -	    (pcie_ports_native || host->native_pme)) {
> +	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT && host->native_pme) {
>  		services |= PCIE_PORT_SERVICE_PME;
>  
>  		/*
> -- 
> 2.17.1
> 

Re: [PATCH v8 1/5] PCI: Conditionally initialize host bridge native_* members

[Kuppuswamy, Sathyanarayanan]

On 9/10/20 12:49 PM, Bjorn Helgaas wrote:
> On Fri, Jul 24, 2020 at 08:58:52PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>
>> If CONFIG_PCIEPORTBUS is not enabled in kernel then initialing
>> struct pci_host_bridge PCIe specific native_* members to "1" is
>> incorrect. So protect the PCIe specific member initialization
>> with CONFIG_PCIEPORTBUS.
> 
> s/initialing/initializing/
will fix it in next version.
> 
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>>   drivers/pci/probe.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index 2f66988cea25..a94b97564ceb 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -588,12 +588,14 @@ 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.
>>   	 */
>> +#ifdef CONFIG_PCIEPORTBUS
>>   	bridge->native_aer = 1;
>>   	bridge->native_pcie_hotplug = 1;
>> -	bridge->native_shpc_hotplug = 1;
>>   	bridge->native_pme = 1;
>>   	bridge->native_ltr = 1;
> 
> native_ltr isn't dependent on PCIEPORTBUS either, is it?  It's only
> used for ASPM.
Agreed. I was confused due to a comment in include/linux/pci.h

  unsigned int    native_ltr:1;           /* OS may use PCIe LTR */

> 
>>   	bridge->native_dpc = 1;
>> +#endif
>> +	bridge->native_shpc_hotplug = 1;
>>   
>>   	device_initialize(&bridge->dev);
>>   }
>> -- 
>> 2.17.1
>>

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v8 1/5] PCI: Conditionally initialize host bridge native_* members

[Kuppuswamy, Sathyanarayanan]

On 9/10/20 2:00 PM, Kuppuswamy, Sathyanarayanan wrote:
> 
> 
> On 9/10/20 12:49 PM, Bjorn Helgaas wrote:
>> On Fri, Jul 24, 2020 at 08:58:52PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>
>>> If CONFIG_PCIEPORTBUS is not enabled in kernel then initialing
>>> struct pci_host_bridge PCIe specific native_* members to "1" is
>>> incorrect. So protect the PCIe specific member initialization
>>> with CONFIG_PCIEPORTBUS.
>>
>> s/initialing/initializing/
> will fix it in next version.
>>
>>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>> ---
>>>   drivers/pci/probe.c | 4 +++-
>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>>> index 2f66988cea25..a94b97564ceb 100644
>>> --- a/drivers/pci/probe.c
>>> +++ b/drivers/pci/probe.c
>>> @@ -588,12 +588,14 @@ 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.
>>>        */
>>> +#ifdef CONFIG_PCIEPORTBUS
>>>       bridge->native_aer = 1;
>>>       bridge->native_pcie_hotplug = 1;
>>> -    bridge->native_shpc_hotplug = 1;
>>>       bridge->native_pme = 1;
>>>       bridge->native_ltr = 1;
>>
>> native_ltr isn't dependent on PCIEPORTBUS either, is it?  It's only
>> used for ASPM.
> Agreed. I was confused due to a comment in include/linux/pci.h
> 
>   unsigned int    native_ltr:1;           /* OS may use PCIe LTR */
Currently there is no code dependency between LTR and CONFIG_PCIEPORTBUS.

But I am wondering whether its correct to move LTR code under
CONFIG_PCIEPORTBUS?. As per PCIe spec v5.0 sec 7.8.2, LTR is a optional
PCIe extended capability. So why is not moved under drivers/pci/pcie/*. What is
the criteria for code to be placed under drivers/pci/pcie/*
> 
>>
>>>       bridge->native_dpc = 1;
>>> +#endif
>>> +    bridge->native_shpc_hotplug = 1;
>>>       device_initialize(&bridge->dev);
>>>   }
>>> -- 
>>> 2.17.1
>>>
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v8 2/5] ACPI/PCI: Ignore _OSC negotiation result if pcie_ports_native is set.

[Kuppuswamy, Sathyanarayanan]

On 9/10/20 1:14 PM, Bjorn Helgaas wrote:
> On Fri, Jul 24, 2020 at 08:58:53PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>
>> pcie_ports_native is set only if user requests native handling
>> of PCIe capabilities via pcie_port_setup command line option.
>> User input takes precedence over _OSC based control negotiation
>> result. So consider the _OSC negotiated result only if
>> pcie_ports_native is unset.
>>
>> Also, since struct pci_host_bridge ->native_* members caches the
>> ownership status of various PCIe capabilities, use them instead
>> of distributed checks for pcie_ports_native.
>>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>>   drivers/acpi/pci_root.c           | 61 ++++++++++++++++++++++++++-----
>>   drivers/pci/hotplug/pciehp_core.c |  2 +-
>>   drivers/pci/pci-acpi.c            |  3 --
>>   drivers/pci/pcie/aer.c            |  2 +-
>>   drivers/pci/pcie/portdrv_core.c   |  9 ++---
>>   5 files changed, 56 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index f90e841c59f5..f8981d4e044d 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -145,6 +145,17 @@ static struct pci_osc_bit_struct pci_osc_control_bit[] = {
>>   	{ OSC_PCI_EXPRESS_DPC_CONTROL, "DPC" },
>>   };
>>   

>> +		else
>> +			dev_warn(&bus->dev, "OS overrides %s firmware control",
>> +			get_osc_desc(OSC_PCI_EXPRESS_NATIVE_HP_CONTROL));
> 
> There's got to be a way to write this more concisely.  Maybe something
> like this?
> 
>    #define OSC_OWNER(ctrl, bit, flag) \
>      if (!(ctrl & bit)) \
>        flag = 0;
> 
>    if (pcie_ports_native)
>      decode_osc_control(root, "OS forcibly taking over", ~0);
BIT1 and BIT6 does not have PCIe dependency. And BIT7-31 are reserved.
So we can't force all _OSC bits based on pcie_ports_native value.
So, IM0, its better to handle PCIe features seperatly.
>    else {
>      ctrl = root->osc_control_set;
>      OSC_OWNER(ctrl, OSC_PCI_EXPRESS_AER_CONTROL, host_bridge->native_aer);
>      OSC_OWNER(ctrl, OSC_PCI_EXPRESS_PME_CONTROL, host_bridge->native_pme);
>      ...
>    }


-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v8 1/5] PCI: Conditionally initialize host bridge native_* members

[Bjorn Helgaas]

On Sun, Sep 13, 2020 at 01:49:26PM -0700, Kuppuswamy, Sathyanarayanan wrote:
> On 9/10/20 2:00 PM, Kuppuswamy, Sathyanarayanan wrote:
> > On 9/10/20 12:49 PM, Bjorn Helgaas wrote:
> > > On Fri, Jul 24, 2020 at 08:58:52PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> > > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > > 
> > > > If CONFIG_PCIEPORTBUS is not enabled in kernel then initialing
> > > > struct pci_host_bridge PCIe specific native_* members to "1" is
> > > > incorrect. So protect the PCIe specific member initialization
> > > > with CONFIG_PCIEPORTBUS.
> > > 
> > > s/initialing/initializing/
> > will fix it in next version.
> > > 
> > > > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > > ---
> > > >   drivers/pci/probe.c | 4 +++-
> > > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > > index 2f66988cea25..a94b97564ceb 100644
> > > > --- a/drivers/pci/probe.c
> > > > +++ b/drivers/pci/probe.c
> > > > @@ -588,12 +588,14 @@ 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.
> > > >        */
> > > > +#ifdef CONFIG_PCIEPORTBUS
> > > >       bridge->native_aer = 1;
> > > >       bridge->native_pcie_hotplug = 1;
> > > > -    bridge->native_shpc_hotplug = 1;
> > > >       bridge->native_pme = 1;
> > > >       bridge->native_ltr = 1;
> > > 
> > > native_ltr isn't dependent on PCIEPORTBUS either, is it?  It's only
> > > used for ASPM.
> > Agreed. I was confused due to a comment in include/linux/pci.h
> > 
> >   unsigned int    native_ltr:1;           /* OS may use PCIe LTR */

> Currently there is no code dependency between LTR and CONFIG_PCIEPORTBUS.
> 
> But I am wondering whether its correct to move LTR code under
> CONFIG_PCIEPORTBUS?. As per PCIe spec v5.0 sec 7.8.2, LTR is a
> optional PCIe extended capability. So why is not moved under
> drivers/pci/pcie/*. What is the criteria for code to be placed under
> drivers/pci/pcie/*

Some folks think drivers/pci/pcie/ should not exist, and I tend to
agree, but it's a fair bit of churn to remove it.  We do have quite a
bit of PCIe extended capability support in drivers/pci -- ats.c,
iov.c, vc.c.

There's no need to move LTR under CONFIG_PCIEPORTBUS because
CONFIG_PCIEPORTBUS enables portdrv, and AFAIK there's nothing
LTR-related that relies on portdrv.

The stuff currently in drivers/pci/pcie is mostly just portdrv and
services that depend on it.  aspm.c and ptm.c are exceptions and
really should be in drivers/pci.

> > > >       bridge->native_dpc = 1;
> > > > +#endif
> > > > +    bridge->native_shpc_hotplug = 1;
> > > >       device_initialize(&bridge->dev);
> > > >   }
> > > > -- 
> > > > 2.17.1
> > > > 
> > 
> 
> -- 
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer

Re: [PATCH v8 2/5] ACPI/PCI: Ignore _OSC negotiation result if pcie_ports_native is set.

[Bjorn Helgaas]

On Sun, Sep 13, 2020 at 01:54:38PM -0700, Kuppuswamy, Sathyanarayanan wrote:
> On 9/10/20 1:14 PM, Bjorn Helgaas wrote:
> > On Fri, Jul 24, 2020 at 08:58:53PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > 
> > > pcie_ports_native is set only if user requests native handling
> > > of PCIe capabilities via pcie_port_setup command line option.
> > > User input takes precedence over _OSC based control negotiation
> > > result. So consider the _OSC negotiated result only if
> > > pcie_ports_native is unset.
> > > 
> > > Also, since struct pci_host_bridge ->native_* members caches the
> > > ownership status of various PCIe capabilities, use them instead
> > > of distributed checks for pcie_ports_native.
> > > 
> > > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > ---
> > >   drivers/acpi/pci_root.c           | 61 ++++++++++++++++++++++++++-----
> > >   drivers/pci/hotplug/pciehp_core.c |  2 +-
> > >   drivers/pci/pci-acpi.c            |  3 --
> > >   drivers/pci/pcie/aer.c            |  2 +-
> > >   drivers/pci/pcie/portdrv_core.c   |  9 ++---
> > >   5 files changed, 56 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> > > index f90e841c59f5..f8981d4e044d 100644
> > > --- a/drivers/acpi/pci_root.c
> > > +++ b/drivers/acpi/pci_root.c
> > > @@ -145,6 +145,17 @@ static struct pci_osc_bit_struct pci_osc_control_bit[] = {
> > >   	{ OSC_PCI_EXPRESS_DPC_CONTROL, "DPC" },
> > >   };
> 
> > > +		else
> > > +			dev_warn(&bus->dev, "OS overrides %s firmware control",
> > > +			get_osc_desc(OSC_PCI_EXPRESS_NATIVE_HP_CONTROL));
> > 
> > There's got to be a way to write this more concisely.  Maybe something
> > like this?
> > 
> >    #define OSC_OWNER(ctrl, bit, flag) \
> >      if (!(ctrl & bit)) \
> >        flag = 0;
> > 
> >    if (pcie_ports_native)
> >      decode_osc_control(root, "OS forcibly taking over", ~0);
>
> BIT1 and BIT6 does not have PCIe dependency. And BIT7-31 are reserved.
> So we can't force all _OSC bits based on pcie_ports_native value.
> So, IM0, its better to handle PCIe features seperatly.

Yes, we may need to handle a few bits specially.  But we need to
figure out a nicer-looking way of coding this.  It's too cumbersome to
check pcie_ports_native and log a message for every _OSC bit
individually.

> >    else {
> >      ctrl = root->osc_control_set;
> >      OSC_OWNER(ctrl, OSC_PCI_EXPRESS_AER_CONTROL, host_bridge->native_aer);
> >      OSC_OWNER(ctrl, OSC_PCI_EXPRESS_PME_CONTROL, host_bridge->native_pme);
> >      ...
> >    }
> 
> 
> -- 
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer

Re: [PATCH v8 1/5] PCI: Conditionally initialize host bridge native_* members

[Kuppuswamy, Sathyanarayanan]

On 9/15/20 3:17 PM, Bjorn Helgaas wrote:
> On Sun, Sep 13, 2020 at 01:49:26PM -0700, Kuppuswamy, Sathyanarayanan wrote:
>> On 9/10/20 2:00 PM, Kuppuswamy, Sathyanarayanan wrote:
>>> On 9/10/20 12:49 PM, Bjorn Helgaas wrote:
>>>> On Fri, Jul 24, 2020 at 08:58:52PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>>>>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>>>

>>
>> But I am wondering whether its correct to move LTR code under
>> CONFIG_PCIEPORTBUS?. As per PCIe spec v5.0 sec 7.8.2, LTR is a
>> optional PCIe extended capability. So why is not moved under
>> drivers/pci/pcie/*. What is the criteria for code to be placed under
>> drivers/pci/pcie/*
> 
> Some folks think drivers/pci/pcie/ should not exist, and I tend to
> agree, but it's a fair bit of churn to remove it.  We do have quite a
> bit of PCIe extended capability support in drivers/pci -- ats.c,
> iov.c, vc.c.
> 
> There's no need to move LTR under CONFIG_PCIEPORTBUS because
> CONFIG_PCIEPORTBUS enables portdrv, and AFAIK there's nothing
> LTR-related that relies on portdrv.
> 
> The stuff currently in drivers/pci/pcie is mostly just portdrv and
> services that depend on it.  aspm.c and ptm.c are exceptions and
> really should be in drivers/pci.
Thanks for the clarification. I will remove the CONFIG_PCIEPORTBUS
dependency.
> 

>>>>> -- 
>>>>> 2.17.1
>>>>>
>>>
>>
>> -- 
>> Sathyanarayanan Kuppuswamy
>> Linux Kernel Developer

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v8 2/5] ACPI/PCI: Ignore _OSC negotiation result if pcie_ports_native is set.

[Kuppuswamy, Sathyanarayanan]

On 9/15/20 3:20 PM, Bjorn Helgaas wrote:
> On Sun, Sep 13, 2020 at 01:54:38PM -0700, Kuppuswamy, Sathyanarayanan wrote:
>> On 9/10/20 1:14 PM, Bjorn Helgaas wrote:
>>> On Fri, Jul 24, 2020 at 08:58:53PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>>>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>>
>>>> pcie_ports_native is set only if user requests native handling
>>>> of PCIe capabilities via pcie_port_setup command line option.
>>>> User input takes precedence over _OSC based control negotiation
>>>> result. So consider the _OSC negotiated result only if
>>>> pcie_ports_native is unset.
>>>>
>>>> Also, since struct pci_host_bridge ->native_* members caches the
>>>> ownership status of various PCIe capabilities, use them instead
>>>> of distributed checks for pcie_ports_native.
>>>>
>>>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>> ---
>>>>    drivers/acpi/pci_root.c           | 61 ++++++++++++++++++++++++++-----
>>>>    drivers/pci/hotplug/pciehp_core.c |  2 +-
>>>>    drivers/pci/pci-acpi.c            |  3 --
>>>>    drivers/pci/pcie/aer.c            |  2 +-
>>>>    drivers/pci/pcie/portdrv_core.c   |  9 ++---
>>>>    5 files changed, 56 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>>>> index f90e841c59f5..f8981d4e044d 100644
>>>> --- a/drivers/acpi/pci_root.c
>>>> +++ b/drivers/acpi/pci_root.c
>>>> @@ -145,6 +145,17 @@ static struct pci_osc_bit_struct pci_osc_control_bit[] = {
>>>>    	{ OSC_PCI_EXPRESS_DPC_CONTROL, "DPC" },
>>>>    };
>>
>>>> +		else
>>>> +			dev_warn(&bus->dev, "OS overrides %s firmware control",
>>>> +			get_osc_desc(OSC_PCI_EXPRESS_NATIVE_HP_CONTROL));
>>>
>>> There's got to be a way to write this more concisely.  Maybe something
>>> like this?
>>>
>>>     #define OSC_OWNER(ctrl, bit, flag) \
>>>       if (!(ctrl & bit)) \
>>>         flag = 0;
>>>
>>>     if (pcie_ports_native)
>>>       decode_osc_control(root, "OS forcibly taking over", ~0);
>>
>> BIT1 and BIT6 does not have PCIe dependency. And BIT7-31 are reserved.
>> So we can't force all _OSC bits based on pcie_ports_native value.
>> So, IM0, its better to handle PCIe features seperatly.
> 
> Yes, we may need to handle a few bits specially.  But we need to
> figure out a nicer-looking way of coding this.  It's too cumbersome to
> check pcie_ports_native and log a message for every _OSC bit
> individually.
ok. Let me check how to simplify it.
> 
>>>     else {
>>>       ctrl = root->osc_control_set;
>>>       OSC_OWNER(ctrl, OSC_PCI_EXPRESS_AER_CONTROL, host_bridge->native_aer);
>>>       OSC_OWNER(ctrl, OSC_PCI_EXPRESS_PME_CONTROL, host_bridge->native_pme);
>>>       ...
>>>     }
>>
>>
>> -- 
>> Sathyanarayanan Kuppuswamy
>> Linux Kernel Developer

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer