Re: [PATCH REVIEW] hwrng: add exynos Secure World RNG device driver

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 28/03/2024 13:50, Alexey Klimov wrote:
> The Exynos TRNG device is controlled by firmware and shared between

No, it is not. I have TRNG perfectly usable on my board. Maybe you are
refer to some specific SoC...

Please always Cc existing TRNG driver maintainer.

> non-secure world and secure world. Access to it is exposed via SMC
> interface which is implemented here. The firmware code does
> additional security checks, start-up test and some checks on resume.
> 
> This device is found, for instance, in Google Tensor GS101-family
> of devices.

Nothing here explains why this cannot be integrated into existing
driver. Maybe it, maybe it cannot...

You try to upstream again vendor driver ignoring that community already
did it and instead it might be enough to customize it.

Guys, the same as all the MCT, PHYs and PCI in previous works of various
people: stop duplicating drivers by upstreaming new vendor stuff with
all the issues we already fixed and please work on re-using existing
drivers.

Sometimes work cannot be combined, so come with arguments. Otherwise we
keep repeating and repeating the same feedback.

> 
> Signed-off-by: Alexey Klimov <alexey.klimov@linaro.org>
> ---
>  drivers/char/hw_random/Kconfig           |  12 +
>  drivers/char/hw_random/Makefile          |   1 +
>  drivers/char/hw_random/exynos-swd-trng.c | 423 +++++++++++++++++++++++
>  3 files changed, 436 insertions(+)
>  create mode 100644 drivers/char/hw_random/exynos-swd-trng.c
> 
> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
> index 442c40efb200..bff7c3ec129a 100644
> --- a/drivers/char/hw_random/Kconfig
> +++ b/drivers/char/hw_random/Kconfig
> @@ -479,6 +479,18 @@ config HW_RANDOM_EXYNOS
>  
>  	  If unsure, say Y.
>  
> +config HW_RANDOM_EXYNOS_SWD
> +	tristate "Exynos SWD HW random number generator support"

What is SWD?

> +	default n
> +	help
> +	  This driver provides kernel-side support for accessing Samsung
> +	  TRNG hardware located in secure world using smc calls.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called exynos-swd-trng.
> +
> +	  If unsure, say N.
> +


...

> +
> +static UNIVERSAL_DEV_PM_OPS(exyswd_rng_pm_ops, exyswd_rng_suspend,
> +			    exyswd_rng_resume, NULL);
> +
> +static struct platform_driver exyswd_rng_driver = {
> +	.probe		= exyswd_rng_probe,
> +	.remove		= exyswd_rng_remove,
> +	.driver		= {
> +		.name	= DRVNAME,
> +		.owner	= THIS_MODULE,

So this was fixed ~8-10 years ago. Yet it re-appears. Please do not use
downstream code as template.

Take upstream driver and either change it or customize it.


> +		.pm     = &exyswd_rng_pm_ops,
> +	},
> +};
> +
> +static struct platform_device *exyswd_rng_pdev;

And if I have multiple devices?

> +
> +static int __init exyswd_rng_init(void)
> +{
> +	int ret;
> +
> +	exyswd_rng_pdev = platform_device_register_simple(DRVNAME, -1, NULL, 0);
> +	if (IS_ERR(exyswd_rng_pdev))
> +		pr_err(DRVNAME ": could not register device: %ld\n",
> +		       PTR_ERR(exyswd_rng_pdev));

This is some oddity... Why do you create devices based on module load?
So I load this on Qualcomm anbd you create exynos device? This does not
make sense.


> +
> +	ret = platform_driver_register(&exyswd_rng_driver);
> +	if (ret) {
> +		platform_device_unregister(exyswd_rng_pdev);
> +		return ret;
> +	}
> +
> +	pr_info("ExyRNG driver, (c) 2014 Samsung Electronics\n");

Drop, dirvers should not print code just because I load a driver. Again:
imagine I load it on Qualcomm.


Best regards,
Krzysztof