[PATCH 1/2] hwrng: msm: add a spinlock and support for blocking reads

[PATCH 1/2] hwrng: msm: add a spinlock and support for blocking reads

[Timur Tabi]

The hwrng.read callback includes a boolean parameter called 'wait'
which indicates whether the function should block and wait for
more data.

When 'wait' is true, the driver spins on the DATA_AVAIL bit or until
a reasonable timeout.  The timeout can occur if there is a heavy load
on reading the PRNG.

The same code also needs a spinlock to protect against race conditions.

If multiple cores hammer on the PRNG, it's possible for a race
condition to occur between reading the status register and
reading the data register.  Add a spinlock to protect against
that.

1. Core 1 reads status register, shows data is available.
2. Core 2 also reads status register, same result
3. Core 2 reads data register, depleting all entropy
4. Core 1 reads data register, which returns 0

Signed-off-by: Timur Tabi <timur@codeaurora.org>
---
 drivers/char/hw_random/msm-rng.c | 57 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 50 insertions(+), 7 deletions(-)

diff --git a/drivers/char/hw_random/msm-rng.c b/drivers/char/hw_random/msm-rng.c
index 841fee845ec9..44580588b938 100644
--- a/drivers/char/hw_random/msm-rng.c
+++ b/drivers/char/hw_random/msm-rng.c
@@ -15,9 +15,11 @@
 #include <linux/err.h>
 #include <linux/hw_random.h>
 #include <linux/io.h>
+#include <linux/iopoll.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/spinlock.h>
 
 /* Device specific register offsets */
 #define PRNG_DATA_OUT		0x0000
@@ -35,10 +37,22 @@
 #define MAX_HW_FIFO_SIZE	(MAX_HW_FIFO_DEPTH * 4)
 #define WORD_SZ			4
 
+/*
+ * Normally, this would be the maximum time it takes to refill the FIFO,
+ * after a read.  Under heavy load, tests show that this delay is either
+ * below 50us or above 2200us.  The higher value is probably what happens
+ * when entropy is completely depleted.
+ *
+ * Since we don't want to wait 2ms in a spinlock, set the timeout to the
+ * lower value.  Under extreme situations, that timeout can extend to 100us.
+ */
+#define TIMEOUT		50
+
 struct msm_rng {
 	void __iomem *base;
 	struct clk *clk;
 	struct hwrng hwrng;
+	spinlock_t lock;
 };
 
 #define to_msm_rng(p)	container_of(p, struct msm_rng, hwrng)
