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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider:
- [High] Missing `err` argument in `dev_err_probe()` call.
- [High] Passing `ERR_PTR(err)` to the integer `err` argument of `dev_err_probe()`.
- [High] Call to undefined function `tegra_pcie_deinit()` in the suspend callback.
- [High] The probe function returns success when the link is down but frees resources, causing a kernel crash on removal.
- [High] The driver calls a non-existent runtime PM API `devm_pm_runtime_active_enabled()`.
- [High] The PCIe bandwidth request calculation always evaluates to 0 MBps.
- [High] Calling sleeping functions inside a noirq PM callback.
- [High] Ignoring the return value of `pm_runtime_get_sync()` leads to unsafe hardware access and kernel crashes.
- [Medium] Leaking the BPMP reference if runtime PM initialization fails.
- [Medium] Unbalanced `pm_runtime_disable()` call during driver removal.
- [Medium] Wake IRQ is mapped and enabled for wakeup but never actually requested.
- [Low] Missing `pm_ptr()` wrapper for the PM operations structure pointer.
--

commit f706a8f756af69546e040928eae0b5849025ac5c
Author: Thierry Reding <treding@nvidia.com>
Subject: PCI: tegra: Add Tegra264 support

This patch adds a driver for the PCIe controller found on NVIDIA Tegra264 SoCs,
setting up address translation registers and 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..0b8e0f724e251
> --- /dev/null
> +++ b/drivers/pci/controller/pcie-tegra264.c
[ ... ]
> +static void tegra264_pcie_icc_set(struct tegra264_pcie *pcie)
> +{
> +	u32 value, speed, width;
> +	int err;
> +
> +	value = readw(pcie->ecam + XTL_RC_PCIE_CFG_LINK_STATUS);
> +	speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, value);
> +	width = FIELD_GET(PCI_EXP_LNKSTA_NLW, value);
> +
> +	value = Mbps_to_icc(width * PCIE_SPEED2MBS_ENC(speed));

[Severity: High]
Does this calculation evaluate to 0 MBps? The speed variable is populated
with the raw hardware CLS value, but PCIE_SPEED2MBS_ENC() expects a mapped
enum pci_bus_speed constant. If the raw values do not match the expected
constants, the macro evaluates to 0, which would heavily throttle performance.

[ ... ]
> +static int tegra264_pcie_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	if (tegra_is_silicon()) {
> +		pcie->bpmp = tegra_bpmp_get_with_id(dev, &pcie->ctl_id);
> +		if (IS_ERR(pcie->bpmp))
> +			return dev_err_probe(dev, PTR_ERR(pcie->bpmp),
> +					     "failed to get BPMP\n");
> +	}
> +
> +	err = devm_pm_runtime_active_enabled(dev);

[Severity: High]
Does devm_pm_runtime_active_enabled() exist in the kernel tree? It appears
this might be a typo for devm_pm_runtime_enable() or a similar helper, which
would cause a compilation error due to an unresolved symbol.

[Severity: Medium]
If the devm_pm_runtime_active_enabled() call fails and the function returns
early, does this leak the BPMP reference? It looks like tegra_bpmp_put()
is not called on this error path.

> +	if (err < 0)
> +		return dev_err_probe(dev, "failed to enable runtime PM\n");

[Severity: High]
Does this dev_err_probe() call have the correct number of arguments? It seems
the integer error code argument is missing. This will cause the format string
to be cast to the integer err parameter, leading to an uninitialized format
string and a likely crash or compilation error.

> +
> +	pm_runtime_get_sync(dev);

[Severity: High]
Should the return value of pm_runtime_get_sync() be checked here? If powering
on the device fails, the driver proceeds to read MMIO registers during the
subsequent tegra264_pcie_check_ranges() call. Accessing unpowered hardware
registers can trigger a synchronous external abort and kernel panic.

[ ... ]
> +	tegra264_pcie_init(pcie);
> +
> +	if (!pcie->link_up)
> +		goto free_ecam;

[Severity: High]
If the link is not up, does this branch cause probe() to return success
while freeing the ECAM resources? The err variable is not set to a negative
error code before the goto. This would cause the driver to bind successfully,
but pcie->bridge->bus would remain uninitialized, leading to a NULL pointer
dereference during unbind in tegra264_pcie_remove().

> +
> +	err = pci_host_probe(bridge);
> +	if (err < 0) {
> +		dev_err_probe(dev, ERR_PTR(err), "failed to register host\n");

[Severity: High]
Is ERR_PTR(err) the correct argument to pass here? dev_err_probe() expects an
integer as its second parameter. Passing a pointer could cause a build
breakage or result in incorrect error logging.

[ ... ]
> +static void tegra264_pcie_remove(struct platform_device *pdev)
> +{
[ ... ]
> +	pm_runtime_put_sync(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);

[Severity: Medium]
Since a devres helper is used in probe to enable runtime PM, it will
automatically queue a pm_runtime_disable() call on removal. Does calling it
explicitly here cause an unbalanced disable_depth counter? If the driver is
unbound and rebound, the counter might remain greater than 0, breaking
runtime PM for this device permanently.

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

[Severity: High]
Is tegra_pcie_deinit() a typo? The driver defines the deinitialization
function as tegra264_pcie_deinit(pcie). Calling an undefined function will
cause a build failure.

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

[Severity: Medium]
Is there a registered interrupt handler for this wake IRQ? The IRQ is mapped
and enabled for wakeup, but it does not seem to be requested anywhere via
request_irq() or dev_pm_set_wake_irq(). When the IRQ triggers, the kernel
will likely find no action handler, log a spurious interrupt error, and
permanently mask the IRQ, breaking subsequent wakeups.

[ ... ]
> +static int tegra264_pcie_resume_noirq(struct device *dev)
> +{
[ ... ]
> +	if (pcie->link_up == false)
> +		return 0;
> +
> +	tegra264_pcie_init(pcie);

[Severity: High]
Can tegra264_pcie_init() safely run in a noirq callback? The callback executes
with hardware interrupts disabled, but tegra264_pcie_init() invokes sleeping
functions like usleep_range() and msleep(). Calling sleeping functions in an
atomic or noirq context will trigger a system hang or kernel bug.

[ ... ]
> +static struct platform_driver tegra264_pcie_driver = {
> +	.probe = tegra264_pcie_probe,
> +	.remove = tegra264_pcie_remove,
> +	.driver = {
> +		.name = "tegra264-pcie",
> +		.pm = &tegra264_pcie_pm_ops,

[Severity: Low]
Should the pm operations pointer be wrapped with pm_ptr()? Directly assigning
it without the wrapper prevents the compiler from eliminating the dev_pm_ops
structure as dead code when PM support is disabled.

> +		.of_match_table = tegra264_pcie_of_match,
> +	},
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260526-tegra264-pcie-v5-0-84a813b979d7@nvidia.com?part=3