[PATCH] hw_random: timeriomem-rng: add configurable read width and data mask

[PATCH] hw_random: timeriomem-rng: add configurable read width and data mask

[Jad Keskes]

The TODO for supporting read sizes other than 32 bits and masking has
been sitting in this driver since 2009.  Implement it.

Add width (8, 16, or 32 bits) and mask properties to the platform data
and device tree bindings.  The read loop dispatches on width using
readb/readw/readl so a configured 8-bit access doesn't trigger a bus
error on hardware that rejects 32-bit reads to that address.  The mask
is ANDed with the value before storing.

These are platform properties, not runtime policy -- width depends on
SoC integration, mask reflects which output bits carry entropy.

The alignment check in probe is updated to verify the resource is
aligned to the configured width instead of hardcoding 4-byte alignment.

Signed-off-by: Jad Keskes <inasj268@gmail.com>
---
 .../bindings/rng/timeriomem-rng.yaml          | 76 ++++++++++++++++++
 drivers/char/hw_random/timeriomem-rng.c       | 78 +++++++++++++++----
 include/linux/timeriomem-rng.h                | 12 +++
 3 files changed, 153 insertions(+), 13 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/rng/timeriomem-rng.yaml

diff --git a/Documentation/devicetree/bindings/rng/timeriomem-rng.yaml b/Documentation/devicetree/bindings/rng/timeriomem-rng.yaml
new file mode 100644
index 000000000000..0d8460e9f916
--- /dev/null
+++ b/Documentation/devicetree/bindings/rng/timeriomem-rng.yaml
@@ -0,0 +1,76 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/rng/timeriomem-rng.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Timer IOMEM Hardware Random Number Generator
+
+description: |
+  This binding covers platforms that have a single IO memory address which
+  provides periodic random data.  The driver reads from the address at a
+  fixed interval, returning a configurable-width value masked to the desired
+  bits.
+
+maintainers:
+  - Alexander Clouter <alex@digriz.org.uk>
+
+properties:
+  compatible:
+    enum:
+      - timeriomem_rng
+
+  reg:
+    maxItems: 1
+
+  period:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Interval in microseconds between reads.  New random data is expected to
+      be available at this rate.
+
+  quality:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    default: 0
+    description:
+      Estimated entropy per 1024 bits of data, in the same scale as the
+      kernel's hwrng core (0-1024).
+
+  width:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    default: 32
+    enum: [8, 16, 32]
+    description:
+      Access width in bits.  Determines whether the read is performed as
+      an 8-bit, 16-bit, or 32-bit bus access.
+
+  mask:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    default: 0xFFFFFFFF
+    description:
+      Mask applied to the value read from the register.  Bits set to 0 in
+      the mask are cleared in the output data.  Default (no mask) passes
+      all bits through.
+
+required:
+  - compatible
+  - reg
+  - period
+
+additionalProperties: false
+
+examples:
+  - |
+    rng@f0001000 {
+        compatible = "timeriomem_rng";
+        reg = <0xf0001000 0x4>;
+        period = <100000>;
+    };
+
+    rng@f0002000 {
+        compatible = "timeriomem_rng";
+        reg = <0xf0002000 0x1>;
+        period = <50000>;
+        width = <8>;
+        mask = <0xFF>;
+    };
diff --git a/drivers/char/hw_random/timeriomem-rng.c b/drivers/char/hw_random/timeriomem-rng.c
index e61f06393209..930898852147 100644
--- a/drivers/char/hw_random/timeriomem-rng.c
+++ b/drivers/char/hw_random/timeriomem-rng.c
@@ -14,7 +14,9 @@
  *   has to do is provide the address and 'wait time' that new data becomes
  *   available.
  *
- * TODO: add support for reading sizes other than 32bits and masking
+ * The read width (8, 16, or 32 bits) and an optional data mask can be
+ * configured through platform data or device tree properties.  Default is
+ * 32-bit reads with no mask.
  */
 
 #include <linux/completion.h>
