Re: [PATCH] bus: sunxi-rsb: fail hardware init on soft-reset timeout

[sashiko-bot@kernel.org]

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?

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