[PATCH v3 0/8] tpm: rng: Move TPM RNG functionality to driver model

[PATCH v3 0/8] tpm: rng: Move TPM RNG functionality to driver model

[Sughosh Ganu]

The TPM device provides the random number generator(RNG)
functionality, whereby sending a command to the TPM device results in
the TPM device responding with random bytes.

There was a discussion on the mailing list earlier[1], where it was
explained that platforms with a TPM device can install the
EFI_RNG_PROTOCOL for getting the random bytes instead of populating
the dtb with the kaslr-seed property. That would make it possible to
measure the dtb.

This patchset moves the already existing functions for getting random
bytes from the TPM device to drivers complying with the RNG
uclass. This is done since the EFI_RNG_PROTOCOL's get_rng routine uses
the RNG uclass's dm_rng_read api to get the random bytes.

The TPM uclass driver adds the RNG child device as part of it's
post_probe function. The TPM uclass driver's child_pre_probe function
initialises the TPM parent device for use -- this enables the RNG
child device to be used subsequently.

Some additional changes have also been made to facilitate the
use of the RNG devices, including extending the 'rng' command to take
the RNG device as one of the command-line parameters.

This series depends on a patch[2] from Simon Glass for moving the TPM
device version detection functions to the tpm_api.h header as static
inline functions. Using this patch, a couple of patches[3][4] from the
series are no longer needed.

[1] - https://lore.kernel.org/u-boot/20220103120738.47835-1-ilias.apalodimas@linaro.org/
[2] - https://lore.kernel.org/u-boot/20220301001125.1554442-2-sjg@chromium.org/T/#u
[3] - https://lore.kernel.org/u-boot/20220228120638.678137-7-sughosh.ganu@linaro.org/T/#u
[4] - https://lore.kernel.org/u-boot/20220228120638.678137-6-sughosh.ganu@linaro.org/T/#u

Changes since V2:

* Export the existing tpm*_get_random functions to the driver model
  instead of moving them to the drivers/rng/ directory.
* Enable DM_RNG when CONFIG_TPM is enabled to build the RNG uclass
  code
* Additional patch for a document for the 'rng' command as suggested
  by Simon Glass
* Additional patch for unit test for the 'rng' command as suggested by
  Simon Glass

Sughosh Ganu (8):
  tpm: rng: Change tpm_get_random to return an int
  tpm: Fix the return type of tpm_startup
  tpm: rng: Add driver model interface for TPM RNG device
  tpm: Add the RNG child device
  qemu: arm: Remove platform specific function to get RNG device
  cmd: rng: Add support for selecting RNG device
  doc: rng: Add documentation for the rng command
  test: rng: Add a UT testcase for the rng command

 board/emulation/qemu-arm/qemu-arm.c | 42 --------------------
 cmd/Kconfig                         |  1 +
 cmd/rng.c                           | 31 +++++++++++----
 doc/usage/index.rst                 |  1 +
 doc/usage/rng.rst                   | 25 ++++++++++++
 drivers/tpm/tpm-uclass.c            | 60 +++++++++++++++++++++++++++--
 include/tpm-v1.h                    |  4 +-
 include/tpm-v2.h                    |  4 +-
 include/tpm_api.h                   |  6 +--
 lib/Kconfig                         |  1 +
 lib/tpm-v1.c                        | 28 ++++++++++----
 lib/tpm-v2.c                        | 21 +++++++---
 lib/tpm_api.c                       | 28 +++++++++++---
 test/dm/rng.c                       | 29 ++++++++++++++
 14 files changed, 201 insertions(+), 80 deletions(-)
 create mode 100644 doc/usage/rng.rst

-- 
2.25.1

[PATCH v3 1/8] tpm: rng: Change tpm_get_random to return an int

[Sughosh Ganu]

The tpm random number generation functionality will be moved to the
driver model. With that, the tpm_get_random function will call the
common driver model api instead of separate functions for tpmv1 and
tpmv2. Return an int instead of a u32 to comply with the return value
of the driver model function.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

Changes since V2: None

 include/tpm_api.h | 4 ++--
 lib/tpm_api.c     | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/tpm_api.h b/include/tpm_api.h
