Re: [PATCH net-next 06/12] can: rcar_canfd: Check can_clk request for errors

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch adds a dev_err_probe() call in rcar_canfd_probe()
  (drivers/net/can/rcar/rcar_canfd.c:2228) whose format string, "cannot…

--

can: rcar_canfd: Check can_clk request for errors

This commit switches the can_clk request in rcar_canfd_probe() from
devm_clk_get() to devm_clk_get_optional(). Errors such as -EPROBE_DEFER
are now passed back to the caller. Before this, the driver silently fell
back to the canfd or fck clock. The comment describing the fallback is
also updated.

Link: https://patch.msgid.link/20261019121356.2421761-2-claudiu.beznea@kernel.org

> diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> index 74c3c3f20991f..bb2d03b75142a 100644
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c
> @@ -2220,11 +2220,14 @@ static int rcar_canfd_probe(struct platform_device *pdev)

[ ... ]

> -	gpriv->can_clk = devm_clk_get(dev, "can_clk");
> -	if (IS_ERR(gpriv->can_clk) || (clk_get_rate(gpriv->can_clk) == 0)) {
> +	gpriv->can_clk = devm_clk_get_optional(dev, "can_clk");
> +	if (IS_ERR(gpriv->can_clk))
> +		return dev_err_probe(dev, PTR_ERR(gpriv->can_clk), "cannot get can_clk");

[Severity: Low]
This isn't a bug, but should this format string end with "\n"? All the
other dev_err_probe() calls in rcar_canfd_probe() have one ("cannot get
peripheral clock\n", "cannot get canfd clock\n", "cannot get ram
clock\n", and so on).

With -EPROBE_DEFER, the case this patch is about, __dev_probe_failed()
does not print the message. It stores it as-is through
device_set_deferred_probe_reason().

The stored string is then printed with no newline added. One place is
deferred_devs_show() in drivers/base/dd.c:

    seq_printf(s, "%s\t%s", dev_name(curr->device),
               curr->deferred_probe_reason ?: "\n");

The other is deferred_probe_timeout_work_func():

    dev_warn(p->device, "deferred probe pending: %s",
             p->deferred_probe_reason ?: "(reason unknown)\n");

So when can_clk defers, the next entry in
/sys/kernel/debug/devices_deferred would run onto the same line. The
deferred probe timeout warning would also have no line ending.

> +
> +	if (!gpriv->can_clk || (clk_get_rate(gpriv->can_clk) == 0)) {
>  		if (info->fcan_pclk) {
>  			fcan_freq = clk_get_rate(gpriv->clkp);
>  			gpriv->can_clk = NULL;

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