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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing `status = "disabled";` property in the device tree nodes, contradicting the commit message.
- [High] Unbounded DMA transfer length in hardware hash requests.
- [Medium] Software fallback logic unnecessarily evaluates unused scatterlist entries.
--

--- Patch [4]: [PATCH v3 4/4] arm64: dts: rockchip: Add crypto node to rk3588-base ---
commit 02aef3c847464eaa142ef8468b37da657834130b
Author: Dawid Olesinski <[email protected]>

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

This commit adds the device tree node for the V2 cryptographic hardware
accelerator on RK3588. It uses the ARM SCMI interface to obtain clocks
and the reset line from TrustZone firmware to avoid asynchronous bus faults.
The device tree node is intended to be disabled by default.

> The node is disabled by default; board files that wish to use hardware
> crypto offload must enable it.

[Severity: High]
Does this commit message match the device tree node definition? The node is
missing the status property, which means it will be enabled by default. Does
this also apply to the crypto@fe380000 node in rk356x-base.dtsi?

> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
> index fc1fdbfd31622..028781dd0d5e7 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
> @@ -2447,6 +2447,17 @@ sdhci: mmc@fe2e0000 {
>  		status = "disabled";
>  	};
>  
> +	crypto: crypto@fe370000 {
> +		compatible = "rockchip,rk3588-crypto", "rockchip,rk3568-crypto";
> +		reg = <0x0 0xfe370000 0x0 0x2000>;
> +		interrupts = <GIC_SPI 209 IRQ_TYPE_LEVEL_HIGH 0>;

[Severity: High]
Should there be a status = "disabled"; property included here? Probing the
hardware crypto offloader on boards without proper firmware configuration
can trigger asynchronous bus faults and boot failures.

[ ... ]

[Severity: High]
Can this hardware enablement trigger DMA transfer issues in the driver?

drivers/crypto/rockchip/rk2_crypto_ahash.c:rk2_hash_run() {
    ...
	dd->src_addr = cpu_to_le32(lower_32_bits(sg_dma_address(sgs)));
	dd->src_len  = cpu_to_le32(sg_dma_len(sgs));
    ...
}

If the crypto API submits an asynchronous hash request where the scatterlist
element's mapped length (sg_dma_len(sgs)) is larger than the requested hash
size (areq->nbytes), does this code program the DMA descriptor src_len to the
full mapped length?

Because the hardware only expects areq->nbytes, commanding the DMA engine
to push excess data could cause it to stall indefinitely when the crypto
engine stops accepting data, leading to a timeout and operation failure.

[Severity: Medium]
Does the software fallback logic unnecessarily evaluate unused scatterlist
entries?

drivers/crypto/rockchip/rk2_crypto_ahash.c:rk2_ahash_need_fallback() {
    ...
	sg = areq->src;
	while (sg) {
		if (!IS_ALIGNED(sg->offset, sizeof(u32))) {
			atomic_long_inc(&algt->stat_fb_align);
			return true;
		}
		if (sg->length % 4) {
			atomic_long_inc(&algt->stat_fb_sglen);
			return true;
		}
		sg = sg_next(sg);
	}
    ...
}

If a hash request has an areq->nbytes that fits entirely within the first
scatterlist entry (nents == 1), but the request includes a longer scatterlist
chain with trailing, unused entries that have unaligned offsets or lengths,
will this loop evaluate them and force an incorrect fallback to software
hashing?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4