Re: [PATCH v7 4/4] PCI: of: introduce of_pci_verify_node()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] PCI: of: of_pci_verify_node() defined outside CONFIG_PCI guard
--

commit 3ae9eba1c84ba51b7f2ee89655e65140eb2d2cd0
Author: Alex Elder <elder@riscstar.com>

PCI: of: introduce of_pci_verify_node()

This commit introduces of_pci_verify_node() and calls it from
pci_bus_add_device() to check that devicetree nodes for PCI bridges have a
device_type property of "pci" or "pciex", and that PCI endpoint nodes do not.

> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index a51dff91b196d..5a040ed836744 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -1085,3 +1085,35 @@ int of_pci_get_equalization_presets(struct device *dev,
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
> +
> +/**
> + * of_pci_verify_node - Sanity check some PCI device node properties
> + * @pdev: The PCI device whose device node is checked
> + *
> + * PCI enumeration authoritatively discovers what we need to know about
> + * a PCI device.  A devicetree-based platform will represent a PCI root
> + * bridge with a node, but otherwise devicetree doesn't typically include
> + * many PCI nodes.  Where such nodes do exist, experience has shown that
> + * the "device_type" property is sometimes wrong, so warn about that.
> + */
> +void of_pci_verify_node(struct pci_dev *pdev)
> +{
> +	struct device_node *np = pci_device_to_OF_node(pdev);
> +	bool device_is_bridge;
> +	bool device_type_pci;
> +
> +	/* Nothing to check if there's no pre-existing devicetree node */
> +	if (!np)
> +		return;
> +
> +	device_is_bridge = pci_is_bridge(pdev);
> +	device_type_pci = of_node_is_type(np, "pci") ||
> +			  of_node_is_type(np, "pciex");
> +
> +	/* Bridges should have device type "pci"; endpoints should not */
> +	if (device_is_bridge == device_type_pci)
> +		return;
> +
> +	dev_err(&pdev->dev, "PCI %s have \"pci\" device_type property\n",
> +		device_is_bridge ? "bridge should" : "endpoint should not");
> +}

[Severity: Low]
Should of_pci_verify_node() be placed inside the #ifdef CONFIG_PCI block in
drivers/pci/of.c rather than at the end of the file?

drivers/pci/of.o is built whenever CONFIG_OF is enabled so that PCI endpoint
controller drivers can use the exported devicetree helpers defined after
the #endif /* CONFIG_PCI */ guard:

drivers/pci/of.c:
    bool of_pci_supply_present(struct device_node *np)
    {
        ...
    }
    EXPORT_SYMBOL_GPL(of_pci_supply_present);

    #endif /* CONFIG_PCI */

    int of_pci_get_max_link_speed(struct device_node *node)

Since of_pci_verify_node() is only called from pci_bus_add_device() in
drivers/pci/bus.c (which is only built when CONFIG_PCI is enabled), defining
it after #endif /* CONFIG_PCI */ causes it to be compiled as unused code when
CONFIG_OF=y and CONFIG_PCI=n.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105153801.435566-1-elder@riscstar.com?part=4