[PATCH v7 0/3] hwrng: add hwrng support for Rockchip RK3568

[Dragan Simic]

Hello Diederik,

On 2024-07-17 10:22, Diederik de Haas wrote:
> On Wednesday, 17 July 2024 04:58:51 CEST Chen-Yu Tsai wrote:
>> On Wed, Jul 17, 2024 at 10:25 AM Daniel Golle <daniel at makrotopia.org> 
>> wrote:
>> > On Tue, Jul 16, 2024 at 07:19:35PM +0200, Diederik de Haas wrote:
>> > > On Tuesday, 16 July 2024 18:53:43 CEST Diederik de Haas wrote:
>> > > > rngtest: FIPS 140-2(2001-10-10) Long run: 0
>> > >
>> > > I don't know if it means something, but I noticed that I have
>> > > ``Long run: 0`` with all my poor results,
>> > > while Chen-Yu had ``Long run: 1``.
>> > >
>> > > Different SoC (RK3399), but Anand had ``Long run: 0`` too on their
>> > > very poor result (100% failure):
>> > > https://lore.kernel.org/linux-rockchip/CANAwSgTTzZOwBaR9zjJ5VMpxm5BydtW6
>> > > rB2S7jg+dnoX8hAoWg at mail.gmail.com/>
>> > The conclusions I draw from that rather ugly situation are:
>> >  - The hwrng should not be enabled by default, but it should by done
>> >
>> >    for each board on which it is known to work well.
>> >
>> >  - RK_RNG_SAMPLE_CNT as well as the assumed rng quality should be
>> >
>> >    defined in DT for each board:
>> >    * introduce new 'rochchip,rng-sample-count' property
>> >    * read 'quality' property already used for timeriomem_rng
>> >
>> > I will prepare a follow-up patch taking those conclusions into account.
>> >
>> > Just for completeness, here my test result on the NanoPi R5C:
>> > root at OpenWrt:~# cat /dev/hwrng | rngtest -c 1000
>> > rngtest 6.15
>> > Copyright (c) 2004 by Henrique de Moraes Holschuh
>> > This is free software; see the source for copying conditions.  There is NO
>> > warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR
>> > PURPOSE.
>> >
>> > rngtest: starting FIPS tests...
>> > rngtest: bits received from input: 20000032
>> > rngtest: FIPS 140-2 successes: 875
>> > rngtest: FIPS 140-2 failures: 125
>> > rngtest: FIPS 140-2(2001-10-10) Monobit: 123
>> > rngtest: FIPS 140-2(2001-10-10) Poker: 5
>> > rngtest: FIPS 140-2(2001-10-10) Runs: 4
>> > rngtest: FIPS 140-2(2001-10-10) Long run: 0
>> > rngtest: FIPS 140-2(2001-10-10) Continuous run: 0
>> > rngtest: input channel speed: (min=85.171; avg=141.102;
>> > max=4882812.500)Kibits/s rngtest: FIPS tests speed: (min=17.809;
>> > avg=19.494; max=60.169)Mibits/s rngtest: Program run time: 139628605
>> > microseconds
>> 
>> I doubt this is per-board. The RNG is inside the SoC, so it could be a 
>> chip
>> quality thing.
> 
> I agree with ChenYu (and others) that this is isn't a per-board level 
> thing.
> I'd even go further: 's/I doubt/It can't be that/' (for the same reason
> though; this is inside the SoC).
> 
> Before I saw these latest emails, I was going to suggest:
> 1. Enable it only on RK3568 for now. I would be fine if this would be 
> accepted
> by the maintainer

I think we need more testing on more units of the RK3568 SoC, simply
because the HWRNG may work badly on some units.  I know that it sucks,
but we basically need just one "bad apple" to mark an SoC as having
an unreliable HWRNG, which is IMHO the only right thing to do.

Of course, unless we can prove that tweaking the HWRNG knobs makes such
"bad apples" work well.

> 2. Ask that you make a special version (for me) where I could play with 
> the
> params without having to compile a new kernel for each variant (it 
> generally
> takes me more then 24h on my Q64-A). Either through kernel module 
> properties
> or properties defined in the DeviceTree is fine with me.
> 
> 3. Based on the results make  a choice to not enable it on rk3566 at 
> all or
> (indeed) introduce DT properties to configure it differently per SoC.

See above; unfortunately, we already have some "bad RK3566 apples".

> 4. Hope/Ask for more test results
> 
>> On the RK3399 we also saw wildly varying results.
> 
> On my Rock64('s) (RK3328) it doesn't work at all:
> 
> ```
> root at cs21:~# cat /dev/hwrng | rngtest -c 1000
> rngtest 5
> ...
> rngtest: starting FIPS tests...
> cat: /dev/hwrng: No such device
> rngtest: entropy source drained
> ```