* [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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).