Re: [PATCH v15 4/4] crypto: spacc - Add SPAcc Kconfig and Makefile

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Boolean Kconfig option selecting tristate crypto algorithms forces them to be built-in (`=y`), even when the driver is built as a module (`=m`).
- [Medium] Hardware operational tuning parameters are hardcoded as compile-time Kconfig options.
- [Low] Proliferation of custom Kconfig debug options instead of using standard kernel tracing.
- [Low] Misspellings in the Kconfig help texts ('oprations', 'opration').
--

commit 02c8050dd555ff6b5029092b4cb5ab0b1d97221f
Author: Pavitrakumar Managutte <pavitrakumarm@vayavyalabs.com>

crypto: spacc - Add SPAcc Kconfig and Makefile

This commit adds the Makefile and Kconfig entries for the new SPAcc
driver, integrating it into the drivers/crypto subsystem.

> diff --git a/drivers/crypto/dwc-spacc/Kconfig b/drivers/crypto/dwc-spacc/Kconfig
> new file mode 100644
> index 0000000000000..a9172606d77aa
> --- /dev/null
> +++ b/drivers/crypto/dwc-spacc/Kconfig
> @@ -0,0 +1,83 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +config CRYPTO_DEV_SPACC
> +	tristate "Support for dwc_spacc Security Protocol Accelerator"
> +	depends on HAS_DMA
> +	select CRYPTO_ENGINE
> +	default n
> +
> +	help
> +	  This enables support for SPAcc Hardware Accelerator.
> +
> +if CRYPTO_DEV_SPACC
> +
> +config CRYPTO_DEV_SPACC_HASH
> +	bool "Enable HASH functionality"
> +	default y
> +	select CRYPTO_HASH
> +	select CRYPTO_SHA1
> +	select CRYPTO_MD5

[Severity: Medium]
Since CRYPTO_DEV_SPACC_HASH is a boolean option, does selecting these tristate
core crypto symbols force them to be built-in (=y), even when the driver
itself is built as a module (=m)?

This could prevent distributions from compiling these generic algorithms as
modules and cause unnecessary vmlinux bloat.

[ ... ]

> +config CRYPTO_DEV_SPACC_DEBUG_TRACE_IO
> +	bool "Enable Trace MMIO reads/writes stats"
> +	default n
> +	help
> +	  Say y to enable Trace MMIO reads/writes stats.
> +	  To Debug and trace IO register read/write oprations.

[Severity: Low]
Is it necessary to use custom Kconfig options for tracing MMIO reads and 
writes, rather than utilizing standard kernel tracepoints or dynamic debug?

[Severity: Low]
This isn't a bug, but there is a typo here: "oprations" should be "operations".

> +
> +config CRYPTO_DEV_SPACC_DEBUG_TRACE_DDT
> +	bool "Enable Trace DDT entries stats"
> +	default n
> +	help
> +	  Say y to enable Enable DDT entry stats.
> +	  To Debug and trace DDT opration

[Severity: Low]
This isn't a bug, but there is a typo here: "opration" should be "operation".

> +
> +config CRYPTO_DEV_SPACC_SECURE_MODE
> +	bool "Enable Spacc secure mode stats"
> +	default n
> +	help
> +	  Say y to enable SPAcc secure modes stats.
> +
> +config CRYPTO_DEV_SPACC_PRIORITY
> +	int "VSPACC priority value"
> +	range 0 15
> +	default 1

[Severity: Medium]
Could this hardware priority parameter be configured dynamically via Device
Tree or module parameters instead of a compile-time Kconfig option?

Hardcoding this in Kconfig enforces a single global configuration for all
instances and prevents tuning across multi-SoC environments without a full
kernel recompilation.

> +	help
> +	  Default arbitration priority weight for this Virtual SPAcc instance.
> +	  Hardware resets this to 1. Higher values means higher priority.
> +
> +config CRYPTO_DEV_SPACC_INTERNAL_COUNTER
> +	int "SPAcc internal counter value"
> +	range 100000 1048575
> +	default 100000

[Severity: Medium]
Similarly, does hardcoding this hardware timeout limit in Kconfig enforce a
single global configuration across all instances?

Could this be exposed via Device Tree or module parameters instead to allow
platform-specific tuning at runtime?

> +	help
> +	  This value configures a hardware watchdog counter in the SPAcc engine.
> +	  The counter starts ticking when a completed cryptographic job is
> +	  sitting in the STATUS FIFO. If the job remains unprocessed for the
> +	  configured duration, an interrupt is triggered to ensure it is serviced.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701122941.2149121-1-pavitrakumarm@vayavyalabs.com?part=4