@@ -96,11 +110,39 @@ static int msm_rng_read(struct hwrng *hwrng, void *data, size_t max, bool wait)
 
 	/* read random data from hardware */
 	do {
-		val = readl_relaxed(rng->base + PRNG_STATUS);
-		if (!(val & PRNG_STATUS_DATA_AVAIL))
-			break;
+		spin_lock(&rng->lock);
+
+		/*
+		 * First check the status bit.  If 'wait' is true, then wait
+		 * up to TIMEOUT microseconds for data to be available.
+		 */
+		if (wait) {
+			int ret;
+
+			ret = readl_poll_timeout_atomic(rng->base + PRNG_STATUS,
+				val, val & PRNG_STATUS_DATA_AVAIL, 0, TIMEOUT);
+			if (ret) {
+				/* Timed out */
+				spin_unlock(&rng->lock);
+				break;
+			}
+		} else {
+			val = readl_relaxed(rng->base + PRNG_STATUS);
+			if (!(val & PRNG_STATUS_DATA_AVAIL)) {
+				spin_unlock(&rng->lock);
+				break;
+			}
+		}
 
 		val = readl_relaxed(rng->base + PRNG_DATA_OUT);
+		spin_unlock(&rng->lock);
+
+		/*
+		 * Zero is technically a valid random number, but it's also
+		 * the value returned if the PRNG is not enabled properly.
+		 * To avoid accidentally returning all zeros, treat it as
+		 * invalid and just return what we've already read.
+		 */
 		if (!val)
 			break;
 
@@ -148,10 +190,11 @@ static int msm_rng_probe(struct platform_device *pdev)
 	if (IS_ERR(rng->clk))
 		return PTR_ERR(rng->clk);
 
-	rng->hwrng.name = KBUILD_MODNAME,
-	rng->hwrng.init = msm_rng_init,
-	rng->hwrng.cleanup = msm_rng_cleanup,
-	rng->hwrng.read = msm_rng_read,
+	rng->hwrng.name = KBUILD_MODNAME;
+	rng->hwrng.init = msm_rng_init;
+	rng->hwrng.cleanup = msm_rng_cleanup;
+	rng->hwrng.read = msm_rng_read;
+	spin_lock_init(&rng->lock);
 
 	ret = devm_hwrng_register(&pdev->dev, &rng->hwrng);
 	if (ret) {
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 2/2] hwrng: msm: add ACPI support

[Timur Tabi]

Add support for probing on ACPI systems, with ACPI HID QCOM8160.

On ACPI systems, clocks are always enabled, the PRNG should
already be enabled, and the register region is read-only.
The driver only verifies that the hardware is already
enabled never tries to disable or configure it.

Signed-off-by: Timur Tabi <timur@codeaurora.org>
---
 drivers/char/hw_random/msm-rng.c | 40 ++++++++++++++++++++++++++++++++++------
 1 file changed, 34 insertions(+), 6 deletions(-)

diff --git a/drivers/char/hw_random/msm-rng.c b/drivers/char/hw_random/msm-rng.c
index 44580588b938..f34713d23d77 100644
--- a/drivers/char/hw_random/msm-rng.c
+++ b/drivers/char/hw_random/msm-rng.c
@@ -20,6 +20,7 @@
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/spinlock.h>
+#include <linux/acpi.h>
 
 /* Device specific register offsets */
 #define PRNG_DATA_OUT		0x0000
@@ -186,16 +187,32 @@ static int msm_rng_probe(struct platform_device *pdev)
 	if (IS_ERR(rng->base))
 		return PTR_ERR(rng->base);
 
-	rng->clk = devm_clk_get(&pdev->dev, "core");
-	if (IS_ERR(rng->clk))
-		return PTR_ERR(rng->clk);
-
 	rng->hwrng.name = KBUILD_MODNAME;
-	rng->hwrng.init = msm_rng_init;
-	rng->hwrng.cleanup = msm_rng_cleanup;
 	rng->hwrng.read = msm_rng_read;
 	spin_lock_init(&rng->lock);
 
+	/*
+	 * ACPI systems have v2 hardware. The clocks are always enabled,
+	 * the PRNG register space is read-only, and the PRNG should
+	 * already be enabled.
+	 */
+	if (has_acpi_companion(&pdev->dev)) {
+		u32 val;
+
+		val = readl(rng->base + PRNG_CONFIG);
+		if (!(val & PRNG_CONFIG_HW_ENABLE)) {
+			dev_err(&pdev->dev, "device is not enabled\n");
+			return -ENODEV;
+		}
+	} else {
+		rng->clk = devm_clk_get(&pdev->dev, "core");
+		if (IS_ERR(rng->clk))
+			return PTR_ERR(rng->clk);
+
+		rng->hwrng.init = msm_rng_init;
+		rng->hwrng.cleanup = msm_rng_cleanup;
+	}
+
 	ret = devm_hwrng_register(&pdev->dev, &rng->hwrng);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to register hwrng\n");
@@ -211,11 +228,22 @@ static int msm_rng_probe(struct platform_device *pdev)
 };
 MODULE_DEVICE_TABLE(of, msm_rng_of_match);
 
+#if IS_ENABLED(CONFIG_ACPI)
+static const struct acpi_device_id msm_rng_acpi_match[] = {
+	{
+		.id = "QCOM8160",	/* v2 PRNG */
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, msm_rng_acpi_match);
+#endif
+
 static struct platform_driver msm_rng_driver = {
 	.probe = msm_rng_probe,
 	.driver = {
 		.name = KBUILD_MODNAME,
 		.of_match_table = of_match_ptr(msm_rng_of_match),
+		.acpi_match_table = ACPI_PTR(msm_rng_acpi_match),
 	}
 };
 module_platform_driver(msm_rng_driver);
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 1/2] hwrng: msm: add a spinlock and support for blocking reads

[Vinod]

