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

[Dragan Simic <dsimic@manjaro.org>]

Hello Diederik,

On 2025-01-15 15:59, Diederik de Haas wrote:
> 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.

Thanks a lot for providing a detailed description of the series
of events that have led to this patch!

> 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.

Removing the defaults from the Kconfig would be a much better
solution, but I'm afraid it would actually be quite disruptive,
requiring changes to various kernel configurations.

> 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.

The only upside of the current solution is that it represents
the least disruptive approach.

I'd suggest that this patch is kept as-is, and I'd add another
patch on top of it, which would remove the defaults from the
Kconfig and update the relevant default configs, of course if
the maintainers are fine with such an approach, and if there
is general agreement toward getting rid of the defaults.

Having two patches, as proposed above, would make the bisecting
later much easier, in case some unforeseen issues are discovered
in the future.

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