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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unbinding the driver causes a NULL pointer dereference crash if the PCIe link was down during probe.
- [High] Kernel panic during system suspend if the PCIe link failed to come up.
- [Medium] The runtime PM usage counter is leaked if `pm_runtime_get_sync()` fails.
--

commit 606e8e105f99594322aaccfdb36dcac03cb19d29
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/pcie-tegra264.c b/drivers/pci/controller/pcie-tegra264.c
> new file mode 100644
> index 0000000000000..e2d295ea4403f
> --- /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]
According to the runtime PM API, pm_runtime_get_sync() increments the usage
counter even if it returns an error. If it fails here, we jump to put_bpmp,
skipping the put_pm label which calls pm_runtime_put_sync(). Does this leak
the usage counter and prevent the device from ever entering runtime suspend?

> +	}
> +
> +	/* sanity check that programmed ranges match what's in DT */
> +	if (!tegra264_pcie_check_ranges(pdev)) {
> +		err = -EINVAL;
> +		goto put_pm;
> +	}

[ ... ]

> +	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;
> +
> +free_ecam:
> +	pci_ecam_free(pcie->cfg);
> +put_pm:
> +	pm_runtime_put_sync(dev);
> +put_bpmp:
> +	tegra_bpmp_put(pcie->bpmp);
> +
> +	return err;
> +}
> +
> +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 is down during probe, tegra264_pcie_probe() returns 0 without
calling pci_host_probe(), leaving pcie->bridge->bus as NULL. When the
driver is later unbound, tegra264_pcie_remove() unconditionally passes
pcie->bridge->bus to pci_stop_root_bus() and pci_remove_root_bus(). 

Will this cause a NULL pointer dereference? Is a check needed to ensure the
bus exists before tearing it down?

> +	pci_unlock_rescan_remove();
> +
> +	pm_runtime_put_sync(&pdev->dev);
> +	tegra_bpmp_put(pcie->bpmp);
> +	pci_ecam_free(pcie->cfg);
> +}
> +
> +static int tegra264_pcie_suspend(struct device *dev)
> +{
> +	struct tegra264_pcie *pcie = dev_get_drvdata(dev);
> +	int err;
> +
> +	tegra264_pcie_deinit(pcie);

[Severity: High]
If the link fails to come up in tegra264_pcie_init() and the port isn't
hotplug-capable, the driver powers off the controller and sets
pcie->link_up = false.

If the system is later suspended, tegra264_pcie_suspend() unconditionally
calls tegra264_pcie_deinit(), which attempts to read from the xtl register
space:

static void tegra264_pcie_deinit(struct tegra264_pcie *pcie)
{
	...
	value = readl(pcie->xtl + XTL_RC_MGMT_PERST_CONTROL);

Will this hardware access to a powered-off controller cause a kernel panic?
Should we check pcie->link_up before calling tegra264_pcie_deinit(),
similar to what is done in tegra264_pcie_resume()?

> +
> +	if (pcie->wake_gpio && device_may_wakeup(dev)) {
> +		err = enable_irq_wake(pcie->wake_irq);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617-tegra264-pcie-v7-0-eae7ae964629@nvidia.com?part=3