| Message ID | cover.1718921174.git.daniel@makrotopia.org |
|---|---|
| Headers | show |
| Series | hwrng: add hwrng support for Rockchip RK3568 | expand |
Hi Daniel, On 2024-06-21 02:24, Daniel Golle wrote: > Rockchip SoCs used to have a random number generator as part of their > crypto device, and support for it has to be added to the corresponding > driver. > > However newer Rockchip SoCs like the RK3568 have an independent True > Random Number Generator device. This patchset adds a driver for it and > enable it in the device tree. > > v2 of this patchset has been submitted by Aurelien Jarno in November > 2022. A follow-up submission addressing the comments received for v2 > never happened. I didn't have the time or energy to continue working on this. Thanks for continuing the work, I'll give it a try at some point. Regards Aurelien
Hello Heiko, On 2024-06-22 22:26, Heiko Stübner wrote: > Am Samstag, 22. Juni 2024, 12:29:33 CEST schrieb Dragan Simic: >> On 2024-06-22 00:16, Uwe Kleine-König wrote: >> > On 6/21/24 20:13, Dragan Simic wrote: >> >> On 2024-06-21 11:57, Krzysztof Kozlowski wrote: >> >>> On 21/06/2024 03:25, Daniel Golle wrote: >> >>>> From: Aurelien Jarno <aurelien@aurel32.net> >> >> >> >> [snip] >> >> >> >>>> + pm_runtime_set_autosuspend_delay(dev, >> >>>> RK_RNG_AUTOSUSPEND_DELAY); >> >>>> + pm_runtime_use_autosuspend(dev); >> >>>> + pm_runtime_enable(dev); >> >>>> + >> >>>> + ret = devm_hwrng_register(dev, &rk_rng->rng); >> >>>> + if (ret) >> >>>> + return dev_err_probe(&pdev->dev, ret, "Failed to register >> >>>> Rockchip hwrng\n"); >> >>>> + >> >>>> + dev_info(&pdev->dev, "Registered Rockchip hwrng\n"); >> >>> >> >>> Drop, driver should be silent on success. >> >> >> >> I respectfully disagree. Many drivers print a single line upon >> >> successful probing, which I find very useful. In this particular >> >> case, it's even more useful, because some people may be concerned >> >> about the use of hardware TRNGs, so we should actually make sure >> >> to announce it. >> > >> > I agree to Krzysztof here. From the POV of a driver author, your own >> > driver is very important and while you write it, it really interests >> > *you* if the driver is successfully probed. However from a system >> > perspective these are annoying: There are easily >50 devices[1] on a >> > system, if all of these print a message in probe, you have little >> > chance >> > to see the relevant messages. Even if every driver author thinks their >> > work is a special snow flake that is worth announcing, in practice >> > users >> > only care about your driver if there is a problem. Additionally each >> > message takes time and so delays the boot process. Additionally each >> > message takes place in the printk ring buffer and so edges out earlier >> > messages that might be more important. >> >> Well, I don't find those messages annoying, for the drivers I've had >> nothing to do with. Also, in my experience, 99.9% of users don't care >> about the kernel messages at all, be it everything hunky-dory, or be >> it something really wrong somewhere. >> >> > So +1 for dropping the dev_info() or at least using dev_debug() for it. > > Just for 2ct ... I'm also in the don't print too much camp ;-) . > When parsing kernel logs to see where things fail, messages just > telling me about sucesses make things more difficult. > > So really this message should be dropped or at least as Uwe suggests > made a dev_dbg. As a note, "dmesg --level=err,warn", for example, is rather useful when it comes to filtering the kernel messages to see only those that are signs of a trouble.
Hello Dragan, On Sat, Jun 22, 2024 at 10:45:22PM +0200, Dragan Simic wrote: > On 2024-06-22 22:26, Heiko Stübner wrote: > > Am Samstag, 22. Juni 2024, 12:29:33 CEST schrieb Dragan Simic: > > > On 2024-06-22 00:16, Uwe Kleine-König wrote: > > > > On 6/21/24 20:13, Dragan Simic wrote: > > > >> On 2024-06-21 11:57, Krzysztof Kozlowski wrote: > > > >>> On 21/06/2024 03:25, Daniel Golle wrote: > > > >>>> + dev_info(&pdev->dev, "Registered Rockchip hwrng\n"); > > > >>> > > > >>> Drop, driver should be silent on success. > > > >> > > [...] > > So really this message should be dropped or at least as Uwe suggests > > made a dev_dbg. > > As a note, "dmesg --level=err,warn", for example, is rather useful > when it comes to filtering the kernel messages to see only those that > are signs of a trouble. IMHO it's a bit sad, that there is such a long thread about something so trivial, but I want to make a few points: - not all dmesg implementations support this: root@machine:~ dmesg --level=err,warn dmesg: unrecognized option '--level=err,warn' BusyBox v1.36.1 () multi-call binary. Usage: dmesg [-cr] [-n LEVEL] [-s SIZE] Print or control the kernel ring buffer -c Clear ring buffer after printing -n LEVEL Set console logging level -s SIZE Buffer size -r Print raw message buffer - Your argument that the output of this dev_info can easily be ignored is a very weak reason to keep it. - I personally consider it hard sometimes to accept feedback if I think it's wrong and there is a good reason to do it the way I want it. But there are now three people opposing your position, who brought forward (IMHO) good reasons and even a constructive alternative was presented. Please stay open minded while weighting the options. The questions to ask here include: - How many people care for this message? During every boot? Is it maybe enough for these people to check in /sys if the device probed successfully? Or maybe even the absence of an error message is enough? - How many people get this message and don't care about the presented information? How many people are even annoyed by it? - Is the delay and memory usage introduced by this message justified then, even if it's small? Best regards Uwe
Hello Uwe, On 2024-06-23 02:20, Uwe Kleine-König wrote: > On Sat, Jun 22, 2024 at 10:45:22PM +0200, Dragan Simic wrote: >> On 2024-06-22 22:26, Heiko Stübner wrote: >> > Am Samstag, 22. Juni 2024, 12:29:33 CEST schrieb Dragan Simic: >> > > On 2024-06-22 00:16, Uwe Kleine-König wrote: >> > > > On 6/21/24 20:13, Dragan Simic wrote: >> > > >> On 2024-06-21 11:57, Krzysztof Kozlowski wrote: >> > > >>> On 21/06/2024 03:25, Daniel Golle wrote: >> > > >>>> + dev_info(&pdev->dev, "Registered Rockchip hwrng\n"); >> > > >>> >> > > >>> Drop, driver should be silent on success. >> > > >> >> > [...] >> > So really this message should be dropped or at least as Uwe suggests >> > made a dev_dbg. >> >> As a note, "dmesg --level=err,warn", for example, is rather useful >> when it comes to filtering the kernel messages to see only those that >> are signs of a trouble. > > IMHO it's a bit sad, that there is such a long thread about something > so > trivial, but I want to make a few points: > > - not all dmesg implementations support this: > > root@machine:~ dmesg --level=err,warn > dmesg: unrecognized option '--level=err,warn' > BusyBox v1.36.1 () multi-call binary. > > Usage: dmesg [-cr] [-n LEVEL] [-s SIZE] > > Print or control the kernel ring buffer > > -c Clear ring buffer after printing > -n LEVEL Set console logging level > -s SIZE Buffer size > -r Print raw message buffer > > - Your argument that the output of this dev_info can easily be ignored > is a very weak reason to keep it. > > - I personally consider it hard sometimes to accept feedback if I > think > it's wrong and there is a good reason to do it the way I want it. > But there are now three people opposing your position, who brought > forward (IMHO) good reasons and even a constructive alternative was > presented. Please stay open minded while weighting the options. > The questions to ask here include: > > - How many people care for this message? During every boot? Is it > maybe enough for these people to check in /sys if the device > probed successfully? Or maybe even the absence of an error > message > is enough? > - How many people get this message and don't care about the > presented information? How many people are even annoyed by it? > - Is the delay and memory usage introduced by this message > justified > then, even if it's small? I'm sorry if my responses caused any inconvenience. I find most of your points totally valid, but there are a couple of them I could continue arguing about. Though, as you also pointed out, my opinion has been already outvoted, so I'll remain silent.
Rockchip SoCs used to have a random number generator as part of their crypto device, and support for it has to be added to the corresponding driver. However newer Rockchip SoCs like the RK3568 have an independent True Random Number Generator device. This patchset adds a driver for it and enable it in the device tree. v2 of this patchset has been submitted by Aurelien Jarno in November 2022. A follow-up submission addressing the comments received for v2 never happened. As I believe that using the TRNG is generally desireable as it reduces the time needed to boot significantly as userspace no longer waits due blocking read of /dev/random while still lacking entropy I have picked up Aurelien's previous work and completed it. Link to v2: https://patchwork.kernel.org/project/linux-arm-kernel/cover/20221128184718.1963353-1-aurelien@aurel32.net/ v2 -> v3: * Patch 1: address comments of Krzysztof Kozlowski, add MAINTAINERS - improved description - meaningful clock-names - add entry in MAINTAINERS files * Patch 2: numerous code-style improvements - drop misleading rk_rng_write_ctl(), simplify I/O writes - drop unused TRNG_RNG_DOUT_[1-7] macros - handle error handling for pm_runtime_get_sync() - use memcpy_fromio() instead of open coding for-loop - some minor white-spaces fixes * Patch 3: - use clock-names as defined in dt-bindings v1 -> v2: * Patch 1: fix issues reported by Rob Herring and Krzysztof Kozlowski: - Rename rockchip-rng.yaml into rockchip,rk3568-rng.yaml - Fix binding title and description - Fix compatible property - Rename clocks and add the corresponding descriptions - Drop reset-names - Add a bus definition with #address-cells and #size-cells to the example. * Patch 2: fix issue reported by kernel test robot <lkp@intel.com> - Do not read the random registers as big endian, looking at the RK3568 TRM this is actually not needed. This fixes a sparse warning. * Patch 3: unchanged Aurelien Jarno (3): dt-bindings: RNG: Add Rockchip RNG bindings hwrng: add Rockchip SoC hwrng driver arm64: dts: rockchip: add DT entry for RNG to RK356x .../bindings/rng/rockchip,rk3568-rng.yaml | 60 +++++ MAINTAINERS | 7 + arch/arm64/boot/dts/rockchip/rk356x.dtsi | 9 + drivers/char/hw_random/Kconfig | 14 ++ drivers/char/hw_random/Makefile | 1 + drivers/char/hw_random/rockchip-rng.c | 229 ++++++++++++++++++ 6 files changed, 320 insertions(+) create mode 100644 Documentation/devicetree/bindings/rng/rockchip,rk3568-rng.yaml create mode 100644 drivers/char/hw_random/rockchip-rng.c