Re: [PATCH v6 3/4] PCI: tegra: Add Tegra264 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] NULL pointer dereference in tegra264_pcie_remove() if PCIe link fails to come up
- [High] System suspend crashes due to MMIO access of a powered-off controller
- [Medium] Runtime PM usage counter leak on pm_runtime_get_sync() error path in probe
- [Low] Missing Kconfig Dependency causing potential Link Failure
--

commit f7c63558c21039995bb013c2149a0292a9cfadb2
Author: Thierry Reding <treding@nvidia.com>

PCI: tegra: Add Tegra264 support

Add a driver for the PCIe controller found on NVIDIA Tegra264 SoCs. The
driver is very small, with its main purpose being to set up the address
translation registers and then creating a standard PCI host using ECAM.

> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> --- a/drivers/pci/controller/Kconfig
> +++ b/drivers/pci/controller/Kconfig
> @@ -255,7 +255,14 @@ config PCI_TEGRA
>  	select IRQ_MSI_LIB
>  	help
>  	  Say Y here if you want support for the PCIe host controller found
> -	  on NVIDIA Tegra SoCs.
> +	  on NVIDIA Tegra SoCs (Tegra20 through Tegra186).
> +
> +config PCIE_TEGRA264
> +	tristate "NVIDIA Tegra264 PCIe controller"
> +	depends on ARCH_TEGRA || COMPILE_TEST

[Severity: Low]
If this driver is built with COMPILE_TEST and no other enabled driver
selects PCI_ECAM, will it cause link-time errors due to unresolved
references to pci_ecam_create and pci_generic_ecam_ops? Should this
entry select PCI_ECAM or PCI_HOST_COMMON?

> diff --git a/drivers/pci/controller/pcie-tegra264.c b/drivers/pci/controller/pcie-tegra264.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/pci/controller/pcie-tegra264.c
[ ... ]
> +static int tegra264_pcie_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	err = devm_pm_runtime_set_active_enabled(dev);
> +	if (err < 0) {
> +		dev_err_probe(dev, err, "failed to enable runtime PM\n");
> +		goto put_bpmp;
> +	}
> +
> +	err = pm_runtime_get_sync(dev);
> +	if (err < 0) {
> +		dev_err_probe(dev, err, "failed to power on device\n");
> +		goto put_bpmp;
> +	}

[Severity: Medium]
Does jumping to put_bpmp on failure here leak the runtime PM usage
counter? Since pm_runtime_get_sync() unconditionally increments the
counter even on failure, it seems a pm_runtime_put_noidle() might
be needed before returning.

[ ... ]
> +	tegra264_pcie_init(pcie);
> +
> +	if (!pcie->link_up)
> +		return 0;
> +
> +	err = pci_host_probe(bridge);
> +	if (err < 0) {
> +		dev_err_probe(dev, err, "failed to register host\n");
> +		goto free_ecam;
> +	}
> +
> +	return 0;
[ ... ]
> +static void tegra264_pcie_remove(struct platform_device *pdev)
> +{
> +	struct tegra264_pcie *pcie = platform_get_drvdata(pdev);
> +
> +	/*
> +	 * If we undo tegra264_pcie_init() then link goes down and need
> +	 * controller reset to bring up the link again. Remove intention is
> +	 * to clean up the root bridge and re-enumerate during bind.
> +	 */
> +	pci_lock_rescan_remove();
> +	pci_stop_root_bus(pcie->bridge->bus);
> +	pci_remove_root_bus(pcie->bridge->bus);

[Severity: High]
If the link fails to come up during probe, pci_host_probe() is
skipped and pcie->bridge->bus remains NULL. If the device is
subsequently unbound via sysfs, will tegra264_pcie_remove() pass
a NULL bus pointer to pci_stop_root_bus() and pci_remove_root_bus()
causing a kernel panic?

[ ... ]
> +static int tegra264_pcie_suspend(struct device *dev)
> +{
> +	struct tegra264_pcie *pcie = dev_get_drvdata(dev);
> +	int err;
> +
> +	tegra264_pcie_deinit(pcie);

[Severity: High]
Can this cause a synchronous external abort during system suspend?

If the PCIe link is down and hot-plug is unsupported,
tegra264_pcie_init() powers off the controller via BPMP firmware.
During suspend, tegra264_pcie_suspend() unconditionally calls
tegra264_pcie_deinit() which attempts to read MMIO registers:

tegra264_pcie_deinit()
    value = readl(pcie->xtl + XTL_RC_MGMT_PERST_CONTROL);

Will accessing these registers on a powered-off controller crash
the kernel? Should the suspend function check if the controller
is active before touching the hardware?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260602-tegra264-pcie-v6-0-edbcfa7a78fe@nvidia.com?part=3