Re: [PATCH 3/4] arm64: dts: rockchip: Add crypto node to rk356x-base

[sashiko-bot@kernel.org]

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

New issues:
- [Low] The `crypto@fe380000` device tree node is placed out of order with respect to its unit address.

Pre-existing issues:
- [High] The driver requests a shared interrupt before device clocks are enabled, and its ISR does not check the power state, leading to a potential fatal unclocked register access.
--

commit 30c6ac6a0076f1ce31fb0365a3eee2a85dadf551
Author: Dawid Olesinski <dawidro@gmail.com>

arm64: dts: rockchip: Add crypto node to rk356x-base

This commit adds the device tree node for the V2 cryptographic hardware
accelerator on RK356x SoCs. The IP block sits in the non-secure peripheral
domain and is disabled by default.

> diff --git a/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi b/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi
> index 64bdd8b7754b5..3b73a56046e77 100644
> --- a/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi
> @@ -1171,6 +1171,18 @@ gpu_leakage: gpu-leakage@1d {
>  		};
>  	};
>  
> +	crypto: crypto@fe380000 {

[Severity: Low]
Is this device tree node placed out of order?

Device tree nodes with unit addresses are typically sorted numerically by
their physical base address. Would it be better to place this node before
the rng@fe388000 node to maintain the established sorting order?

> +		compatible = "rockchip,rk3568-crypto";
> +		reg = <0x0 0xfe380000 0x0 0x2000>;
> +		interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&cru CLK_CRYPTO_NS_CORE>, <&cru ACLK_CRYPTO_NS>,
> +			 <&cru HCLK_CRYPTO_NS>;
> +		clock-names = "core", "aclk", "hclk";
> +		resets = <&cru SRST_CRYPTO_NS_CORE>;
> +		reset-names = "core";
> +		status = "disabled";
> +	};

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential issue
in the driver code that this DT node enables in
drivers/crypto/rockchip/rk2_crypto.c.

The driver requests a shared interrupt in rk2_crypto_probe() before
device clocks are enabled via runtime PM:

rk2_crypto_probe() {
    ...
    err = devm_request_irq(dev, rkc->irq,
                           rk2_crypto_irq_handle, IRQF_SHARED,
                           "rk-crypto", pdev);
    ...
}

If a shared device triggers the interrupt, or a spurious interrupt occurs
while the device clocks are disabled, the ISR rk2_crypto_irq_handle()
will execute and blindly read from the hardware status register:

rk2_crypto_irq_handle() {
    ...
    v = readl(rkc->reg + RK2_CRYPTO_DMA_INT_ST);
    ...
}

Could this cause a fatal external abort (SError) if the MMIO register
is accessed while the clock domain is unpowered?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260530160704.3453555-1-dawidro@gmail.com?part=3