All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v8 0/5] Simplify PCIe native ownership detection logic
@ 2020-07-25  3:58 sathyanarayanan.kuppuswamy
  2020-07-25  3:58 ` [PATCH v8 1/5] PCI: Conditionally initialize host bridge native_* members sathyanarayanan.kuppuswamy
                   ` (5 more replies)
  0 siblings, 6 replies; 18+ messages in thread
From: sathyanarayanan.kuppuswamy @ 2020-07-25  3:58 UTC (permalink / raw)
  To: bhelgaas; +Cc: linux-pci, linux-kernel, ashok.raj, 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


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v8 1/5] PCI: Conditionally initialize host bridge native_* members
  2020-07-25  3:58 [PATCH v8 0/5] Simplify PCIe native ownership detection logic sathyanarayanan.kuppuswamy
@ 2020-07-25  3:58 ` sathyanarayanan.kuppuswamy
  2020-09-10 19:49   ` Bjorn Helgaas
  2020-07-25  3:58 ` [PATCH v8 2/5] ACPI/PCI: Ignore _OSC negotiation result if pcie_ports_native is set sathyanarayanan.kuppuswamy
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 18+ messages in thread
From: sathyanarayanan.kuppuswamy @ 2020-07-25  3:58 UTC (permalink / raw)
  To: bhelgaas; +Cc: linux-pci, linux-kernel, ashok.raj, 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


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v8 2/5] ACPI/PCI: Ignore _OSC negotiation result if pcie_ports_native is set.
  2020-07-25  3:58 [PATCH v8 0/5] Simplify PCIe native ownership detection logic sathyanarayanan.kuppuswamy
  2020-07-25  3:58 ` [PATCH v8 1/5] PCI: Conditionally initialize host bridge native_* members sathyanarayanan.kuppuswamy
@ 2020-07-25  3:58 ` sathyanarayanan.kuppuswamy
  2020-07-25 10:35   ` Andy Shevchenko
  2020-09-10 20:14   ` Bjorn Helgaas
  2020-07-25  3:58 ` [PATCH v8 3/5] ACPI/PCI: Ignore _OSC DPC negotiation result if pcie_ports_dpc_native " sathyanarayanan.kuppuswamy
                   ` (3 subsequent siblings)
  5 siblings, 2 replies; 18+ messages in thread
From: sathyanarayanan.kuppuswamy @ 2020-07-25  3:58 UTC (permalink / raw)
  To: bhelgaas; +Cc: linux-pci, linux-kernel, ashok.raj, 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


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v8 3/5] ACPI/PCI: Ignore _OSC DPC negotiation result if pcie_ports_dpc_native is set.
  2020-07-25  3:58 [PATCH v8 0/5] Simplify PCIe native ownership detection logic sathyanarayanan.kuppuswamy
  2020-07-25  3:58 ` [PATCH v8 1/5] PCI: Conditionally initialize host bridge native_* members sathyanarayanan.kuppuswamy
  2020-07-25  3:58 ` [PATCH v8 2/5] ACPI/PCI: Ignore _OSC negotiation result if pcie_ports_native is set sathyanarayanan.kuppuswamy
@ 2020-07-25  3:58 ` sathyanarayanan.kuppuswamy
  2020-07-25  3:58 ` [PATCH v8 4/5] PCI/portdrv: Remove redundant pci_aer_available() check in DPC enable logic sathyanarayanan.kuppuswamy
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 18+ messages in thread
From: sathyanarayanan.kuppuswamy @ 2020-07-25  3:58 UTC (permalink / raw)
  To: bhelgaas; +Cc: linux-pci, linux-kernel, ashok.raj, 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


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v8 4/5] PCI/portdrv: Remove redundant pci_aer_available() check in DPC enable logic
  2020-07-25  3:58 [PATCH v8 0/5] Simplify PCIe native ownership detection logic sathyanarayanan.kuppuswamy
                   ` (2 preceding siblings ...)
  2020-07-25  3:58 ` [PATCH v8 3/5] ACPI/PCI: Ignore _OSC DPC negotiation result if pcie_ports_dpc_native " sathyanarayanan.kuppuswamy
@ 2020-07-25  3:58 ` sathyanarayanan.kuppuswamy
  2020-07-25  3:58 ` [PATCH v8 5/5] PCI/DPC: Move AER/DPC dependency checks out of DPC driver sathyanarayanan.kuppuswamy
  2020-08-27 18:22 ` [PATCH v8 0/5] Simplify PCIe native ownership detection logic Kuppuswamy, Sathyanarayanan
  5 siblings, 0 replies; 18+ messages in thread
