Re: [PATCH v7 04/15] can: grcan: Add clock handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The fallback clock is acquired with `devm_clk_get()` but is never prepared or enabled.
- [Low] The deferred probe log suppression provided by `dev_err_probe()` is defeated by an unconditional `dev_err()` at the `exit_error` label.
--

commit e67893a17ab7e0f12006c5fe41c33d231ab03961
Author: Daniel Hellstrom <daniel@gaisler.com>

can: grcan: Add clock handling

This commit adds clock handling and error messages to provide a fallback
for when the freq device tree property is missing.

> diff --git a/drivers/net/can/grcan.c b/drivers/net/can/grcan.c
> --- a/drivers/net/can/grcan.c
> +++ b/drivers/net/can/grcan.c
[ ... ]
> @@ -1662,8 +1663,19 @@ static int grcan_probe(struct platform_device *ofdev)
>  
>  	err = of_property_read_u32(np, "freq", &ambafreq);
>  	if (err) {
> -		dev_err(&ofdev->dev, "unable to fetch \"freq\" property\n");
> -		goto exit_error;
> +		clk = devm_clk_get(&ofdev->dev, NULL);

Does this code need to prepare and enable the clock?

When acquired via the Common Clock Framework, merely reading the rate
with clk_get_rate() does not increment the enable count. If the clock is
gateable, the late initcall clk_disable_unused might disable it, which
can lead to a system crash or bus lockup when accessing the CAN controller
registers.

Would devm_clk_get_enabled() be more appropriate here?

> +		if (IS_ERR(clk)) {
> +			dev_err_probe(&ofdev->dev, PTR_ERR(clk),
> +				      "Failed both to get \"freq\" property and clock for fallback\n");
> +			err = PTR_ERR(clk);
> +			goto exit_error;

Does this goto defeat the deferred probe log suppression?

If devm_clk_get returns -EPROBE_DEFER, dev_err_probe() correctly suppresses
its own log message. However, jumping to the exit_error label unconditionally
calls dev_err(), which will log the probe deferral anyway:

exit_error:
	dev_err(&ofdev->dev,
		"%s socket CAN driver initialization failed with error %d\n",
		DRV_NAME, err);
	return err;

Since no manual resource cleanup is required at this stage of the probe
function, could the driver directly return the result of dev_err_probe()
instead?

> +		}
> +
> +		ambafreq = clk_get_rate(clk);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508070121.6918-1-arun.muthusamy@gaisler.com?part=4