@@ -34,6 +36,8 @@ struct timeriomem_rng_private {
 	void __iomem		*io_base;
 	ktime_t			period;
 	unsigned int		present:1;
+	unsigned int		width;
+	u32			mask;
 
 	struct hrtimer		timer;
 	struct completion	completion;
@@ -48,6 +52,7 @@ static int timeriomem_rng_read(struct hwrng *hwrng, void *data,
 		container_of(hwrng, struct timeriomem_rng_private, rng_ops);
 	int retval = 0;
 	int period_us = ktime_to_us(priv->period);
+	int chunk = priv->width / 8;
 
 	/*
 	 * There may not have been enough time for new data to be generated
@@ -71,11 +76,28 @@ static int timeriomem_rng_read(struct hwrng *hwrng, void *data,
 			usleep_range(period_us,
 					period_us + max(1, period_us / 100));
 
-		*(u32 *)data = readl(priv->io_base);
-		retval += sizeof(u32);
-		data += sizeof(u32);
-		max -= sizeof(u32);
-	} while (wait && max > sizeof(u32));
+		switch (priv->width) {
+		case 8: {
+			u8 val = readb(priv->io_base) & priv->mask;
+			*(u8 *)data = val;
+			break;
+		}
+		case 16: {
+			u16 val = readw(priv->io_base) & priv->mask;
+			*(u16 *)data = val;
+			break;
+		}
+		case 32: {
+			u32 val = readl(priv->io_base) & priv->mask;
+			*(u32 *)data = val;
+			break;
+		}
+		}
+
+		retval += chunk;
+		data += chunk;
+		max -= chunk;
+	} while (wait && max > chunk);
 
 	/*
 	 * Block any new callers until the RNG has had time to generate new
@@ -125,11 +147,8 @@ static int timeriomem_rng_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->io_base))
 		return PTR_ERR(priv->io_base);
 
-	if (res->start % 4 != 0 || resource_size(res) < 4) {
-		dev_err(&pdev->dev,
-			"address must be at least four bytes wide and 32-bit aligned\n");
-		return -EINVAL;
-	}
+	priv->width = 32;
+	priv->mask = 0xFFFFFFFF;
 
 	if (pdev->dev.of_node) {
 		int i;
@@ -145,9 +164,42 @@ static int timeriomem_rng_probe(struct platform_device *pdev)
 		if (!of_property_read_u32(pdev->dev.of_node,
 						"quality", &i))
 			priv->rng_ops.quality = i;
+
+		of_property_read_u32(pdev->dev.of_node,
+				     "width", &priv->width);
+		of_property_read_u32(pdev->dev.of_node,
+				     "mask", &priv->mask);
 	} else {
 		period = pdata->period;
 		priv->rng_ops.quality = pdata->quality;
+
+		if (pdata->width_set)
+			priv->width = pdata->width;
+		if (pdata->mask_set)
+			priv->mask = pdata->mask;
+	}
+
+	if (priv->width == 0)
+		priv->width = 32;
+
+	switch (priv->width) {
+	case 8:
+	case 16:
+	case 32:
+		break;
+	default:
+		dev_err(&pdev->dev, "invalid width %u, must be 8, 16, or 32\n",
+			priv->width);
+		return -EINVAL;
+	}
+
+	if (res->start % (priv->width / 8) != 0 ||
+	    resource_size(res) < priv->width / 8) {
+		dev_err(&pdev->dev,
+			"address must be at least %u-bit aligned (%u byte%s)\n",
+			priv->width, priv->width / 8,
+			priv->width / 8 > 1 ? "s" : "");
+		return -EINVAL;
 	}
 
 	priv->period = us_to_ktime(period);
@@ -167,8 +219,8 @@ static int timeriomem_rng_probe(struct platform_device *pdev)
 		return err;
 	}
 
-	dev_info(&pdev->dev, "32bits from 0x%p @ %dus\n",
-			priv->io_base, period);
+	dev_info(&pdev->dev, "%ubit from %p @ %dus\n",
+		 priv->width, priv->io_base, period);
 
 	return 0;
 }
diff --git a/include/linux/timeriomem-rng.h b/include/linux/timeriomem-rng.h
index 672df7fbf6c1..b4202ad2f507 100644
--- a/include/linux/timeriomem-rng.h
+++ b/include/linux/timeriomem-rng.h
@@ -16,6 +16,18 @@ struct timeriomem_rng_data {
 
 	/* bits of entropy per 1024 bits read */
 	unsigned int		quality;
+
+	/* read width (8, 16, or 32), 0 means 32 */
+	unsigned int		width;
+
+	/* set to true if width is explicitly provided */
+	bool			width_set;
+
+	/* mask applied to raw read value */
+	u32			mask;
+
+	/* set to true if mask is explicitly provided */
+	bool			mask_set;
 };
 
 #endif /* _LINUX_TIMERIOMEM_RNG_H */
-- 
2.54.0

Re: [PATCH] hw_random: timeriomem-rng: add configurable read width and data mask

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] The patch introduces a new device tree binding schema file but leaves the old schema intact, causing validation conflicts.
- [Low] Using the modulo operator `%` on a `resource_size_t` variable causes an undefined reference link error on 32-bit platforms.

Pre-existing issues:
- [High] The driver cancels the timer in `remove()` but relies on `devm` cleanup to unregister the hwrng device, creating a teardown race condition that leads to use-after-free or deadlock.
--

commit 57d226c081fc616a1deb344ec82199ee7e9c1059
Author: Jad Keskes <inasj268@gmail.com>

hw_random: timeriomem-rng: add configurable read width and data mask

The TODO for supporting read sizes other than 32 bits and masking has
been sitting in this driver since 2009.  Implement it.

> diff --git a/Documentation/devicetree/bindings/rng/timeriomem-rng.yaml b/Documentation/devicetree/bindings/rng/timeriomem-rng.yaml
> new file mode 100644
> index 0000000000000..0d8460e9f9163
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rng/timeriomem-rng.yaml
> @@ -0,0 +1,76 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/rng/timeriomem-rng.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Timer IOMEM Hardware Random Number Generator

