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

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

[Om Prakash Singh]

This is follow patch on top of [1] to add hwrng support for newer
platform with trng capability.

[1] https://lore.kernel.org/lkml/20230824-topic-sm8550-rng-v2-4-dfcafbb16a3e@linaro.org/

Signed-off-by: Om Prakash Singh <quic_omprsing@quicinc.com>
---
 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>
 
 /* 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;
+	int read_size = 0;
 
 	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)
+		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;
+}
+
 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;
 	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;
 
 	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;
 	}
+	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;
+		}
+	}
 
 	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);
+
 	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,
+	.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 },
 	{}
 };
 MODULE_DEVICE_TABLE(of, qcom_rng_of_match);
-- 
2.25.1

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@linaro.org/
> 
> Signed-off-by: Om Prakash Singh <quic_omprsing@quicinc.com>
> ---
>  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

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

[Krzysztof Kozlowski]

On 01/09/2023 15:15, Om Prakash Singh wrote:
> This is follow patch on top of [1] to add hwrng support for newer
> platform with trng capability.

Please use subject prefixes matching the subsystem.

Best regards,
Krzysztof

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

[Om Prakash Singh]

On 9/1/2023 8:16 PM, Bjorn Andersson wrote:
> 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@linaro.org/
>>
>> Signed-off-by: Om Prakash Singh <quic_omprsing@quicinc.com>
>> ---
>>   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?
All trng supported platform needs to skip initialzation.
we don't need to have both "trng-ee" and "trng".
If "trng-ee" is prefer we shold update it in patch [1] it itself,

> 
>> +	.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?
DT binding patch is submitted by Neil [1]

I will address all other review comments in next patch version.

[1] 
https://lore.kernel.org/lkml/20230824-topic-sm8550-rng-v2-4-dfcafbb16a3e@linaro.org/

> 
> Regards,
> Bjorn

[PATCH V2] crypto: qcom-rng - Add hw_random interface support

[Om Prakash Singh]

Add hw_random interface support in qcom-rng driver as new IP block
in Qualcomm SoC has inbuilt NIST SP800 90B compliant entropic source
to generate true random number.

Keeping current rng_alg interface as well for random number generation
using Kernel Crypto API.

Signed-off-by: Om Prakash Singh <quic_omprsing@quicinc.com>
---

Changes in V2:
- Address review comment from Bjorn and Krzysztof

This patch is depends on [1]
[1] https://lore.kernel.org/lkml/20230824-topic-sm8550-rng-v2-4-dfcafbb16a3e@linaro.org/

 drivers/crypto/qcom-rng.c | 65 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 58 insertions(+), 7 deletions(-)

diff --git a/drivers/crypto/qcom-rng.c b/drivers/crypto/qcom-rng.c
index fb54b8cfc35f..e5a574a3cc59 100644
--- a/drivers/crypto/qcom-rng.c
+++ b/drivers/crypto/qcom-rng.c
@@ -7,6 +7,7 @@
 #include <linux/acpi.h>
 #include <linux/clk.h>
 #include <linux/crypto.h>
+#include <linux/hw_random.h>
 #include <linux/io.h>
 #include <linux/iopoll.h>
 #include <linux/kernel.h>
@@ -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,
@@ -92,6 +98,9 @@ static int qcom_rng_generate(struct crypto_rng *tfm,
 	mutex_unlock(&rng->lock);
 	clk_disable_unprepare(rng->clk);
 
+	if (ret == dlen)
+		ret = 0;
+
 	return ret;
 }
 
@@ -101,6 +110,13 @@ 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)
+{
+	struct qcom_rng *qrng = (struct qcom_rng *)rng->priv;
+
+	return qcom_rng_read(qrng, data, max);
+}
+
 static int qcom_rng_enable(struct qcom_rng *rng)
 {
 	u32 val;
@@ -136,7 +152,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,6 +173,12 @@ 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;
@@ -177,15 +199,29 @@ 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);
+	rng->of_data = (struct qcom_rng_of_data *)of_device_get_match_data(&pdev->dev);
 
 	qcom_rng_dev = rng;
 	ret = crypto_register_rng(&qcom_rng_alg);
 	if (ret) {
 		dev_err(&pdev->dev, "Register crypto rng failed: %d\n", ret);
 		qcom_rng_dev = NULL;
+		return ret;
+	}
+
+	if (rng->of_data->hwrng_support) {
+		qcom_hwrng.priv = (unsigned long)qcom_rng_dev;
+		ret = devm_hwrng_register(&pdev->dev, &qcom_hwrng);
+		if (ret) {
+			dev_err(&pdev->dev, "Register hwrng failed: %d\n", ret);
+			qcom_rng_dev = NULL;
+			goto fail;
+		}
 	}
 
+	return ret;
+fail:
+	crypto_unregister_rng(&qcom_rng_alg);
 	return ret;
 }
 
@@ -198,6 +234,21 @@ static int qcom_rng_remove(struct platform_device *pdev)
 	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,
+	.hwrng_support = true,
+};
+
 static const struct acpi_device_id __maybe_unused qcom_rng_acpi_match[] = {
 	{ .id = "QCOM8160", .driver_data = 1 },
 	{}
@@ -205,9 +256,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 },
 	{}
 };
 MODULE_DEVICE_TABLE(of, qcom_rng_of_match);
-- 
2.25.1

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

[Konrad Dybcio]

On 5.09.2023 04:50, Om Prakash Singh wrote:
> 
> 
> On 9/1/2023 8:16 PM, Bjorn Andersson wrote:
>> 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@linaro.org/
>>>
>>> Signed-off-by: Om Prakash Singh <quic_omprsing@quicinc.com>
>>> ---
[...]

>>
>> 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?
> All trng supported platform needs to skip initialzation.
> we don't need to have both "trng-ee" and "trng".
> If "trng-ee" is prefer we shold update it in patch [1] it itself,
Looking back at ba3ab6371cdd ("crypto: qcom-rng - Add support for prng-ee"),
it was solved in a way that we would stray from today - nowadays
we'd call it qcom,msm8996-prng or something.

The -ee part was only there to discern parts that were initialized
by other software.

Since you said that all TRNGs need that, I'm also for dropping "-ee".

Konrad

Re: [PATCH V2] crypto: qcom-rng - Add hw_random interface support

[Konrad Dybcio]

On 5.09.2023 08:24, Om Prakash Singh wrote:
> Add hw_random interface support in qcom-rng driver as new IP block
> in Qualcomm SoC has inbuilt NIST SP800 90B compliant entropic source
> to generate true random number.
> 
> Keeping current rng_alg interface as well for random number generation
> using Kernel Crypto API.
> 
> Signed-off-by: Om Prakash Singh <quic_omprsing@quicinc.com>
> ---
> 
> Changes in V2:
> - Address review comment from Bjorn and Krzysztof
"make changes" is not a valid changelog, please be more specific
next time around

Konrad

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

[Herbert Xu]

On Fri, Sep 01, 2023 at 06:45:02PM +0530, Om Prakash Singh wrote:
>
> +	qrng = (struct qcom_rng *)rng->priv;

Please stop using rng->priv, it is obsolete.  Instead embed the
rng object inside qcom_rng.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH V2] crypto: qcom-rng - Add hw_random interface support

[Om Prakash Singh]

Add hw_random interface support in qcom-rng driver as new IP block
in Qualcomm SoC has inbuilt NIST SP800 90B compliant entropic source
to generate true random number.

Keeping current rng_alg interface as well for random number generation
using Kernel Crypto API.

Signed-off-by: Om Prakash Singh <quic_omprsing@quicinc.com>
---

Changes in V2:
- Updated patch to fix the return value from qcom_rng_generate() to be
  consistent with current implementation
- Updated patch to make it more concise
- Removed unnecessary use local variable and it's initialization
- Updated patch to use devm_hwrng_register() instead of hwrng_register()
- Updated subject line of the patch

This patch is depends on [1]
[1] https://lore.kernel.org/lkml/20230824-topic-sm8550-rng-v2-4-dfcafbb16a3e@linaro.org/

 drivers/crypto/qcom-rng.c | 65 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 58 insertions(+), 7 deletions(-)

diff --git a/drivers/crypto/qcom-rng.c b/drivers/crypto/qcom-rng.c
index fb54b8cfc35f..e5a574a3cc59 100644
--- a/drivers/crypto/qcom-rng.c
+++ b/drivers/crypto/qcom-rng.c
@@ -7,6 +7,7 @@
 #include <linux/acpi.h>
 #include <linux/clk.h>
 #include <linux/crypto.h>
+#include <linux/hw_random.h>
 #include <linux/io.h>
 #include <linux/iopoll.h>
 #include <linux/kernel.h>
@@ -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,
@@ -92,6 +98,9 @@ static int qcom_rng_generate(struct crypto_rng *tfm,
 	mutex_unlock(&rng->lock);
 	clk_disable_unprepare(rng->clk);
 
+	if (ret == dlen)
+		ret = 0;
+
 	return ret;
 }
 
@@ -101,6 +110,13 @@ 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)
+{
+	struct qcom_rng *qrng = (struct qcom_rng *)rng->priv;
+
+	return qcom_rng_read(qrng, data, max);
+}
+
 static int qcom_rng_enable(struct qcom_rng *rng)
 {
 	u32 val;
@@ -136,7 +152,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,6 +173,12 @@ 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;
@@ -177,15 +199,29 @@ 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);
+	rng->of_data = (struct qcom_rng_of_data *)of_device_get_match_data(&pdev->dev);
 
 	qcom_rng_dev = rng;
 	ret = crypto_register_rng(&qcom_rng_alg);
 	if (ret) {
 		dev_err(&pdev->dev, "Register crypto rng failed: %d\n", ret);
 		qcom_rng_dev = NULL;
+		return ret;
+	}
+
+	if (rng->of_data->hwrng_support) {
+		qcom_hwrng.priv = (unsigned long)qcom_rng_dev;
+		ret = devm_hwrng_register(&pdev->dev, &qcom_hwrng);
+		if (ret) {
+			dev_err(&pdev->dev, "Register hwrng failed: %d\n", ret);
+			qcom_rng_dev = NULL;
+			goto fail;
+		}
 	}
 
+	return ret;
+fail:
+	crypto_unregister_rng(&qcom_rng_alg);
 	return ret;
 }
 
@@ -198,6 +234,21 @@ static int qcom_rng_remove(struct platform_device *pdev)
 	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,
+	.hwrng_support = true,
+};
+
 static const struct acpi_device_id __maybe_unused qcom_rng_acpi_match[] = {
 	{ .id = "QCOM8160", .driver_data = 1 },
 	{}
@@ -205,9 +256,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 },
 	{}
 };
 MODULE_DEVICE_TABLE(of, qcom_rng_of_match);
-- 
2.25.1

[PATCH V2] crypto: qcom-rng - Add hw_random interface support

[Om Prakash Singh]

Add hw_random interface support in qcom-rng driver as new IP block
in Qualcomm SoC has inbuilt NIST SP800 90B compliant entropic source
to generate true random number.

Keeping current rng_alg interface as well for random number generation
using Kernel Crypto API.

Signed-off-by: Om Prakash Singh <quic_omprsing@quicinc.com>
---

Changes in V2:
- Updated patch to fix the return value from qcom_rng_generate() to be
  consistent with current implementation
- Updated patch to make it more concise
- Removed unnecessary use local variable and it's initialization
- Updated patch to use devm_hwrng_register() instead of hwrng_register()
- Updated subject line of the patch

This patch is depends on [1]
[1] https://lore.kernel.org/lkml/20230824-topic-sm8550-rng-v2-4-dfcafbb16a3e@linaro.org/

 drivers/crypto/qcom-rng.c | 65 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 58 insertions(+), 7 deletions(-)

diff --git a/drivers/crypto/qcom-rng.c b/drivers/crypto/qcom-rng.c
index fb54b8cfc35f..e5a574a3cc59 100644
--- a/drivers/crypto/qcom-rng.c
+++ b/drivers/crypto/qcom-rng.c
@@ -7,6 +7,7 @@
 #include <linux/acpi.h>
 #include <linux/clk.h>
 #include <linux/crypto.h>
+#include <linux/hw_random.h>
 #include <linux/io.h>
 #include <linux/iopoll.h>
 #include <linux/kernel.h>
@@ -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,
@@ -92,6 +98,9 @@ static int qcom_rng_generate(struct crypto_rng *tfm,
 	mutex_unlock(&rng->lock);
 	clk_disable_unprepare(rng->clk);
 
+	if (ret == dlen)
+		ret = 0;
+
 	return ret;
 }
 
@@ -101,6 +110,13 @@ 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)
+{
+	struct qcom_rng *qrng = (struct qcom_rng *)rng->priv;
+
+	return qcom_rng_read(qrng, data, max);
+}
+
 static int qcom_rng_enable(struct qcom_rng *rng)
 {
 	u32 val;
@@ -136,7 +152,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,6 +173,12 @@ 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;
@@ -177,15 +199,29 @@ 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);
+	rng->of_data = (struct qcom_rng_of_data *)of_device_get_match_data(&pdev->dev);
 
 	qcom_rng_dev = rng;
 	ret = crypto_register_rng(&qcom_rng_alg);
 	if (ret) {
 		dev_err(&pdev->dev, "Register crypto rng failed: %d\n", ret);
 		qcom_rng_dev = NULL;
+		return ret;
+	}
+
+	if (rng->of_data->hwrng_support) {
+		qcom_hwrng.priv = (unsigned long)qcom_rng_dev;
+		ret = devm_hwrng_register(&pdev->dev, &qcom_hwrng);
+		if (ret) {
+			dev_err(&pdev->dev, "Register hwrng failed: %d\n", ret);
+			qcom_rng_dev = NULL;
+			goto fail;
+		}
 	}
 
+	return ret;
+fail:
+	crypto_unregister_rng(&qcom_rng_alg);
 	return ret;
 }
 
@@ -198,6 +234,21 @@ static int qcom_rng_remove(struct platform_device *pdev)
 	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,
+	.hwrng_support = true,
+};
+
 static const struct acpi_device_id __maybe_unused qcom_rng_acpi_match[] = {
 	{ .id = "QCOM8160", .driver_data = 1 },
 	{}
@@ -205,9 +256,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 },
 	{}
 };
 MODULE_DEVICE_TABLE(of, qcom_rng_of_match);
-- 
2.25.1

Re: [PATCH V2] crypto: qcom-rng - Add hw_random interface support

[Bjorn Andersson]

On Wed, Sep 20, 2023 at 08:34:08AM +0530, Om Prakash Singh wrote:
> Add hw_random interface support in qcom-rng driver as new IP block
> in Qualcomm SoC has inbuilt NIST SP800 90B compliant entropic source
> to generate true random number.
> 
> Keeping current rng_alg interface as well for random number generation
> using Kernel Crypto API.
> 
> Signed-off-by: Om Prakash Singh <quic_omprsing@quicinc.com>
> ---
> 
> Changes in V2:
> - Updated patch to fix the return value from qcom_rng_generate() to be
>   consistent with current implementation

As far as I can tell you didn't change this, see below.

> - Updated patch to make it more concise
> - Removed unnecessary use local variable and it's initialization
> - Updated patch to use devm_hwrng_register() instead of hwrng_register()
> - Updated subject line of the patch
> 
> This patch is depends on [1]
> [1] https://lore.kernel.org/lkml/20230824-topic-sm8550-rng-v2-4-dfcafbb16a3e@linaro.org/
> 
>  drivers/crypto/qcom-rng.c | 65 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 58 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/crypto/qcom-rng.c b/drivers/crypto/qcom-rng.c
> index fb54b8cfc35f..e5a574a3cc59 100644
> --- a/drivers/crypto/qcom-rng.c
> +++ b/drivers/crypto/qcom-rng.c
> @@ -7,6 +7,7 @@
>  #include <linux/acpi.h>
>  #include <linux/clk.h>
>  #include <linux/crypto.h>
> +#include <linux/hw_random.h>
>  #include <linux/io.h>
>  #include <linux/iopoll.h>
>  #include <linux/kernel.h>
> @@ -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;

As I pointed out in my previous review, if the qcom_rng_read() is
requested to read a number of bytes (max) that is not evenly divisible
with 4 (WORD_SZ) the loop will exit without accounting for the last
bytes copied...

>  }
>  
>  static int qcom_rng_generate(struct crypto_rng *tfm,
> @@ -92,6 +98,9 @@ static int qcom_rng_generate(struct crypto_rng *tfm,
>  	mutex_unlock(&rng->lock);
>  	clk_disable_unprepare(rng->clk);
>  
> +	if (ret == dlen)

...this means that if dlen % 4, you're changing the return value of this
function from 0 to dlen.

> +		ret = 0;
> +
>  	return ret;
>  }
>  
> @@ -101,6 +110,13 @@ 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)
> +{
> +	struct qcom_rng *qrng = (struct qcom_rng *)rng->priv;

You missed Herbert's request in [1], which I presume implies that
qcom_hwrng should be moved into struct qcom_rng, which would mean that
you can get the qrng by container_of().

[1] https://lore.kernel.org/lkml/ZQQvlXvGy8p01uJS@gondor.apana.org.au/

> +
> +	return qcom_rng_read(qrng, data, max);
> +}
> +
>  static int qcom_rng_enable(struct qcom_rng *rng)
>  {
>  	u32 val;
> @@ -136,7 +152,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,6 +173,12 @@ static struct rng_alg qcom_rng_alg = {
>  	}
>  };
>  
> +static struct hwrng qcom_hwrng = {
> +	.name = "qcom-hwrng",
> +	.read = qcom_hwrng_read,
> +	.quality = 1024,
> +};

Which would mean not adding this static global variable...

Regards,
Bjorn

Re: [PATCH V2] crypto: qcom-rng - Add hw_random interface support

[Om Prakash Singh]

On 9/22/2023 8:46 PM, Bjorn Andersson wrote:
> On Wed, Sep 20, 2023 at 08:34:08AM +0530, Om Prakash Singh wrote:
>> Add hw_random interface support in qcom-rng driver as new IP block
>> in Qualcomm SoC has inbuilt NIST SP800 90B compliant entropic source
>> to generate true random number.
>>
>> Keeping current rng_alg interface as well for random number generation
>> using Kernel Crypto API.
>>
>> Signed-off-by: Om Prakash Singh <quic_omprsing@quicinc.com>
>> ---
>>
>> Changes in V2:
>> - Updated patch to fix the return value from qcom_rng_generate() to be
>>    consistent with current implementation
> 
> As far as I can tell you didn't change this, see below.
> 
>> - Updated patch to make it more concise
>> - Removed unnecessary use local variable and it's initialization
>> - Updated patch to use devm_hwrng_register() instead of hwrng_register()
>> - Updated subject line of the patch
>>
>> This patch is depends on [1]
>> [1] https://lore.kernel.org/lkml/20230824-topic-sm8550-rng-v2-4-dfcafbb16a3e@linaro.org/
>>
>>   drivers/crypto/qcom-rng.c | 65 ++++++++++++++++++++++++++++++++++-----
>>   1 file changed, 58 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/crypto/qcom-rng.c b/drivers/crypto/qcom-rng.c
>> index fb54b8cfc35f..e5a574a3cc59 100644
>> --- a/drivers/crypto/qcom-rng.c
>> +++ b/drivers/crypto/qcom-rng.c
>> @@ -7,6 +7,7 @@
>>   #include <linux/acpi.h>
>>   #include <linux/clk.h>
>>   #include <linux/crypto.h>
>> +#include <linux/hw_random.h>
>>   #include <linux/io.h>
>>   #include <linux/iopoll.h>
>>   #include <linux/kernel.h>
>> @@ -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;
> 
> As I pointed out in my previous review, if the qcom_rng_read() is
> requested to read a number of bytes (max) that is not evenly divisible
> with 4 (WORD_SZ) the loop will exit without accounting for the last
> bytes copied...

I will update implementation to account currsize for accounting 
remaining bytes

>>   }
>>   
>>   static int qcom_rng_generate(struct crypto_rng *tfm,
>> @@ -92,6 +98,9 @@ static int qcom_rng_generate(struct crypto_rng *tfm,
>>   	mutex_unlock(&rng->lock);
>>   	clk_disable_unprepare(rng->clk);
>>   
>> +	if (ret == dlen)
> 
> ...this means that if dlen % 4, you're changing the return value of this
> function from 0 to dlen.

I will Update logic for success case return value.

> 
>> +		ret = 0;
>> +
>>   	return ret;
>>   }
>>   
>> @@ -101,6 +110,13 @@ 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)
>> +{
>> +	struct qcom_rng *qrng = (struct qcom_rng *)rng->priv;
> 
> You missed Herbert's request in [1], which I presume implies that
> qcom_hwrng should be moved into struct qcom_rng, which would mean that
> you can get the qrng by container_of().
> 
> [1] https://lore.kernel.org/lkml/ZQQvlXvGy8p01uJS@gondor.apana.org.au/

I will address this comment is my next patch set V3.


>> +
>> +	return qcom_rng_read(qrng, data, max);
>> +}
>> +
>>   static int qcom_rng_enable(struct qcom_rng *rng)
>>   {
>>   	u32 val;
>> @@ -136,7 +152,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,6 +173,12 @@ static struct rng_alg qcom_rng_alg = {
>>   	}
>>   };
>>   
>> +static struct hwrng qcom_hwrng = {
>> +	.name = "qcom-hwrng",
>> +	.read = qcom_hwrng_read,
>> +	.quality = 1024,
>> +};
> 
> Which would mean not adding this static global variable...
> 
> Regards,
> Bjorn