On 21-06-18, 10:17, Timur Tabi wrote:
> The hwrng.read callback includes a boolean parameter called 'wait'
> which indicates whether the function should block and wait for
> more data.
> 
> When 'wait' is true, the driver spins on the DATA_AVAIL bit or until
> a reasonable timeout.  The timeout can occur if there is a heavy load
> on reading the PRNG.
> 
> The same code also needs a spinlock to protect against race conditions.
> 
> If multiple cores hammer on the PRNG, it's possible for a race
> condition to occur between reading the status register and
> reading the data register.  Add a spinlock to protect against
> that.
> 
> 1. Core 1 reads status register, shows data is available.
> 2. Core 2 also reads status register, same result
> 3. Core 2 reads data register, depleting all entropy
> 4. Core 1 reads data register, which returns 0
> 
> Signed-off-by: Timur Tabi <timur@codeaurora.org>
> ---
>  drivers/char/hw_random/msm-rng.c | 57 +++++++++++++++++++++++++++++++++++-----
>  1 file changed, 50 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/char/hw_random/msm-rng.c b/drivers/char/hw_random/msm-rng.c
> index 841fee845ec9..44580588b938 100644
> --- a/drivers/char/hw_random/msm-rng.c
> +++ b/drivers/char/hw_random/msm-rng.c
> @@ -15,9 +15,11 @@
>  #include <linux/err.h>
>  #include <linux/hw_random.h>
>  #include <linux/io.h>
> +#include <linux/iopoll.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> +#include <linux/spinlock.h>
>  
>  /* Device specific register offsets */
>  #define PRNG_DATA_OUT		0x0000
> @@ -35,10 +37,22 @@
>  #define MAX_HW_FIFO_SIZE	(MAX_HW_FIFO_DEPTH * 4)
>  #define WORD_SZ			4
>  
> +/*
> + * Normally, this would be the maximum time it takes to refill the FIFO,
> + * after a read.  Under heavy load, tests show that this delay is either
> + * below 50us or above 2200us.  The higher value is probably what happens
> + * when entropy is completely depleted.
> + *
> + * Since we don't want to wait 2ms in a spinlock, set the timeout to the
> + * lower value.  Under extreme situations, that timeout can extend to 100us.
> + */
> +#define TIMEOUT		50
> +
>  struct msm_rng {
>  	void __iomem *base;
>  	struct clk *clk;
>  	struct hwrng hwrng;
> +	spinlock_t lock;
>  };
>  
>  #define to_msm_rng(p)	container_of(p, struct msm_rng, hwrng)
> @@ -96,11 +110,39 @@ static int msm_rng_read(struct hwrng *hwrng, void *data, size_t max, bool wait)
>  
>  	/* read random data from hardware */
>  	do {
> -		val = readl_relaxed(rng->base + PRNG_STATUS);
> -		if (!(val & PRNG_STATUS_DATA_AVAIL))
> -			break;
> +		spin_lock(&rng->lock);
> +
> +		/*
> +		 * First check the status bit.  If 'wait' is true, then wait
> +		 * up to TIMEOUT microseconds for data to be available.
> +		 */
> +		if (wait) {
> +			int ret;
> +
> +			ret = readl_poll_timeout_atomic(rng->base + PRNG_STATUS,
> +				val, val & PRNG_STATUS_DATA_AVAIL, 0, TIMEOUT);
> +			if (ret) {
> +				/* Timed out */
> +				spin_unlock(&rng->lock);
> +				break;
> +			}
> +		} else {
> +			val = readl_relaxed(rng->base + PRNG_STATUS);
> +			if (!(val & PRNG_STATUS_DATA_AVAIL)) {
> +				spin_unlock(&rng->lock);
> +				break;
> +			}
> +		}
>  
>  		val = readl_relaxed(rng->base + PRNG_DATA_OUT);
> +		spin_unlock(&rng->lock);
> +
> +		/*
> +		 * Zero is technically a valid random number, but it's also
> +		 * the value returned if the PRNG is not enabled properly.
> +		 * To avoid accidentally returning all zeros, treat it as
> +		 * invalid and just return what we've already read.
> +		 */
>  		if (!val)
>  			break;
>  
> @@ -148,10 +190,11 @@ static int msm_rng_probe(struct platform_device *pdev)
>  	if (IS_ERR(rng->clk))
>  		return PTR_ERR(rng->clk);
>  
> -	rng->hwrng.name = KBUILD_MODNAME,
> -	rng->hwrng.init = msm_rng_init,
> -	rng->hwrng.cleanup = msm_rng_cleanup,
> -	rng->hwrng.read = msm_rng_read,
> +	rng->hwrng.name = KBUILD_MODNAME;
> +	rng->hwrng.init = msm_rng_init;
> +	rng->hwrng.cleanup = msm_rng_cleanup;
> +	rng->hwrng.read = msm_rng_read;

this should be a separate patch

-- 
~Vinod

[PATCH 1/2] hwrng: msm: add a spinlock and support for blocking reads

[Timur Tabi]

On 6/21/18 11:17 PM, Vinod wrote:
> this should be a separate patch

What exactly should be a separate patch?  This part?

-	rng->hwrng.name = KBUILD_MODNAME,
-	rng->hwrng.init = msm_rng_init,
-	rng->hwrng.cleanup = msm_rng_cleanup,
-	rng->hwrng.read = msm_rng_read,
+	rng->hwrng.name = KBUILD_MODNAME;
+	rng->hwrng.init = msm_rng_init;
+	rng->hwrng.cleanup = msm_rng_cleanup;
+	rng->hwrng.read = msm_rng_read;

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 2/2] hwrng: msm: add ACPI support