From: sathyanarayanan.kuppuswamy @ 2020-07-25  3:58 UTC (permalink / raw)
  To: bhelgaas; +Cc: linux-pci, linux-kernel, ashok.raj, 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


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v8 5/5] PCI/DPC: Move AER/DPC dependency checks out of DPC driver
  2020-07-25  3:58 [PATCH v8 0/5] Simplify PCIe native ownership detection logic sathyanarayanan.kuppuswamy
                   ` (3 preceding siblings ...)
  2020-07-25  3:58 ` [PATCH v8 4/5] PCI/portdrv: Remove redundant pci_aer_available() check in DPC enable logic sathyanarayanan.kuppuswamy
@ 2020-07-25  3:58 ` sathyanarayanan.kuppuswamy
  2020-07-25 10:38   ` Andy Shevchenko
  2020-08-27 18:22 ` [PATCH v8 0/5] Simplify PCIe native ownership detection logic Kuppuswamy, Sathyanarayanan
  5 siblings, 1 reply; 18+ messages in thread
From: sathyanarayanan.kuppuswamy @ 2020-07-25  3:58 UTC (permalink / raw)
  To: bhelgaas; +Cc: linux-pci, linux-kernel, ashok.raj, 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


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH v8 2/5] ACPI/PCI: Ignore _OSC negotiation result if pcie_ports_native is set.
  2020-07-25  3:58 ` [PATCH v8 2/5] ACPI/PCI: Ignore _OSC negotiation result if pcie_ports_native is set sathyanarayanan.kuppuswamy
@ 2020-07-25 10:35   ` Andy Shevchenko
  2020-09-10 20:14   ` Bjorn Helgaas
  1 sibling, 0 replies; 18+ messages in thread
From: Andy Shevchenko @ 2020-07-25 10:35 UTC (permalink / raw)
  To: Kuppuswamy Sathyanarayanan
  Cc: Bjorn Helgaas, linux-pci, Linux Kernel Mailing List, Raj Ashok

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v8 5/5] PCI/DPC: Move AER/DPC dependency checks out of DPC driver
  2020-07-25  3:58 ` [PATCH v8 5/5] PCI/DPC: Move AER/DPC dependency checks out of DPC driver sathyanarayanan.kuppuswamy
@ 2020-07-25 10:38   ` Andy Shevchenko
  0 siblings, 0 replies; 18+ messages in thread
From: Andy Shevchenko @ 2020-07-25 10:38 UTC (permalink / raw)
  To: Kuppuswamy Sathyanarayanan
  Cc: Bjorn Helgaas, linux-pci, Linux Kernel Mailing List, Raj Ashok

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v8 0/5] Simplify PCIe native ownership detection logic
  2020-07-25  3:58 [PATCH v8 0/5] Simplify PCIe native ownership detection logic sathyanarayanan.kuppuswamy
                   ` (4 preceding siblings ...)
  2020-07-25  3:58 ` [PATCH v8 5/5] PCI/DPC: Move AER/DPC dependency checks out of DPC driver sathyanarayanan.kuppuswamy