index 11aa14eb79..249a966942 100644
--- a/include/tpm_api.h
+++ b/include/tpm_api.h
@@ -274,9 +274,9 @@ u32 tpm_find_key_sha1(struct udevice *dev, const u8 auth[20],
  * @param dev		TPM device
  * @param data		output buffer for the random bytes
  * @param count		size of output buffer
- * Return: return code of the operation
+ * Return: 0 if OK, -ve on error
  */
-u32 tpm_get_random(struct udevice *dev, void *data, u32 count);
+int tpm_get_random(struct udevice *dev, void *data, u32 count);
 
 /**
  * tpm_finalise_physical_presence() - Finalise physical presence
diff --git a/lib/tpm_api.c b/lib/tpm_api.c
index 4ac4612c81..7d26ea2c3a 100644
--- a/lib/tpm_api.c
+++ b/lib/tpm_api.c
@@ -264,7 +264,7 @@ u32 tpm_get_permissions(struct udevice *dev, u32 index, u32 *perm)
 		return -ENOSYS;
 }
 
-u32 tpm_get_random(struct udevice *dev, void *data, u32 count)
+int tpm_get_random(struct udevice *dev, void *data, u32 count)
 {
 	if (tpm_is_v1(dev))
 		return tpm1_get_random(dev, data, count);
-- 
2.25.1

Re: [PATCH v3 1/8] tpm: rng: Change tpm_get_random to return an int

[Ilias Apalodimas]

On Fri, Mar 04, 2022 at 07:04:22PM +0530, Sughosh Ganu wrote:
> The tpm random number generation functionality will be moved to the
> driver model. With that, the tpm_get_random function will call the
> common driver model api instead of separate functions for tpmv1 and
> tpmv2. Return an int instead of a u32 to comply with the return value
> of the driver model function.
> 
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes since V2: None
> 
>  include/tpm_api.h | 4 ++--
>  lib/tpm_api.c     | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/tpm_api.h b/include/tpm_api.h
> index 11aa14eb79..249a966942 100644
> --- a/include/tpm_api.h
> +++ b/include/tpm_api.h
> @@ -274,9 +274,9 @@ u32 tpm_find_key_sha1(struct udevice *dev, const u8 auth[20],
>   * @param dev		TPM device
>   * @param data		output buffer for the random bytes
>   * @param count		size of output buffer
> - * Return: return code of the operation
> + * Return: 0 if OK, -ve on error
>   */
> -u32 tpm_get_random(struct udevice *dev, void *data, u32 count);
> +int tpm_get_random(struct udevice *dev, void *data, u32 count);
>  
>  /**
>   * tpm_finalise_physical_presence() - Finalise physical presence
> diff --git a/lib/tpm_api.c b/lib/tpm_api.c
> index 4ac4612c81..7d26ea2c3a 100644
> --- a/lib/tpm_api.c
> +++ b/lib/tpm_api.c
> @@ -264,7 +264,7 @@ u32 tpm_get_permissions(struct udevice *dev, u32 index, u32 *perm)
>  		return -ENOSYS;
>  }
>  
> -u32 tpm_get_random(struct udevice *dev, void *data, u32 count)
> +int tpm_get_random(struct udevice *dev, void *data, u32 count)
>  {
>  	if (tpm_is_v1(dev))
>  		return tpm1_get_random(dev, data, count);
> -- 
> 2.25.1
> 
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

[PATCH v3 2/8] tpm: Fix the return type of tpm_startup

[Sughosh Ganu]

The tpm_startup function returns negative values for error
conditions. Fix the return type of the function to a signed int
instead of a u32.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

Changes since V2: None

 include/tpm_api.h | 2 +-
 lib/tpm_api.c     | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/tpm_api.h b/include/tpm_api.h
index 249a966942..4d77a49719 100644
--- a/include/tpm_api.h
+++ b/include/tpm_api.h
@@ -18,7 +18,7 @@
  * @param mode		TPM startup mode
  * Return: return code of the operation
  */
-u32 tpm_startup(struct udevice *dev, enum tpm_startup_type mode);
+int tpm_startup(struct udevice *dev, enum tpm_startup_type mode);
 
 /**
  * Issue a TPM_SelfTestFull command.
diff --git a/lib/tpm_api.c b/lib/tpm_api.c
index 7d26ea2c3a..da48058abe 100644
--- a/lib/tpm_api.c
+++ b/lib/tpm_api.c
@@ -11,7 +11,8 @@
 #include <tpm-v2.h>
 #include <tpm_api.h>
 
-u32 tpm_startup(struct udevice *dev, enum tpm_startup_type mode)
+
+int tpm_startup(struct udevice *dev, enum tpm_startup_type mode)
 {
 	if (tpm_is_v1(dev)) {
 		return tpm1_startup(dev, mode);
-- 
2.25.1

Re: [PATCH v3 2/8] tpm: Fix the return type of tpm_startup

[Ilias Apalodimas]

On Fri, Mar 04, 2022 at 07:04:23PM +0530, Sughosh Ganu wrote:
> The tpm_startup function returns negative values for error
> conditions. Fix the return type of the function to a signed int
> instead of a u32.
> 
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes since V2: None
> 
>  include/tpm_api.h | 2 +-
>  lib/tpm_api.c     | 3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/include/tpm_api.h b/include/tpm_api.h
> index 249a966942..4d77a49719 100644
> --- a/include/tpm_api.h
> +++ b/include/tpm_api.h
> @@ -18,7 +18,7 @@
>   * @param mode		TPM startup mode
>   * Return: return code of the operation
>   */
> -u32 tpm_startup(struct udevice *dev, enum tpm_startup_type mode);
> +int tpm_startup(struct udevice *dev, enum tpm_startup_type mode);
>  
>  /**
>   * Issue a TPM_SelfTestFull command.
> diff --git a/lib/tpm_api.c b/lib/tpm_api.c
> index 7d26ea2c3a..da48058abe 100644
> --- a/lib/tpm_api.c
> +++ b/lib/tpm_api.c
> @@ -11,7 +11,8 @@
>  #include <tpm-v2.h>
>  #include <tpm_api.h>
>  
> -u32 tpm_startup(struct udevice *dev, enum tpm_startup_type mode)
> +
> +int tpm_startup(struct udevice *dev, enum tpm_startup_type mode)
>  {
>  	if (tpm_is_v1(dev)) {
>  		return tpm1_startup(dev, mode);
> -- 
> 2.25.1
> 
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

[PATCH v3 3/8] tpm: rng: Add driver model interface for TPM RNG device

[Sughosh Ganu]

The TPM device has a builtin random number generator(RNG)
functionality. Expose the RNG functions of the TPM device to the
driver model so that they can be used by the EFI_RNG_PROTOCOL if the
protocol is installed.

Also change the function arguments and return type of the random
number functions to comply with the driver model api.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---

Changes since V2:

* Export the existing tpm*_get_random functions to the driver model
  instead of moving them to the drivers/rng/ directory.

 include/tpm-v1.h |  4 ++--
 include/tpm-v2.h |  4 ++--
 lib/tpm-v1.c     | 28 ++++++++++++++++++++--------
 lib/tpm-v2.c     | 21 ++++++++++++++++-----
 lib/tpm_api.c    | 23 +++++++++++++++++++----
 5 files changed, 59 insertions(+), 21 deletions(-)

diff --git a/include/tpm-v1.h b/include/tpm-v1.h
index 33d53fb695..d2ff8b446d 100644
--- a/include/tpm-v1.h
+++ b/include/tpm-v1.h
@@ -555,9 +555,9 @@ u32 tpm1_find_key_sha1(struct udevice *dev, const u8 auth[20],
  * @param dev		TPM device
  * @param data		output buffer for the random bytes
  * @param count		size of output buffer
- * Return: return code of the operation
+ * Return: 0 if OK, -ve on error
  */
-u32 tpm1_get_random(struct udevice *dev, void *data, u32 count);
+int tpm1_get_random(struct udevice *dev, void *data, size_t count);
 
 /**
  * tpm_finalise_physical_presence() - Finalise physical presence
diff --git a/include/tpm-v2.h b/include/tpm-v2.h
index e79c90b939..4fb1e7a948 100644
--- a/include/tpm-v2.h
+++ b/include/tpm-v2.h
@@ -619,9 +619,9 @@ u32 tpm2_pcr_setauthvalue(struct udevice *dev, const char *pw,
  * @param data		output buffer for the random bytes
  * @param count		size of output buffer
  *
- * Return: return code of the operation
+ * Return: 0 if OK, -ve on error
  */
-u32 tpm2_get_random(struct udevice *dev, void *data, u32 count);
+int tpm2_get_random(struct udevice *dev, void *data, size_t count);
 
 /**
  * Lock data in the TPM
diff --git a/lib/tpm-v1.c b/lib/tpm-v1.c
index 22a769c587..57bb4a39cb 100644
--- a/lib/tpm-v1.c
+++ b/lib/tpm-v1.c
@@ -9,12 +9,14 @@
 #include <common.h>
 #include <dm.h>
 #include <log.h>
-#include <asm/unaligned.h>
-#include <u-boot/sha1.h>
+#include <rng.h>
 #include <tpm-common.h>
 #include <tpm-v1.h>
 #include "tpm-utils.h"
 
+#include <asm/unaligned.h>
+#include <u-boot/sha1.h>
+
 #ifdef CONFIG_TPM_AUTH_SESSIONS
 
 #ifndef CONFIG_SHA1
@@ -869,7 +871,7 @@ u32 tpm1_find_key_sha1(struct udevice *dev, const u8 auth[20],
 
 #endif /* CONFIG_TPM_AUTH_SESSIONS */
 
-u32 tpm1_get_random(struct udevice *dev, void *data, u32 count)
+int tpm1_get_random(struct udevice *dev, void *data, size_t count)
 {
 	const u8 command[14] = {
 		0x0, 0xc1,		/* TPM_TAG */
@@ -892,19 +894,19 @@ u32 tpm1_get_random(struct udevice *dev, void *data, u32 count)
 		if (pack_byte_string(buf, sizeof(buf), "sd",
 				     0, command, sizeof(command),
 				     length_offset, this_bytes))
-			return TPM_LIB_ERROR;
-		err = tpm_sendrecv_command(dev, buf, response,
+			return -EIO;
+		err = tpm_sendrecv_command(dev->parent, buf, response,
 					   &response_length);
 		if (err)
 			return err;
 		if (unpack_byte_string(response, response_length, "d",
 				       data_size_offset, &data_size))
-			return TPM_LIB_ERROR;
+			return -EIO;
 		if (data_size > count)
-			return TPM_LIB_ERROR;
+			return -EIO;
 		if (unpack_byte_string(response, response_length, "s",
 				       data_offset, out, data_size))
-			return TPM_LIB_ERROR;
+			return -EIO;
 
 		count -= data_size;
 		out += data_size;
@@ -912,3 +914,13 @@ u32 tpm1_get_random(struct udevice *dev, void *data, u32 count)
 
 	return 0;
 }
+
+static const struct dm_rng_ops tpm1_rng_ops = {
+	.read = tpm1_get_random,
+};
+
+U_BOOT_DRIVER(tpm1_rng) = {
+	.name	= "tpm1-rng",
+	.id	= UCLASS_RNG,
+	.ops	= &tpm1_rng_ops,
+};
diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
index 1bf627853a..18f64f0886 100644
--- a/lib/tpm-v2.c
+++ b/lib/tpm-v2.c
@@ -6,6 +6,7 @@
 
 #include <common.h>
 #include <dm.h>
+#include <rng.h>
 #include <tpm-common.h>
 #include <tpm-v2.h>
 #include <linux/bitops.h>
@@ -562,7 +563,7 @@ u32 tpm2_pcr_setauthvalue(struct udevice *dev, const char *pw,
 	return tpm_sendrecv_command(dev, command_v2, NULL, NULL);
 }
 
-u32 tpm2_get_random(struct udevice *dev, void *data, u32 count)
+int tpm2_get_random(struct udevice *dev, void *data, size_t count)
 {
 	const u8 command_v2[10] = {
 		tpm_u16(TPM2_ST_NO_SESSIONS),
@@ -585,19 +586,19 @@ u32 tpm2_get_random(struct udevice *dev, void *data, u32 count)
 		if (pack_byte_string(buf, sizeof(buf), "sw",
 				     0, command_v2, sizeof(command_v2),
 				     sizeof(command_v2), this_bytes))
-			return TPM_LIB_ERROR;
-		err = tpm_sendrecv_command(dev, buf, response,
+			return -EIO;
+		err = tpm_sendrecv_command(dev->parent, buf, response,
 					   &response_length);
 		if (err)
 			return err;
 		if (unpack_byte_string(response, response_length, "w",
 				       data_size_offset, &data_size))
-			return TPM_LIB_ERROR;
+			return -EIO;
 		if (data_size > this_bytes)
 			return TPM_LIB_ERROR;
 		if (unpack_byte_string(response, response_length, "s",
 				       data_offset, out, data_size))
-			return TPM_LIB_ERROR;
+			return -EIO;
 
 		count -= data_size;
 		out += data_size;
@@ -669,3 +670,13 @@ u32 tpm2_submit_command(struct udevice *dev, const u8 *sendbuf,
 {
 	return tpm_sendrecv_command(dev, sendbuf, recvbuf, recv_size);
 }
+
+static const struct dm_rng_ops tpm2_rng_ops = {
+	.read = tpm2_get_random,
+};
+
+U_BOOT_DRIVER(tpm2_rng) = {
+	.name	= "tpm2-rng",
+	.id	= UCLASS_RNG,
+	.ops	= &tpm2_rng_ops,
+};
diff --git a/lib/tpm_api.c b/lib/tpm_api.c
index da48058abe..3584fda98c 100644
--- a/lib/tpm_api.c
+++ b/lib/tpm_api.c
@@ -6,6 +6,7 @@
 #include <common.h>
 #include <dm.h>
 #include <log.h>
+#include <rng.h>
 #include <tpm_api.h>
 #include <tpm-v1.h>
 #include <tpm-v2.h>
@@ -265,12 +266,26 @@ u32 tpm_get_permissions(struct udevice *dev, u32 index, u32 *perm)
 		return -ENOSYS;
 }
 
+#if CONFIG_IS_ENABLED(DM_RNG)
 int tpm_get_random(struct udevice *dev, void *data, u32 count)
 {
+	int ret = -ENOSYS;
+	struct udevice *rng_dev;
+
 	if (tpm_is_v1(dev))
-		return tpm1_get_random(dev, data, count);
+		ret = uclass_get_device_by_driver(UCLASS_RNG,
+						  DM_DRIVER_GET(tpm1_rng),
+						  &rng_dev);
 	else if (tpm_is_v2(dev))
-		return -ENOSYS; /* not implemented yet */
-	else
-		return -ENOSYS;
+		ret = uclass_get_device_by_driver(UCLASS_RNG,
+						  DM_DRIVER_GET(tpm1_rng),
+						  &rng_dev);
+
+	if (ret) {
+		log_err("Getting tpm rng device failed\n");
+		return ret;
+	}
+
+	return dm_rng_read(rng_dev, data, (size_t)count);
 }
+#endif /* DM_RNG */
-- 
2.25.1

Re: [PATCH v3 3/8] tpm: rng: Add driver model interface for TPM RNG device

[Simon Glass]

Hi Sugosh,

On Fri, 4 Mar 2022 at 06:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> The TPM device has a builtin random number generator(RNG)
> functionality. Expose the RNG functions of the TPM device to the
> driver model so that they can be used by the EFI_RNG_PROTOCOL if the
> protocol is installed.
>
> Also change the function arguments and return type of the random
> number functions to comply with the driver model api.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>
> Changes since V2:
>
> * Export the existing tpm*_get_random functions to the driver model
>   instead of moving them to the drivers/rng/ directory.
>
>  include/tpm-v1.h |  4 ++--
>  include/tpm-v2.h |  4 ++--
>  lib/tpm-v1.c     | 28 ++++++++++++++++++++--------
>  lib/tpm-v2.c     | 21 ++++++++++++++++-----
>  lib/tpm_api.c    | 23 +++++++++++++++++++----
>  5 files changed, 59 insertions(+), 21 deletions(-)
>
> diff --git a/include/tpm-v1.h b/include/tpm-v1.h
> index 33d53fb695..d2ff8b446d 100644
> --- a/include/tpm-v1.h
> +++ b/include/tpm-v1.h
> @@ -555,9 +555,9 @@ u32 tpm1_find_key_sha1(struct udevice *dev, const u8 auth[20],
>   * @param dev          TPM device
>   * @param data         output buffer for the random bytes
>   * @param count                size of output buffer
> - * Return: return code of the operation
> + * Return: 0 if OK, -ve on error
>   */
> -u32 tpm1_get_random(struct udevice *dev, void *data, u32 count);
> +int tpm1_get_random(struct udevice *dev, void *data, size_t count);
>
>  /**
>   * tpm_finalise_physical_presence() - Finalise physical presence
> diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> index e79c90b939..4fb1e7a948 100644
> --- a/include/tpm-v2.h
> +++ b/include/tpm-v2.h
> @@ -619,9 +619,9 @@ u32 tpm2_pcr_setauthvalue(struct udevice *dev, const char *pw,
>   * @param data         output buffer for the random bytes
>   * @param count                size of output buffer
>   *
> - * Return: return code of the operation
> + * Return: 0 if OK, -ve on error
>   */
> -u32 tpm2_get_random(struct udevice *dev, void *data, u32 count);
> +int tpm2_get_random(struct udevice *dev, void *data, size_t count);
>
>  /**
>   * Lock data in the TPM
> diff --git a/lib/tpm-v1.c b/lib/tpm-v1.c
> index 22a769c587..57bb4a39cb 100644
> --- a/lib/tpm-v1.c
> +++ b/lib/tpm-v1.c
> @@ -9,12 +9,14 @@
>  #include <common.h>
>  #include <dm.h>
>  #include <log.h>
> -#include <asm/unaligned.h>
> -#include <u-boot/sha1.h>
> +#include <rng.h>
>  #include <tpm-common.h>
>  #include <tpm-v1.h>
>  #include "tpm-utils.h"
>
> +#include <asm/unaligned.h>
> +#include <u-boot/sha1.h>
> +
>  #ifdef CONFIG_TPM_AUTH_SESSIONS
>
>  #ifndef CONFIG_SHA1
> @@ -869,7 +871,7 @@ u32 tpm1_find_key_sha1(struct udevice *dev, const u8 auth[20],
>
>  #endif /* CONFIG_TPM_AUTH_SESSIONS */
>
> -u32 tpm1_get_random(struct udevice *dev, void *data, u32 count)
> +int tpm1_get_random(struct udevice *dev, void *data, size_t count)
>  {
>         const u8 command[14] = {
>                 0x0, 0xc1,              /* TPM_TAG */
> @@ -892,19 +894,19 @@ u32 tpm1_get_random(struct udevice *dev, void *data, u32 count)
>                 if (pack_byte_string(buf, sizeof(buf), "sd",
>                                      0, command, sizeof(command),
>                                      length_offset, this_bytes))
> -                       return TPM_LIB_ERROR;
> -               err = tpm_sendrecv_command(dev, buf, response,
> +                       return -EIO;
> +               err = tpm_sendrecv_command(dev->parent, buf, response,
>                                            &response_length);

Here it is a bit confused whether dev is the parent tpm device (as the
comment for this function says), or the random device.

>                 if (err)
>                         return err;
>                 if (unpack_byte_string(response, response_length, "d",
>                                        data_size_offset, &data_size))
> -                       return TPM_LIB_ERROR;
> +                       return -EIO;
>                 if (data_size > count)
> -                       return TPM_LIB_ERROR;
> +                       return -EIO;
>                 if (unpack_byte_string(response, response_length, "s",
>                                        data_offset, out, data_size))
> -                       return TPM_LIB_ERROR;
> +                       return -EIO;
>
>                 count -= data_size;
>                 out += data_size;
> @@ -912,3 +914,13 @@ u32 tpm1_get_random(struct udevice *dev, void *data, u32 count)
>
>         return 0;
>  }
> +
> +static const struct dm_rng_ops tpm1_rng_ops = {
> +       .read = tpm1_get_random,
> +};
> +
> +U_BOOT_DRIVER(tpm1_rng) = {
> +       .name   = "tpm1-rng",
> +       .id     = UCLASS_RNG,
> +       .ops    = &tpm1_rng_ops,
> +};

This declaration and the ops should go in the random driver. There
should be a op function in that driver which does the API call to the
TPM. Here you are duplicating this driver in two places.

Then you don't need to change the signature of tpm1_get_random().

> diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
> index 1bf627853a..18f64f0886 100644
> --- a/lib/tpm-v2.c
> +++ b/lib/tpm-v2.c
> @@ -6,6 +6,7 @@
>
>  #include <common.h>
>  #include <dm.h>
> +#include <rng.h>
>  #include <tpm-common.h>
>  #include <tpm-v2.h>
>  #include <linux/bitops.h>
> @@ -562,7 +563,7 @@ u32 tpm2_pcr_setauthvalue(struct udevice *dev, const char *pw,
>         return tpm_sendrecv_command(dev, command_v2, NULL, NULL);
>  }
>
> -u32 tpm2_get_random(struct udevice *dev, void *data, u32 count)
> +int tpm2_get_random(struct udevice *dev, void *data, size_t count)
>  {
>         const u8 command_v2[10] = {
>                 tpm_u16(TPM2_ST_NO_SESSIONS),
> @@ -585,19 +586,19 @@ u32 tpm2_get_random(struct udevice *dev, void *data, u32 count)
>                 if (pack_byte_string(buf, sizeof(buf), "sw",
>                                      0, command_v2, sizeof(command_v2),
>                                      sizeof(command_v2), this_bytes))
> -                       return TPM_LIB_ERROR;
> -               err = tpm_sendrecv_command(dev, buf, response,
> +                       return -EIO;
> +               err = tpm_sendrecv_command(dev->parent, buf, response,
>                                            &response_length);
>                 if (err)
>                         return err;
>                 if (unpack_byte_string(response, response_length, "w",
>                                        data_size_offset, &data_size))
> -                       return TPM_LIB_ERROR;
> +                       return -EIO;
>                 if (data_size > this_bytes)
>                         return TPM_LIB_ERROR;
>                 if (unpack_byte_string(response, response_length, "s",
>                                        data_offset, out, data_size))
> -                       return TPM_LIB_ERROR;
> +                       return -EIO;
>
>                 count -= data_size;
>                 out += data_size;
> @@ -669,3 +670,13 @@ u32 tpm2_submit_command(struct udevice *dev, const u8 *sendbuf,
>  {
>         return tpm_sendrecv_command(dev, sendbuf, recvbuf, recv_size);
>  }
> +
> +static const struct dm_rng_ops tpm2_rng_ops = {
> +       .read = tpm2_get_random,
> +};
> +
> +U_BOOT_DRIVER(tpm2_rng) = {
> +       .name   = "tpm2-rng",
> +       .id     = UCLASS_RNG,
> +       .ops    = &tpm2_rng_ops,
> +};

See above.

> diff --git a/lib/tpm_api.c b/lib/tpm_api.c
> index da48058abe..3584fda98c 100644
> --- a/lib/tpm_api.c
> +++ b/lib/tpm_api.c
> @@ -6,6 +6,7 @@
>  #include <common.h>
>  #include <dm.h>
>  #include <log.h>
> +#include <rng.h>
>  #include <tpm_api.h>
>  #include <tpm-v1.h>
>  #include <tpm-v2.h>
> @@ -265,12 +266,26 @@ u32 tpm_get_permissions(struct udevice *dev, u32 index, u32 *perm)
>                 return -ENOSYS;
>  }
>
> +#if CONFIG_IS_ENABLED(DM_RNG)
>  int tpm_get_random(struct udevice *dev, void *data, u32 count)
>  {
> +       int ret = -ENOSYS;
> +       struct udevice *rng_dev;
> +
>         if (tpm_is_v1(dev))
> -               return tpm1_get_random(dev, data, count);
> +               ret = uclass_get_device_by_driver(UCLASS_RNG,
> +                                                 DM_DRIVER_GET(tpm1_rng),
> +                                                 &rng_dev);

Er, tpm_get_random() should take a tpm device. The random device
should be handled by the caller, which should call
tpm_get_random(rand_dev->parent...


>         else if (tpm_is_v2(dev))
> -               return -ENOSYS; /* not implemented yet */
> -       else
> -               return -ENOSYS;
> +               ret = uclass_get_device_by_driver(UCLASS_RNG,
> +                                                 DM_DRIVER_GET(tpm1_rng),
> +                                                 &rng_dev);
> +
> +       if (ret) {
> +               log_err("Getting tpm rng device failed\n");
> +               return ret;
> +       }
> +
> +       return dm_rng_read(rng_dev, data, (size_t)count);
>  }
> +#endif /* DM_RNG */
> --
> 2.25.1
>

Regards,
Simon

Re: [PATCH v3 3/8] tpm: rng: Add driver model interface for TPM RNG device

[Sughosh Ganu]

hi Simon,

Thanks for looking into this. I now have a fair idea of the structure
that you are looking for this interface.

On Wed, 9 Mar 2022 at 08:05, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sugosh,
>
> On Fri, 4 Mar 2022 at 06:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > The TPM device has a builtin random number generator(RNG)
> > functionality. Expose the RNG functions of the TPM device to the
> > driver model so that they can be used by the EFI_RNG_PROTOCOL if the
> > protocol is installed.
> >
> > Also change the function arguments and return type of the random
> > number functions to comply with the driver model api.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >
> > Changes since V2:
> >
> > * Export the existing tpm*_get_random functions to the driver model
> >   instead of moving them to the drivers/rng/ directory.
> >
> >  include/tpm-v1.h |  4 ++--
> >  include/tpm-v2.h |  4 ++--
> >  lib/tpm-v1.c     | 28 ++++++++++++++++++++--------
> >  lib/tpm-v2.c     | 21 ++++++++++++++++-----
> >  lib/tpm_api.c    | 23 +++++++++++++++++++----
> >  5 files changed, 59 insertions(+), 21 deletions(-)
> >
> > diff --git a/include/tpm-v1.h b/include/tpm-v1.h
> > index 33d53fb695..d2ff8b446d 100644
> > --- a/include/tpm-v1.h
> > +++ b/include/tpm-v1.h
> > @@ -555,9 +555,9 @@ u32 tpm1_find_key_sha1(struct udevice *dev, const u8 auth[20],
> >   * @param dev          TPM device
> >   * @param data         output buffer for the random bytes
> >   * @param count                size of output buffer
> > - * Return: return code of the operation
> > + * Return: 0 if OK, -ve on error
> >   */
> > -u32 tpm1_get_random(struct udevice *dev, void *data, u32 count);
> > +int tpm1_get_random(struct udevice *dev, void *data, size_t count);
> >
> >  /**
> >   * tpm_finalise_physical_presence() - Finalise physical presence
> > diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> > index e79c90b939..4fb1e7a948 100644
> > --- a/include/tpm-v2.h
> > +++ b/include/tpm-v2.h
> > @@ -619,9 +619,9 @@ u32 tpm2_pcr_setauthvalue(struct udevice *dev, const char *pw,
> >   * @param data         output buffer for the random bytes
> >   * @param count                size of output buffer
> >   *
> > - * Return: return code of the operation
> > + * Return: 0 if OK, -ve on error
> >   */
> > -u32 tpm2_get_random(struct udevice *dev, void *data, u32 count);
> > +int tpm2_get_random(struct udevice *dev, void *data, size_t count);
> >
> >  /**
> >   * Lock data in the TPM
> > diff --git a/lib/tpm-v1.c b/lib/tpm-v1.c
> > index 22a769c587..57bb4a39cb 100644
> > --- a/lib/tpm-v1.c
> > +++ b/lib/tpm-v1.c
> > @@ -9,12 +9,14 @@
> >  #include <common.h>
> >  #include <dm.h>
> >  #include <log.h>
> > -#include <asm/unaligned.h>
> > -#include <u-boot/sha1.h>
> > +#include <rng.h>
> >  #include <tpm-common.h>
> >  #include <tpm-v1.h>
> >  #include "tpm-utils.h"
> >
> > +#include <asm/unaligned.h>
> > +#include <u-boot/sha1.h>
> > +
> >  #ifdef CONFIG_TPM_AUTH_SESSIONS
> >
> >  #ifndef CONFIG_SHA1
> > @@ -869,7 +871,7 @@ u32 tpm1_find_key_sha1(struct udevice *dev, const u8 auth[20],
> >
> >  #endif /* CONFIG_TPM_AUTH_SESSIONS */
> >
> > -u32 tpm1_get_random(struct udevice *dev, void *data, u32 count)
> > +int tpm1_get_random(struct udevice *dev, void *data, size_t count)
> >  {
> >         const u8 command[14] = {
> >                 0x0, 0xc1,              /* TPM_TAG */
> > @@ -892,19 +894,19 @@ u32 tpm1_get_random(struct udevice *dev, void *data, u32 count)
> >                 if (pack_byte_string(buf, sizeof(buf), "sd",
> >                                      0, command, sizeof(command),
> >                                      length_offset, this_bytes))
> > -                       return TPM_LIB_ERROR;
> > -               err = tpm_sendrecv_command(dev, buf, response,
> > +                       return -EIO;
> > +               err = tpm_sendrecv_command(dev->parent, buf, response,
> >                                            &response_length);
>
> Here it is a bit confused whether dev is the parent tpm device (as the
> comment for this function says), or the random device.

Yes, since this function is being exported as a rng uclass's read
callback, the first argument to this function will indeed be the rng
device. But this can be fixed with the suggestion that you have posted
below.

>
> >                 if (err)
> >                         return err;
> >                 if (unpack_byte_string(response, response_length, "d",
> >                                        data_size_offset, &data_size))
> > -                       return TPM_LIB_ERROR;
> > +                       return -EIO;
> >                 if (data_size > count)
> > -                       return TPM_LIB_ERROR;
> > +                       return -EIO;
> >                 if (unpack_byte_string(response, response_length, "s",
> >                                        data_offset, out, data_size))
> > -                       return TPM_LIB_ERROR;
> > +                       return -EIO;
> >
> >                 count -= data_size;
> >                 out += data_size;
> > @@ -912,3 +914,13 @@ u32 tpm1_get_random(struct udevice *dev, void *data, u32 count)
> >
> >         return 0;
> >  }
> > +
> > +static const struct dm_rng_ops tpm1_rng_ops = {
> > +       .read = tpm1_get_random,
> > +};
> > +
> > +U_BOOT_DRIVER(tpm1_rng) = {
> > +       .name   = "tpm1-rng",
> > +       .id     = UCLASS_RNG,
> > +       .ops    = &tpm1_rng_ops,
> > +};
>
> This declaration and the ops should go in the random driver. There
> should be a op function in that driver which does the API call to the
> TPM. Here you are duplicating this driver in two places.

Actually, I am not duplicating the driver, since I removed the driver
declaration from under drivers/rng/. But I will re-introduce that and
put an API call to tpm like you are suggesting.

>
> Then you don't need to change the signature of tpm1_get_random().
>
> > diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
> > index 1bf627853a..18f64f0886 100644
> > --- a/lib/tpm-v2.c
> > +++ b/lib/tpm-v2.c
> > @@ -6,6 +6,7 @@
> >
> >  #include <common.h>
> >  #include <dm.h>
> > +#include <rng.h>
> >  #include <tpm-common.h>
> >  #include <tpm-v2.h>
> >  #include <linux/bitops.h>
> > @@ -562,7 +563,7 @@ u32 tpm2_pcr_setauthvalue(struct udevice *dev, const char *pw,
> >         return tpm_sendrecv_command(dev, command_v2, NULL, NULL);
> >  }
> >
> > -u32 tpm2_get_random(struct udevice *dev, void *data, u32 count)
> > +int tpm2_get_random(struct udevice *dev, void *data, size_t count)
> >  {
> >         const u8 command_v2[10] = {
> >                 tpm_u16(TPM2_ST_NO_SESSIONS),
> > @@ -585,19 +586,19 @@ u32 tpm2_get_random(struct udevice *dev, void *data, u32 count)
> >                 if (pack_byte_string(buf, sizeof(buf), "sw",
> >                                      0, command_v2, sizeof(command_v2),
> >                                      sizeof(command_v2), this_bytes))
> > -                       return TPM_LIB_ERROR;
> > -               err = tpm_sendrecv_command(dev, buf, response,
> > +                       return -EIO;
> > +               err = tpm_sendrecv_command(dev->parent, buf, response,
> >                                            &response_length);
> >                 if (err)
> >                         return err;
> >                 if (unpack_byte_string(response, response_length, "w",
> >                                        data_size_offset, &data_size))
> > -                       return TPM_LIB_ERROR;
> > +                       return -EIO;
> >                 if (data_size > this_bytes)
> >                         return TPM_LIB_ERROR;
> >                 if (unpack_byte_string(response, response_length, "s",
> >                                        data_offset, out, data_size))
> > -                       return TPM_LIB_ERROR;
> > +                       return -EIO;
> >
> >                 count -= data_size;
> >                 out += data_size;
> > @@ -669,3 +670,13 @@ u32 tpm2_submit_command(struct udevice *dev, const u8 *sendbuf,
> >  {
> >         return tpm_sendrecv_command(dev, sendbuf, recvbuf, recv_size);
> >  }
> > +
> > +static const struct dm_rng_ops tpm2_rng_ops = {
> > +       .read = tpm2_get_random,
> > +};
> > +
> > +U_BOOT_DRIVER(tpm2_rng) = {
> > +       .name   = "tpm2-rng",
> > +       .id     = UCLASS_RNG,
> > +       .ops    = &tpm2_rng_ops,
> > +};
>
> See above.
>
> > diff --git a/lib/tpm_api.c b/lib/tpm_api.c
> > index da48058abe..3584fda98c 100644
> > --- a/lib/tpm_api.c
> > +++ b/lib/tpm_api.c
> > @@ -6,6 +6,7 @@
> >  #include <common.h>
> >  #include <dm.h>
> >  #include <log.h>
> > +#include <rng.h>
> >  #include <tpm_api.h>
> >  #include <tpm-v1.h>
> >  #include <tpm-v2.h>
> > @@ -265,12 +266,26 @@ u32 tpm_get_permissions(struct udevice *dev, u32 index, u32 *perm)
> >                 return -ENOSYS;
> >  }
> >
> > +#if CONFIG_IS_ENABLED(DM_RNG)
> >  int tpm_get_random(struct udevice *dev, void *data, u32 count)
> >  {
> > +       int ret = -ENOSYS;
> > +       struct udevice *rng_dev;
> > +
> >         if (tpm_is_v1(dev))
> > -               return tpm1_get_random(dev, data, count);
> > +               ret = uclass_get_device_by_driver(UCLASS_RNG,
> > +                                                 DM_DRIVER_GET(tpm1_rng),
> > +                                                 &rng_dev);
>
> Er, tpm_get_random() should take a tpm device. The random device
> should be handled by the caller, which should call
> tpm_get_random(rand_dev->parent...

Okay. I will make the changes as per your suggestion. Thanks for the
review of the patch.

-sughosh

>
>
> >         else if (tpm_is_v2(dev))
> > -               return -ENOSYS; /* not implemented yet */
> > -       else
> > -               return -ENOSYS;
> > +               ret = uclass_get_device_by_driver(UCLASS_RNG,
> > +                                                 DM_DRIVER_GET(tpm1_rng),
> > +                                                 &rng_dev);
> > +
> > +       if (ret) {
> > +               log_err("Getting tpm rng device failed\n");
> > +               return ret;
> > +       }
> > +
> > +       return dm_rng_read(rng_dev, data, (size_t)count);
> >  }
> > +#endif /* DM_RNG */
> > --
> > 2.25.1
> >
>
> Regards,
> Simon

Re: [PATCH v3 3/8] tpm: rng: Add driver model interface for TPM RNG device

[Sughosh Ganu]

hi Simon,

On Wed, 9 Mar 2022 at 11:30, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> Thanks for looking into this. I now have a fair idea of the structure
> that you are looking for this interface.
>
> On Wed, 9 Mar 2022 at 08:05, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sugosh,
> >
> > On Fri, 4 Mar 2022 at 06:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > The TPM device has a builtin random number generator(RNG)
> > > functionality. Expose the RNG functions of the TPM device to the
> > > driver model so that they can be used by the EFI_RNG_PROTOCOL if the
> > > protocol is installed.
> > >
> > > Also change the function arguments and return type of the random
> > > number functions to comply with the driver model api.
> > >
> > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > ---
> > >
> > > Changes since V2:
> > >
> > > * Export the existing tpm*_get_random functions to the driver model
> > >   instead of moving them to the drivers/rng/ directory.

<snip>

> > > diff --git a/lib/tpm_api.c b/lib/tpm_api.c
> > > index da48058abe..3584fda98c 100644
> > > --- a/lib/tpm_api.c
> > > +++ b/lib/tpm_api.c
> > > @@ -6,6 +6,7 @@
> > >  #include <common.h>
> > >  #include <dm.h>
> > >  #include <log.h>
> > > +#include <rng.h>
> > >  #include <tpm_api.h>
> > >  #include <tpm-v1.h>
> > >  #include <tpm-v2.h>
> > > @@ -265,12 +266,26 @@ u32 tpm_get_permissions(struct udevice *dev, u32 index, u32 *perm)
> > >                 return -ENOSYS;
> > >  }
> > >
> > > +#if CONFIG_IS_ENABLED(DM_RNG)
> > >  int tpm_get_random(struct udevice *dev, void *data, u32 count)
> > >  {
> > > +       int ret = -ENOSYS;
> > > +       struct udevice *rng_dev;
> > > +
> > >         if (tpm_is_v1(dev))
> > > -               return tpm1_get_random(dev, data, count);
> > > +               ret = uclass_get_device_by_driver(UCLASS_RNG,
> > > +                                                 DM_DRIVER_GET(tpm1_rng),
> > > +                                                 &rng_dev);
> >
> > Er, tpm_get_random() should take a tpm device. The random device
> > should be handled by the caller, which should call
> > tpm_get_random(rand_dev->parent...
>
> Okay. I will make the changes as per your suggestion. Thanks for the
> review of the patch.

Having had a relook at this, the tpm_get_random is indeed getting the
TPM device. Which is why the call to tpm_is_v1 is being called with
the same 'dev' argument. So I believe this function is currently as
per what you are looking for. Getting the TPM device as the first
argument.

-sughosh

Re: [PATCH v3 3/8] tpm: rng: Add driver model interface for TPM RNG device

[Simon Glass]

Hi Sughosh.

On Wed, 9 Mar 2022 at 04:18, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Wed, 9 Mar 2022 at 11:30, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > hi Simon,
> >
> > Thanks for looking into this. I now have a fair idea of the structure
> > that you are looking for this interface.
> >
> > On Wed, 9 Mar 2022 at 08:05, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Sugosh,
> > >
> > > On Fri, 4 Mar 2022 at 06:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > The TPM device has a builtin random number generator(RNG)
> > > > functionality. Expose the RNG functions of the TPM device to the
> > > > driver model so that they can be used by the EFI_RNG_PROTOCOL if the
> > > > protocol is installed.
> > > >
> > > > Also change the function arguments and return type of the random
> > > > number functions to comply with the driver model api.
> > > >
> > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > ---
> > > >
> > > > Changes since V2:
> > > >
> > > > * Export the existing tpm*_get_random functions to the driver model
> > > >   instead of moving them to the drivers/rng/ directory.
>
> <snip>
>
> > > > diff --git a/lib/tpm_api.c b/lib/tpm_api.c
> > > > index da48058abe..3584fda98c 100644
> > > > --- a/lib/tpm_api.c
> > > > +++ b/lib/tpm_api.c
> > > > @@ -6,6 +6,7 @@
> > > >  #include <common.h>
> > > >  #include <dm.h>
> > > >  #include <log.h>
> > > > +#include <rng.h>
> > > >  #include <tpm_api.h>
> > > >  #include <tpm-v1.h>
> > > >  #include <tpm-v2.h>
> > > > @@ -265,12 +266,26 @@ u32 tpm_get_permissions(struct udevice *dev, u32 index, u32 *perm)
> > > >                 return -ENOSYS;
> > > >  }
> > > >
> > > > +#if CONFIG_IS_ENABLED(DM_RNG)
> > > >  int tpm_get_random(struct udevice *dev, void *data, u32 count)
> > > >  {
> > > > +       int ret = -ENOSYS;
> > > > +       struct udevice *rng_dev;
> > > > +
> > > >         if (tpm_is_v1(dev))
> > > > -               return tpm1_get_random(dev, data, count);
> > > > +               ret = uclass_get_device_by_driver(UCLASS_RNG,
> > > > +                                                 DM_DRIVER_GET(tpm1_rng),
> > > > +                                                 &rng_dev);
> > >
> > > Er, tpm_get_random() should take a tpm device. The random device
> > > should be handled by the caller, which should call
> > > tpm_get_random(rand_dev->parent...
> >
> > Okay. I will make the changes as per your suggestion. Thanks for the
> > review of the patch.
>
> Having had a relook at this, the tpm_get_random is indeed getting the
> TPM device. Which is why the call to tpm_is_v1 is being called with
> the same 'dev' argument. So I believe this function is currently as
> per what you are looking for. Getting the TPM device as the first
> argument.

OK.

Regards,
Simon

[PATCH v3 4/8] tpm: Add the RNG child device

[Sughosh Ganu]

The TPM device comes with the random number generator(RNG)
functionality which is built into the TPM device. Add logic to add the
RNG child device in the TPM uclass post probe callback.

The RNG device can then be used to pass a set of random bytes to the
linux kernel, need for address space randomisation through the
EFI_RNG_PROTOCOL interface.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---

Changes since V2:
* Enable DM_RNG when CONFIG_TPM is enabled to build the RNG uclass
  code

 drivers/tpm/tpm-uclass.c | 60 +++++++++++++++++++++++++++++++++++++---
 lib/Kconfig              |  1 +
 2 files changed, 57 insertions(+), 4 deletions(-)

diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
index f67fe1019b..d1b9e0a757 100644
--- a/drivers/tpm/tpm-uclass.c
+++ b/drivers/tpm/tpm-uclass.c
@@ -11,10 +11,16 @@
 #include <log.h>
 #include <linux/delay.h>
 #include <linux/unaligned/be_byteshift.h>
+#include <tpm_api.h>
 #include <tpm-v1.h>
 #include <tpm-v2.h>
 #include "tpm_internal.h"
 
+#include <dm/lists.h>
+
+#define TPM_RNG1_DRV_NAME	"tpm1-rng"
+#define TPM_RNG2_DRV_NAME	"tpm2-rng"
+
 int tpm_open(struct udevice *dev)
 {
 	struct tpm_ops *ops = tpm_get_ops(dev);
@@ -136,12 +142,58 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_TPM)
+static int tpm_uclass_post_probe(struct udevice *dev)
+{
+	int ret;
+	const char *drv = tpm_is_v1(dev) ?
+		TPM_RNG1_DRV_NAME : TPM_RNG2_DRV_NAME;
+	struct udevice *child;
+
+	ret = device_bind_driver(dev, drv, "tpm-rng0", &child);
+	if (ret == -ENOENT) {
+		log_err("No driver configured for tpm-rng device\n");
+		return 0;
+	}
+
+	if (ret) {
+		log_err("Unable to bind rng driver with the tpm-rng device\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int tpm_uclass_child_pre_probe(struct udevice *dev)
+{
+	int ret;
+
+	ret = tpm_open(dev->parent);
+	if (ret == -EBUSY) {
+		log_info("TPM device already opened\n");
+	} else if (ret) {
+		log_err("Unable to open TPM device\n");
+		return ret;
+	}
+
+	ret = tpm_startup(dev->parent, TPM_ST_CLEAR);
+	if (ret)
+		log_err("Unable to start TPM device\n");
+
+	return ret;
+}
+#endif /* CONFIG_TPM */
+
 UCLASS_DRIVER(tpm) = {
-	.id		= UCLASS_TPM,
-	.name		= "tpm",
-	.flags		= DM_UC_FLAG_SEQ_ALIAS,
+	.id			= UCLASS_TPM,
+	.name			= "tpm",
+	.flags			= DM_UC_FLAG_SEQ_ALIAS,
 #if CONFIG_IS_ENABLED(OF_REAL)
-	.post_bind	= dm_scan_fdt_dev,
+	.post_bind		= dm_scan_fdt_dev,
+#endif
+#if IS_ENABLED(CONFIG_TPM)
+	.post_probe		= tpm_uclass_post_probe,
+	.child_pre_probe	= tpm_uclass_child_pre_probe,
 #endif
 	.per_device_auto	= sizeof(struct tpm_chip_priv),
 };
diff --git a/lib/Kconfig b/lib/Kconfig
index 3c6fa99b1a..0f05c97afc 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -341,6 +341,7 @@ source lib/crypt/Kconfig
 config TPM
 	bool "Trusted Platform Module (TPM) Support"
 	depends on DM
+	select DM_RNG
 	help
 	  This enables support for TPMs which can be used to provide security
 	  features for your board. The TPM can be connected via LPC or I2C
-- 
2.25.1

Re: [PATCH v3 4/8] tpm: Add the RNG child device

[Simon Glass]

Hi,

On Fri, 4 Mar 2022 at 06:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> The TPM device comes with the random number generator(RNG)
> functionality which is built into the TPM device. Add logic to add the
> RNG child device in the TPM uclass post probe callback.
>
> The RNG device can then be used to pass a set of random bytes to the
> linux kernel, need for address space randomisation through the
> EFI_RNG_PROTOCOL interface.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>
> Changes since V2:
> * Enable DM_RNG when CONFIG_TPM is enabled to build the RNG uclass
>   code
>
>  drivers/tpm/tpm-uclass.c | 60 +++++++++++++++++++++++++++++++++++++---
>  lib/Kconfig              |  1 +
>  2 files changed, 57 insertions(+), 4 deletions(-)

No new comments from last time, still needs to be addressed.

>
> diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> index f67fe1019b..d1b9e0a757 100644
> --- a/drivers/tpm/tpm-uclass.c
> +++ b/drivers/tpm/tpm-uclass.c
> @@ -11,10 +11,16 @@
>  #include <log.h>
>  #include <linux/delay.h>
>  #include <linux/unaligned/be_byteshift.h>
> +#include <tpm_api.h>
>  #include <tpm-v1.h>
>  #include <tpm-v2.h>
>  #include "tpm_internal.h"
>
> +#include <dm/lists.h>
> +
> +#define TPM_RNG1_DRV_NAME      "tpm1-rng"
> +#define TPM_RNG2_DRV_NAME      "tpm2-rng"
> +
>  int tpm_open(struct udevice *dev)
>  {
>         struct tpm_ops *ops = tpm_get_ops(dev);
> @@ -136,12 +142,58 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
>         return 0;
>  }
>
> +#if IS_ENABLED(CONFIG_TPM)
> +static int tpm_uclass_post_probe(struct udevice *dev)
> +{
> +       int ret;
> +       const char *drv = tpm_is_v1(dev) ?
> +               TPM_RNG1_DRV_NAME : TPM_RNG2_DRV_NAME;
> +       struct udevice *child;
> +
> +       ret = device_bind_driver(dev, drv, "tpm-rng0", &child);
> +       if (ret == -ENOENT) {
> +               log_err("No driver configured for tpm-rng device\n");
> +               return 0;
> +       }
> +
> +       if (ret) {
> +               log_err("Unable to bind rng driver with the tpm-rng device\n");
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static int tpm_uclass_child_pre_probe(struct udevice *dev)
> +{
> +       int ret;
> +
> +       ret = tpm_open(dev->parent);
> +       if (ret == -EBUSY) {
> +               log_info("TPM device already opened\n");
> +       } else if (ret) {
> +               log_err("Unable to open TPM device\n");
> +               return ret;
> +       }
> +
> +       ret = tpm_startup(dev->parent, TPM_ST_CLEAR);
> +       if (ret)
> +               log_err("Unable to start TPM device\n");
> +
> +       return ret;
> +}
> +#endif /* CONFIG_TPM */
> +
>  UCLASS_DRIVER(tpm) = {
> -       .id             = UCLASS_TPM,
> -       .name           = "tpm",
> -       .flags          = DM_UC_FLAG_SEQ_ALIAS,
> +       .id                     = UCLASS_TPM,
> +       .name                   = "tpm",
> +       .flags                  = DM_UC_FLAG_SEQ_ALIAS,
>  #if CONFIG_IS_ENABLED(OF_REAL)
> -       .post_bind      = dm_scan_fdt_dev,
> +       .post_bind              = dm_scan_fdt_dev,
> +#endif
> +#if IS_ENABLED(CONFIG_TPM)
> +       .post_probe             = tpm_uclass_post_probe,
> +       .child_pre_probe        = tpm_uclass_child_pre_probe,
>  #endif
>         .per_device_auto        = sizeof(struct tpm_chip_priv),
>  };
> diff --git a/lib/Kconfig b/lib/Kconfig
> index 3c6fa99b1a..0f05c97afc 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -341,6 +341,7 @@ source lib/crypt/Kconfig
>  config TPM
>         bool "Trusted Platform Module (TPM) Support"
>         depends on DM
> +       select DM_RNG
>         help
>           This enables support for TPMs which can be used to provide security
>           features for your board. The TPM can be connected via LPC or I2C
> --
> 2.25.1
>

Regards,
Simon

Re: [PATCH v3 4/8] tpm: Add the RNG child device

[Sughosh Ganu]

hi Simon,

On Wed, 9 Mar 2022 at 08:05, Simon Glass <sjg@chromium.org> wrote:
>
> Hi,
>
> On Fri, 4 Mar 2022 at 06:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > The TPM device comes with the random number generator(RNG)
> > functionality which is built into the TPM device. Add logic to add the
> > RNG child device in the TPM uclass post probe callback.
> >
> > The RNG device can then be used to pass a set of random bytes to the
> > linux kernel, need for address space randomisation through the
> > EFI_RNG_PROTOCOL interface.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >
> > Changes since V2:
> > * Enable DM_RNG when CONFIG_TPM is enabled to build the RNG uclass
> >   code
> >
> >  drivers/tpm/tpm-uclass.c | 60 +++++++++++++++++++++++++++++++++++++---
> >  lib/Kconfig              |  1 +
> >  2 files changed, 57 insertions(+), 4 deletions(-)
>
> No new comments from last time, still needs to be addressed.

Like I mentioned in the discussion on this patch, I will remove the
child_pre_probe callback, which was starting the TPM device. I will
keep the addition of the RNG child device only for the u-boot proper
stage, using the CONFIG_SPL_BUILD and CONFIG_TPL_BUILD guards.

-sughosh

>
> >
> > diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> > index f67fe1019b..d1b9e0a757 100644
> > --- a/drivers/tpm/tpm-uclass.c
> > +++ b/drivers/tpm/tpm-uclass.c
> > @@ -11,10 +11,16 @@
> >  #include <log.h>
> >  #include <linux/delay.h>
> >  #include <linux/unaligned/be_byteshift.h>
> > +#include <tpm_api.h>
> >  #include <tpm-v1.h>
> >  #include <tpm-v2.h>
> >  #include "tpm_internal.h"
> >
> > +#include <dm/lists.h>
> > +
> > +#define TPM_RNG1_DRV_NAME      "tpm1-rng"
> > +#define TPM_RNG2_DRV_NAME      "tpm2-rng"
> > +
> >  int tpm_open(struct udevice *dev)
> >  {
> >         struct tpm_ops *ops = tpm_get_ops(dev);
> > @@ -136,12 +142,58 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
> >         return 0;
> >  }
> >
> > +#if IS_ENABLED(CONFIG_TPM)
> > +static int tpm_uclass_post_probe(struct udevice *dev)
> > +{
> > +       int ret;
> > +       const char *drv = tpm_is_v1(dev) ?
> > +               TPM_RNG1_DRV_NAME : TPM_RNG2_DRV_NAME;
> > +       struct udevice *child;
> > +
> > +       ret = device_bind_driver(dev, drv, "tpm-rng0", &child);
> > +       if (ret == -ENOENT) {
> > +               log_err("No driver configured for tpm-rng device\n");
> > +               return 0;
> > +       }
> > +
> > +       if (ret) {
> > +               log_err("Unable to bind rng driver with the tpm-rng device\n");
> > +               return ret;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int tpm_uclass_child_pre_probe(struct udevice *dev)
> > +{
> > +       int ret;
> > +
> > +       ret = tpm_open(dev->parent);
> > +       if (ret == -EBUSY) {
> > +               log_info("TPM device already opened\n");
> > +       } else if (ret) {
> > +               log_err("Unable to open TPM device\n");
> > +               return ret;
> > +       }
> > +
> > +       ret = tpm_startup(dev->parent, TPM_ST_CLEAR);
> > +       if (ret)
> > +               log_err("Unable to start TPM device\n");
> > +
> > +       return ret;
> > +}
> > +#endif /* CONFIG_TPM */
> > +
> >  UCLASS_DRIVER(tpm) = {
> > -       .id             = UCLASS_TPM,
> > -       .name           = "tpm",
> > -       .flags          = DM_UC_FLAG_SEQ_ALIAS,
> > +       .id                     = UCLASS_TPM,
> > +       .name                   = "tpm",
> > +       .flags                  = DM_UC_FLAG_SEQ_ALIAS,
> >  #if CONFIG_IS_ENABLED(OF_REAL)
> > -       .post_bind      = dm_scan_fdt_dev,
> > +       .post_bind              = dm_scan_fdt_dev,
> > +#endif
> > +#if IS_ENABLED(CONFIG_TPM)
> > +       .post_probe             = tpm_uclass_post_probe,
> > +       .child_pre_probe        = tpm_uclass_child_pre_probe,
> >  #endif
> >         .per_device_auto        = sizeof(struct tpm_chip_priv),
> >  };
> > diff --git a/lib/Kconfig b/lib/Kconfig
> > index 3c6fa99b1a..0f05c97afc 100644
> > --- a/lib/Kconfig
> > +++ b/lib/Kconfig
> > @@ -341,6 +341,7 @@ source lib/crypt/Kconfig
> >  config TPM
> >         bool "Trusted Platform Module (TPM) Support"
> >         depends on DM
> > +       select DM_RNG
> >         help
> >           This enables support for TPMs which can be used to provide security
> >           features for your board. The TPM can be connected via LPC or I2C
> > --
> > 2.25.1
> >
>
> Regards,
> Simon

[PATCH v3 5/8] qemu: arm: Remove platform specific function to get RNG device

[Sughosh Ganu]

The Qemu platform has a function defined to get the random number
generator(RNG) device. However, the RNG device can be obtained simply
by searching for a device belonging to the RNG uclass. Remove the
superfluous platform function defined for the Qemu platform for
getting the RNG device.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
Tested-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---

Changes since V2: None

 board/emulation/qemu-arm/qemu-arm.c | 42 -----------------------------
 1 file changed, 42 deletions(-)

diff --git a/board/emulation/qemu-arm/qemu-arm.c b/board/emulation/qemu-arm/qemu-arm.c
index 16d5a97167..c9e886e44a 100644
--- a/board/emulation/qemu-arm/qemu-arm.c
+++ b/board/emulation/qemu-arm/qemu-arm.c
@@ -107,48 +107,6 @@ void enable_caches(void)
 	 dcache_enable();
 }
 
-#if defined(CONFIG_EFI_RNG_PROTOCOL)
-#include <efi_loader.h>
-#include <efi_rng.h>
-
-#include <dm/device-internal.h>
-
-efi_status_t platform_get_rng_device(struct udevice **dev)
-{
-	int ret;
-	efi_status_t status = EFI_DEVICE_ERROR;
-	struct udevice *bus, *devp;
-
-	for (uclass_first_device(UCLASS_VIRTIO, &bus); bus;
-	     uclass_next_device(&bus)) {
-		for (device_find_first_child(bus, &devp); devp;
-		     device_find_next_child(&devp)) {
-			if (device_get_uclass_id(devp) == UCLASS_RNG) {
-				*dev = devp;
-				status = EFI_SUCCESS;
-				break;
-			}
-		}
-	}
-
-	if (status != EFI_SUCCESS) {
-		debug("No rng device found\n");
-		return EFI_DEVICE_ERROR;
-	}
-
-	if (*dev) {
-		ret = device_probe(*dev);
-		if (ret)
-			return EFI_DEVICE_ERROR;
-	} else {
-		debug("Couldn't get child device\n");
-		return EFI_DEVICE_ERROR;
-	}
-
-	return EFI_SUCCESS;
-}
-#endif /* CONFIG_EFI_RNG_PROTOCOL */
-
 #ifdef CONFIG_ARM64
 #define __W	"w"
 #else
-- 
2.25.1

Re: [PATCH v3 5/8] qemu: arm: Remove platform specific function to get RNG device

[Simon Glass]

On Fri, 4 Mar 2022 at 06:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> The Qemu platform has a function defined to get the random number
> generator(RNG) device. However, the RNG device can be obtained simply
> by searching for a device belonging to the RNG uclass. Remove the
> superfluous platform function defined for the Qemu platform for
> getting the RNG device.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> Tested-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>
> Changes since V2: None
>
>  board/emulation/qemu-arm/qemu-arm.c | 42 -----------------------------
>  1 file changed, 42 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v3 5/8] qemu: arm: Remove platform specific function to get RNG device

[Ilias Apalodimas]

On Wed, 9 Mar 2022 at 04:35, Simon Glass <sjg@chromium.org> wrote:
>
> On Fri, 4 Mar 2022 at 06:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > The Qemu platform has a function defined to get the random number
> > generator(RNG) device. However, the RNG device can be obtained simply
> > by searching for a device belonging to the RNG uclass. Remove the
> > superfluous platform function defined for the Qemu platform for
> > getting the RNG device.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > Tested-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > ---
> >
> > Changes since V2: None
> >
> >  board/emulation/qemu-arm/qemu-arm.c | 42 -----------------------------
> >  1 file changed, 42 deletions(-)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

[PATCH v3 6/8] cmd: rng: Add support for selecting RNG device

[Sughosh Ganu]

The 'rng' u-boot command is used for printing a select number of
random bytes on the console. Currently, the RNG device from which the
random bytes are read is fixed. However, a platform can have multiple
RNG devices, one example being qemu, which has a virtio RNG device and
the RNG pseudo device through the TPM chip.

Extend the 'rng' command so that the user can provide the RNG device
number from which the random bytes are to be read. This will be the
device index under the RNG uclass.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
Tested-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---

Changes since V2: None

 cmd/rng.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/cmd/rng.c b/cmd/rng.c
index 1ad5a096c0..bb89cfa784 100644
--- a/cmd/rng.c
+++ b/cmd/rng.c
@@ -13,19 +13,34 @@
 
 static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
-	size_t n = 0x40;
+	size_t n;
 	struct udevice *dev;
 	void *buf;
+	int devnum;
 	int ret = CMD_RET_SUCCESS;
 
-	if (uclass_get_device(UCLASS_RNG, 0, &dev) || !dev) {
+	switch (argc) {
+	case 1:
+		devnum = 0;
+		n = 0x40;
+		break;
+	case 2:
+		devnum = hextoul(argv[1], NULL);
+		n = 0x40;
+		break;
+	case 3:
+		devnum = hextoul(argv[1], NULL);
+		n = hextoul(argv[2], NULL);
+		break;
+	default:
+		return CMD_RET_USAGE;
+	}
+
+	if (uclass_get_device(UCLASS_RNG, devnum, &dev) || !dev) {
 		printf("No RNG device\n");
 		return CMD_RET_FAILURE;
 	}
 
-	if (argc >= 2)
-		n = hextoul(argv[1], NULL);
-
 	buf = malloc(n);
 	if (!buf) {
 		printf("Out of memory\n");
@@ -46,12 +61,12 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 
 #ifdef CONFIG_SYS_LONGHELP
 static char rng_help_text[] =
-	"[n]\n"
-	"  - print n random bytes\n";
+	"[dev [n]]\n"
+	"  - print n random bytes read from dev\n";
 #endif
 
 U_BOOT_CMD(
-	rng, 2, 0, do_rng,
+	rng, 3, 0, do_rng,
 	"print bytes from the hardware random number generator",
 	rng_help_text
 );
-- 
2.25.1

Re: [PATCH v3 6/8] cmd: rng: Add support for selecting RNG device

[Simon Glass]

Hi Sughosh,

On Fri, 4 Mar 2022 at 06:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> The 'rng' u-boot command is used for printing a select number of
> random bytes on the console. Currently, the RNG device from which the
> random bytes are read is fixed. However, a platform can have multiple
> RNG devices, one example being qemu, which has a virtio RNG device and
> the RNG pseudo device through the TPM chip.
>
> Extend the 'rng' command so that the user can provide the RNG device
> number from which the random bytes are to be read. This will be the
> device index under the RNG uclass.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> Tested-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>
> Changes since V2: None
>
>  cmd/rng.c | 31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)

Please add docs to doc/ and a test for the command.

Regards,
Simon

Re: [PATCH v3 6/8] cmd: rng: Add support for selecting RNG device

[Sughosh Ganu]

hi Simon,

On Wed, 9 Mar 2022 at 08:05, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Fri, 4 Mar 2022 at 06:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > The 'rng' u-boot command is used for printing a select number of
> > random bytes on the console. Currently, the RNG device from which the
> > random bytes are read is fixed. However, a platform can have multiple
> > RNG devices, one example being qemu, which has a virtio RNG device and
> > the RNG pseudo device through the TPM chip.
> >
> > Extend the 'rng' command so that the user can provide the RNG device
> > number from which the random bytes are to be read. This will be the
> > device index under the RNG uclass.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > Tested-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > ---
> >
> > Changes since V2: None
> >
> >  cmd/rng.c | 31 +++++++++++++++++++++++--------
> >  1 file changed, 23 insertions(+), 8 deletions(-)
>
> Please add docs to doc/ and a test for the command.

That is being done in patches 7 and 8 of this series.

-sughosh

>
> Regards,
> Simon

Re: [PATCH v3 6/8] cmd: rng: Add support for selecting RNG device

[Ilias Apalodimas]

On Fri, Mar 04, 2022 at 07:04:27PM +0530, Sughosh Ganu wrote:
> The 'rng' u-boot command is used for printing a select number of
> random bytes on the console. Currently, the RNG device from which the
> random bytes are read is fixed. However, a platform can have multiple
> RNG devices, one example being qemu, which has a virtio RNG device and
> the RNG pseudo device through the TPM chip.
> 
> Extend the 'rng' command so that the user can provide the RNG device
> number from which the random bytes are to be read. This will be the
> device index under the RNG uclass.
> 
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> Tested-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> 
> Changes since V2: None
> 
>  cmd/rng.c | 31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/cmd/rng.c b/cmd/rng.c
> index 1ad5a096c0..bb89cfa784 100644
> --- a/cmd/rng.c
> +++ b/cmd/rng.c
> @@ -13,19 +13,34 @@
>  
>  static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>  {
> -	size_t n = 0x40;
> +	size_t n;
>  	struct udevice *dev;
>  	void *buf;
> +	int devnum;
>  	int ret = CMD_RET_SUCCESS;
>  
> -	if (uclass_get_device(UCLASS_RNG, 0, &dev) || !dev) {
> +	switch (argc) {
> +	case 1:
> +		devnum = 0;
> +		n = 0x40;
> +		break;
> +	case 2:
> +		devnum = hextoul(argv[1], NULL);
> +		n = 0x40;
> +		break;
> +	case 3:
> +		devnum = hextoul(argv[1], NULL);
> +		n = hextoul(argv[2], NULL);
> +		break;
> +	default:
> +		return CMD_RET_USAGE;
> +	}
> +
> +	if (uclass_get_device(UCLASS_RNG, devnum, &dev) || !dev) {
>  		printf("No RNG device\n");
>  		return CMD_RET_FAILURE;
>  	}
>  
> -	if (argc >= 2)
> -		n = hextoul(argv[1], NULL);
> -
>  	buf = malloc(n);
>  	if (!buf) {
>  		printf("Out of memory\n");
> @@ -46,12 +61,12 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>  
>  #ifdef CONFIG_SYS_LONGHELP
>  static char rng_help_text[] =
> -	"[n]\n"
> -	"  - print n random bytes\n";
> +	"[dev [n]]\n"
> +	"  - print n random bytes read from dev\n";
>  #endif
>  
>  U_BOOT_CMD(
> -	rng, 2, 0, do_rng,
> +	rng, 3, 0, do_rng,
>  	"print bytes from the hardware random number generator",
>  	rng_help_text
>  );
> -- 
> 2.25.1
> 
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH v3 6/8] cmd: rng: Add support for selecting RNG device

[Simon Glass]

Hi Sugosh,

On Fri, 4 Mar 2022 at 06:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> The 'rng' u-boot command is used for printing a select number of
> random bytes on the console. Currently, the RNG device from which the
> random bytes are read is fixed. However, a platform can have multiple
> RNG devices, one example being qemu, which has a virtio RNG device and
> the RNG pseudo device through the TPM chip.
>
> Extend the 'rng' command so that the user can provide the RNG device
> number from which the random bytes are to be read. This will be the
> device index under the RNG uclass.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> Tested-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>
> Changes since V2: None
>
>  cmd/rng.c | 31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/cmd/rng.c b/cmd/rng.c
> index 1ad5a096c0..bb89cfa784 100644
> --- a/cmd/rng.c
> +++ b/cmd/rng.c
> @@ -13,19 +13,34 @@
>
>  static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>  {
> -       size_t n = 0x40;
> +       size_t n;
>         struct udevice *dev;
>         void *buf;
> +       int devnum;
>         int ret = CMD_RET_SUCCESS;
>
> -       if (uclass_get_device(UCLASS_RNG, 0, &dev) || !dev) {
> +       switch (argc) {
> +       case 1:
> +               devnum = 0;
> +               n = 0x40;
> +               break;
> +       case 2:
> +               devnum = hextoul(argv[1], NULL);
> +               n = 0x40;
> +               break;
> +       case 3:
> +               devnum = hextoul(argv[1], NULL);
> +               n = hextoul(argv[2], NULL);
> +               break;
> +       default:
> +               return CMD_RET_USAGE;
> +       }
> +
> +       if (uclass_get_device(UCLASS_RNG, devnum, &dev) || !dev) {

Devices are numbered by aliases, so you should use
uclass_get_device_by_seq() here.

>                 printf("No RNG device\n");
>                 return CMD_RET_FAILURE;
>         }
>
> -       if (argc >= 2)
> -               n = hextoul(argv[1], NULL);
> -
>         buf = malloc(n);

No need to malloc(), just set a limit for (say 64) bytes. See how
cmd_mem.c does it.

>         if (!buf) {
>                 printf("Out of memory\n");
> @@ -46,12 +61,12 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>
>  #ifdef CONFIG_SYS_LONGHELP
>  static char rng_help_text[] =
> -       "[n]\n"
> -       "  - print n random bytes\n";
> +       "[dev [n]]\n"
> +       "  - print n random bytes read from dev\n";
>  #endif
>
>  U_BOOT_CMD(
> -       rng, 2, 0, do_rng,
> +       rng, 3, 0, do_rng,
>         "print bytes from the hardware random number generator",
>         rng_help_text
>  );
> --
> 2.25.1
>

Regards,
Simon

Re: [PATCH v3 6/8] cmd: rng: Add support for selecting RNG device

[Sughosh Ganu]

hi Simon,

On Wed, 9 Mar 2022 at 21:02, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sugosh,
>
> On Fri, 4 Mar 2022 at 06:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > The 'rng' u-boot command is used for printing a select number of
> > random bytes on the console. Currently, the RNG device from which the
> > random bytes are read is fixed. However, a platform can have multiple
> > RNG devices, one example being qemu, which has a virtio RNG device and
> > the RNG pseudo device through the TPM chip.
> >
> > Extend the 'rng' command so that the user can provide the RNG device
> > number from which the random bytes are to be read. This will be the
> > device index under the RNG uclass.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > Tested-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > ---
> >
> > Changes since V2: None
> >
> >  cmd/rng.c | 31 +++++++++++++++++++++++--------
> >  1 file changed, 23 insertions(+), 8 deletions(-)
> >
> > diff --git a/cmd/rng.c b/cmd/rng.c
> > index 1ad5a096c0..bb89cfa784 100644
> > --- a/cmd/rng.c
> > +++ b/cmd/rng.c
> > @@ -13,19 +13,34 @@
> >
> >  static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> >  {
> > -       size_t n = 0x40;
> > +       size_t n;
> >         struct udevice *dev;
> >         void *buf;
> > +       int devnum;
> >         int ret = CMD_RET_SUCCESS;
> >
> > -       if (uclass_get_device(UCLASS_RNG, 0, &dev) || !dev) {
> > +       switch (argc) {
> > +       case 1:
> > +               devnum = 0;
> > +               n = 0x40;
> > +               break;
> > +       case 2:
> > +               devnum = hextoul(argv[1], NULL);
> > +               n = 0x40;
> > +               break;
> > +       case 3:
> > +               devnum = hextoul(argv[1], NULL);
> > +               n = hextoul(argv[2], NULL);
> > +               break;
> > +       default:
> > +               return CMD_RET_USAGE;
> > +       }
> > +
> > +       if (uclass_get_device(UCLASS_RNG, devnum, &dev) || !dev) {
>
> Devices are numbered by aliases, so you should use
> uclass_get_device_by_seq() here.
>
> >                 printf("No RNG device\n");
> >                 return CMD_RET_FAILURE;
> >         }
> >
> > -       if (argc >= 2)
> > -               n = hextoul(argv[1], NULL);
> > -
> >         buf = malloc(n);
>
> No need to malloc(), just set a limit for (say 64) bytes. See how
> cmd_mem.c does it.

These changes were not made as part of this patch. This is already
existing code. I will make the changes that you suggest nonetheless.
Btw, can you please take a look at the v4 patchset for this series.
Thanks.

-sughosh

>
> >         if (!buf) {
> >                 printf("Out of memory\n");
> > @@ -46,12 +61,12 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> >
> >  #ifdef CONFIG_SYS_LONGHELP
> >  static char rng_help_text[] =
> > -       "[n]\n"
> > -       "  - print n random bytes\n";
> > +       "[dev [n]]\n"
> > +       "  - print n random bytes read from dev\n";
> >  #endif
> >
> >  U_BOOT_CMD(
> > -       rng, 2, 0, do_rng,
> > +       rng, 3, 0, do_rng,
> >         "print bytes from the hardware random number generator",
> >         rng_help_text
> >  );
> > --
> > 2.25.1
> >
>
> Regards,
> Simon

Re: [PATCH v3 6/8] cmd: rng: Add support for selecting RNG device

[Simon Glass]

Hi Sughosh,

On Thu, 10 Mar 2022 at 05:43, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Wed, 9 Mar 2022 at 21:02, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sugosh,
> >
> > On Fri, 4 Mar 2022 at 06:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > The 'rng' u-boot command is used for printing a select number of
> > > random bytes on the console. Currently, the RNG device from which the
> > > random bytes are read is fixed. However, a platform can have multiple
> > > RNG devices, one example being qemu, which has a virtio RNG device and
> > > the RNG pseudo device through the TPM chip.
> > >
> > > Extend the 'rng' command so that the user can provide the RNG device
> > > number from which the random bytes are to be read. This will be the
> > > device index under the RNG uclass.
> > >
> > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > Tested-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > ---
> > >
> > > Changes since V2: None
> > >
> > >  cmd/rng.c | 31 +++++++++++++++++++++++--------
> > >  1 file changed, 23 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/cmd/rng.c b/cmd/rng.c
> > > index 1ad5a096c0..bb89cfa784 100644
> > > --- a/cmd/rng.c
> > > +++ b/cmd/rng.c
> > > @@ -13,19 +13,34 @@
> > >
> > >  static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> > >  {
> > > -       size_t n = 0x40;
> > > +       size_t n;
> > >         struct udevice *dev;
> > >         void *buf;
> > > +       int devnum;
> > >         int ret = CMD_RET_SUCCESS;
> > >
> > > -       if (uclass_get_device(UCLASS_RNG, 0, &dev) || !dev) {
> > > +       switch (argc) {
> > > +       case 1:
> > > +               devnum = 0;
> > > +               n = 0x40;
> > > +               break;
> > > +       case 2:
> > > +               devnum = hextoul(argv[1], NULL);
> > > +               n = 0x40;
> > > +               break;
> > > +       case 3:
> > > +               devnum = hextoul(argv[1], NULL);
> > > +               n = hextoul(argv[2], NULL);
> > > +               break;
> > > +       default:
> > > +               return CMD_RET_USAGE;
> > > +       }
> > > +
> > > +       if (uclass_get_device(UCLASS_RNG, devnum, &dev) || !dev) {
> >
> > Devices are numbered by aliases, so you should use
> > uclass_get_device_by_seq() here.
> >
> > >                 printf("No RNG device\n");
> > >                 return CMD_RET_FAILURE;
> > >         }
> > >
> > > -       if (argc >= 2)
> > > -               n = hextoul(argv[1], NULL);
> > > -
> > >         buf = malloc(n);
> >
> > No need to malloc(), just set a limit for (say 64) bytes. See how
> > cmd_mem.c does it.
>
> These changes were not made as part of this patch. This is already
> existing code. I will make the changes that you suggest nonetheless.
> Btw, can you please take a look at the v4 patchset for this series.
> Thanks.

Ah I see, then you could do it as another patch.

Regards,
Simon

[PATCH v3 7/8] doc: rng: Add documentation for the rng command

[Sughosh Ganu]

Add a usage document for the 'rng' u-boot command.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---

Changes since V2: None

 doc/usage/index.rst |  1 +
 doc/usage/rng.rst   | 25 +++++++++++++++++++++++++
 2 files changed, 26 insertions(+)
 create mode 100644 doc/usage/rng.rst

diff --git a/doc/usage/index.rst b/doc/usage/index.rst
index 0aacf531b2..5712a924ae 100644
--- a/doc/usage/index.rst
+++ b/doc/usage/index.rst
@@ -45,6 +45,7 @@ Shell commands
    pstore
    qfw
    reset
+   rng
    sbi
    sf
    scp03
diff --git a/doc/usage/rng.rst b/doc/usage/rng.rst
new file mode 100644
index 0000000000..87758f7d66
--- /dev/null
+++ b/doc/usage/rng.rst
@@ -0,0 +1,25 @@
+.. SPDX-License-Identifier: GPL-2.0+
+
+rng command
+===========
+
+Synopsis
+--------
+
+::
+
+    rng [devnum [n]]
+
+Description
+-----------
+
+The *rng* command reads the random number generator(RNG) device and
+prints the random bytes read on the console.
+
+devnum
+    The RNG device from which the random bytes are to be
+    read. Defaults to 0.
+
+n
+    Number of random bytes to be read and displayed on the
+    console. Default value is 0x40.
-- 
2.25.1

Re: [PATCH v3 7/8] doc: rng: Add documentation for the rng command

[Simon Glass]

On Fri, 4 Mar 2022 at 06:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> Add a usage document for the 'rng' u-boot command.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>
> Changes since V2: None
>
>  doc/usage/index.rst |  1 +
>  doc/usage/rng.rst   | 25 +++++++++++++++++++++++++
>  2 files changed, 26 insertions(+)
>  create mode 100644 doc/usage/rng.rst

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v3 7/8] doc: rng: Add documentation for the rng command

[Ilias Apalodimas]

On Fri, Mar 04, 2022 at 07:04:28PM +0530, Sughosh Ganu wrote:
> Add a usage document for the 'rng' u-boot command.
> 
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
> 
> Changes since V2: None
> 
>  doc/usage/index.rst |  1 +
>  doc/usage/rng.rst   | 25 +++++++++++++++++++++++++
>  2 files changed, 26 insertions(+)
>  create mode 100644 doc/usage/rng.rst
> 
> diff --git a/doc/usage/index.rst b/doc/usage/index.rst
> index 0aacf531b2..5712a924ae 100644
> --- a/doc/usage/index.rst
> +++ b/doc/usage/index.rst
> @@ -45,6 +45,7 @@ Shell commands
>     pstore
>     qfw
>     reset
> +   rng
>     sbi
>     sf
>     scp03
> diff --git a/doc/usage/rng.rst b/doc/usage/rng.rst
> new file mode 100644
> index 0000000000..87758f7d66
> --- /dev/null
> +++ b/doc/usage/rng.rst
> @@ -0,0 +1,25 @@
> +.. SPDX-License-Identifier: GPL-2.0+
> +
> +rng command
> +===========
> +
> +Synopsis
> +--------
> +
> +::
> +
> +    rng [devnum [n]]
> +
> +Description
> +-----------
> +
> +The *rng* command reads the random number generator(RNG) device and
> +prints the random bytes read on the console.
> +
> +devnum
> +    The RNG device from which the random bytes are to be
> +    read. Defaults to 0.
> +
> +n
> +    Number of random bytes to be read and displayed on the
> +    console. Default value is 0x40.
> -- 
> 2.25.1
> 
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

[PATCH v3 8/8] test: rng: Add a UT testcase for the rng command

[Sughosh Ganu]

The 'rng' command dumps a number of random bytes on the console. Add a
set of tests for the 'rng' command. The test function performs basic
sanity testing of the command.

Since a unit test is being added for the command, enable it by default
in the sandbox platforms.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---

Changes since V2: None

 cmd/Kconfig   |  1 +
 test/dm/rng.c | 29 +++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 5e25e45fd2..47f1e23ef0 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1810,6 +1810,7 @@ config CMD_GETTIME
 config CMD_RNG
 	bool "rng command"
 	depends on DM_RNG
+	default y if SANDBOX
 	select HEXDUMP
 	help
 	  Print bytes from the hardware random number generator.
diff --git a/test/dm/rng.c b/test/dm/rng.c
index 5b34c93ed6..6d1f68848d 100644
--- a/test/dm/rng.c
+++ b/test/dm/rng.c
@@ -25,3 +25,32 @@ static int dm_test_rng_read(struct unit_test_state *uts)
 	return 0;
 }
 DM_TEST(dm_test_rng_read, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
+
+/* Test the rng command */
+static int dm_test_rng_cmd(struct unit_test_state *uts)
+{
+	struct udevice *dev;
+
+	ut_assertok(uclass_get_device(UCLASS_RNG, 0, &dev));
+	ut_assertnonnull(dev);
+
+	ut_assertok(console_record_reset_enable());
+
+	run_command("rng", 0);
+	ut_assert_nextlinen("00000000:");
+	ut_assert_nextlinen("00000010:");
+	ut_assert_nextlinen("00000020:");
+	ut_assert_nextlinen("00000030:");
+	ut_assert_console_end();
+
+	run_command("rng 0 10", 0);
+	ut_assert_nextlinen("00000000:");
+	ut_assert_console_end();
+
+	run_command("rng 20", 0);
+	ut_assert_nextlinen("No RNG device");
+	ut_assert_console_end();
+
+	return 0;
+}
+DM_TEST(dm_test_rng_cmd, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT | UT_TESTF_CONSOLE_REC);
-- 
2.25.1

Re: [PATCH v3 8/8] test: rng: Add a UT testcase for the rng command

[Simon Glass]

Hi Sughosh,

On Fri, 4 Mar 2022 at 06:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> The 'rng' command dumps a number of random bytes on the console. Add a
> set of tests for the 'rng' command. The test function performs basic
> sanity testing of the command.
>
> Since a unit test is being added for the command, enable it by default
> in the sandbox platforms.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>
> Changes since V2: None
>
>  cmd/Kconfig   |  1 +
>  test/dm/rng.c | 29 +++++++++++++++++++++++++++++
>  2 files changed, 30 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 5e25e45fd2..47f1e23ef0 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -1810,6 +1810,7 @@ config CMD_GETTIME
>  config CMD_RNG
>         bool "rng command"
>         depends on DM_RNG
> +       default y if SANDBOX
>         select HEXDUMP
>         help
>           Print bytes from the hardware random number generator.
> diff --git a/test/dm/rng.c b/test/dm/rng.c
> index 5b34c93ed6..6d1f68848d 100644
> --- a/test/dm/rng.c
> +++ b/test/dm/rng.c
> @@ -25,3 +25,32 @@ static int dm_test_rng_read(struct unit_test_state *uts)
>         return 0;
>  }
>  DM_TEST(dm_test_rng_read, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
> +
> +/* Test the rng command */
> +static int dm_test_rng_cmd(struct unit_test_state *uts)
> +{
> +       struct udevice *dev;
> +
> +       ut_assertok(uclass_get_device(UCLASS_RNG, 0, &dev));
> +       ut_assertnonnull(dev);
> +
> +       ut_assertok(console_record_reset_enable());
> +
> +       run_command("rng", 0);
> +       ut_assert_nextlinen("00000000:");
> +       ut_assert_nextlinen("00000010:");
> +       ut_assert_nextlinen("00000020:");
> +       ut_assert_nextlinen("00000030:");

This is good enough for testing, except that here you don't have any
actual data. How come?

> +       ut_assert_console_end();
> +
> +       run_command("rng 0 10", 0);
> +       ut_assert_nextlinen("00000000:");
> +       ut_assert_console_end();
> +
> +       run_command("rng 20", 0);
> +       ut_assert_nextlinen("No RNG device");
> +       ut_assert_console_end();
> +
> +       return 0;
> +}
> +DM_TEST(dm_test_rng_cmd, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT | UT_TESTF_CONSOLE_REC);
> --
> 2.25.1
>

Regards,
SImon

Re: [PATCH v3 8/8] test: rng: Add a UT testcase for the rng command

[Sughosh Ganu]

hi Simon,

On Wed, 9 Mar 2022 at 08:06, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Fri, 4 Mar 2022 at 06:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > The 'rng' command dumps a number of random bytes on the console. Add a
> > set of tests for the 'rng' command. The test function performs basic
> > sanity testing of the command.
> >
> > Since a unit test is being added for the command, enable it by default
> > in the sandbox platforms.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >
> > Changes since V2: None
> >
> >  cmd/Kconfig   |  1 +
> >  test/dm/rng.c | 29 +++++++++++++++++++++++++++++
> >  2 files changed, 30 insertions(+)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> >
> > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > index 5e25e45fd2..47f1e23ef0 100644
> > --- a/cmd/Kconfig
> > +++ b/cmd/Kconfig
> > @@ -1810,6 +1810,7 @@ config CMD_GETTIME
> >  config CMD_RNG
> >         bool "rng command"
> >         depends on DM_RNG
> > +       default y if SANDBOX
> >         select HEXDUMP
> >         help
> >           Print bytes from the hardware random number generator.
> > diff --git a/test/dm/rng.c b/test/dm/rng.c
> > index 5b34c93ed6..6d1f68848d 100644
> > --- a/test/dm/rng.c
> > +++ b/test/dm/rng.c
> > @@ -25,3 +25,32 @@ static int dm_test_rng_read(struct unit_test_state *uts)
> >         return 0;
> >  }
> >  DM_TEST(dm_test_rng_read, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
> > +
> > +/* Test the rng command */
> > +static int dm_test_rng_cmd(struct unit_test_state *uts)
> > +{
> > +       struct udevice *dev;
> > +
> > +       ut_assertok(uclass_get_device(UCLASS_RNG, 0, &dev));
> > +       ut_assertnonnull(dev);
> > +
> > +       ut_assertok(console_record_reset_enable());
> > +
> > +       run_command("rng", 0);
> > +       ut_assert_nextlinen("00000000:");
> > +       ut_assert_nextlinen("00000010:");
> > +       ut_assert_nextlinen("00000020:");
> > +       ut_assert_nextlinen("00000030:");
>
> This is good enough for testing, except that here you don't have any
> actual data. How come?

This being a test for a random number device, we cannot anticipate
what data we will be getting. The sandbox RNG driver does not return a
fixed set of values on each invocation of the command.

-sughosh

>
> > +       ut_assert_console_end();
> > +
> > +       run_command("rng 0 10", 0);
> > +       ut_assert_nextlinen("00000000:");
> > +       ut_assert_console_end();
> > +
> > +       run_command("rng 20", 0);
> > +       ut_assert_nextlinen("No RNG device");
> > +       ut_assert_console_end();
> > +
> > +       return 0;
> > +}
> > +DM_TEST(dm_test_rng_cmd, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT | UT_TESTF_CONSOLE_REC);
> > --
> > 2.25.1
> >
>
> Regards,
> SImon

Re: [PATCH v3 8/8] test: rng: Add a UT testcase for the rng command

[Simon Glass]

Hi Sughosh,

On Tue, 8 Mar 2022 at 23:18, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Wed, 9 Mar 2022 at 08:06, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Fri, 4 Mar 2022 at 06:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > The 'rng' command dumps a number of random bytes on the console. Add a
> > > set of tests for the 'rng' command. The test function performs basic
> > > sanity testing of the command.
> > >
> > > Since a unit test is being added for the command, enable it by default
> > > in the sandbox platforms.
> > >
> > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > ---
> > >
> > > Changes since V2: None
> > >
> > >  cmd/Kconfig   |  1 +
> > >  test/dm/rng.c | 29 +++++++++++++++++++++++++++++
> > >  2 files changed, 30 insertions(+)
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > >
> > > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > > index 5e25e45fd2..47f1e23ef0 100644
> > > --- a/cmd/Kconfig
> > > +++ b/cmd/Kconfig
> > > @@ -1810,6 +1810,7 @@ config CMD_GETTIME
> > >  config CMD_RNG
> > >         bool "rng command"
> > >         depends on DM_RNG
> > > +       default y if SANDBOX
> > >         select HEXDUMP
> > >         help
> > >           Print bytes from the hardware random number generator.
> > > diff --git a/test/dm/rng.c b/test/dm/rng.c
> > > index 5b34c93ed6..6d1f68848d 100644
> > > --- a/test/dm/rng.c
> > > +++ b/test/dm/rng.c
> > > @@ -25,3 +25,32 @@ static int dm_test_rng_read(struct unit_test_state *uts)
> > >         return 0;
> > >  }
> > >  DM_TEST(dm_test_rng_read, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
> > > +
> > > +/* Test the rng command */
> > > +static int dm_test_rng_cmd(struct unit_test_state *uts)
> > > +{
> > > +       struct udevice *dev;
> > > +
> > > +       ut_assertok(uclass_get_device(UCLASS_RNG, 0, &dev));
> > > +       ut_assertnonnull(dev);
> > > +
> > > +       ut_assertok(console_record_reset_enable());
> > > +
> > > +       run_command("rng", 0);
> > > +       ut_assert_nextlinen("00000000:");
> > > +       ut_assert_nextlinen("00000010:");
> > > +       ut_assert_nextlinen("00000020:");
> > > +       ut_assert_nextlinen("00000030:");
> >
> > This is good enough for testing, except that here you don't have any
> > actual data. How come?
>
> This being a test for a random number device, we cannot anticipate
> what data we will be getting. The sandbox RNG driver does not return a
> fixed set of values on each invocation of the command.

How come? Can you fix that by setting the random seed at the start of the test?

Regards,
Simon



>
> -sughosh
>
> >
> > > +       ut_assert_console_end();
> > > +
> > > +       run_command("rng 0 10", 0);
> > > +       ut_assert_nextlinen("00000000:");
> > > +       ut_assert_console_end();
> > > +
> > > +       run_command("rng 20", 0);
> > > +       ut_assert_nextlinen("No RNG device");
> > > +       ut_assert_console_end();
> > > +
> > > +       return 0;
> > > +}
> > > +DM_TEST(dm_test_rng_cmd, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT | UT_TESTF_CONSOLE_REC);
> > > --
> > > 2.25.1
> > >
> >
> > Regards,
> > SImon

Re: [PATCH v3 8/8] test: rng: Add a UT testcase for the rng command

[Sughosh Ganu]

hi Simon,

On Sat, 12 Mar 2022 at 08:14, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Tue, 8 Mar 2022 at 23:18, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Wed, 9 Mar 2022 at 08:06, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Fri, 4 Mar 2022 at 06:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > The 'rng' command dumps a number of random bytes on the console. Add a
> > > > set of tests for the 'rng' command. The test function performs basic
> > > > sanity testing of the command.
> > > >
> > > > Since a unit test is being added for the command, enable it by default
> > > > in the sandbox platforms.
> > > >
> > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > ---
> > > >
> > > > Changes since V2: None
> > > >
> > > >  cmd/Kconfig   |  1 +
> > > >  test/dm/rng.c | 29 +++++++++++++++++++++++++++++
> > > >  2 files changed, 30 insertions(+)
> > >
> > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > >
> > > >
> > > > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > > > index 5e25e45fd2..47f1e23ef0 100644
> > > > --- a/cmd/Kconfig
> > > > +++ b/cmd/Kconfig
> > > > @@ -1810,6 +1810,7 @@ config CMD_GETTIME
> > > >  config CMD_RNG
> > > >         bool "rng command"
> > > >         depends on DM_RNG
> > > > +       default y if SANDBOX
> > > >         select HEXDUMP
> > > >         help
> > > >           Print bytes from the hardware random number generator.
> > > > diff --git a/test/dm/rng.c b/test/dm/rng.c
> > > > index 5b34c93ed6..6d1f68848d 100644
> > > > --- a/test/dm/rng.c
> > > > +++ b/test/dm/rng.c
> > > > @@ -25,3 +25,32 @@ static int dm_test_rng_read(struct unit_test_state *uts)
> > > >         return 0;
> > > >  }
> > > >  DM_TEST(dm_test_rng_read, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
> > > > +
> > > > +/* Test the rng command */
> > > > +static int dm_test_rng_cmd(struct unit_test_state *uts)
> > > > +{
> > > > +       struct udevice *dev;
> > > > +
> > > > +       ut_assertok(uclass_get_device(UCLASS_RNG, 0, &dev));
> > > > +       ut_assertnonnull(dev);
> > > > +
> > > > +       ut_assertok(console_record_reset_enable());
> > > > +
> > > > +       run_command("rng", 0);
> > > > +       ut_assert_nextlinen("00000000:");
> > > > +       ut_assert_nextlinen("00000010:");
> > > > +       ut_assert_nextlinen("00000020:");
> > > > +       ut_assert_nextlinen("00000030:");
> > >
> > > This is good enough for testing, except that here you don't have any
> > > actual data. How come?
> >
> > This being a test for a random number device, we cannot anticipate
> > what data we will be getting. The sandbox RNG driver does not return a
> > fixed set of values on each invocation of the command.
>
> How come? Can you fix that by setting the random seed at the start of the test?

Well I can, but do we really need to do that. I think the current test
that we have for the rng command and the uclass is fine.

-sughosh

>
> Regards,
> Simon
>
>
>
> >
> > -sughosh
> >
> > >
> > > > +       ut_assert_console_end();
> > > > +
> > > > +       run_command("rng 0 10", 0);
> > > > +       ut_assert_nextlinen("00000000:");
> > > > +       ut_assert_console_end();
> > > > +
> > > > +       run_command("rng 20", 0);
> > > > +       ut_assert_nextlinen("No RNG device");
> > > > +       ut_assert_console_end();
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +DM_TEST(dm_test_rng_cmd, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT | UT_TESTF_CONSOLE_REC);
> > > > --
> > > > 2.25.1
> > > >
> > >
> > > Regards,
> > > SImon

Re: [PATCH v3 8/8] test: rng: Add a UT testcase for the rng command

[Simon Glass]

Hi Sughosh,

On Sun, 13 Mar 2022 at 05:08, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Sat, 12 Mar 2022 at 08:14, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Tue, 8 Mar 2022 at 23:18, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > hi Simon,
> > >
> > > On Wed, 9 Mar 2022 at 08:06, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Fri, 4 Mar 2022 at 06:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > >
> > > > > The 'rng' command dumps a number of random bytes on the console. Add a
> > > > > set of tests for the 'rng' command. The test function performs basic
> > > > > sanity testing of the command.
> > > > >
> > > > > Since a unit test is being added for the command, enable it by default
> > > > > in the sandbox platforms.
> > > > >
> > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > ---
> > > > >
> > > > > Changes since V2: None
> > > > >
> > > > >  cmd/Kconfig   |  1 +
> > > > >  test/dm/rng.c | 29 +++++++++++++++++++++++++++++
> > > > >  2 files changed, 30 insertions(+)
> > > >
> > > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > > >
> > > > >
> > > > > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > > > > index 5e25e45fd2..47f1e23ef0 100644
> > > > > --- a/cmd/Kconfig
> > > > > +++ b/cmd/Kconfig
> > > > > @@ -1810,6 +1810,7 @@ config CMD_GETTIME
> > > > >  config CMD_RNG
> > > > >         bool "rng command"
> > > > >         depends on DM_RNG
> > > > > +       default y if SANDBOX
> > > > >         select HEXDUMP
> > > > >         help
> > > > >           Print bytes from the hardware random number generator.
> > > > > diff --git a/test/dm/rng.c b/test/dm/rng.c
> > > > > index 5b34c93ed6..6d1f68848d 100644
> > > > > --- a/test/dm/rng.c
> > > > > +++ b/test/dm/rng.c
> > > > > @@ -25,3 +25,32 @@ static int dm_test_rng_read(struct unit_test_state *uts)
> > > > >         return 0;
> > > > >  }
> > > > >  DM_TEST(dm_test_rng_read, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
> > > > > +
> > > > > +/* Test the rng command */
> > > > > +static int dm_test_rng_cmd(struct unit_test_state *uts)
> > > > > +{
> > > > > +       struct udevice *dev;
> > > > > +
> > > > > +       ut_assertok(uclass_get_device(UCLASS_RNG, 0, &dev));
> > > > > +       ut_assertnonnull(dev);
> > > > > +
> > > > > +       ut_assertok(console_record_reset_enable());
> > > > > +
> > > > > +       run_command("rng", 0);
> > > > > +       ut_assert_nextlinen("00000000:");
> > > > > +       ut_assert_nextlinen("00000010:");
> > > > > +       ut_assert_nextlinen("00000020:");
> > > > > +       ut_assert_nextlinen("00000030:");
> > > >
> > > > This is good enough for testing, except that here you don't have any
> > > > actual data. How come?
> > >
> > > This being a test for a random number device, we cannot anticipate
> > > what data we will be getting. The sandbox RNG driver does not return a
> > > fixed set of values on each invocation of the command.
> >
> > How come? Can you fix that by setting the random seed at the start of the test?
>
> Well I can, but do we really need to do that. I think the current test
> that we have for the rng command and the uclass is fine.

It is OK, but not great, since if there is a bug in the data output or
in the collection of the random data, then you will not find it with
this test.

Regards,
Simon

Re: [PATCH v3 8/8] test: rng: Add a UT testcase for the rng command

[Sughosh Ganu]

hi Heinrich,

On Fri, 4 Mar 2022 at 19:05, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> The 'rng' command dumps a number of random bytes on the console. Add a
> set of tests for the 'rng' command. The test function performs basic
> sanity testing of the command.
>
> Since a unit test is being added for the command, enable it by default
> in the sandbox platforms.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>
> Changes since V2: None
>
>  cmd/Kconfig   |  1 +
>  test/dm/rng.c | 29 +++++++++++++++++++++++++++++
>  2 files changed, 30 insertions(+)

Can you please pick this up, as this is unrelated to the TPM RNG
changes which are still under discussion. Thanks.

-sughosh

>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 5e25e45fd2..47f1e23ef0 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -1810,6 +1810,7 @@ config CMD_GETTIME
>  config CMD_RNG
>         bool "rng command"
>         depends on DM_RNG
> +       default y if SANDBOX
>         select HEXDUMP
>         help
>           Print bytes from the hardware random number generator.
> diff --git a/test/dm/rng.c b/test/dm/rng.c
> index 5b34c93ed6..6d1f68848d 100644
> --- a/test/dm/rng.c
> +++ b/test/dm/rng.c
> @@ -25,3 +25,32 @@ static int dm_test_rng_read(struct unit_test_state *uts)
>         return 0;
>  }
>  DM_TEST(dm_test_rng_read, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
> +
> +/* Test the rng command */
> +static int dm_test_rng_cmd(struct unit_test_state *uts)
> +{
> +       struct udevice *dev;
> +
> +       ut_assertok(uclass_get_device(UCLASS_RNG, 0, &dev));
> +       ut_assertnonnull(dev);
> +
> +       ut_assertok(console_record_reset_enable());
> +
> +       run_command("rng", 0);
> +       ut_assert_nextlinen("00000000:");
> +       ut_assert_nextlinen("00000010:");
> +       ut_assert_nextlinen("00000020:");
> +       ut_assert_nextlinen("00000030:");
> +       ut_assert_console_end();
> +
> +       run_command("rng 0 10", 0);
> +       ut_assert_nextlinen("00000000:");
> +       ut_assert_console_end();
> +
> +       run_command("rng 20", 0);
> +       ut_assert_nextlinen("No RNG device");
> +       ut_assert_console_end();
> +
> +       return 0;
> +}
> +DM_TEST(dm_test_rng_cmd, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT | UT_TESTF_CONSOLE_REC);
> --
> 2.25.1
>