diff mbox series

bus: sunxi-rsb: fail hardware init on soft-reset timeout

Message ID 20260625003416.15841-1-pengpeng@iscas.ac.cn (mailing list archive)
State New
Headers
Series bus: sunxi-rsb: fail hardware init on soft-reset timeout |

Commit Message

Pengpeng Hou June 25, 2026, 12:34 a.m. UTC 
sunxi_rsb_hw_init() issues a controller soft reset and waits for the reset
bit to clear, but it ignores readl_poll_timeout()'s return value.

If the controller never leaves reset, probe can continue to publish child
devices and system resume can report success without a usable RSB
controller.

Return the timeout error, assert reset again, and disable the clock before
leaving the hardware-init path.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/bus/sunxi-rsb.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

sashiko-bot@kernel.org June 25, 2026, 12:45 a.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Asserting the hardware reset line on soft-reset timeout during system resume leaves the IP in reset, which causes a fatal bus hang when child devices subsequently attempt to access the controller.

Pre-existing issues:
- [High] If a malformed Device Tree explicitly sets `clock-frequency = <0>;`, it causes a kernel panic due to division by zero.
- [High] The IRQ handler is requested before the hardware is initialized and its module clock is enabled, risking a fatal bus error.
--

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

bus: sunxi-rsb: fail hardware init on soft-reset timeout

This commit adds a check for the controller soft reset in sunxi_rsb_hw_init.
If the reset times out, the code now asserts the hardware reset line,
disables the clock, and returns a timeout error instead of continuing.

> diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
> index daf0ea563e477..f83224dea804d 100644
> --- a/drivers/bus/sunxi-rsb.c
> +++ b/drivers/bus/sunxi-rsb.c
> @@ -669,8 +669,12 @@ static int sunxi_rsb_hw_init(struct sunxi_rsb *rsb)
>  
>  	/* reset the controller */
>  	writel(RSB_CTRL_SOFT_RST, rsb->regs + RSB_CTRL);
> -	readl_poll_timeout(rsb->regs + RSB_CTRL, reg,
> -			   !(reg & RSB_CTRL_SOFT_RST), 1000, 100000);
> +	ret = readl_poll_timeout(rsb->regs + RSB_CTRL, reg,
> +				 !(reg & RSB_CTRL_SOFT_RST), 1000, 100000);
> +	if (ret) {
> +		dev_err(dev, "soft reset timed out\n");
> +		goto err_reset_assert;
> +	}
>  
>  	/*
>  	 * Clock frequency and delay calculation code is from

[ ... ]

> @@ -696,6 +700,8 @@ static int sunxi_rsb_hw_init(struct sunxi_rsb *rsb)
>  
>  	return 0;
>  
> +err_reset_assert:
> +	reset_control_assert(rsb->rstc);

[Severity: High]
If sunxi_rsb_hw_init() fails due to this soft-reset timeout during system
resume, it asserts the hardware reset line and returns an error:

drivers/bus/sunxi-rsb.c:sunxi_rsb_resume() {
	struct sunxi_rsb *rsb = dev_get_drvdata(dev);

	return sunxi_rsb_hw_init(rsb);
}

The PM core logs the resume error but continues system resume. When child
devices, such as PMICs, are subsequently resumed, they attempt to access the
RSB bus to restore registers. The driver's runtime resume callback only
enables the clock, which means the driver will attempt MMIO access while the
controller is still held in hardware reset.

Will writing to an IP block held in hardware reset cause a synchronous
external abort and fatal bus hang on Allwinner SoCs?

>  err_clk_disable:
>  	clk_disable_unprepare(rsb->clk);
>  

[Severity: High]
This isn't a bug introduced by this patch, but if a malformed Device Tree
explicitly sets clock-frequency = <0>, sunxi_rsb_probe() successfully reads
the 0 and bypasses the upper boundary check:

drivers/bus/sunxi-rsb.c:sunxi_rsb_probe() {
    ...
    if (of_property_read_u32(np, "clock-frequency", &clk_freq)) {
        ...
    } else if (clk_freq > RSB_MAX_FREQ) {
        ...
    }
    ...
}

Later, sunxi_rsb_hw_init() uses this unchecked value as a divisor:

drivers/bus/sunxi-rsb.c:sunxi_rsb_hw_init() {
    ...
    clk_div = p_clk_freq / rsb->clk_freq / 2;
    ...
}

Can this cause a fatal division by zero exception when rsb->clk_freq is 0?

[Severity: High]
This is a pre-existing issue, but devm_request_irq() is called before
sunxi_rsb_hw_init() initializes the hardware and enables its module clock:

drivers/bus/sunxi-rsb.c:sunxi_rsb_probe() {
    ...
    ret = devm_request_irq(dev, irq, sunxi_rsb_irq, 0, RSB_CTRL_NAME, rsb);
    if (ret)
        return dev_err_probe(...);

    ret = sunxi_rsb_hw_init(rsb);
    ...
}

If an interrupt fires immediately from a pending state from a previous boot
stage, sunxi_rsb_irq() will execute and access the RSB_INTS register using
readl() before the clock is enabled:

drivers/bus/sunxi-rsb.c:sunxi_rsb_irq() {
    ...
    status = readl(rsb->regs + RSB_INTS);
    ...
}

Could accessing unclocked or reset MMIO space on Allwinner SoCs cause a fatal
synchronous external abort here?
Jernej Škrabec June 27, 2026, 6:40 a.m. UTC | #2 
Dne četrtek, 25. junij 2026 ob 02:34:16 Srednjeevropski poletni čas je Pengpeng Hou napisal(a):
> sunxi_rsb_hw_init() issues a controller soft reset and waits for the reset
> bit to clear, but it ignores readl_poll_timeout()'s return value.
> 
> If the controller never leaves reset, probe can continue to publish child
> devices and system resume can report success without a usable RSB
> controller.
> 
> Return the timeout error, assert reset again, and disable the clock before
> leaving the hardware-init path.
> 
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>

Acked-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Best regards,
Jernej
diff mbox series

Patch

diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
index daf0ea563..f83224dea 100644
--- a/drivers/bus/sunxi-rsb.c
+++ b/drivers/bus/sunxi-rsb.c
@@ -669,8 +669,12 @@  static int sunxi_rsb_hw_init(struct sunxi_rsb *rsb)
 
 	/* reset the controller */
 	writel(RSB_CTRL_SOFT_RST, rsb->regs + RSB_CTRL);
-	readl_poll_timeout(rsb->regs + RSB_CTRL, reg,
-			   !(reg & RSB_CTRL_SOFT_RST), 1000, 100000);
+	ret = readl_poll_timeout(rsb->regs + RSB_CTRL, reg,
+				 !(reg & RSB_CTRL_SOFT_RST), 1000, 100000);
+	if (ret) {
+		dev_err(dev, "soft reset timed out\n");
+		goto err_reset_assert;
+	}
 
 	/*
 	 * Clock frequency and delay calculation code is from
@@ -696,6 +700,8 @@  static int sunxi_rsb_hw_init(struct sunxi_rsb *rsb)
 
 	return 0;
 
+err_reset_assert:
+	reset_control_assert(rsb->rstc);
 err_clk_disable:
 	clk_disable_unprepare(rsb->clk);