[Vinod]

On 21-06-18, 10:17, Timur Tabi wrote:
> Add support for probing on ACPI systems, with ACPI HID QCOM8160.
> 
> On ACPI systems, clocks are always enabled, the PRNG should
> already be enabled, and the register region is read-only.
> The driver only verifies that the hardware is already
> enabled never tries to disable or configure it.

so if you are using v2 hardware, are you pointing to High Level OS EE or
some other..?

> Signed-off-by: Timur Tabi <timur@codeaurora.org>
> ---
>  drivers/char/hw_random/msm-rng.c | 40 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 34 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/char/hw_random/msm-rng.c b/drivers/char/hw_random/msm-rng.c
> index 44580588b938..f34713d23d77 100644
> --- a/drivers/char/hw_random/msm-rng.c
> +++ b/drivers/char/hw_random/msm-rng.c
> @@ -20,6 +20,7 @@
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/spinlock.h>
> +#include <linux/acpi.h>
>  
>  /* Device specific register offsets */
>  #define PRNG_DATA_OUT		0x0000
> @@ -186,16 +187,32 @@ static int msm_rng_probe(struct platform_device *pdev)
>  	if (IS_ERR(rng->base))
>  		return PTR_ERR(rng->base);
>  
> -	rng->clk = devm_clk_get(&pdev->dev, "core");
> -	if (IS_ERR(rng->clk))
> -		return PTR_ERR(rng->clk);
> -
>  	rng->hwrng.name = KBUILD_MODNAME;
> -	rng->hwrng.init = msm_rng_init;
> -	rng->hwrng.cleanup = msm_rng_cleanup;
>  	rng->hwrng.read = msm_rng_read;
>  	spin_lock_init(&rng->lock);
>  
> +	/*
> +	 * ACPI systems have v2 hardware. The clocks are always enabled,
> +	 * the PRNG register space is read-only, and the PRNG should
> +	 * already be enabled.
> +	 */
> +	if (has_acpi_companion(&pdev->dev)) {
> +		u32 val;
> +
> +		val = readl(rng->base + PRNG_CONFIG);

v2 EEs dont seem to have CONFIG register, so not sure about this one

> +		if (!(val & PRNG_CONFIG_HW_ENABLE)) {
> +			dev_err(&pdev->dev, "device is not enabled\n");
> +			return -ENODEV;
> +		}
> +	} else {
> +		rng->clk = devm_clk_get(&pdev->dev, "core");
> +		if (IS_ERR(rng->clk))
> +			return PTR_ERR(rng->clk);
> +
> +		rng->hwrng.init = msm_rng_init;
> +		rng->hwrng.cleanup = msm_rng_cleanup;
> +	}
> +
>  	ret = devm_hwrng_register(&pdev->dev, &rng->hwrng);
>  	if (ret) {
>  		dev_err(&pdev->dev, "failed to register hwrng\n");
> @@ -211,11 +228,22 @@ static int msm_rng_probe(struct platform_device *pdev)
>  };
>  MODULE_DEVICE_TABLE(of, msm_rng_of_match);
>  
> +#if IS_ENABLED(CONFIG_ACPI)
> +static const struct acpi_device_id msm_rng_acpi_match[] = {
> +	{
> +		.id = "QCOM8160",	/* v2 PRNG */
> +	},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(acpi, msm_rng_acpi_match);
> +#endif
> +
>  static struct platform_driver msm_rng_driver = {
>  	.probe = msm_rng_probe,
>  	.driver = {
>  		.name = KBUILD_MODNAME,
>  		.of_match_table = of_match_ptr(msm_rng_of_match),
> +		.acpi_match_table = ACPI_PTR(msm_rng_acpi_match),
>  	}
>  };
>  module_platform_driver(msm_rng_driver);
> -- 
> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
> Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
> Code Aurora Forum, a Linux Foundation Collaborative Project.

-- 
~Vinod

[PATCH 1/2] hwrng: msm: add a spinlock and support for blocking reads

[Vinod]

On 21-06-18, 23:18, Timur Tabi wrote:
> On 6/21/18 11:17 PM, Vinod wrote:
> > this should be a separate patch
> 
> What exactly should be a separate patch?  This part?
> 
> -	rng->hwrng.name = KBUILD_MODNAME,
> -	rng->hwrng.init = msm_rng_init,
> -	rng->hwrng.cleanup = msm_rng_cleanup,
> -	rng->hwrng.read = msm_rng_read,
> +	rng->hwrng.name = KBUILD_MODNAME;
> +	rng->hwrng.init = msm_rng_init;
> +	rng->hwrng.cleanup = msm_rng_cleanup;
> +	rng->hwrng.read = msm_rng_read;

yes

-- 
~Vinod

[PATCH 2/2] hwrng: msm: add ACPI support

[Timur Tabi]

On 6/21/18 11:23 PM, Vinod wrote:
> On 21-06-18, 10:17, Timur Tabi wrote:
>> Add support for probing on ACPI systems, with ACPI HID QCOM8160.
>>
>> On ACPI systems, clocks are always enabled, the PRNG should
>> already be enabled, and the register region is read-only.
>> The driver only verifies that the hardware is already
>> enabled never tries to disable or configure it.
> 
> so if you are using v2 hardware, are you pointing to High Level OS EE or
> some other..?

I'm not sure what you mean.

>>  +	/*
>> +	 * ACPI systems have v2 hardware. The clocks are always enabled,
>> +	 * the PRNG register space is read-only, and the PRNG should
>> +	 * already be enabled.
>> +	 */
>> +	if (has_acpi_companion(&pdev->dev)) {
>> +		u32 val;
>> +
>> +		val = readl(rng->base + PRNG_CONFIG);
> 
> v2 EEs dont seem to have CONFIG register, so not sure about this one

I can post the register set, but this works on my silicon.  The first 
device has all the registers.  Then there are about 12-13 other devices 
with their own 64K register regions, and those don't have a config 
register.

I don't know why you would choose to support a one of the secondary 
register sets when you can use the primary.


-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 1/2] hwrng: msm: add a spinlock and support for blocking reads

[Timur Tabi]

On 6/21/18 11:24 PM, Vinod wrote:
> On 21-06-18, 23:18, Timur Tabi wrote:
>> On 6/21/18 11:17 PM, Vinod wrote:
>>> this should be a separate patch
>> What exactly should be a separate patch?  This part?
>>
>> -	rng->hwrng.name = KBUILD_MODNAME,
>> -	rng->hwrng.init = msm_rng_init,
>> -	rng->hwrng.cleanup = msm_rng_cleanup,
>> -	rng->hwrng.read = msm_rng_read,
>> +	rng->hwrng.name = KBUILD_MODNAME;
>> +	rng->hwrng.init = msm_rng_init;
>> +	rng->hwrng.cleanup = msm_rng_cleanup;
>> +	rng->hwrng.read = msm_rng_read;
> yes

II consider this to be a minor clean-up that's not worthy of its own 
patch, but I can make this its own patch if you really want.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 2/2] hwrng: msm: add ACPI support