@ 2020-08-27 18:22 ` Kuppuswamy, Sathyanarayanan
  5 siblings, 0 replies; 18+ messages in thread
From: Kuppuswamy, Sathyanarayanan @ 2020-08-27 18:22 UTC (permalink / raw)
  To: bhelgaas; +Cc: linux-pci, linux-kernel, ashok.raj

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v8 1/5] PCI: Conditionally initialize host bridge native_* members
  2020-07-25  3:58 ` [PATCH v8 1/5] PCI: Conditionally initialize host bridge native_* members sathyanarayanan.kuppuswamy
@ 2020-09-10 19:49   ` Bjorn Helgaas
  2020-09-10 21:00     ` Kuppuswamy, Sathyanarayanan
  0 siblings, 1 reply; 18+ messages in thread
From: Bjorn Helgaas @ 2020-09-10 19:49 UTC (permalink / raw)
  To: sathyanarayanan.kuppuswamy; +Cc: bhelgaas, linux-pci, linux-kernel, ashok.raj

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
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v8 2/5] ACPI/PCI: Ignore _OSC negotiation result if pcie_ports_native is set.
  2020-07-25  3:58 ` [PATCH v8 2/5] ACPI/PCI: Ignore _OSC negotiation result if pcie_ports_native is set sathyanarayanan.kuppuswamy
  2020-07-25 10:35   ` Andy Shevchenko
@ 2020-09-10 20:14   ` Bjorn Helgaas
  2020-09-13 20:54     ` Kuppuswamy, Sathyanarayanan
  1 sibling, 1 reply; 18+ messages in thread
From: Bjorn Helgaas @ 2020-09-10 20:14 UTC (permalink / raw)
  To: sathyanarayanan.kuppuswamy; +Cc: bhelgaas, linux-pci, linux-kernel, ashok.raj

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
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v8 1/5] PCI: Conditionally initialize host bridge native_* members
  2020-09-10 19:49   ` Bjorn Helgaas
@ 2020-09-10 21:00     ` Kuppuswamy, Sathyanarayanan
  2020-09-13 20:49       ` Kuppuswamy, Sathyanarayanan
  0 siblings, 1 reply; 18+ messages in thread
From: Kuppuswamy, Sathyanarayanan @ 2020-09-10 21:00 UTC (permalink / raw)
  To: Bjorn Helgaas; +Cc: bhelgaas, linux-pci, linux-kernel, ashok.raj



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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v8 1/5] PCI: Conditionally initialize host bridge native_* members
  2020-09-10 21:00     ` Kuppuswamy, Sathyanarayanan
@ 2020-09-13 20:49       ` Kuppuswamy, Sathyanarayanan
  2020-09-15 22:17         ` Bjorn Helgaas
  0 siblings, 1 reply; 18+ messages in thread
From: Kuppuswamy, Sathyanarayanan @ 2020-09-13 20:49 UTC (permalink / raw)
  To: Bjorn Helgaas; +Cc: bhelgaas, linux-pci, linux-kernel, ashok.raj



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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v8 2/5] ACPI/PCI: Ignore _OSC negotiation result if pcie_ports_native is set.
  2020-09-10 20:14   ` Bjorn Helgaas
@ 2020-09-13 20:54     ` Kuppuswamy, Sathyanarayanan
  2020-09-15 22:20       ` Bjorn Helgaas
  0 siblings, 1 reply; 18+ messages in thread
From: Kuppuswamy, Sathyanarayanan @ 2020-09-13 20:54 UTC (permalink / raw)
  To: Bjorn Helgaas; +Cc: bhelgaas, linux-pci, linux-kernel, ashok.raj



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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v8 1/5] PCI: Conditionally initialize host bridge native_* members
  2020-09-13 20:49       ` Kuppuswamy, Sathyanarayanan
@ 2020-09-15 22:17         ` Bjorn Helgaas
  2020-09-16  2:30           ` Kuppuswamy, Sathyanarayanan
  0 siblings, 1 reply; 18+ messages in thread
