lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-Id: <20230521115141.2384444-1-vladimir.oltean@nxp.com>
Date:   Sun, 21 May 2023 14:51:41 +0300
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     linux-pci@...r.kernel.org
Cc:     netdev@...r.kernel.org, Bjorn Helgaas <bhelgaas@...gle.com>,
        Rob Herring <robh@...nel.org>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Michael Walle <michael@...le.cc>, linux-kernel@...r.kernel.org
Subject: [PATCH pci] PCI: don't skip probing entire device if first fn OF node has status = "disabled"

pci_scan_child_bus_extend() calls pci_scan_slot() with devfn
(bus:device:function) being a multiple of 8, i.e. for each unique
device.

pci_scan_slot() has logic to say that if the function 0 of a device is
absent, the entire device is absent and we can skip the other functions
entirely. Traditionally, this has meant that pci_bus_read_dev_vendor_id()
returns an error code for that function.

However, since the blamed commit, there is an extra confounding
condition: function 0 of the device exists and has a valid vendor id,
but it is disabled in the device tree. In that case, pci_scan_slot()
would incorrectly skip the entire device instead of just that function.

Such is the case with the NXP LS1028A SoC, which has an ECAM
for embedded Ethernet (see pcie@...000000 in
arm64/boot/dts/freescale/fsl-ls1028a.dtsi). Each Ethernet port
represents a function within the ENETC ECAM, with function 0 going
to ENETC Ethernet port 0, connected to SERDES port 0 (SGMII or USXGMII).

When using a SERDES protocol such as 0x9999, all 4 SERDES lanes go to
the Ethernet switch (function 5 on this ECAM) and none go to ENETC
port 0. So, ENETC port 0 needs to have status = "disabled", and embedded
Ethernet takes place just through the other functions (fn 2 is the DSA
master, fn 3 is the MDIO controller, fn 5 is the DSA switch etc).
Contrast this with other SERDES protocols like 0x85bb, where the switch
takes up a single SERDES lane and uses the QSGMII protocol - so ENETC
port 0 also gets access to a SERDES lane.

Therefore, here, function 0 being unused has nothing to do with the
entire PCI device being unused.

Add a "bool present_but_skipped" which is propagated from the caller
of pci_set_of_node() all the way to pci_scan_slot(), so that it can
distinguish an error reading the ECAM from a disabled device in the
device tree.

Fixes: 6fffbc7ae137 ("PCI: Honor firmware's device disabled status")
Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
---
 drivers/pci/pci.h   |  1 +
 drivers/pci/probe.c | 58 +++++++++++++++++++++++++++++++--------------
 2 files changed, 41 insertions(+), 18 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 2475098f6518..dc11e0945744 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -240,6 +240,7 @@ bool pci_bus_generic_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *pl,
 					int crs_timeout);
 int pci_idt_bus_quirk(struct pci_bus *bus, int devfn, u32 *pl, int crs_timeout);
 
+int __pci_setup_device(struct pci_dev *dev, bool *present_but_skipped);
 int pci_setup_device(struct pci_dev *dev);
 int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 		    struct resource *res, unsigned int reg);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 0b2826c4a832..17a51fa55020 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1811,17 +1811,7 @@ static void early_dump_pci_device(struct pci_dev *pdev)
 		       value, 256, false);
 }
 
-/**
- * pci_setup_device - Fill in class and map information of a device
- * @dev: the device structure to fill
- *
- * Initialize the device structure with information about the device's
- * vendor,class,memory and IO-space addresses, IRQ lines etc.
- * Called at initialisation of the PCI subsystem and by CardBus services.
- * Returns 0 on success and negative if unknown type of device (not normal,
- * bridge or CardBus).
- */
-int pci_setup_device(struct pci_dev *dev)
+int __pci_setup_device(struct pci_dev *dev, bool *present_but_skipped)
 {
 	u32 class;
 	u16 cmd;
@@ -1841,8 +1831,10 @@ int pci_setup_device(struct pci_dev *dev)
 	set_pcie_port_type(dev);
 
 	err = pci_set_of_node(dev);
-	if (err)
+	if (err) {
+		*present_but_skipped = true;
 		return err;
+	}
 	pci_set_acpi_fwnode(dev);
 
 	pci_dev_assign_slot(dev);
@@ -1995,6 +1987,23 @@ int pci_setup_device(struct pci_dev *dev)
 	return 0;
 }
 