[Vinod]

On 21-06-18, 23:26, Timur Tabi wrote:
> On 6/21/18 11:23 PM, Vinod wrote:
> > On 21-06-18, 10:17, Timur Tabi wrote:
> > > Add support for probing on ACPI systems, with ACPI HID QCOM8160.
> > > 
> > > On ACPI systems, clocks are always enabled, the PRNG should
> > > already be enabled, and the register region is read-only.
> > > The driver only verifies that the hardware is already
> > > enabled never tries to disable or configure it.
> > 
> > so if you are using v2 hardware, are you pointing to High Level OS EE or
> > some other..?
> 
> I'm not sure what you mean.
> 
> > >  +	/*
> > > +	 * ACPI systems have v2 hardware. The clocks are always enabled,
> > > +	 * the PRNG register space is read-only, and the PRNG should
> > > +	 * already be enabled.
> > > +	 */
> > > +	if (has_acpi_companion(&pdev->dev)) {
> > > +		u32 val;
> > > +
> > > +		val = readl(rng->base + PRNG_CONFIG);
> > 
> > v2 EEs dont seem to have CONFIG register, so not sure about this one
> 
> I can post the register set, but this works on my silicon.  The first device
> has all the registers.  Then there are about 12-13 other devices with their
> own 64K register regions, and those don't have a config register.

The one on MSM8996 have 4K register space for each region and first two
regions are not accessible to the SW. They are part of security ring and
hence the CONFIG register is not accessible. If i try to access then it
borks!

Are you sure you are supposed to use the TZ there, I would presume the
lower level firmware would use that?

> I don't know why you would choose to support a one of the secondary register
> sets when you can use the primary.

Access denied is the reason :D

So this make me think you should do 2 level support for ACPI, one ACPI
support and one V2 support where we dont touch CONFIG register. That way
both regions will work

-- 
~Vinod

[PATCH 2/2] hwrng: msm: add ACPI support

[Timur Tabi]

On 6/21/18 11:44 PM, Vinod wrote:
> So this make me think you should do 2 level support for ACPI, one ACPI
> support and one V2 support where we dont touch CONFIG register. That way
> both regions will work

The ACPI system is a v2 system.  If you want, I can just remove the read 
of the CONFIG register.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 2/2] hwrng: msm: add ACPI support

