Subject: [PATCH v9 0/5] Simplify PCIe native ownership detection logic

From: Kuppuswamy Sathyanarayanan <[email protected]>

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 v8:
* Simplified setting _OSC ownwership logic
* Moved bridge->native_ltr out of #ifdef CONFIG_PCIEPORTBUS.

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 | 37 ++++++++++++++++++++++---------
drivers/pci/hotplug/pciehp_core.c | 2 +-
drivers/pci/pci-acpi.c | 3 ---
drivers/pci/pcie/aer.c | 2 +-
drivers/pci/pcie/dpc.c | 3 ---
drivers/pci/pcie/portdrv.h | 2 --
drivers/pci/pcie/portdrv_core.c | 13 +++++------
drivers/pci/probe.c | 6 +++--
include/linux/acpi.h | 2 ++
include/linux/pci.h | 2 ++
10 files changed, 42 insertions(+), 30 deletions(-)

--
2.17.1


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

From: Kuppuswamy Sathyanarayanan <[email protected]>

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 <[email protected]>
---
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

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

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 <[email protected]>
---
drivers/acpi/pci_root.c | 33 +++++++++++++++++++++----------
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 +++------
include/linux/acpi.h | 2 ++
6 files changed, 30 insertions(+), 21 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index f90e841c59f5..9749b7abdd7e 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -43,6 +43,10 @@ static int acpi_pci_root_scan_dependent(struct acpi_device *adev)
| OSC_PCI_CLOCK_PM_SUPPORT \
| OSC_PCI_MSI_SUPPORT)

+#define OSC_OWNER(ctrl, bit, flag) \
+ if (!(ctrl & bit)) \
+ flag = 0;
+
static const struct acpi_device_id root_device_ids[] = {
{"PNP0A03", 0},
{"", 0},
@@ -889,6 +893,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
struct pci_bus *bus;
struct pci_host_bridge *host_bridge;
union acpi_object *obj;
+ u32 ctrl;

info->root = root;
info->bridge = device;
@@ -914,18 +919,26 @@ 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 (pcie_ports_native) {
+ decode_osc_control(root, "OS forcibly taking over",
+ OSC_PCI_EXPRESS_CONTROL_MASKS);
+ } else {
+ ctrl = root->osc_control_set;
+ OSC_OWNER(ctrl, OSC_PCI_EXPRESS_NATIVE_HP_CONTROL,
+ host_bridge->native_pcie_hotplug);
+ OSC_OWNER(ctrl, OSC_PCI_EXPRESS_AER_CONTROL,
+ host_bridge->native_aer);
+ OSC_OWNER(ctrl, OSC_PCI_EXPRESS_PME_CONTROL,
+ host_bridge->native_pme);
+ OSC_OWNER(ctrl, OSC_PCI_EXPRESS_LTR_CONTROL,
+ host_bridge->native_ltr);
+ OSC_OWNER(ctrl, OSC_PCI_EXPRESS_DPC_CONTROL,
+ host_bridge->native_dpc);
+ }
+
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;

/*
* 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 ad3393930ecb..d1831e6bf60a 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -256,7 +256,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 d5869a03f748..2e53d8bf1fe5 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 65dff5f3457a..79bb441139c2 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;

/*
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 1e4cdc6c7ae2..a1b2b16f34ce 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -569,6 +569,8 @@ extern bool osc_pc_lpi_support_confirmed;
#define OSC_PCI_EXPRESS_LTR_CONTROL 0x00000020
#define OSC_PCI_EXPRESS_DPC_CONTROL 0x00000080
#define OSC_PCI_CONTROL_MASKS 0x000000bf
+/* Masks specific to PCIe Capabilities */
+#define OSC_PCI_EXPRESS_CONTROL_MASKS 0x000000bd

#define ACPI_GSB_ACCESS_ATTRIB_QUICK 0x00000002
#define ACPI_GSB_ACCESS_ATTRIB_SEND_RCV 0x00000004
--
2.17.1

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

Hi Bjorn,

On 9/27/20 6:11 PM, Kuppuswamy Sathyanarayanan wrote:
> 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.
>
Did you get this patch set or do I need to send it again?
> This patchset removes the distributed checks for pcie_ports_native,
> pcie_ports_dpc_native parameters.
>
> Changes since v8:
> * Simplified setting _OSC ownwership logic
> * Moved bridge->native_ltr out of #ifdef CONFIG_PCIEPORTBUS.
>
> 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 | 37 ++++++++++++++++++++++---------
> drivers/pci/hotplug/pciehp_core.c | 2 +-
> drivers/pci/pci-acpi.c | 3 ---
> drivers/pci/pcie/aer.c | 2 +-
> drivers/pci/pcie/dpc.c | 3 ---
> drivers/pci/pcie/portdrv.h | 2 --
> drivers/pci/pcie/portdrv_core.c | 13 +++++------
> drivers/pci/probe.c | 6 +++--
> include/linux/acpi.h | 2 ++
> include/linux/pci.h | 2 ++
> 10 files changed, 42 insertions(+), 30 deletions(-)
>

--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

2020-09-29 21:05:53

by Bjorn Helgaas

[permalink] [raw]
Subject: Re: [PATCH v9 0/5] Simplify PCIe native ownership detection logic

On Tue, Sep 29, 2020 at 4:00 PM Kuppuswamy, Sathyanarayanan
<[email protected]> wrote:
>
> Hi Bjorn,
>
> On 9/27/20 6:11 PM, Kuppuswamy Sathyanarayanan wrote:
> > 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.
> >
> Did you get this patch set or do I need to send it again?

I got it, thanks. More importantly, it looks like linux-pci got it, too :)

