diff mbox series

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

Message ID a5da506cb5cd5d590d88da8537ad01c0167840da.1595006564.git.sathyanarayanan.kuppuswamy@linux.intel.com (mailing list archive)
State Superseded, archived
Delegated to: Bjorn Helgaas
Headers show
Series Simplify PCIe native ownership detection logic | expand

Commit Message

Kuppuswamy Sathyanarayanan July 17, 2020, 5:23 p.m. UTC 
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           | 62 ++++++++++++++++++++++++++-----
 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, 57 insertions(+), 21 deletions(-)

Comments

kernel test robot July 18, 2020, 8:01 a.m. UTC | #1 
Hi,

I love your patch! Perhaps something to improve:

[auto build test WARNING on pci/next]
[also build test WARNING on pm/linux-next linux/master linus/master v5.8-rc5]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/sathyanarayanan-kuppuswamy-linux-intel-com/Simplify-PCIe-native-ownership-detection-logic/20200718-012614
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: x86_64-randconfig-c002-20200717 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


coccinelle warnings: (new ones prefixed by >>)

>> drivers/acpi/pci_root.c:150:37-38: WARNING: Use ARRAY_SIZE

Please review and possibly fold the followup patch.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Joe Perches July 18, 2020, 2:27 p.m. UTC | #2 
On Sat, 2020-07-18 at 16:01 +0800, kernel test robot wrote:
> From: kernel test robot <lkp@intel.com>
> 
> drivers/acpi/pci_root.c:150:37-38: WARNING: Use ARRAY_SIZE
> 
>  Use ARRAY_SIZE instead of dividing sizeof array with sizeof an element
[]
>  static char *get_osc_desc(u32 bit)
>  {
> -	int len = sizeof(pci_osc_control_bit) / sizeof(pci_osc_control_bit[0]);
> +	int len = ARRAY_SIZE(pci_osc_control_bit);
>  	int i = 0;
>  
>  	for (i = 0; i <len; i++)

And likely better to not declare len at all
and ARRAY_SIZE directly instead.

static char *get_osc_desc(u32 bit)
{
	int i;

	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;
}

and also likely both arrays should be const.

Something like:
---
 drivers/acpi/pci_root.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index f90e841c59f5..bfb437cf749a 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -122,10 +122,10 @@ static acpi_status try_get_root_bridge_busnr(acpi_handle handle,
 
 struct pci_osc_bit_struct {
 	u32 bit;
-	char *desc;
+	const char *desc;
 };
 
-static struct pci_osc_bit_struct pci_osc_support_bit[] = {
+static const struct pci_osc_bit_struct pci_osc_support_bit[] = {
 	{ OSC_PCI_EXT_CONFIG_SUPPORT, "ExtendedConfig" },
 	{ OSC_PCI_ASPM_SUPPORT, "ASPM" },
 	{ OSC_PCI_CLOCK_PM_SUPPORT, "ClockPM" },
@@ -135,7 +135,7 @@ static struct pci_osc_bit_struct pci_osc_support_bit[] = {
 	{ OSC_PCI_HPX_TYPE_3_SUPPORT, "HPX-Type3" },
 };
 
-static struct pci_osc_bit_struct pci_osc_control_bit[] = {
+static const struct pci_osc_bit_struct pci_osc_control_bit[] = {
 	{ OSC_PCI_EXPRESS_NATIVE_HP_CONTROL, "PCIeHotplug" },
 	{ OSC_PCI_SHPC_NATIVE_HP_CONTROL, "SHPCHotplug" },
 	{ OSC_PCI_EXPRESS_PME_CONTROL, "PME" },
@@ -146,17 +146,18 @@ static struct pci_osc_bit_struct pci_osc_control_bit[] = {
 };
 
 static void decode_osc_bits(struct acpi_pci_root *root, char *msg, u32 word,
-			    struct pci_osc_bit_struct *table, int size)
+			    const struct pci_osc_bit_struct *table, int size)
 {
 	char buf[80];
 	int i, len = 0;
-	struct pci_osc_bit_struct *entry;
 
 	buf[0] = '\0';
-	for (i = 0, entry = table; i < size; i++, entry++)
-		if (word & entry->bit)
+	for (i = 0; i < size; i++) {
+		if (word & table->bit)
 			len += scnprintf(buf + len, sizeof(buf) - len, "%s%s",
-					len ? " " : "", entry->desc);
+					len ? " " : "", table->desc);
+		table++;
+	}
 
 	dev_info(&root->device->dev, "_OSC: %s [%s]\n", msg, buf);
 }
diff mbox series

Patch

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index f90e841c59f5..a82069d064fe 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -145,6 +145,18 @@  static struct pci_osc_bit_struct pci_osc_control_bit[] = {
 	{ OSC_PCI_EXPRESS_DPC_CONTROL, "DPC" },
 };
 
+static char *get_osc_desc(u32 bit)
+{
+	int len = sizeof(pci_osc_control_bit) / sizeof(pci_osc_control_bit[0]);
+	int i = 0;
+
+	for (i = 0; i <len; 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 +926,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;
 
 		/*