[Vinod]

On 21-06-18, 23:46, Timur Tabi wrote:
> On 6/21/18 11:44 PM, Vinod wrote:
> > So this make me think you should do 2 level support for ACPI, one ACPI
> > support and one V2 support where we dont touch CONFIG register. That way
> > both regions will work
> 
> The ACPI system is a v2 system.  If you want, I can just remove the read of
> the CONFIG register.

Okay, so in this case who configures v2. My guess here is that firmware
hasn't locked access, default everyone can access per spec

-- 
~Vinod

[PATCH 1/2] hwrng: msm: add a spinlock and support for blocking reads

[Stephen Boyd]

Quoting Timur Tabi (2018-06-21 08:17:55)
> @@ -96,11 +110,39 @@ static int msm_rng_read(struct hwrng *hwrng, void *data, size_t max, bool wait)
>  
>         /* read random data from hardware */
>         do {
> -               val = readl_relaxed(rng->base + PRNG_STATUS);
> -               if (!(val & PRNG_STATUS_DATA_AVAIL))
> -                       break;
> +               spin_lock(&rng->lock);
> +
> +               /*
> +                * First check the status bit.  If 'wait' is true, then wait
> +                * up to TIMEOUT microseconds for data to be available.
> +                */
> +               if (wait) {
> +                       int ret;

Please don't do local variables like this. Put them at the top of the
function.

> +
> +                       ret = readl_poll_timeout_atomic(rng->base + PRNG_STATUS,
> +                               val, val & PRNG_STATUS_DATA_AVAIL, 0, TIMEOUT);
> +                       if (ret) {
> +                               /* Timed out */
> +                               spin_unlock(&rng->lock);
> +                               break;
> +                       }
> +               } else {
> +                       val = readl_relaxed(rng->base + PRNG_STATUS);
> +                       if (!(val & PRNG_STATUS_DATA_AVAIL)) {
> +                               spin_unlock(&rng->lock);
> +                               break;
> +                       }
> +               }
>  
>                 val = readl_relaxed(rng->base + PRNG_DATA_OUT);
> +               spin_unlock(&rng->lock);

Maybe this should be written as:

		spin_lock()
		if (wait) {
			has_data = readl_poll_timeout_atomic(...) == 0;
		} else {
			val = readl_relaxed(rng->base + PRNG_STATUS);
			has_data = val & PRNG_STATUS_DATA_AVAIL;
		}

		if (has_data)
			val = readl_relaxed(rng->base + PRNG_DATA_OUT);
		spin_unlock();

		if (!has_data)
			break;

> +
> +               /*
> +                * Zero is technically a valid random number, but it's also
> +                * the value returned if the PRNG is not enabled properly.
> +                * To avoid accidentally returning all zeros, treat it as
> +                * invalid and just return what we've already read.
> +                */
>                 if (!val)
>                         break;

Is this a related change? Looks like a nice comment that isn't related
to locking.

[PATCH 1/2] hwrng: msm: add a spinlock and support for blocking reads

[Timur Tabi]

On 6/22/18 12:36 AM, Stephen Boyd wrote:
> Quoting Timur Tabi (2018-06-21 08:17:55)
>> @@ -96,11 +110,39 @@ static int msm_rng_read(struct hwrng *hwrng, void *data, size_t max, bool wait)
>>   
>>          /* read random data from hardware */
>>          do {
>> -               val = readl_relaxed(rng->base + PRNG_STATUS);
>> -               if (!(val & PRNG_STATUS_DATA_AVAIL))
>> -                       break;
>> +               spin_lock(&rng->lock);
>> +
>> +               /*
>> +                * First check the status bit.  If 'wait' is true, then wait
>> +                * up to TIMEOUT microseconds for data to be available.
>> +                */
>> +               if (wait) {
>> +                       int ret;
> Please don't do local variables like this. Put them at the top of the
> function.

Ok.

> 
>> +
>> +                       ret = readl_poll_timeout_atomic(rng->base + PRNG_STATUS,
>> +                               val, val & PRNG_STATUS_DATA_AVAIL, 0, TIMEOUT);
>> +                       if (ret) {
>> +                               /* Timed out */
>> +                               spin_unlock(&rng->lock);
>> +                               break;
>> +                       }
>> +               } else {
>> +                       val = readl_relaxed(rng->base + PRNG_STATUS);
>> +                       if (!(val & PRNG_STATUS_DATA_AVAIL)) {
>> +                               spin_unlock(&rng->lock);
>> +                               break;
>> +                       }
>> +               }
>>   
>>                  val = readl_relaxed(rng->base + PRNG_DATA_OUT);
>> +               spin_unlock(&rng->lock);
> Maybe this should be written as:
> 
> 		spin_lock()
> 		if (wait) {
> 			has_data = readl_poll_timeout_atomic(...) == 0;
> 		} else {
> 			val = readl_relaxed(rng->base + PRNG_STATUS);
> 			has_data = val & PRNG_STATUS_DATA_AVAIL;
> 		}
> 
> 		if (has_data)
> 			val = readl_relaxed(rng->base + PRNG_DATA_OUT);
> 		spin_unlock();
> 
> 		if (!has_data)
> 			break;

That would work, but I don't really see this as better, just different.

>> +               /*
>> +                * Zero is technically a valid random number, but it's also
>> +                * the value returned if the PRNG is not enabled properly.
>> +                * To avoid accidentally returning all zeros, treat it as
>> +                * invalid and just return what we've already read.
>> +                */
>>                  if (!val)
>>                          break;
> Is this a related change? Looks like a nice comment that isn't related
> to locking.

It's slightly related in that the locking is needed to reduce the number 
of times we read zero from the DATA_OUT register.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 1/2] hwrng: msm: add a spinlock and support for blocking reads

[Stanimir Varbanov]

Hi,

On 06/21/2018 06:17 PM, Timur Tabi wrote:
> The hwrng.read callback includes a boolean parameter called 'wait'
> which indicates whether the function should block and wait for
> more data.
> 
> When 'wait' is true, the driver spins on the DATA_AVAIL bit or until
> a reasonable timeout.  The timeout can occur if there is a heavy load
> on reading the PRNG.
> 
> The same code also needs a spinlock to protect against race conditions.
> 
> If multiple cores hammer on the PRNG, it's possible for a race
> condition to occur between reading the status register and
> reading the data register.  Add a spinlock to protect against
> that.

Before entering into the read function we already hold a mutex which
serializes data reading so I cannot imagine how below sequence could
happen. Can you explain how to reproduce this race?

> 
> 1. Core 1 reads status register, shows data is available.
> 2. Core 2 also reads status register, same result
> 3. Core 2 reads data register, depleting all entropy
> 4. Core 1 reads data register, which returns 0
> 
> Signed-off-by: Timur Tabi <timur@codeaurora.org>
> ---
>  drivers/char/hw_random/msm-rng.c | 57 +++++++++++++++++++++++++++++++++++-----
>  1 file changed, 50 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/char/hw_random/msm-rng.c b/drivers/char/hw_random/msm-rng.c
> index 841fee845ec9..44580588b938 100644
> --- a/drivers/char/hw_random/msm-rng.c
> +++ b/drivers/char/hw_random/msm-rng.c
> @@ -15,9 +15,11 @@

-- 
regards,
Stan

[PATCH 1/2] hwrng: msm: add a spinlock and support for blocking reads

[Timur Tabi]

On 6/22/18 10:38 AM, Stanimir Varbanov wrote:
> Before entering into the read function we already hold a mutex which
> serializes data reading so I cannot imagine how below sequence could
> happen. Can you explain how to reproduce this race?
> 
>> 1. Core 1 reads status register, shows data is available.
>> 2. Core 2 also reads status register, same result
>> 3. Core 2 reads data register, depleting all entropy
>> 4. Core 1 reads data register, which returns 0

I have a test which spawns 100 copies of rngtest on a 48-core machine. 
Without the spinlock, the driver returns no data much more often.

If there really is a mutex that serializes data reads across all cores, 
then I don't have an explanation.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 1/2] hwrng: msm: add a spinlock and support for blocking reads

[Stephen Boyd]

Quoting Timur Tabi (2018-06-22 08:41:11)
> On 6/22/18 10:38 AM, Stanimir Varbanov wrote:
> > Before entering into the read function we already hold a mutex which
> > serializes data reading so I cannot imagine how below sequence could
> > happen. Can you explain how to reproduce this race?
> > 
> >> 1. Core 1 reads status register, shows data is available.
> >> 2. Core 2 also reads status register, same result
> >> 3. Core 2 reads data register, depleting all entropy
> >> 4. Core 1 reads data register, which returns 0
> 
> I have a test which spawns 100 copies of rngtest on a 48-core machine. 
> Without the spinlock, the driver returns no data much more often.
> 
> If there really is a mutex that serializes data reads across all cores, 
> then I don't have an explanation.
> 

Perhaps it's because you implemented the 'wait' functionality in this
driver? Before the patch there wasn't any sort of wait check so we would
bail out if there wasn't any data even if the caller requested that we
wait for randomness to be available.

[PATCH 1/2] hwrng: msm: add a spinlock and support for blocking reads

[Timur Tabi]

On 6/22/18 12:51 PM, Stephen Boyd wrote:
> Perhaps it's because you implemented the 'wait' functionality in this
> driver? Before the patch there wasn't any sort of wait check so we would
> bail out if there wasn't any data even if the caller requested that we
> wait for randomness to be available.

No, my tests showed the race condition (or at least something that looks 
like a race condition) even without the 'wait' feature.  I added support 
for 'wait' only recently, but I've been working with the crypto people 
for a month on everything else.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 1/2] hwrng: msm: add a spinlock and support for blocking reads

[Timur Tabi]

On 06/22/2018 01:03 PM, Timur Tabi wrote:
> 
>> Perhaps it's because you implemented the 'wait' functionality in this
>> driver? Before the patch there wasn't any sort of wait check so we would
>> bail out if there wasn't any data even if the caller requested that we
>> wait for randomness to be available.
> 
> No, my tests showed the race condition (or at least something that looks 
> like a race condition) even without the 'wait' feature.? I added support 
> for 'wait' only recently, but I've been working with the crypto people 
> for a month on everything else.

Looks like I was wrong.

I created some new tests to reproduce the problem, but I can't reproduce 
it any more.  I can only assume that what I saw as a race condition back 
then was something else entirely.

So all of the spinlock code needs to go.  It looks like at this point, 
if Vinod can add support for QCOM8160 to his patches, the only thing I 
can contribute is support for 'wait'.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.