From: Bjorn Helgaas @ 2020-09-15 22:17 UTC (permalink / raw)
  To: Kuppuswamy, Sathyanarayanan; +Cc: bhelgaas, linux-pci, linux-kernel, ashok.raj

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v8 2/5] ACPI/PCI: Ignore _OSC negotiation result if pcie_ports_native is set.
  2020-09-13 20:54     ` Kuppuswamy, Sathyanarayanan
@ 2020-09-15 22:20       ` Bjorn Helgaas
  2020-09-16  2:33         ` Kuppuswamy, Sathyanarayanan
  0 siblings, 1 reply; 18+ messages in thread
From: Bjorn Helgaas @ 2020-09-15 22:20 UTC (permalink / raw)
  To: Kuppuswamy, Sathyanarayanan; +Cc: bhelgaas, linux-pci, linux-kernel, ashok.raj

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v8 1/5] PCI: Conditionally initialize host bridge native_* members
  2020-09-15 22:17         ` Bjorn Helgaas
@ 2020-09-16  2:30           ` Kuppuswamy, Sathyanarayanan
  0 siblings, 0 replies; 18+ messages in thread
From: Kuppuswamy, Sathyanarayanan @ 2020-09-16  2:30 UTC (permalink / raw)
  To: Bjorn Helgaas; +Cc: bhelgaas, linux-pci, linux-kernel, ashok.raj



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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v8 2/5] ACPI/PCI: Ignore _OSC negotiation result if pcie_ports_native is set.
  2020-09-15 22:20       ` Bjorn Helgaas
@ 2020-09-16  2:33         ` Kuppuswamy, Sathyanarayanan
  0 siblings, 0 replies; 18+ messages in thread
From: Kuppuswamy, Sathyanarayanan @ 2020-09-16  2:33 UTC (permalink / raw)
  To: Bjorn Helgaas; +Cc: bhelgaas, linux-pci, linux-kernel, ashok.raj



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

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2020-09-16  2:33 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-25  3:58 [PATCH v8 0/5] Simplify PCIe native ownership detection logic sathyanarayanan.kuppuswamy
2020-07-25  3:58 ` [PATCH v8 1/5] PCI: Conditionally initialize host bridge native_* members sathyanarayanan.kuppuswamy
2020-09-10 19:49   ` Bjorn Helgaas
2020-09-10 21:00     ` Kuppuswamy, Sathyanarayanan
2020-09-13 20:49       ` Kuppuswamy, Sathyanarayanan
2020-09-15 22:17         ` Bjorn Helgaas
2020-09-16  2:30           ` Kuppuswamy, Sathyanarayanan
2020-07-25  3:58 ` [PATCH v8 2/5] ACPI/PCI: Ignore _OSC negotiation result if pcie_ports_native is set sathyanarayanan.kuppuswamy
2020-07-25 10:35   ` Andy Shevchenko
2020-09-10 20:14   ` Bjorn Helgaas
2020-09-13 20:54     ` Kuppuswamy, Sathyanarayanan
2020-09-15 22:20       ` Bjorn Helgaas
2020-09-16  2:33         ` Kuppuswamy, Sathyanarayanan
2020-07-25  3:58 ` [PATCH v8 3/5] ACPI/PCI: Ignore _OSC DPC negotiation result if pcie_ports_dpc_native " sathyanarayanan.kuppuswamy
2020-07-25  3:58 ` [PATCH v8 4/5] PCI/portdrv: Remove redundant pci_aer_available() check in DPC enable logic sathyanarayanan.kuppuswamy
2020-07-25  3:58 ` [PATCH v8 5/5] PCI/DPC: Move AER/DPC dependency checks out of DPC driver sathyanarayanan.kuppuswamy
2020-07-25 10:38   ` Andy Shevchenko
2020-08-27 18:22 ` [PATCH v8 0/5] Simplify PCIe native ownership detection logic Kuppuswamy, Sathyanarayanan

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.