hwrng: msm: add a spinlock and support for blocking reads

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 <[email protected]>

---
 drivers/char/hw_random/msm-rng.c | 40 ++++++++++++++++++++++++++++++++++------
 1 file changed, 34 insertions(+), 6 deletions(-)

drivers/char/hw_random/msm-rng.c +1 -6

index 44580588b938..f34713d23d77 100644
@@ -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);

drivers/char/hw_random/msm-rng.c +24 -0

+		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);

Signature · 3 lines

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.

Quoted 6 lines · 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.

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

drivers/char/hw_random/msm-rng.c static int msm_rng_probe(struct platform_device *pdev) · line 202

Quoted 9 lines

+ /*

drivers/char/hw_random/msm-rng.c +8 -0

+	 * 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

Quoted 10 lines · Vinod

Quoted 6 lines · 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.

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.

Quoted 11 lines

drivers/char/hw_random/msm-rng.c static int msm_rng_probe(struct platform_device *pdev) · line 202

Quoted 9 lines

+ /*

drivers/char/hw_random/msm-rng.c +8 -0

+	 * 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.

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

Quoted 3 lines · Vinod

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.

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 <[email protected]>

---
 drivers/char/hw_random/msm-rng.c | 57 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 50 insertions(+), 7 deletions(-)

drivers/char/hw_random/msm-rng.c +45 -3

index 841fee845ec9..44580588b938 100644
@@ -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))

this should be a separate patch

Quoted 4 lines · Timur Tabi

Quoted 1 line · Vinod

this should be a separate patch

What exactly should be a separate patch? This part?

drivers/char/hw_random/msm-rng.c static int msm_rng_probe(struct platform_device *pdev) · line 196

Quoted 8 lines

drivers/char/hw_random/msm-rng.c +4 -4

-	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

drivers/char/hw_random/msm-rng.c +0 -1

@@ -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);

drivers/char/hw_random/msm-rng.c +8 -0

+
+                       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;
+                       }

Quoting Timur Tabi (2018-06-21 08:17:55)

Quoted 5 lines

drivers/char/hw_random/msm-rng.c +0 -1

@@ -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);

drivers/char/hw_random/msm-rng.c static int msm_rng_read(struct hwrng *hwrng, void *data, size_t max, bool wait) · line 120

Quoted 10 lines

drivers/char/hw_random/msm-rng.c +8 -2

-               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.

Quoted 9 lines

drivers/char/hw_random/msm-rng.c +8 -0

+
+                       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;
+                       }

drivers/char/hw_random/msm-rng.c static int msm_rng_read(struct hwrng *hwrng, void *data, size_t max, bool wait) · line 138

Quoted 10 lines

drivers/char/hw_random/msm-rng.c +8 -0

+               } 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;

drivers/char/hw_random/msm-rng.c static int msm_rng_read(struct hwrng *hwrng, void *data, size_t max, bool wait) · line 147

Quoted 8 lines

drivers/char/hw_random/msm-rng.c +6 -0

+               /*
+                * 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.

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 <[email protected]>

---
 drivers/char/hw_random/msm-rng.c | 57 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 50 insertions(+), 7 deletions(-)

drivers/char/hw_random/msm-rng.c +0 -0

index 841fee845ec9..44580588b938 100644
@@ -15,9 +15,11 @@

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?

Quoted 4 lines

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

Quoted 8 lines

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?

Quoted 4 lines

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.

Quoted 5 lines

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.