drivers/pci/pci.h | 1 + drivers/pci/probe.c | 58 +++++++++++++++++++++++++++++++-------------- 2 files changed, 41 insertions(+), 18 deletions(-)
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@1f0000000 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@nxp.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.1On Sun, May 21, 2023 at 02:51:41PM +0300, Vladimir Oltean wrote:
> 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@1f0000000 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.
Can you write this description in terms of PCI topology? The
nitty-gritty SERDES details are not relevant at this level, except to
say that Function 0 is present in some cases but not others, and when
it is not present, *other* functions may be present.
Sigh. Per spec (PCIe r6.0, sec 7.5.1.1.9), software is not permitted
to probe for Functions other than 0 unless "explicitly indicated by
another mechanism, such as an ARI or SR-IOV Capability."
Does it "work" to probe when the spec prohibits it? Probably. Does
it lead to some breakage elsewhere eventually? Quite possibly. They
didn't put "software must not probe" in the spec just to make
enumeration faster.
So I'm a little grumpy about further complicating this already messy
path just to accommodate a new non-compliant SoC. Everybody pays the
price of understanding all this stuff, and it doesn't seem in balance.
Can you take advantage of some existing mechanism like
PCI_SCAN_ALL_PCIE_DEVS or hypervisor_isolated_pci_functions() (which
could be renamed and made more general)?
> 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@nxp.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
>
On Tue, May 30, 2023 at 04:58:55PM -0500, Bjorn Helgaas wrote: > Can you write this description in terms of PCI topology? The > nitty-gritty SERDES details are not relevant at this level, except to > say that Function 0 is present in some cases but not others, and when > it is not present, *other* functions may be present. No. It is to say that within the device, all PCIe functions (including 0) are always available and have the same number, but depending on SERDES configuration, their PCIe presence might be practically useful or not. So that's how function 0 may end having status = "disabled" in the device tree. > Sigh. Per spec (PCIe r6.0, sec 7.5.1.1.9), software is not permitted > to probe for Functions other than 0 unless "explicitly indicated by > another mechanism, such as an ARI or SR-IOV Capability." > > Does it "work" to probe when the spec prohibits it? Probably. Does > it lead to some breakage elsewhere eventually? Quite possibly. They > didn't put "software must not probe" in the spec just to make > enumeration faster. > > So I'm a little grumpy about further complicating this already messy > path just to accommodate a new non-compliant SoC. Everybody pays the > price of understanding all this stuff, and it doesn't seem in balance. > > Can you take advantage of some existing mechanism like > PCI_SCAN_ALL_PCIE_DEVS or hypervisor_isolated_pci_functions() (which > could be renamed and made more general)? Not responding yet to the rest of the email since it's not clear to me that you've understood function 0 is absolutely present and responds to all config space accesses - it's just disabled in the device tree because the user doesn't have something useful to do with it.
On Wed, May 31, 2023 at 01:04:36AM +0300, Vladimir Oltean wrote: > On Tue, May 30, 2023 at 04:58:55PM -0500, Bjorn Helgaas wrote: > > Can you write this description in terms of PCI topology? The > > nitty-gritty SERDES details are not relevant at this level, except to > > say that Function 0 is present in some cases but not others, and when > > it is not present, *other* functions may be present. > > No. It is to say that within the device, all PCIe functions (including 0) > are always available and have the same number, but depending on SERDES > configuration, their PCIe presence might be practically useful or not. > So that's how function 0 may end having status = "disabled" in the > device tree. > > > Sigh. Per spec (PCIe r6.0, sec 7.5.1.1.9), software is not permitted > > to probe for Functions other than 0 unless "explicitly indicated by > > another mechanism, such as an ARI or SR-IOV Capability." > > > > Does it "work" to probe when the spec prohibits it? Probably. Does > > it lead to some breakage elsewhere eventually? Quite possibly. They > > didn't put "software must not probe" in the spec just to make > > enumeration faster. > > > > So I'm a little grumpy about further complicating this already messy > > path just to accommodate a new non-compliant SoC. Everybody pays the > > price of understanding all this stuff, and it doesn't seem in balance. > > > > Can you take advantage of some existing mechanism like > > PCI_SCAN_ALL_PCIE_DEVS or hypervisor_isolated_pci_functions() (which > > could be renamed and made more general)? > > Not responding yet to the rest of the email since it's not clear to me > that you've understood function 0 is absolutely present and responds > to all config space accesses - it's just disabled in the device tree > because the user doesn't have something useful to do with it. Ah, you're right, sorry I missed that. Dispensing with the SERDES details would make this more obvious. Not sure why this needs to change the pci_scan_slot() path, since Function 0 is present and enumerable even though it's not useful in some cases. Seems like something in pci_set_of_node() or a quirk could do whatever you need to do. Bjorn
On Tue, May 30, 2023 at 05:27:24PM -0500, Bjorn Helgaas wrote: > Ah, you're right, sorry I missed that. Dispensing with the SERDES > details would make this more obvious. Lesson learned. When I had just gotten out of college, every time I asked the coworkers in my company what they're up to, I was amazed by them just proceeding to tell me all the nitty gritty details of what they're doing and debugging, like I was supposed to understand or care for that matter. "Dude, can't you just paint the high level idea without using dorky words?" Now I'm one of them... > Not sure why this needs to change the pci_scan_slot() path, since > Function 0 is present and enumerable even though it's not useful in > some cases. Well, the rationale for me was pretty simple: it's the pci_scan_slot() logic that I want to change - continue enumeration in some cases when the pci_dev for fn 0 is NULL - and I'm otherwise perfectly okay with pci_scan_slot() getting a NULL pci_dev from pci_setup_device() for fn 0. That wasn't something I had in mind to change. This patch is what it takes to propagate a qualifier, without leaving a mark in any structure, for that NULL return code: is it NULL because enumeration came up with nothing, or is it NULL because pci_set_of_node() said so? > Seems like something in pci_set_of_node() or a quirk could do whatever > you need to do. Could you help me out with a more detailed hint here? I'm not really familiar with the PCI core code. You probably mean to suggest leaving a stateful flag somewhere, though I'm not exactly sure where that is, that would reach pci_scan_slot() enough to be able to alter its decision.
On Wed, May 31, 2023 at 02:15:09AM +0300, Vladimir Oltean wrote: > On Tue, May 30, 2023 at 05:27:24PM -0500, Bjorn Helgaas wrote: > > Ah, you're right, sorry I missed that. Dispensing with the SERDES > > details would make this more obvious. > > Lesson learned. When I had just gotten out of college, every time I asked > the coworkers in my company what they're up to, I was amazed by them just > proceeding to tell me all the nitty gritty details of what they're doing > and debugging, like I was supposed to understand or care for that matter. > "Dude, can't you just paint the high level idea without using dorky words?" > Now I'm one of them... Haha :) Communication is the hardest part! > > Seems like something in pci_set_of_node() or a quirk could do whatever > > you need to do. > > Could you help me out with a more detailed hint here? I'm not really > familiar with the PCI core code. You probably mean to suggest leaving a > stateful flag somewhere, though I'm not exactly sure where that is, that > would reach pci_scan_slot() enough to be able to alter its decision. What bad things happen without this patch? I guess we enumerate Function 0 but in some cases it's not useful? That in itself wouldn't be a disaster; there are lots of things we enumerate but don't use. But in this case, maybe a driver would claim Function 0 but it wouldn't work as expected? Bjorn
On Wed, May 31, 2023 at 11:56:02AM -0500, Bjorn Helgaas wrote: > On Wed, May 31, 2023 at 02:15:09AM +0300, Vladimir Oltean wrote: > > On Tue, May 30, 2023 at 05:27:24PM -0500, Bjorn Helgaas wrote: > > > Ah, you're right, sorry I missed that. Dispensing with the SERDES > > > details would make this more obvious. > > > > Lesson learned. When I had just gotten out of college, every time I asked > > the coworkers in my company what they're up to, I was amazed by them just > > proceeding to tell me all the nitty gritty details of what they're doing > > and debugging, like I was supposed to understand or care for that matter. > > "Dude, can't you just paint the high level idea without using dorky words?" > > Now I'm one of them... > > Haha :) Communication is the hardest part! I know... > What bad things happen without this patch? It's in the commit title: probing the entire device (PCI device!!!) is skipped if function 0 has status = "disabled". Aka PCIe functions 1, 2, 3, 4, ...
Hi Bjorn,
On Sun, May 21, 2023 at 02:51:41PM +0300, Vladimir Oltean wrote:
> 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@1f0000000 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@nxp.com>
> ---
Do you have some comments here?
Thanks,
Vladimir
© 2016 - 2026 Red Hat, Inc.