Re: [PATCH net-next 35/37] can: sun4i_can: sun4ican_probe(): fix clk leak

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix is incomplete: the of_clk_get() clock reference still
  leaks when the device is removed.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  sun4ican_probe() replaces the real error codes from of_clk_get() and…

--

can: sun4i_can: sun4ican_probe(): fix clk leak

This commit adds an exit_put_clk label to sun4ican_probe(). The clock
reference taken with of_clk_get() is now released with clk_put() when
platform_get_irq(), devm_platform_ioremap_resource(), alloc_candev() or
register_candev() fails.

Link: https://patch.msgid.link/20260917104514.2147347-1-vulab@iscas.ac.cn

> diff --git a/drivers/net/can/sun4i_can.c b/drivers/net/can/sun4i_can.c
> index af52285d5a4e0..311526107ed81 100644
> --- a/drivers/net/can/sun4i_can.c
> +++ b/drivers/net/can/sun4i_can.c
> @@ -854,13 +854,13 @@ static int sun4ican_probe(struct platform_device *pdev)
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0) {
>  		err = -ENODEV;
> -		goto exit;
> +		goto exit_put_clk;
>  	}

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch, but
this block is being touched anyway. Does replacing the return value of
platform_get_irq() with -ENODEV break probe deferral?

platform_get_irq() can return -EPROBE_DEFER. For example, of_irq_get()
returns it when irq_find_host() cannot find the interrupt controller
domain yet.

The of_clk_get() error path a few lines above does the same thing:

	clk = of_clk_get(np, 0);
	if (IS_ERR(clk)) {
		dev_err(&pdev->dev, "unable to request clock\n");
		err = -ENODEV;
		goto exit;
	}

of_clk_get() returns -EPROBE_DEFER when the clock provider has not
registered yet.

The driver core only retries a probe that returns -EPROBE_DEFER. If the
clock provider or interrupt controller registers after this driver
probes, would the CAN interface ever bind? Would it be better to return
PTR_ERR(clk) and irq instead?

[ ... ]

> @@ -908,6 +908,8 @@ static int sun4ican_probe(struct platform_device *pdev)
>  
>  exit_free:
>  	free_candev(dev);
> +exit_put_clk:
> +	clk_put(clk);
>  exit:
>  	return err;
>  }

[Severity: High]
This covers the probe error paths. Does the clock reference from
of_clk_get() still leak when the device is removed?

On a successful probe the reference is stored in priv->clk:

	priv->clk = clk;

sun4ican_remove() never drops it:

static void sun4ican_remove(struct platform_device *pdev)
{
	struct net_device *dev = platform_get_drvdata(pdev);

	unregister_netdev(dev);
	free_candev(dev);
}

free_candev() calls free_netdev(), which frees the private area that
holds priv->clk. After that, nothing is left to release the reference.
The clk_put() added here is the only one in the file.

So each successful probe followed by a module unload or sysfs unbind
would still leak the struct clk. It would also leave the clk_core
refcount and the clock provider's module reference held for good.

Would switching to devm_clk_get(&pdev->dev, NULL) be simpler? It would
handle both the probe error paths and the remove path, and the
exit_put_clk label would no longer be needed.

If the manual clk_put() is kept, sun4ican_remove() would need to read
priv->clk before calling free_candev(), because priv lives inside the
netdev allocation.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009134323.64064-2-mkl%40pengutronix.de