+/**
+ * pci_setup_device - Fill in class and map information of a device
+ * @dev: the device structure to fill
+ *
+ * Initialize the device structure with information about the device's
+ * vendor,class,memory and IO-space addresses, IRQ lines etc.
+ * Called at initialisation of the PCI subsystem and by CardBus services.
+ * Returns 0 on success and negative if unknown type of device (not normal,
+ * bridge or CardBus).
+ */
+int pci_setup_device(struct pci_dev *dev)
+{
+	bool present_but_skipped = false;
+
+	return __pci_setup_device(dev, &present_but_skipped);
+}
+
 static void pci_configure_mps(struct pci_dev *dev)
 {
 	struct pci_dev *bridge = pci_upstream_bridge(dev);
@@ -2414,7 +2423,8 @@ EXPORT_SYMBOL(pci_bus_read_dev_vendor_id);
  * Read the config data for a PCI device, sanity-check it,
  * and fill in the dev structure.
  */
-static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)
+static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn,
+				       bool *present_but_skipped)
 {
 	struct pci_dev *dev;
 	u32 l;
@@ -2430,7 +2440,7 @@ static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)
 	dev->vendor = l & 0xffff;
 	dev->device = (l >> 16) & 0xffff;
 
-	if (pci_setup_device(dev)) {
+	if (__pci_setup_device(dev, present_but_skipped)) {
 		pci_bus_put(dev->bus);
 		kfree(dev);
 		return NULL;
@@ -2575,17 +2585,20 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
 	WARN_ON(ret < 0);
 }
 
-struct pci_dev *pci_scan_single_device(struct pci_bus *bus, int devfn)
+static struct pci_dev *__pci_scan_single_device(struct pci_bus *bus, int devfn,
+						bool *present_but_skipped)
 {
 	struct pci_dev *dev;
 
+	*present_but_skipped = false;
+
 	dev = pci_get_slot(bus, devfn);
 	if (dev) {
 		pci_dev_put(dev);
 		return dev;
 	}
 
-	dev = pci_scan_device(bus, devfn);
+	dev = pci_scan_device(bus, devfn, present_but_skipped);
 	if (!dev)
 		return NULL;
 
@@ -2593,6 +2606,13 @@ struct pci_dev *pci_scan_single_device(struct pci_bus *bus, int devfn)
 
 	return dev;
 }
+
+struct pci_dev *pci_scan_single_device(struct pci_bus *bus, int devfn)
+{
+	bool present_but_skipped;
+
+	return __pci_scan_single_device(bus, devfn, &present_but_skipped);
+}
 EXPORT_SYMBOL(pci_scan_single_device);
 
 static int next_ari_fn(struct pci_bus *bus, struct pci_dev *dev, int fn)
@@ -2665,6 +2685,7 @@ static int only_one_child(struct pci_bus *bus)
  */
 int pci_scan_slot(struct pci_bus *bus, int devfn)
 {
+	bool present_but_skipped;
 	struct pci_dev *dev;
 	int fn = 0, nr = 0;
 
@@ -2672,13 +2693,14 @@ int pci_scan_slot(struct pci_bus *bus, int devfn)
 		return 0; /* Already scanned the entire slot */
 
 	do {
-		dev = pci_scan_single_device(bus, devfn + fn);
+		dev = __pci_scan_single_device(bus, devfn + fn,
+					       &present_but_skipped);
 		if (dev) {
 			if (!pci_dev_is_added(dev))
 				nr++;
 			if (fn > 0)
 				dev->multifunction = 1;
-		} else if (fn == 0) {
+		} else if (fn == 0 && !present_but_skipped) {
 			/*
 			 * Function 0 is required unless we are running on
 			 * a hypervisor that passes through individual PCI
-- 
2.34.1

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