[Severity: Medium]
Will this new device tree binding schema file conflict with the existing
schema?

The patch introduces this new schema but does not appear to replace or remove
the existing timeriomem_rng.yaml file. Since both schemas match the same
compatible = "timeriomem_rng" string and the old schema specifies
additionalProperties: false, any device tree node including the newly added
width or mask properties might pass this new schema but fail the old schema's
validation, which would break dt_binding_check.

> diff --git a/drivers/char/hw_random/timeriomem-rng.c b/drivers/char/hw_random/timeriomem-rng.c
> index e61f063932090..9308988521473 100644
> --- a/drivers/char/hw_random/timeriomem-rng.c
> +++ b/drivers/char/hw_random/timeriomem-rng.c
[ ... ]
> @@ -71,11 +76,28 @@ static int timeriomem_rng_read(struct hwrng *hwrng, void *data,
>  			usleep_range(period_us,
>  					period_us + max(1, period_us / 100));
>  
> -		*(u32 *)data = readl(priv->io_base);
> -		retval += sizeof(u32);
> -		data += sizeof(u32);
> -		max -= sizeof(u32);
> -	} while (wait && max > sizeof(u32));
> +		switch (priv->width) {
> +		case 8: {
> +			u8 val = readb(priv->io_base) & priv->mask;
> +			*(u8 *)data = val;
> +			break;
> +		}
> +		case 16: {
> +			u16 val = readw(priv->io_base) & priv->mask;
> +			*(u16 *)data = val;
> +			break;
> +		}
> +		case 32: {
> +			u32 val = readl(priv->io_base) & priv->mask;
> +			*(u32 *)data = val;
> +			break;
> +		}
> +		}
> +
> +		retval += chunk;
> +		data += chunk;
> +		max -= chunk;
> +	} while (wait && max > chunk);

[Severity: High]
This is a pre-existing issue, but does the driver have a teardown race
condition between remove() and devres cleanup?

In timeriomem_rng_remove(), hrtimer_cancel() is explicitly called to stop the
timer. However, because the RNG device was registered with
devm_hwrng_register() during probe, it remains accessible to userspace and the
hwrng core until the devres cleanups run strictly after remove() returns.

If a concurrent read is blocked on wait_for_completion() when the timer is
cancelled, it could hang indefinitely, deadlocking the reader thread and the
subsequent hwrng_unregister() call in devres cleanup.

Alternatively, if a reader completes its wait and unconditionally calls
hrtimer_restart() in timeriomem_rng_read() after the cancellation, the timer
might be re-armed:

drivers/char/hw_random/timeriomem-rng.c:timeriomem_rng_read() {
    ...
	priv->present = 0;
	reinit_completion(&priv->completion);
	hrtimer_forward_now(&priv->timer, priv->period);
	hrtimer_restart(&priv->timer);
    ...
}

When the devres cleanup later unregisters the RNG and frees the priv
structure, this re-armed timer will eventually fire, potentially dereferencing
a freed pointer.

[ ... ]
> @@ -145,9 +164,42 @@ static int timeriomem_rng_probe(struct platform_device *pdev)
>  		if (!of_property_read_u32(pdev->dev.of_node,
>  						"quality", &i))
>  			priv->rng_ops.quality = i;
> +
> +		of_property_read_u32(pdev->dev.of_node,
> +				     "width", &priv->width);
> +		of_property_read_u32(pdev->dev.of_node,
> +				     "mask", &priv->mask);
>  	} else {
>  		period = pdata->period;
>  		priv->rng_ops.quality = pdata->quality;
> +
> +		if (pdata->width_set)
> +			priv->width = pdata->width;
> +		if (pdata->mask_set)
> +			priv->mask = pdata->mask;
> +	}
> +
> +	if (priv->width == 0)
> +		priv->width = 32;
> +
> +	switch (priv->width) {
> +	case 8:
> +	case 16:
> +	case 32:
> +		break;
> +	default:
> +		dev_err(&pdev->dev, "invalid width %u, must be 8, 16, or 32\n",
> +			priv->width);
> +		return -EINVAL;
> +	}
> +
> +	if (res->start % (priv->width / 8) != 0 ||
> +	    resource_size(res) < priv->width / 8) {

[Severity: Low]
Does using the modulo operator here cause an undefined reference to __umoddi3
on 32-bit platforms?

The variable res->start is of type resource_size_t, which can be a 64-bit
integer on 32-bit platforms with PAE enabled (CONFIG_PHYS_ADDR_T_64BIT=y).

Since the divisor (priv->width / 8) is a runtime value rather than a
compile-time constant, the compiler might emit a call to __umoddi3 for the
64-bit division, which leads to a linker error on 32-bit architectures.
Could IS_ALIGNED() or div_u64() be used instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615170922.1132642-1-inasj268@gmail.com?part=1