Replacing rk35xx-montjoie-crypto-v2-rk35xx.patch with rk35xx-crypto-v3.patch for Linux kernel v6.18 v7.0 v7.1#9796
Conversation
…3.patch for Linux kernel v6.18 v7.0 v7.1
Dawidro
requested review from
ColorfulRhino,
HeyMeco,
JohnTheCoolingFan,
SuperKali,
TheSnowfield,
Tonymac32,
ahoneybun,
amazingfate,
brentr,
catalinii,
chainsx,
efectn,
iav,
igorpecovnik,
joekhoobyar,
krachlatte,
lanefu,
paolosabatino,
prahal,
pyavitz,
rpardini and
schwar3kat
as code owners
📝 WalkthroughWalkthroughThis pull request adds Rockchip RK3588/RK3568 V2 crypto offloader support across three kernel versions (6.18, 7.0, 7.1) via device-tree bindings, DTS node integration, reset handling changes, and a new crypto engine driver supporting AES and hash algorithms with DMA acceleration and software fallback paths. ChangesRockchip RK2 Crypto V2 Driver Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
05
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
patch/kernel/archive/rockchip64-7.0/rk35xx-crypto-v3.patch (4)
1585-1589:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMirror the
-ENODEVguard from the skcipher path.
get_rk2_crypto()can returnNULLduring teardown becauserk2_crypto_remove()drops the device fromrocklistbefore unregistering the algorithms. The skcipher path already handles that, but the hash path dereferencesdev->engineunconditionally.Suggested fix
dev = get_rk2_crypto(); + if (!dev) + return -ENODEV; rctx->dev = dev; engine = dev->engine;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/rockchip64-7.0/rk35xx-crypto-v3.patch` around lines 1585 - 1589, The hash path dereferences dev->engine after calling get_rk2_crypto() without handling a NULL return; mirror the skcipher path by checking the return value of get_rk2_crypto() and returning -ENODEV (or performing the same cleanup/exit logic used in the skcipher path) when dev is NULL to avoid dereferencing during teardown (reference functions/variables: get_rk2_crypto(), rctx, dev, engine, rk2_crypto_remove(), and the skcipher code path which already performs this guard).
1579-1583:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle empty-message digests before inspecting the scatterlist.
rk2_ahash_need_fallback()walksreq->srcbefore the zero-length fast path runs. A validreq->nbytes == 0digest can come withreq->src == NULL, so this can oops instead of returning the precomputed empty-message hash.Suggested fix
- if (rk2_ahash_need_fallback(req)) - return rk2_ahash_digest_fb(req); - if (!req->nbytes) return zero_message_process(req); + + if (rk2_ahash_need_fallback(req)) + return rk2_ahash_digest_fb(req);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/rockchip64-7.0/rk35xx-crypto-v3.patch` around lines 1579 - 1583, The code calls rk2_ahash_need_fallback(req) before checking for a zero-length message, which can dereference req->src when nbytes == 0 and src is NULL; reorder the checks so you handle the empty-message fast path first (if (!req->nbytes) return zero_message_process(req);) and only then call rk2_ahash_need_fallback(req) and rk2_ahash_digest_fb(req) to avoid walking the scatterlist for zero-length requests.
2247-2251:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the processed byte count when rolling the IV.
Both offsets are derived from the full SG element length, but this loop only processes
todo = min(..., len)bytes per DMA step. If the request ends in the middle of an SG entry, CBC/XTS will pick the next IV from bytes that were never part of the operation.Suggested fix
if (areq->iv && crypto_skcipher_ivsize(tfm) > 0) { if (rctx->mode & RK2_CRYPTO_DEC) { - offset = sgs->length - ivsize; + offset = min(len, sgs->length) - ivsize; scatterwalk_map_and_copy(rctx->backup_iv, sgs, offset, ivsize, 0); } } @@ if (areq->iv && ivsize > 0) { - offset = sgd->length - ivsize; + offset = todo - ivsize; if (rctx->mode & RK2_CRYPTO_DEC) { memcpy(areq->iv, rctx->backup_iv, ivsize); memzero_explicit(rctx->backup_iv, ivsize);Also applies to: 2380-2387
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/rockchip64-7.0/rk35xx-crypto-v3.patch` around lines 2247 - 2251, The IV-roll code uses the full SG element length (sgs->length) to compute offset which is wrong when the DMA step processed only todo/min(len) bytes; update the logic in the decryption branch inside the IV handling (the block referencing areq->iv, crypto_skcipher_ivsize(tfm), rctx->mode & RK2_CRYPTO_DEC, offset, sgs and scatterwalk_map_and_copy) to compute offset from the actual processed byte count (use the local processed/todo variable or len_processed) instead of sgs->length so the backup_iv is copied from the bytes that were actually consumed; apply the same fix to the analogous loop around the other occurrence referenced in the comment (the block at the other occurrence using the same symbols).
357-361:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the original clock acquisition errno.
If
devm_clk_bulk_get_all()returns a negative errno such as-EPROBE_DEFER, the current code skips the error check and proceeds directly to the count comparison, incorrectly reporting "Missing clocks" instead of propagating the real error. This breaks deferred probing for late SCMI/CRU clock providers.Add an explicit check for negative return values before the count comparison:
Suggested fix
dev->num_clks = devm_clk_bulk_get_all(dev->dev, &dev->clks); +if (dev->num_clks < 0) + return dev->num_clks; + if (dev->num_clks < dev->variant->num_clks) { dev_err(dev->dev, "Missing clocks, got %d instead of %d\n", dev->num_clks, dev->variant->num_clks); return -EINVAL;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/rockchip64-7.0/rk35xx-crypto-v3.patch` around lines 357 - 361, The code calls devm_clk_bulk_get_all() and immediately treats dev->num_clks as a count; first check if the return value is negative and propagate that errno instead of comparing counts—i.e. after calling devm_clk_bulk_get_all(dev->dev, &dev->clks) inspect dev->num_clks for < 0 and return that value (preserving -EPROBE_DEFER etc.); only then compare dev->num_clks against dev->variant->num_clks and use dev_err for the "Missing clocks" message when the count is insufficient.patch/kernel/archive/rockchip64-7.1/rk35xx-crypto-v3.patch (2)
1585-1590:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard the ahash path against an empty device list.
get_rk2_crypto()can returnNULLduring teardown/error paths. Dereferencingdev->enginehere would Oops the kernel; the skcipher path already handles the same case with-ENODEV.Suggested fix
dev = get_rk2_crypto(); + if (!dev) + return -ENODEV; rctx->dev = dev; engine = dev->engine;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/rockchip64-7.1/rk35xx-crypto-v3.patch` around lines 1585 - 1590, The ahash path must guard against get_rk2_crypto() returning NULL: call get_rk2_crypto(), check if dev is NULL and return -ENODEV (matching the skcipher path) before assigning rctx->dev or dereferencing dev->engine; only set rctx->dev and read engine when dev is non-NULL, then call crypto_transfer_hash_request_to_engine(engine, req).
357-362:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate
devm_clk_bulk_get_all()errors instead of collapsing them to-EINVAL.The
devm_clk_bulk_get_all()function returns negative errno codes on error, including-EPROBE_DEFERwhen clock providers are not yet available. The current code'sif (dev->num_clks < dev->variant->num_clks)check does not guard against negative returns, so-EPROBE_DEFER(a negative value) passes the condition as true, causingreturn -EINVALto suppress the error and convert a recoverable deferred probe into a permanent failure.Suggested fix
dev->num_clks = devm_clk_bulk_get_all(dev->dev, &dev->clks); + if (dev->num_clks < 0) + return dev->num_clks; + if (dev->num_clks < dev->variant->num_clks) { dev_err(dev->dev, "Missing clocks, got %d instead of %d\n", dev->num_clks, dev->variant->num_clks);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patch/kernel/archive/rockchip64-7.1/rk35xx-crypto-v3.patch` around lines 357 - 362, The code calls devm_clk_bulk_get_all() and compares dev->num_clks against dev->variant->num_clks but fails to handle negative error returns (e.g. -EPROBE_DEFER), collapsing all errors to -EINVAL; change the logic in the block around devm_clk_bulk_get_all(), check the raw return value (store it in a variable like ret), if ret < 0 return ret to propagate errors (or specifically return -EPROBE_DEFER when seen), otherwise compare dev->num_clks to dev->variant->num_clks and only return -EINVAL when the count is legitimately too small; references: devm_clk_bulk_get_all, dev->num_clks, dev->variant->num_clks, and the existing return -EINVAL.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@patch/kernel/archive/rockchip64-6.18/rk35xx-crypto-v3.patch`:
- Around line 5-27: The patch header contains a duplicated changelog block
describing changes to
Documentation/devicetree/bindings/crypto/rockchip,rk3588-crypto.yaml,
drivers/crypto/Makefile and rk2_crypto.h; remove the repeated copy and keep a
single consolidated changelog paragraph that lists the three items once (retain
the corrected clock-names, reg size, root Makefile hook addition, and the
rk2_crypto.h additions like dbgfs_info, `#include` <linux/reset.h>, and the
fallback_req ordering comment). Ensure only one instance of that block remains
and that its text preserves all unique details from both copies.
In `@patch/kernel/archive/rockchip64-7.1/rk35xx-crypto-v3.patch`:
- Around line 1020-1027: crypto_engine_start(rkc->engine) is called without
checking its return value; capture its return (e.g., err =
crypto_engine_start(rkc->engine)), and if non-zero handle the failure by
cleaning up the allocated engine and jumping to the existing err_dma error path
so probe fails instead of continuing. Specifically, after
crypto_engine_alloc_init and before init_completion, check the return of
crypto_engine_start(rkc->engine); on error free/stop the engine via the same
cleanup used for rkc->engine (e.g., call the engine cleanup routine used
elsewhere in this file to release rkc->engine) and goto err_dma.
---
Outside diff comments:
In `@patch/kernel/archive/rockchip64-7.0/rk35xx-crypto-v3.patch`:
- Around line 1585-1589: The hash path dereferences dev->engine after calling
get_rk2_crypto() without handling a NULL return; mirror the skcipher path by
checking the return value of get_rk2_crypto() and returning -ENODEV (or
performing the same cleanup/exit logic used in the skcipher path) when dev is
NULL to avoid dereferencing during teardown (reference functions/variables:
get_rk2_crypto(), rctx, dev, engine, rk2_crypto_remove(), and the skcipher code
path which already performs this guard).
- Around line 1579-1583: The code calls rk2_ahash_need_fallback(req) before
checking for a zero-length message, which can dereference req->src when nbytes
== 0 and src is NULL; reorder the checks so you handle the empty-message fast
path first (if (!req->nbytes) return zero_message_process(req);) and only then
call rk2_ahash_need_fallback(req) and rk2_ahash_digest_fb(req) to avoid walking
the scatterlist for zero-length requests.
- Around line 2247-2251: The IV-roll code uses the full SG element length
(sgs->length) to compute offset which is wrong when the DMA step processed only
todo/min(len) bytes; update the logic in the decryption branch inside the IV
handling (the block referencing areq->iv, crypto_skcipher_ivsize(tfm),
rctx->mode & RK2_CRYPTO_DEC, offset, sgs and scatterwalk_map_and_copy) to
compute offset from the actual processed byte count (use the local
processed/todo variable or len_processed) instead of sgs->length so the
backup_iv is copied from the bytes that were actually consumed; apply the same
fix to the analogous loop around the other occurrence referenced in the comment
(the block at the other occurrence using the same symbols).
- Around line 357-361: The code calls devm_clk_bulk_get_all() and immediately
treats dev->num_clks as a count; first check if the return value is negative and
propagate that errno instead of comparing counts—i.e. after calling
devm_clk_bulk_get_all(dev->dev, &dev->clks) inspect dev->num_clks for < 0 and
return that value (preserving -EPROBE_DEFER etc.); only then compare
dev->num_clks against dev->variant->num_clks and use dev_err for the "Missing
clocks" message when the count is insufficient.
In `@patch/kernel/archive/rockchip64-7.1/rk35xx-crypto-v3.patch`:
- Around line 1585-1590: The ahash path must guard against get_rk2_crypto()
returning NULL: call get_rk2_crypto(), check if dev is NULL and return -ENODEV
(matching the skcipher path) before assigning rctx->dev or dereferencing
dev->engine; only set rctx->dev and read engine when dev is non-NULL, then call
crypto_transfer_hash_request_to_engine(engine, req).
- Around line 357-362: The code calls devm_clk_bulk_get_all() and compares
dev->num_clks against dev->variant->num_clks but fails to handle negative error
returns (e.g. -EPROBE_DEFER), collapsing all errors to -EINVAL; change the logic
in the block around devm_clk_bulk_get_all(), check the raw return value (store
it in a variable like ret), if ret < 0 return ret to propagate errors (or
specifically return -EPROBE_DEFER when seen), otherwise compare dev->num_clks to
dev->variant->num_clks and only return -EINVAL when the count is legitimately
too small; references: devm_clk_bulk_get_all, dev->num_clks,
dev->variant->num_clks, and the existing return -EINVAL.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3d6fa89a-1998-430f-82f6-5d97db4ff876
📒 Files selected for processing (3)
patch/kernel/archive/rockchip64-6.18/rk35xx-crypto-v3.patchpatch/kernel/archive/rockchip64-7.0/rk35xx-crypto-v3.patchpatch/kernel/archive/rockchip64-7.1/rk35xx-crypto-v3.patch
| Documentation/devicetree/bindings/crypto/rockchip,rk3588-crypto.yaml | ||
| Clock names corrected. Original had "a" and "h" as clock-names items. v3 corrects them to "core", "aclk", "hclk" matching the actual clock signal names in the hardware and passing make dt_binding_check. | ||
| YAML example reg size fixed. Original example said 0x4000 (16KB). v3 corrects to 0x2000 (8KB) to match both DTS nodes and the actual hardware register map. | ||
|
|
||
| Signed-off-by: Corentin Labbe <clabbe@baylibre.com> | ||
| --- | ||
| Documentation/devicetree/bindings/crypto/rockchip,rk3588-crypto.yaml | 65 ++++++++++ | ||
| 1 file changed, 65 insertions(+) | ||
| drivers/crypto/Makefile | ||
| Root Makefile hook added. v3 adds obj-$(CONFIG_CRYPTO_DEV_ROCKCHIP2) += rockchip/ to the root crypto Makefile. Without this, if CONFIG_CRYPTO_DEV_ROCKCHIP (the V1 driver) is disabled, the build system never descends into drivers/crypto/rockchip/ and the V2 driver silently doesn't build regardless of Kconfig selection. | ||
|
|
||
| rk2_crypto.h | ||
| dbgfs_info field added to struct rockchip_ip. Original was missing it, causing register_debugfs to overwrite dbgfs_stats with the info file pointer. v3 adds struct dentry *dbgfs_info as the third debugfs member. | ||
| #include <linux/reset.h> added. Moved from rk2_crypto.c so that rk2_crypto_ahash.c and rk2_crypto_skcipher.c can call reset_control_assert/deassert directly in their DMA error recovery paths. | ||
| fallback_req ordering documented. Added // keep at the end comment on struct skcipher_request fallback_req in rk2_cipher_rctx. Same ordering maintained in rk2_ahash_rctx with struct ahash_request fallback_req as last member — required for the flexible array __ctx placement to work correctly. | ||
|
|
||
| Documentation/devicetree/bindings/crypto/rockchip,rk3588-crypto.yaml | ||
| Clock names corrected. Original had "a" and "h" as clock-names items. v3 corrects them to "core", "aclk", "hclk" matching the actual clock signal names in the hardware and passing make dt_binding_check. | ||
| YAML example reg size fixed. Original example said 0x4000 (16KB). v3 corrects to 0x2000 (8KB) to match both DTS nodes and the actual hardware register map. | ||
|
|
||
| drivers/crypto/Makefile | ||
| Root Makefile hook added. v3 adds obj-$(CONFIG_CRYPTO_DEV_ROCKCHIP2) += rockchip/ to the root crypto Makefile. Without this, if CONFIG_CRYPTO_DEV_ROCKCHIP (the V1 driver) is disabled, the build system never descends into drivers/crypto/rockchip/ and the V2 driver silently doesn't build regardless of Kconfig selection. | ||
|
|
||
| rk2_crypto.h | ||
| dbgfs_info field added to struct rockchip_ip. Original was missing it, causing register_debugfs to overwrite dbgfs_stats with the info file pointer. v3 adds struct dentry *dbgfs_info as the third debugfs member. | ||
| #include <linux/reset.h> added. Moved from rk2_crypto.c so that rk2_crypto_ahash.c and rk2_crypto_skcipher.c can call reset_control_assert/deassert directly in their DMA error recovery paths. | ||
| fallback_req ordering documented. Added // keep at the end comment on struct skcipher_request fallback_req in rk2_cipher_rctx. Same ordering maintained in rk2_ahash_rctx with struct ahash_request fallback_req as last member — required for the flexible array __ctx placement to work correctly. |
There was a problem hiding this comment.
Duplicate changelog block in patch header.
Lines 5-15 and lines 17-27 are byte-identical (Documentation/devicetree/bindings, drivers/crypto/Makefile, rk2_crypto.h sections appear twice). Looks like a copy/paste leftover; please collapse to a single block to keep the prose changelog readable.
✏️ Proposed fix
@@
fallback_req ordering documented. Added // keep at the end comment on struct skcipher_request fallback_req in rk2_cipher_rctx. Same ordering maintained in rk2_ahash_rctx with struct ahash_request fallback_req as last member — required for the flexible array __ctx placement to work correctly.
-Documentation/devicetree/bindings/crypto/rockchip,rk3588-crypto.yaml
-Clock names corrected. Original had "a" and "h" as clock-names items. v3 corrects them to "core", "aclk", "hclk" matching the actual clock signal names in the hardware and passing make dt_binding_check.
-YAML example reg size fixed. Original example said 0x4000 (16KB). v3 corrects to 0x2000 (8KB) to match both DTS nodes and the actual hardware register map.
-
-drivers/crypto/Makefile
-Root Makefile hook added. v3 adds obj-$(CONFIG_CRYPTO_DEV_ROCKCHIP2) += rockchip/ to the root crypto Makefile. Without this, if CONFIG_CRYPTO_DEV_ROCKCHIP (the V1 driver) is disabled, the build system never descends into drivers/crypto/rockchip/ and the V2 driver silently doesn't build regardless of Kconfig selection.
-
-rk2_crypto.h
-dbgfs_info field added to struct rockchip_ip. Original was missing it, causing register_debugfs to overwrite dbgfs_stats with the info file pointer. v3 adds struct dentry *dbgfs_info as the third debugfs member.
-#include <linux/reset.h> added. Moved from rk2_crypto.c so that rk2_crypto_ahash.c and rk2_crypto_skcipher.c can call reset_control_assert/deassert directly in their DMA error recovery paths.
-fallback_req ordering documented. Added // keep at the end comment on struct skcipher_request fallback_req in rk2_cipher_rctx. Same ordering maintained in rk2_ahash_rctx with struct ahash_request fallback_req as last member — required for the flexible array __ctx placement to work correctly.
-
rk2_crypto_ahash.c📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Documentation/devicetree/bindings/crypto/rockchip,rk3588-crypto.yaml | |
| Clock names corrected. Original had "a" and "h" as clock-names items. v3 corrects them to "core", "aclk", "hclk" matching the actual clock signal names in the hardware and passing make dt_binding_check. | |
| YAML example reg size fixed. Original example said 0x4000 (16KB). v3 corrects to 0x2000 (8KB) to match both DTS nodes and the actual hardware register map. | |
| Signed-off-by: Corentin Labbe <clabbe@baylibre.com> | |
| --- | |
| Documentation/devicetree/bindings/crypto/rockchip,rk3588-crypto.yaml | 65 ++++++++++ | |
| 1 file changed, 65 insertions(+) | |
| drivers/crypto/Makefile | |
| Root Makefile hook added. v3 adds obj-$(CONFIG_CRYPTO_DEV_ROCKCHIP2) += rockchip/ to the root crypto Makefile. Without this, if CONFIG_CRYPTO_DEV_ROCKCHIP (the V1 driver) is disabled, the build system never descends into drivers/crypto/rockchip/ and the V2 driver silently doesn't build regardless of Kconfig selection. | |
| rk2_crypto.h | |
| dbgfs_info field added to struct rockchip_ip. Original was missing it, causing register_debugfs to overwrite dbgfs_stats with the info file pointer. v3 adds struct dentry *dbgfs_info as the third debugfs member. | |
| #include <linux/reset.h> added. Moved from rk2_crypto.c so that rk2_crypto_ahash.c and rk2_crypto_skcipher.c can call reset_control_assert/deassert directly in their DMA error recovery paths. | |
| fallback_req ordering documented. Added // keep at the end comment on struct skcipher_request fallback_req in rk2_cipher_rctx. Same ordering maintained in rk2_ahash_rctx with struct ahash_request fallback_req as last member — required for the flexible array __ctx placement to work correctly. | |
| Documentation/devicetree/bindings/crypto/rockchip,rk3588-crypto.yaml | |
| Clock names corrected. Original had "a" and "h" as clock-names items. v3 corrects them to "core", "aclk", "hclk" matching the actual clock signal names in the hardware and passing make dt_binding_check. | |
| YAML example reg size fixed. Original example said 0x4000 (16KB). v3 corrects to 0x2000 (8KB) to match both DTS nodes and the actual hardware register map. | |
| drivers/crypto/Makefile | |
| Root Makefile hook added. v3 adds obj-$(CONFIG_CRYPTO_DEV_ROCKCHIP2) += rockchip/ to the root crypto Makefile. Without this, if CONFIG_CRYPTO_DEV_ROCKCHIP (the V1 driver) is disabled, the build system never descends into drivers/crypto/rockchip/ and the V2 driver silently doesn't build regardless of Kconfig selection. | |
| rk2_crypto.h | |
| dbgfs_info field added to struct rockchip_ip. Original was missing it, causing register_debugfs to overwrite dbgfs_stats with the info file pointer. v3 adds struct dentry *dbgfs_info as the third debugfs member. | |
| #include <linux/reset.h> added. Moved from rk2_crypto.c so that rk2_crypto_ahash.c and rk2_crypto_skcipher.c can call reset_control_assert/deassert directly in their DMA error recovery paths. | |
| fallback_req ordering documented. Added // keep at the end comment on struct skcipher_request fallback_req in rk2_cipher_rctx. Same ordering maintained in rk2_ahash_rctx with struct ahash_request fallback_req as last member — required for the flexible array __ctx placement to work correctly. | |
| Documentation/devicetree/bindings/crypto/rockchip,rk3588-crypto.yaml | |
| Clock names corrected. Original had "a" and "h" as clock-names items. v3 corrects them to "core", "aclk", "hclk" matching the actual clock signal names in the hardware and passing make dt_binding_check. | |
| YAML example reg size fixed. Original example said 0x4000 (16KB). v3 corrects to 0x2000 (8KB) to match both DTS nodes and the actual hardware register map. | |
| drivers/crypto/Makefile | |
| Root Makefile hook added. v3 adds obj-$(CONFIG_CRYPTO_DEV_ROCKCHIP2) += rockchip/ to the root crypto Makefile. Without this, if CONFIG_CRYPTO_DEV_ROCKCHIP (the V1 driver) is disabled, the build system never descends into drivers/crypto/rockchip/ and the V2 driver silently doesn't build regardless of Kconfig selection. | |
| rk2_crypto.h | |
| dbgfs_info field added to struct rockchip_ip. Original was missing it, causing register_debugfs to overwrite dbgfs_stats with the info file pointer. v3 adds struct dentry *dbgfs_info as the third debugfs member. | |
| `#include` <linux/reset.h> added. Moved from rk2_crypto.c so that rk2_crypto_ahash.c and rk2_crypto_skcipher.c can call reset_control_assert/deassert directly in their DMA error recovery paths. | |
| fallback_req ordering documented. Added // keep at the end comment on struct skcipher_request fallback_req in rk2_cipher_rctx. Same ordering maintained in rk2_ahash_rctx with struct ahash_request fallback_req as last member — required for the flexible array __ctx placement to work correctly. | |
| rk2_crypto_ahash.c |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@patch/kernel/archive/rockchip64-6.18/rk35xx-crypto-v3.patch` around lines 5 -
27, The patch header contains a duplicated changelog block describing changes to
Documentation/devicetree/bindings/crypto/rockchip,rk3588-crypto.yaml,
drivers/crypto/Makefile and rk2_crypto.h; remove the repeated copy and keep a
single consolidated changelog paragraph that lists the three items once (retain
the corrected clock-names, reg size, root Makefile hook addition, and the
rk2_crypto.h additions like dbgfs_info, `#include` <linux/reset.h>, and the
fallback_req ordering comment). Ensure only one instance of that block remains
and that its text preserves all unique details from both copies.
| + rkc->engine = crypto_engine_alloc_init(dev, true); | ||
| + if (!rkc->engine) { | ||
| + err = -ENOMEM; | ||
| + goto err_dma; | ||
| + } | ||
| + | ||
| + rkc->engine = crypto_engine_alloc_init(&pdev->dev, true); | ||
| + crypto_engine_start(rkc->engine); | ||
| + init_completion(&rkc->complete); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "rk35xx-crypto-v3.patch" -type fRepository: armbian/build
Length of output: 241
🏁 Script executed:
cat -n ./patch/kernel/archive/rockchip64-7.1/rk35xx-crypto-v3.patch | sed -n '1015,1035p'Repository: armbian/build
Length of output: 604
🌐 Web query:
crypto_engine_start linux kernel return value error handling
💡 Result:
The Linux kernel function crypto_engine_start(struct crypto_engine *engine) returns an int: 0 on success, or a negative errno code on failure [1][2]. The only explicit error condition in the source code is when the engine is already running or busy (engine->running || engine->busy), in which case it returns -EBUSY (-16) [1][2][3]. There are no other return paths shown in the implementation, so callers should check if the return value is 0; if negative, treat as failure, typically logging the errno or handling -EBUSY by retrying later. Official kernel documentation mentions starting the engine but does not detail return values or errors [4][5]. This behavior is consistent across mainline kernel sources [2].
Citations:
- 1: https://github.com/NeroReflex/linux/blob/3e43442d4994c9e1e202c98129a87e330f7faaed/crypto/crypto_engine.c
- 2: https://github.com/torvalds/linux/blob/master/crypto/crypto_engine.c
- 3: https://kernel.googlesource.com/pub/scm/linux/kernel/git/mark/linux/+/refs/heads/arm/perf/acpi/crypto/crypto_engine.c
- 4: https://www.kernel.org/doc/html/v6.4/crypto/crypto_engine.html
- 5: https://docs.kernel.org/crypto/crypto_engine.html
🏁 Script executed:
cat -n ./patch/kernel/archive/rockchip64-7.1/rk35xx-crypto-v3.patch | sed -n '980,1050p'Repository: armbian/build
Length of output: 2263
🏁 Script executed:
cat -n ./patch/kernel/archive/rockchip64-7.1/rk35xx-crypto-v3.patch | grep -A 5 -B 5 "err_engine:"Repository: armbian/build
Length of output: 484
Check the result of crypto_engine_start().
The engine is started without checking its return value. If crypto_engine_start() fails (e.g., returns -EBUSY), the probe continues and registers algorithms against a failed engine. This masks the error, causing the probe to succeed and deferring the failure to the first crypto request instead of failing cleanly at probe time.
Suggested fix
rkc->engine = crypto_engine_alloc_init(dev, true);
if (!rkc->engine) {
err = -ENOMEM;
goto err_dma;
}
- crypto_engine_start(rkc->engine);
+ err = crypto_engine_start(rkc->engine);
+ if (err)
+ goto err_engine;
+
init_completion(&rkc->complete);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@patch/kernel/archive/rockchip64-7.1/rk35xx-crypto-v3.patch` around lines 1020
- 1027, crypto_engine_start(rkc->engine) is called without checking its return
value; capture its return (e.g., err = crypto_engine_start(rkc->engine)), and if
non-zero handle the failure by cleaning up the allocated engine and jumping to
the existing err_dma error path so probe fails instead of continuing.
Specifically, after crypto_engine_alloc_init and before init_completion, check
the return of crypto_engine_start(rkc->engine); on error free/stop the engine
via the same cleanup used for rkc->engine (e.g., call the engine cleanup routine
used elsewhere in this file to release rkc->engine) and goto err_dma.
|
✅ This PR has been reviewed and approved — all set for merge! |
igorpecovnik
left a comment
igorpecovnik
left a comment
There was a problem hiding this comment.
Please check / resolve AI comments.
|
I would not merge this before release because for once the v2 patch has proven to work or at least I am not aware of anything crypto related that has been reported problematic. And for the other on this v3 I guess nobody tested this yet. So I'd say merge right after release so testing happens more or less automatically in the upcoming development cycle. |
igorpecovnik
added
08
|
With all due respect, merging or maintaining a faulty driver is problematic. I previously flagged that there are critical issues with updated_driver_output.txt I provide two logs via pastebin for comparison: one for my updated driver (
|
I wouldn't mind mergeing WiP as long as it was submitted to the LKML -- can you share the lore link? |
|
My apologies, I was bit busy last week but managed finalizing the patch. Patch series can be found at: https://github.com/Dawidro/linux/tree/rk3588-crypto It's ready for submission. I will let know about the progress in due course. |
|
open concerns from rabbit must be addressed or dismissed. |
4 participants
Description
Fixes and updates to
This issue for instance:
[Bug]: rk3588 crypto unaccelerated #7703
Documentation summary for feature / change
Documentation/devicetree/bindings/crypto/rockchip,rk3588-crypto.yaml
Clock names corrected. Original had "a" and "h" as clock-names items. v3 corrects them to "core", "aclk", "hclk" matching the actual clock signal names in the hardware and passing make dt_binding_check.
YAML example reg size fixed. Original example said 0x4000 (16KB). v3 corrects to 0x2000 (8KB) to match both DTS nodes and the actual hardware register map.
drivers/crypto/Makefile
Root Makefile hook added. v3 adds obj-$(CONFIG_CRYPTO_DEV_ROCKCHIP2) += rockchip/ to the root crypto Makefile. Without this, if CONFIG_CRYPTO_DEV_ROCKCHIP (the V1 driver) is disabled, the build system never descends into drivers/crypto/rockchip/ and the V2 driver silently doesn't build regardless of Kconfig selection.
rk2_crypto.h
dbgfs_info field added to struct rockchip_ip. Original was missing it, causing register_debugfs to overwrite dbgfs_stats with the info file pointer. v3 adds struct dentry *dbgfs_info as the third debugfs member.
#include <linux/reset.h> added. Moved from rk2_crypto.c so that rk2_crypto_ahash.c and rk2_crypto_skcipher.c can call reset_control_assert/deassert directly in their DMA error recovery paths.
fallback_req ordering documented. Added // keep at the end comment on struct skcipher_request fallback_req in rk2_cipher_rctx. Same ordering maintained in rk2_ahash_rctx with struct ahash_request fallback_req as last member — required for the flexible array __ctx placement to work correctly.
rk2_crypto.c
pm_init return value fixed. Original returned err at the end, which after pm_runtime_enable would always be 0 but was misleading. v3 explicitly return 0.
IRQ handler fires complete() only on LISTDONE or hard error. Original called complete() on any non-zero DMA_INT_ST. Intermediate SRC_INT per-LLI-entry interrupts would prematurely wake the hash path before all data was processed, producing wrong digests. v3 only signals completion for RK2_CRYPTO_DMA_INT_LISTDONE (BIT(0)) or error bits (0xF8).
Consistent spin_lock_bh throughout. get_rk2_crypto(), probe, and remove all use spin_lock_bh/spin_unlock_bh. Original used plain spin_lock in get_rk2_crypto() and probe, risking deadlock if a crypto engine callback ran on the same CPU.
Debugfs overwrite bug fixed. Original register_debugfs wrote to rocklist.dbgfs_stats twice — the second write (info file) overwrote the stats file pointer. v3 correctly uses rocklist.dbgfs_info for the second file.
pm_runtime_resume_and_get return value checked in probe. Original ignored this return value. v3 checks it and jumps to err_pm on failure.
PM reference count balanced on probe success. Original left usage count at 1 after successful registration, pinning the device ON forever. v3 calls pm_runtime_mark_last_busy + pm_runtime_put_autosuspend after successful registration.
Separate err_register_alg label with list_del + pm_runtime_put_sync. Original jumped to err_pm on registration failure, which skipped both the list cleanup and the PM put. v3 has a dedicated label that removes the device from the list and decrements the PM count before falling through to rk2_crypto_pm_exit.
crypto_engine_exit called before rk2_crypto_pm_exit in remove. Original order was reversed — clocks disabled before engine drained. v3 exits the engine first to flush all in-flight requests, then disables PM.
algt->dev handoff on multi-device remove. When the registering device is removed while a second device survives, v3 iterates all algorithm templates and updates algt->dev to the surviving device, preventing use-after-free in tfm_init/tfm_exit logging.
dma_free_coherent added to rk2_crypto_remove. The LLI table is a non-managed allocation. Original never freed it on normal remove, leaking it on every rmmod. v3 frees it at the end of remove.
pm_resume does full assert/udelay/deassert cycle. Ensures hardware is cleanly initialised with clocks running on every PM wakeup, not just deassert.
rk2_crypto_ahash.c
Multi-SG hardware fallback. Added if (sg_nents(areq->src) > 1) check at the top of rk2_ahash_need_fallback. The RK3568/3588 hardware padding engine (HW_PAD) loses block-count state across LLI descriptor boundaries, producing wrong digests for any multi-SG request. Single-SG requests continue to use hardware; multi-SG routes to software fallback and increments stat_fb_sgdiff.
Explicit payload length for hardware padding. Added writel(areq->nbytes, rkc->reg + RK2_CRYPTO_CH0_PC_LEN_0) before starting DMA. The HW_PAD bit requires the total message length to be programmed in advance so the hardware knows where to apply the Merkle–Damgård padding. Without this, single-SG hardware hashes would also produce wrong results.
INT_EN write-enable mask fixed. Original wrote RK2_CRYPTO_DMA_INT_LISTDONE | 0x7F with no upper bits, which is silently discarded by the HIWORD_UPDATE register pattern. v3 first clears stale pending interrupts (writel(0x7F, DMA_INT_ST)), then writes 0x007F007F so enable bits and their write-enable mask are both set.
LIST_INT added to last LLI descriptor. LISTDONE (BIT(0)) is only set in DMA_INT_ST when the last LLI entry has RK2_LLI_DMA_CTRL_LIST_INT (BIT(8)) set in dma_ctrl. Without it, DMA completes silently, no interrupt fires, 2-second timeout every request. v3 adds RK2_LLI_DMA_CTRL_LIST_INT | RK2_LLI_DMA_CTRL_SRC_INT to the last descriptor.
Uninterruptible completion wait. Changed from wait_for_completion_interruptible_timeout to wait_for_completion_timeout. The interruptible variant can return early on SIGTERM/SIGKILL while DMA is still writing to memory, causing data corruption or use-after-free.
timeout return value captured and checked. Original discarded the return value, making timeout undetectable. v3 stores the return in unsigned long timeout and checks !timeout for ETIMEDOUT, then separately checks !rkc->status for EIO.
rctx->nrsgs = 0 initialised before dma_map_sg. Ensures rk2_hash_unprepare never calls dma_unmap_sg on an uninitialised count.
Safe dma_unmap_sg in unprepare. Added if (rctx->nrsgs) guard before dma_unmap_sg, preventing a kernel panic if dma_map_sg failed in the prepare step.
MAX_LLI overflow check. Added if (ddi >= MAX_LLI) at the top of the LLI build loop with dev_err and -EINVAL. Prevents overflowing the DMA-coherent LLI table with a scatter-list longer than 20 entries.
readl_poll_timeout_atomic return value checked. Original discarded the return, allowing wrong digest output if HASH_VALID never set. v3 assigns to err and jumps to theend on timeout.
be32_to_cpu in digest readback. Hardware outputs all digest words (SHA1, SHA256, SHA384, SHA512, SM3, MD5) in big-endian register format. readl() on LE ARM64 gives a byte-swapped value; be32_to_cpu corrects it before put_unaligned_le32.
pm_runtime_mark_last_busy before put_autosuspend. Refreshes the autosuspend timer on each request completion so the device doesn't suspend between back-to-back requests.
DMA error hardware reset. On !rkc->status (DMA error interrupt), v3 performs reset_control_assert/udelay(10)/reset_control_deassert to recover the DMA engine from a stuck state before returning -EIO.
crypto_ahash_set_statesize promotion in rk2_hash_init_tfm. After allocating the fallback, v3 queries its statesize and promotes the template's statesize if the fallback needs more space. Fixes -EOVERFLOW on export()/import() when ARM Crypto Extensions drivers (sha1-ce, sha256-ce) advertise a larger statesize than the raw hash state.
rk2_crypto_skcipher.c
rk2_print gated with #ifdef CONFIG_CRYPTO_DEV_ROCKCHIP2_DEBUG. Register dumps on every DMA error flood production logs. v3 wraps the entire function and its callsite with the debug config guard.
OTP KEY VALID bit corrected. Original checked BIT(2) for both HASH BUSY and OTP KEY VALID (copy-paste error). v3 uses BIT(3) for OTP KEY VALID.
Multi-bit switch statements corrected. All field switches now use (v >> N) & mask before comparing case values, fixing cases that could never match after masking.
dd->dma_ctrl = assignment not |=. The LLI table persists across requests. Using |= accumulates stale flag bits from previous requests. v3 uses = for the initial value.
RK2_CRYPTO_DMA_CTL_START << 16 not literal 1 << 16. Ensures the write-enable mask is always correct regardless of the constant value.
get_rk2_crypto() with null check for cipher. Original used algt->dev (always first device) for all cipher requests, bypassing load balancing. v3 uses get_rk2_crypto() matching the hash path, with if (!rkc) return -ENODEV.
DMA unmap moved before timeout/error checks. Original goto theend on timeout bypassed dma_unmap_sg, permanently leaking the mapping. v3 unmaps unconditionally before checking results.
wait_for_completion_timeout (uninterruptible). Same fix as hash — prevents signal interruption while DMA is active.
Separate timeout/error errnos. ETIMEDOUT for timeout, EIO for DMA error with rk2_print debug dump.
INT_EN write-enable mask + stale clear. Same fix as hash path: writel(0x7F, DMA_INT_ST) then writel(0x007F007F, DMA_INT_EN).
LIST_INT added to cipher descriptor. RK2_LLI_DMA_CTRL_LIST_INT added to dd->dma_ctrl so LISTDONE fires in DMA_INT_ST.
pm_runtime_mark_last_busy before put_autosuspend.
DMA error hardware reset. Same reset_control_assert/deassert recovery as hash.
Fallback log changed to dev_dbg. Original dev_info in rk2_cipher_tfm_init flooded logs during PM stress testing (hundreds of TFM allocations per second). v3 uses dev_dbg.
Stricter XTS fallback check. Changed sg_nents(areq->src) > 1 to != 1 for the XTS-specific SG check. XTS requires exactly one contiguous buffer — more than one SG is wrong but so is zero.
include/dt-bindings/reset/rockchip,rk3588-cru.h
Patch correctly uses mainline SCMI for RK3588. mainline Linux kernel Commit 849d9db form Feb 22, 2025 "dt-bindings: reset: Add SCMI reset IDs for RK3588" so there is no reason to modify this file as entries exist already. Please note the patch still removes the entire RK3588_SECURECRU_RESET_OFFSET macro and all its entries as keeping them in rst-rk3588.c alongside SCMI would be redundant and potentially dangerous.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Please also note any relevant details for your test configuration.
CONFIG_CRYPTO_SELFTESTS=y
CONFIG_CRYPTO_SELFTESTS_FULL=y
modprobe tcrypt mode=38 # AES-ECB
modprobe tcrypt mode=39 # AES-CBC
modprobe tcrypt mode=400 # SHA-256
modprobe tcrypt mode=404 # SHA-512
modprobe tcrypt mode=405 # MD5
dmesg | grep -E "tcrypt|testing|passed|failed"
cat /proc/crypto | grep -A5 "rk2"
driver : rk2-sm3
module : rk_crypto2
priority : 300
refcnt : 1
selftest : passed
internal : no
driver : rk2-sha512
module : rk_crypto2
priority : 300
refcnt : 1
selftest : passed
internal : no
driver : rk2-sha384
module : rk_crypto2
priority : 300
refcnt : 1
selftest : passed
internal : no
driver : rk2-sha256
module : rk_crypto2
priority : 300
refcnt : 1
selftest : passed
internal : no
driver : rk2-sha1
module : rk_crypto2
priority : 300
refcnt : 1
selftest : passed
internal : no
driver : rk2-md5
module : rk_crypto2
priority : 300
refcnt : 1
selftest : passed
internal : no
driver : xts-aes-rk2
module : rk_crypto2
priority : 300
refcnt : 1
selftest : passed
internal : no
driver : cbc-aes-rk2
module : rk_crypto2
priority : 300
refcnt : 1
selftest : passed
internal : no
driver : ecb-aes-rk2
module : rk_crypto2
priority : 300
refcnt : 1
selftest : passed
internal : no
cat /sys/kernel/debug/rk2_crypto/stats
rk2-crypto fe370000.crypto requests: 738
ecb-aes-rk2 ecb(aes) reqs=144 fallback=1960
fallback due to length: 303
fallback due to alignment: 1651
fallback due to SGs: 0
cbc-aes-rk2 cbc(aes) reqs=183 fallback=2109
fallback due to length: 308
fallback due to alignment: 1790
fallback due to SGs: 7
xts-aes-rk2 xts(aes) reqs=174 fallback=2170
fallback due to length: 74
fallback due to alignment: 763
fallback due to SGs: 0
rk2-md5 md5 reqs=34 fallback=718
rk2-sha1 sha1 reqs=46 fallback=654
rk2-sha256 sha256 reqs=34 fallback=609
rk2-sha384 sha384 reqs=41 fallback=666
rk2-sha512 sha512 reqs=42 fallback=654
rk2-sm3 sm3 reqs=40 fallback=690
cat /sys/kernel/debug/rk2_crypto/info
CRYPTO_CLK_CTL 1
CRYPTO_RST_CTL 0
CRYPTO_AES_VERSION 707ff
AES 192
CRYPTO_DES_VERSION 30033
CRYPTO_SM4_VERSION 7ff
CRYPTO_HASH_VERSION 1ff
CRYPTO_HMAC_VERSION 1f
CRYPTO_RNG_VERSION 1000000
CRYPTO_PKA_VERSION 1000000
CRYPTO_CRYPTO_VERSION 2000001
cryptsetup benchmark --cipher aes-cbc --key-size 256
Tests are approximate using memory only (no storage IO).
Algorithm | Key | Encryption | Decryption
rapid suspend/resume
for i in $(seq 1 1000); do
echo -n "test$i" | sha256sum
sleep 0.003
done
for i in $(seq 1 20); do
modprobe rk_crypto2
sleep 1
rmmod rk_crypto2
sleep 0.5
done
Checklist:
Please delete options that are not relevant.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation