Re: [PATCH net] can: flexcan: Fix probing for m68k/coldfire

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Network device is registered and exposed to userspace before hardware and state initialization are complete, leading to race conditions and a PM runtime underflow on probe failure.
--

commit 3c3a1287a919f302aebc8b9802a583cf3a9bb1c9
Author: Uwe Kleine-König (The Capable Hub) <u.kleine-koenig@baylibre.com>

can: flexcan: Fix probing for m68k/coldfire

When determining the device details was reworked in commit 5e6c3454b405
("net: can: Use device_get_match_data()") there was no replacement for
the previous handling of non-of instantiated devices via the device's
id_entry which then results in a NULL pointer exception. The only
in-tree provider of such a device is arch/m68k/coldfire/device.c.

Cc: stable@vger.kernel.org

> diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
> index f5d22c61503fa..05b8ca72ca2ad 100644
> --- a/drivers/net/can/flexcan/flexcan-core.c
> +++ b/drivers/net/can/flexcan/flexcan-core.c

[ ... ]

> @@ -2148,6 +2147,15 @@ static int flexcan_probe(struct platform_device *pdev)
>  		return PTR_ERR(regs);
>  
>  	devtype_data = device_get_match_data(&pdev->dev);
> +	if (!devtype_data)
> +		/*
> +		 * If the device was instantiated via the traditional platform
> +		 * mechanisms (i.e. not by device tree), device_get_match_data()
> +		 * returns NULL. The only remaining such device is mcf_flexcan0
> +		 * (defined in arch/m68k/coldfire/device.c). Pick the right
> +		 * device type for that.
> +		 */
> +		devtype_data = &fsl_mcf5441x_devtype_data;
>  
>  	if ((devtype_data->quirks & FLEXCAN_QUIRK_SUPPORT_FD) &&
>  	    !((devtype_data->quirks &

[Severity: High]
This isn't a bug introduced by this patch, but later in this same function
flexcan_probe(), the network device is registered and exposed to userspace
before hardware and state initialization are complete.

Specifically, flexcan_probe() calls register_flexcandev(), which exposes the
interface to userspace via register_netdev(). Then register_flexcandev()
alters the hardware state and drops the PM runtime count:

register_flexcandev() {
	err = register_candev(dev);
	if (err)
		goto out_chip_disable;

	/* Disable core and let pm_runtime_put() disable the clocks. */
	flexcan_chip_disable(priv);
	pm_runtime_put(priv->dev);
}

Because the device is already visible, userspace can concurrently bring the
interface up, invoking flexcan_open() and flexcan_chip_start(). Doesn't this
race with the unlocked flexcan_chip_disable() call, potentially leaving the
hardware disabled while the network core considers it up?

Also in flexcan_probe(), if flexcan_setup_stop_mode() fails after the device
is registered, it goes to the failed_setup_stop_mode cleanup path:

flexcan_probe() {
	err = register_flexcandev(dev);
	...
	err = flexcan_setup_stop_mode(pdev);
	if (err < 0) {
		goto failed_setup_stop_mode;
	}

	of_can_transceiver(dev);
	...
 failed_setup_stop_mode:
	unregister_flexcandev(dev);
 failed_register:
	pm_runtime_put_noidle(&pdev->dev);
	pm_runtime_disable(&pdev->dev);
}

Since pm_runtime_put() was already called on success inside
register_flexcandev(), does the call to pm_runtime_put_noidle() in the
failed_register path cause a PM runtime usage counter underflow?

Finally, of_can_transceiver() is called after device registration to
initialize priv->can.bitrate_max. Could userspace configure the CAN bitrate
before these maximum limits are enforced?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618140147.142489-2-u.kleine-koenig@baylibre.com?part=1