Re: [PATCH] crypto: qcom-rng: Add hwrng support

[Bjorn Andersson]

On Fri, Sep 01, 2023 at 06:45:02PM +0530, Om Prakash Singh wrote:
> This is follow patch on top of [1]

This information does not add value to the git history, if you need to
inform the maintainer that the patch should be applied after some
in-flight dependency then state so after the "---" line below.

But, this patch strictly conflicts with [1], so the statement won't make
sense if this is merged.

> to add hwrng support for newer platform with trng capability.

Please rewrite this so that it's clear that the problem you're trying to
solve with this patch (i.e. the problem description) is that newer
platforms has trng. Describe how this relates to the existing driver
(e.g. same/similar hardware interface). State that you purposefully kept
the crypto interface in place for the new hardware as well (so that it's
clear that this isn't an accident or oversight).

> 
> [1] https://lore.kernel.org/lkml/20230824-topic-sm8550-rng-v2-4-dfcafbb16a3e@xxxxxxxxxx/
> 
> Signed-off-by: Om Prakash Singh <quic_omprsing@xxxxxxxxxxx>
> ---
>  drivers/crypto/qcom-rng.c | 72 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 63 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/crypto/qcom-rng.c b/drivers/crypto/qcom-rng.c
> index fb54b8cfc35f..f78ffdcc66ec 100644
> --- a/drivers/crypto/qcom-rng.c
> +++ b/drivers/crypto/qcom-rng.c
> @@ -13,6 +13,7 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> +#include <linux/hw_random.h>

Please keep these sorted, alphabetically.

>  
>  /* Device specific register offsets */
>  #define PRNG_DATA_OUT		0x0000
> @@ -32,13 +33,18 @@ struct qcom_rng {
>  	struct mutex lock;
>  	void __iomem *base;
>  	struct clk *clk;
> -	unsigned int skip_init;
> +	struct qcom_rng_of_data *of_data;
>  };
>  
>  struct qcom_rng_ctx {
>  	struct qcom_rng *rng;
>  };
>  
> +struct qcom_rng_of_data {
> +	bool skip_init;
> +	bool hwrng_support;
> +};
> +
>  static struct qcom_rng *qcom_rng_dev;
>  
>  static int qcom_rng_read(struct qcom_rng *rng, u8 *data, unsigned int max)
> @@ -70,7 +76,7 @@ static int qcom_rng_read(struct qcom_rng *rng, u8 *data, unsigned int max)
>  		}
>  	} while (currsize < max);
>  
> -	return 0;
> +	return currsize;
>  }
>  
>  static int qcom_rng_generate(struct crypto_rng *tfm,
> @@ -79,7 +85,8 @@ static int qcom_rng_generate(struct crypto_rng *tfm,
>  {
>  	struct qcom_rng_ctx *ctx = crypto_rng_ctx(tfm);
>  	struct qcom_rng *rng = ctx->rng;
> -	int ret;
> +	int ret = -EFAULT;

This initialization is useless, the very first thing you do with ret is
to overwrite it with the return value of clk_prepare_enable().

> +	int read_size = 0;

Similarly, the first use of this variable is an assignment. And "ret"
was a good, short, variable name.

>  
>  	ret = clk_prepare_enable(rng->clk);
>  	if (ret)
> @@ -87,11 +94,14 @@ static int qcom_rng_generate(struct crypto_rng *tfm,
>  
>  	mutex_lock(&rng->lock);
>  
> -	ret = qcom_rng_read(rng, dstn, dlen);
> +	read_size = qcom_rng_read(rng, dstn, dlen);
>  
>  	mutex_unlock(&rng->lock);
>  	clk_disable_unprepare(rng->clk);
>  
> +	if (read_size == dlen)

This function used to return < 0 if qcom_rng_read() returned an error,
and 0 in all other cases.

Now you will pass through negative values, you will return 0 if the loop
in qcom_rng_read() reached "dlen" ("max)", and you will return some
positive number if the break condition in the loop hit.

In other words, this is wrong.

> +		ret = 0;
> +
>  	return ret;
>  }
>  
> @@ -101,6 +111,16 @@ static int qcom_rng_seed(struct crypto_rng *tfm, const u8 *seed,
>  	return 0;
>  }
>  
> +static int qcom_hwrng_read(struct hwrng *rng, void *data, size_t max, bool wait)
> +{
> +	int ret;
> +	struct qcom_rng *qrng;
> +
> +	qrng = (struct qcom_rng *)rng->priv;
> +	ret = qcom_rng_read(qrng, data, max);
> +	return ret;

Initialize qrng directly when you define it, drop ret and just return
qcom_rng_read() directly.

> +}
> +
>  static int qcom_rng_enable(struct qcom_rng *rng)
>  {
>  	u32 val;
> @@ -136,7 +156,7 @@ static int qcom_rng_init(struct crypto_tfm *tfm)
>  
>  	ctx->rng = qcom_rng_dev;
>  
> -	if (!ctx->rng->skip_init)
> +	if (!ctx->rng->of_data->skip_init)
>  		return qcom_rng_enable(ctx->rng);
>  
>  	return 0;
> @@ -157,9 +177,16 @@ static struct rng_alg qcom_rng_alg = {
>  	}
>  };
>  
> +static struct hwrng qcom_hwrng = {
> +	.name = "qcom-hwrng",
> +	.read = qcom_hwrng_read,
> +	.quality = 1024,
> +};
> +
>  static int qcom_rng_probe(struct platform_device *pdev)
>  {
>  	struct qcom_rng *rng;
> +	const struct qcom_rng_of_data *data;

Move this one line up, so we maintain the reverse xmas tree.

>  	int ret;
>  
>  	rng = devm_kzalloc(&pdev->dev, sizeof(*rng), GFP_KERNEL);
> @@ -177,7 +204,8 @@ static int qcom_rng_probe(struct platform_device *pdev)
>  	if (IS_ERR(rng->clk))
>  		return PTR_ERR(rng->clk);
>  
> -	rng->skip_init = (unsigned long)device_get_match_data(&pdev->dev);
> +	data = of_device_get_match_data(&pdev->dev);
> +	rng->of_data = (struct qcom_rng_of_data *)data;

Why didn't you assign rng->of_data directly?

Also, of_device_get_match_data() returns a void *, so you should have to
explicitly cast this.

>  
>  	qcom_rng_dev = rng;
>  	ret = crypto_register_rng(&qcom_rng_alg);
> @@ -185,6 +213,14 @@ static int qcom_rng_probe(struct platform_device *pdev)
>  		dev_err(&pdev->dev, "Register crypto rng failed: %d\n", ret);
>  		qcom_rng_dev = NULL;
>  	}

Would be nice with a newline here, to get some separation between the
"paragraphs".

> +	if (rng->of_data->hwrng_support) {
> +		qcom_hwrng.priv = (unsigned long)qcom_rng_dev;
> +		ret = hwrng_register(&qcom_hwrng);
> +		if (ret) {
> +			dev_err(&pdev->dev, "Register hwrng failed: %d\n", ret);
> +			qcom_rng_dev = NULL;

You're leaving the rng registered with crypto here. Not sure if that
will result in a use after free, or just a NULL pointer dereference -
but it's not good either way.

> +		}
> +	}
>  
>  	return ret;
>  }
> @@ -193,11 +229,29 @@ static int qcom_rng_remove(struct platform_device *pdev)
>  {
>  	crypto_unregister_rng(&qcom_rng_alg);
>  
> +	if (qcom_rng_dev->of_data->hwrng_support)
> +		hwrng_unregister(&qcom_hwrng);

Why not use devm_hwrng_register() above instead? Then you wouldn't have
to unregister it here.

> +
>  	qcom_rng_dev = NULL;
>  
>  	return 0;
>  }
>  
> +struct qcom_rng_of_data qcom_prng_of_data = {
> +	.skip_init = false,
> +	.hwrng_support = false,
> +};
> +
> +struct qcom_rng_of_data qcom_prng_ee_of_data = {
> +	.skip_init = true,
> +	.hwrng_support = false,
> +};
> +
> +struct qcom_rng_of_data qcom_trng_of_data = {
> +	.skip_init = true,

Can you please confirm that it's appropriate to name this "trng" without
the "-ee" suffix. Should all trng instances (v2 and v3) skip
initialization?

> +	.hwrng_support = true,
> +};
> +
>  static const struct acpi_device_id __maybe_unused qcom_rng_acpi_match[] = {
>  	{ .id = "QCOM8160", .driver_data = 1 },
>  	{}
> @@ -205,9 +259,9 @@ static const struct acpi_device_id __maybe_unused qcom_rng_acpi_match[] = {
>  MODULE_DEVICE_TABLE(acpi, qcom_rng_acpi_match);
>  
>  static const struct of_device_id __maybe_unused qcom_rng_of_match[] = {
> -	{ .compatible = "qcom,prng", .data = (void *)0},
> -	{ .compatible = "qcom,prng-ee", .data = (void *)1},
> -	{ .compatible = "qcom,trng", .data = (void *)1},
> +	{ .compatible = "qcom,prng", .data = &qcom_prng_of_data },
> +	{ .compatible = "qcom,prng-ee", .data = &qcom_prng_ee_of_data },
> +	{ .compatible = "qcom,trng", .data = &qcom_trng_of_data },

So, should this be qcom,trng or qcom,trng-ee?


Where is your devicetree binding patch?

Regards,
Bjorn