$ b4 am -om/ https://lore.kernel.org/r/a640e9043db50f5adee8e38f5c60ff8423f3f598.1600457297.git.sathyanarayanan.kuppuswamy@linux.intel.com
Looking up https://lore.kernel.org/r/a640e9043db50f5adee8e38f5c60ff8423f3f598.1600457297.git.sathyanarayanan.kuppuswamy%40linux.intel.com
Grabbing thread from lore.kernel.org/linux-pci
Reduced thread to strict matches only (18->10)
Analyzing 10 messages in the thread
---
Writing m/v9_20200922_sathyanarayanan_kuppuswamy_simplify_pcie_native_ownership_detection_logic.mbx
[PATCH v9 1/5] PCI: Conditionally initialize host bridge native_* members
[PATCH v9 2/5] ACPI/PCI: Ignore _OSC negotiation result if
pcie_ports_native is set.
[PATCH v9 3/5] ACPI/PCI: Ignore _OSC DPC negotiation result if
pcie_ports_dpc_native is set.
[PATCH v9 4/5] PCI/portdrv: Remove redundant pci_aer_available()
check in DPC enable logic
[PATCH v9 5/5] PCI/DPC: Move AER/DPC dependency checks out of DPC driver
---
Total patches: 5
---
Cover: m/v9_20200922_sathyanarayanan_kuppuswamy_simplify_pcie_native_ownership_detection_logic.cover
Link: https://lore.kernel.org/r/[email protected]
Base: not found (applies clean to current tree)
git am m/v9_20200922_sathyanarayanan_kuppuswamy_simplify_pcie_native_ownership_detection_logic.mbx

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

Hi,

On 9/29/20 2:04 PM, Bjorn Helgaas wrote:
> On Tue, Sep 29, 2020 at 4:00 PM Kuppuswamy, Sathyanarayanan
> <[email protected]> wrote:
>>
>> Hi Bjorn,
>>
>> On 9/27/20 6:11 PM, Kuppuswamy Sathyanarayanan wrote:
>>> 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.
>>>
>> Did you get this patch set or do I need to send it again?
>
> I got it, thanks. More importantly, it looks like linux-pci got it, too :)
Thanks for the confirmation.
>
> $ b4 am -om/ https://lore.kernel.org/r/a640e9043db50f5adee8e38f5c60ff8423f3f598.1600457297.git.sathyanarayanan.kuppuswamy@linux.intel.com
> Looking up https://lore.kernel.org/r/a640e9043db50f5adee8e38f5c60ff8423f3f598.1600457297.git.sathyanarayanan.kuppuswamy%40linux.intel.com
> Grabbing thread from lore.kernel.org/linux-pci
> Reduced thread to strict matches only (18->10)
> Analyzing 10 messages in the thread
> ---
> Writing m/v9_20200922_sathyanarayanan_kuppuswamy_simplify_pcie_native_ownership_detection_logic.mbx
> [PATCH v9 1/5] PCI: Conditionally initialize host bridge native_* members
> [PATCH v9 2/5] ACPI/PCI: Ignore _OSC negotiation result if
> pcie_ports_native is set.
> [PATCH v9 3/5] ACPI/PCI: Ignore _OSC DPC negotiation result if
> pcie_ports_dpc_native is set.
> [PATCH v9 4/5] PCI/portdrv: Remove redundant pci_aer_available()
> check in DPC enable logic
> [PATCH v9 5/5] PCI/DPC: Move AER/DPC dependency checks out of DPC driver
> ---
> Total patches: 5
> ---
> Cover: m/v9_20200922_sathyanarayanan_kuppuswamy_simplify_pcie_native_ownership_detection_logic.cover
> Link: https://lore.kernel.org/r/[email protected]
> Base: not found (applies clean to current tree)
> git am m/v9_20200922_sathyanarayanan_kuppuswamy_simplify_pcie_native_ownership_detection_logic.mbx
>

--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer