Re: [PATCH] can: rockchip_canfd: Propagate reset failures on start

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unsafe reset error handling introduces bus faults and incomplete teardown.

Pre-existing issues:
- [High] Missing can_rx_offload_disable() in rkcanfd_open() error path causes BUG_ON panic.
- [High] Asynchronous pm_runtime_put() races with pm_runtime_disable() during device removal.
- [High] Calling non-atomic reset control APIs from the hardirq handler causes a panic.
--

commit a6e0fcd15347d005fd23e1237e5a026a9096879a
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

can: rockchip_canfd: Propagate reset failures on start

The Rockchip CAN-FD driver requires a reset controller, but
rkcanfd_chip_set_reset_mode() ignores reset assert/deassert failures.
This patch returns reset errors from the reset/start helpers and propagates
them before enabling RX offload, requesting the IRQ, or starting the netdev
queue.

> diff --git a/drivers/net/can/rockchip/rockchip_canfd-core.c b/drivers/net/can/rockchip/rockchip_canfd-core.c
> index 29de0c01e4edc..89bc402b76e85 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd-core.c
> +++ b/drivers/net/can/rockchip/rockchip_canfd-core.c
[ ... ]
> @@ -102,13 +102,23 @@ static const struct can_bittiming_const rkcanfd_data_bittiming_const = {
>  	.brp_inc = 2,	/* value from data sheet x2 */
>  };
>  
> -static void rkcanfd_chip_set_reset_mode(const struct rkcanfd_priv *priv)
> +static int rkcanfd_chip_set_reset_mode(const struct rkcanfd_priv *priv)
>  {
> -	reset_control_assert(priv->reset);
> +	int err;
> +
> +	err = reset_control_assert(priv->reset);
> +	if (err)
> +		return err;

[Severity: High]
This is a pre-existing issue, but does calling reset_control_assert() from
this context cause a scheduling while atomic panic?

If a state error occurs on the bus, the hardirq handler rkcanfd_irq()
calls rkcanfd_handle_state_error_int() which initiates a stop:

rkcanfd_handle_state_error_int()
    rkcanfd_chip_stop()
        __rkcanfd_chip_stop()
            rkcanfd_chip_set_reset_mode()
                reset_control_assert()

Since this reset control uses a mutex internally, can this sleep in interrupt
context?

[Severity: High]
If reset_control_assert() fails, we return early. Does this skip the
fallback software reset and leave the controller active on the CAN bus?

Previously, this function would fall back to software reset:
    rkcanfd_write(priv, RKCANFD_REG_MODE, 0x0);

With the early return, the controller might still be active while the driver
considers it stopped.

>  	udelay(2);
> -	reset_control_deassert(priv->reset);
> +
> +	err = reset_control_deassert(priv->reset);
> +	if (err)
> +		return err;
>  
>  	rkcanfd_write(priv, RKCANFD_REG_MODE, 0x0);
> +
> +	return 0;
>  }
[ ... ]
> @@ -309,13 +322,16 @@ static void rkcanfd_chip_start(struct rkcanfd_priv *priv)
>  
>  	netdev_dbg(priv->ndev, "%s: reg_mode=0x%08x\n", __func__,
>  		   rkcanfd_read(priv, RKCANFD_REG_MODE));
> +
> +	return 0;
>  }
>  
>  static void __rkcanfd_chip_stop(struct rkcanfd_priv *priv, const enum can_state state)
>  {
>  	priv->can.state = state;
>  
> -	rkcanfd_chip_set_reset_mode(priv);
> +	if (rkcanfd_chip_set_reset_mode(priv))
> +		netdev_err(priv->ndev, "Failed to reset CAN-FD controller\n");
>  	rkcanfd_chip_interrupts_disable(priv);

[Severity: High]
If reset_control_deassert() fails in rkcanfd_chip_set_reset_mode(), the
controller is left actively held in reset.

Will calling rkcanfd_chip_interrupts_disable() immediately after cause a
synchronous external abort (bus fault)?

Accessing MMIO of a peripheral currently held in reset on Rockchip SoCs
typically causes a kernel panic.

>  }
[ ... ]
> @@ -719,7 +739,10 @@ static int rkcanfd_open(struct net_device *ndev)
>  	if (err)
>  		goto out_close_candev;
>  
> -	rkcanfd_chip_start(priv);
> +	err = rkcanfd_chip_start(priv);
> +	if (err)
> +		goto out_runtime_put;
> +
>  	can_rx_offload_enable(&priv->offload);
>  
>  	err = request_irq(ndev->irq, rkcanfd_irq, IRQF_SHARED, ndev->name, priv);
> @@ -734,6 +757,7 @@ static int rkcanfd_open(struct net_device *ndev)
>  
>  out_rkcanfd_chip_stop:
>  	rkcanfd_chip_stop_sync(priv, CAN_STATE_STOPPED);

[Severity: High]
This isn't a bug introduced by this patch, but if request_irq() fails,
does this error path miss calling can_rx_offload_disable()?

A subsequent attempt to open the interface will call can_rx_offload_enable()
again, which calls napi_enable() on an already-enabled NAPI context.

Could this trigger a BUG_ON(!test_bit(NAPI_STATE_SCHED, &n->state)) in
net/core/dev.c?

> +out_runtime_put:
>  	pm_runtime_put(ndev->dev.parent);

[Severity: High]
This is a pre-existing issue, but does using the asynchronous
pm_runtime_put() here and in rkcanfd_stop() race with pm_runtime_disable()
during device removal?

When the device is unregistered, rkcanfd_unregister() calls
unregister_candev() which stops the interface, queueing an asynchronous
suspend via pm_runtime_put().

Immediately after, rkcanfd_unregister() calls pm_runtime_disable() which
cancels the pending async idle/suspend work.

Could this leave the hardware permanently powered on? Would using
pm_runtime_put_sync() ensure the suspend completes?

>  out_close_candev:
>  	close_candev(ndev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624055026.24789-1-pengpeng@iscas.ac.cn?part=1