Re: [PATCH 3/3] hwrng: Don't default to HW_RANDOM when UML_RANDOM is the trigger

["Diederik de Haas" <didi.debian@cknow.org>]

[-- Attachment #1: Type: text/plain, Size: 4821 bytes --]

Hi,

On Wed Jan 15, 2025 at 2:07 PM CET, Dragan Simic wrote:
> Since the commit 72d3e093afae (um: random: Register random as hwrng-core
> device), selecting the UML_RANDOM option may result in various HW_RANDOM_*
> options becoming selected as well, which doesn't make much sense for UML
> that obviously cannot use any of those HWRNG devices.
>
> Let's have the HW_RANDOM_* options selected by default only when UML_RANDOM
> actually isn't already selected.  With that in place, selecting UML_RANDOM
> no longer "triggers" the selection of various HW_RANDOM_* options.
>
> Fixes: 72d3e093afae (um: random: Register random as hwrng-core device)
> Reported-by: Diederik de Haas <didi.debian@cknow.org>
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> ---
>  drivers/char/hw_random/Kconfig | 76 +++++++++++++++++-----------------
>  1 file changed, 38 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
> index e84c7f431840..283aba711af5 100644
> --- a/drivers/char/hw_random/Kconfig
> +++ b/drivers/char/hw_random/Kconfig
> @@ -38,47 +38,47 @@ config HW_RANDOM_TIMERIOMEM
>  config HW_RANDOM_INTEL
>  	tristate "Intel HW Random Number Generator support"
>  	depends on (X86 || COMPILE_TEST) && PCI
> -	default HW_RANDOM
> +	default HW_RANDOM if !UML_RANDOM
>  	help
>  	  This driver provides kernel-side support for the Random Number
>  	  Generator hardware found on Intel i8xx-based motherboards.
>  
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called intel-rng.
>  
>  	  If unsure, say Y.
>  
>  config HW_RANDOM_AMD
>  	tristate "AMD HW Random Number Generator support"
>  	depends on (X86 || COMPILE_TEST)
>  	depends on PCI && HAS_IOPORT_MAP
> -	default HW_RANDOM
> +	default HW_RANDOM if !UML_RANDOM
>  	help
>  	  This driver provides kernel-side support for the Random Number
>  	  Generator hardware found on AMD 76x-based motherboards.
>  
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called amd-rng.
>  
>  	  If unsure, say Y.
>  
>  config HW_RANDOM_AIROHA
> ...
> @@ -603,7 +603,7 @@ config HW_RANDOM_ROCKCHIP
>  	tristate "Rockchip True Random Number Generator"
>  	depends on HW_RANDOM && (ARCH_ROCKCHIP || COMPILE_TEST)
>  	depends on HAS_IOMEM
> -	default HW_RANDOM
> +	default HW_RANDOM if !UML_RANDOM
>  	help
>  	  This driver provides kernel-side support for the True Random Number
>  	  Generator hardware found on some Rockchip SoC like RK3566 or RK3568.

Context:
I wanted to enable the HW_RANDOM_ROCKCHIP module in the Debian kernel
so I send a MR to enable it as module. One of the reviewers remarked
that this would *change* the module config from ``=y`` to ``=m`` as
``HW_RANDOM`` is configured ``=y`` due to Debian bug #1041007 [1].
IOW: if you don't say you want a HWRNG module, it will be built-in to
the Debian kernel, while Debian normally uses ``=m`` if possible.

So that's when I realized almost all modules have ``default HW_RANDOM``
and then found that UML_RANDOM selects HW_RANDOM which in turn would
enable (almost) all HWRNG modules unless you specify otherwise.
It's actually the depends which would mostly 'prevent' that.
This to me looks excessive, discussed the problem with Dragan which
resulted in this patch set.

But why not just remove (most of) the ``default HW_RANDOM`` lines
whereby a HWRNG module thus becomes opt-in instead of opt-out?

For ``HW_RANDOM_ROCKCHIP`` it's for the SoC found in *only* the rk3566
and rk3568 SoCs, but none of the others, and it's (currently) effective
only on rk3568 based devices (due to deliberate DT config).
In the help text of other modules I see mention of specific (series of)
motherboards, so also there it may not be useful for all.

I did a partial ``git blame`` to get an idea as to why those defaults
were there and found the following:

fed806f4072b ("[PATCH] allow hwrandom core to be a module")
from 2006-12-06 with the goal to have them modular

2d9cab5194c8 ("hwrng: Fix a few driver dependencies and defaults")
from 2014-04-08 which added several ... for consistency sake

e53ca8efcc5e ("hwrng: airoha - add support for Airoha EN7581 TRNG")
from 2024-10-17 with no explicit mention why it was done, so that was
most likely as that was used elsewhere (thus consistency)

So while this patch does prevent accidental enablement due to UML_RANDOM
enablement, it does seem to me to be needlessly complex and making it
opt-in, which was the assumption of my MR to begin with, much simpler.

I can be missing other considerations why the current solution would be
better, but I figured I'd mention my perspective.

Cheers,
  Diederik

[1] https://bugs.debian.org/1041007

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]