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

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

[Pengpeng Hou]

The Rockchip CAN-FD driver requires a reset controller, but
rkcanfd_chip_set_reset_mode() ignores reset assert/deassert failures.
rkcanfd_open() and CAN_MODE_START can then report success after the
controller failed to enter a known reset state.

Return reset errors from the reset/start helpers and propagate them
before enabling RX offload, requesting the IRQ, or starting the netdev
queue.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 .../net/can/rockchip/rockchip_canfd-core.c    | 40 +++++++++++++++----
 1 file changed, 32 insertions(+), 8 deletions(-)

diff --git a/drivers/net/can/rockchip/rockchip_canfd-core.c b/drivers/net/can/rockchip/rockchip_canfd-core.c
index 29de0c01e4ed..89bc402b76e8 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;
+
 	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;
 }
 
 static void rkcanfd_chip_set_work_mode(const struct rkcanfd_priv *priv)
@@ -246,11 +256,14 @@ static void rkcanfd_chip_fifo_setup(struct rkcanfd_priv *priv)
 	netdev_reset_queue(priv->ndev);
 }
 
-static void rkcanfd_chip_start(struct rkcanfd_priv *priv)
+static int rkcanfd_chip_start(struct rkcanfd_priv *priv)
 {
 	u32 reg;
+	int err;
 
-	rkcanfd_chip_set_reset_mode(priv);
+	err = rkcanfd_chip_set_reset_mode(priv);
+	if (err)
+		return err;
 
 	/* Receiving Filter: accept all */
 	rkcanfd_write(priv, RKCANFD_REG_IDCODE, 0x0);
@@ -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);
 }
 
@@ -339,10 +355,14 @@ static int rkcanfd_set_mode(struct net_device *ndev,
 			    enum can_mode mode)
 {
 	struct rkcanfd_priv *priv = netdev_priv(ndev);
+	int err;
 
 	switch (mode) {
 	case CAN_MODE_START:
-		rkcanfd_chip_start(priv);
+		err = rkcanfd_chip_start(priv);
+		if (err)
+			return err;
+
 		rkcanfd_chip_interrupts_enable(priv);
 		netif_wake_queue(ndev);
 		break;
@@ -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);
+out_runtime_put:
 	pm_runtime_put(ndev->dev.parent);
 out_close_candev:
 	close_candev(ndev);
-- 
2.50.1 (Apple Git-155)

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

[sashiko-bot]

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