Re: [PATCH v5 7/9] cmd: rng: Use a statically allocated array for random bytes
hi Heinrich,
On Sun, 13 Mar 2022 at 20:19, Sughosh Ganu wrote:
>
> Use a statically allocated buffer on stack instead of using malloc for
> reading the random bytes. Using a local array is faster than
> allocating heap memory on every initiation of the command.
>
> Signed-off-by: Sughosh Ganu
> ---
>
> Changes since V4:
>
> * New patch based on review comments from Simon to not use the malloc
> call
>
> cmd/rng.c | 17 +++--
> 1 file changed, 7 insertions(+), 10 deletions(-)
Can you please pick this up, as this has been reviewed and is not
related to the TPM RNG changes which are still under discussion.
Thanks.
-sughosh
>
> diff --git a/cmd/rng.c b/cmd/rng.c
> index 2ddf27545f..81a23964b8 100644
> --- a/cmd/rng.c
> +++ b/cmd/rng.c
> @@ -14,9 +14,9 @@
> static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const
> argv[])
> {
> size_t n;
> - struct udevice *dev;
> - void *buf;
> + u8 buf[64];
> int devnum;
> + struct udevice *dev;
> int ret = CMD_RET_SUCCESS;
>
> switch (argc) {
> @@ -41,11 +41,10 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int
> argc, char *const argv[])
> return CMD_RET_FAILURE;
> }
>
> - buf = malloc(n);
> - if (!buf) {
> - printf("Out of memory\n");
> - return CMD_RET_FAILURE;
> - }
> + if (!n)
> + return 0;
> +
> + n = min(n, sizeof(buf));
>
> if (dm_rng_read(dev, buf, n)) {
> printf("Reading RNG failed\n");
> @@ -54,15 +53,13 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int
> argc, char *const argv[])
> print_hex_dump_bytes("", DUMP_PREFIX_OFFSET, buf, n);
> }
>
> - free(buf);
> -
> return ret;
> }
>
> #ifdef CONFIG_SYS_LONGHELP
> static char rng_help_text[] =
> "[dev [n]]\n"
> - " - print n random bytes read from dev\n";
> + " - print n random bytes(max 64) read from dev\n";
> #endif
>
> U_BOOT_CMD(
> --
> 2.25.1
>
Re: [PATCH v5 7/9] cmd: rng: Use a statically allocated array for random bytes
Hi Sughosh,
On Mon, 14 Mar 2022 at 05:27, Sughosh Ganu wrote:
>
> hi Simon,
>
> On Mon, 14 Mar 2022 at 03:53, Simon Glass wrote:
> >
> > Hi Sughosh,
> >
> > On Sun, 13 Mar 2022 at 08:49, Sughosh Ganu wrote:
> > >
> > > Use a statically allocated buffer on stack instead of using malloc for
> > > reading the random bytes. Using a local array is faster than
> > > allocating heap memory on every initiation of the command.
> > >
> > > Signed-off-by: Sughosh Ganu
> > > ---
> > >
> > > Changes since V4:
> > >
> > > * New patch based on review comments from Simon to not use the malloc
> > > call
> > >
> > > cmd/rng.c | 17 +++--
> > > 1 file changed, 7 insertions(+), 10 deletions(-)
> >
> > It might be easier to put this patch before the other one.
> >
> > >
> > > diff --git a/cmd/rng.c b/cmd/rng.c
> > > index 2ddf27545f..81a23964b8 100644
> > > --- a/cmd/rng.c
> > > +++ b/cmd/rng.c
> > > @@ -14,9 +14,9 @@
> > > static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const
> > > argv[])
> > > {
> > > size_t n;
> > > - struct udevice *dev;
> > > - void *buf;
> > > + u8 buf[64];
> > > int devnum;
> > > + struct udevice *dev;
> > > int ret = CMD_RET_SUCCESS;
> > >
> > > switch (argc) {
> > > @@ -41,11 +41,10 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag,
> > > int argc, char *const argv[])
> > > return CMD_RET_FAILURE;
> > > }
> > >
> > > - buf = malloc(n);
> > > - if (!buf) {
> > > - printf("Out of memory\n");
> > > - return CMD_RET_FAILURE;
> > > - }
> > > + if (!n)
> > > + return 0;
> > > +
> > > + n = min(n, sizeof(buf));
> > >
> > > if (dm_rng_read(dev, buf, n)) {
> > > printf("Reading RNG failed\n");
> >
> > This looks like a Windows-style error. How about adding "(err=%d)",
> > ret to this so we can see the error?
>
> Again, this is not being changed by my patch. This is existing code
> which is not being touched by the patch. These kinds of fixes should
> be taken up separately. Thanks.
Ah yes I keep missing that.
- Simon
[..]
Re: [PATCH v5 7/9] cmd: rng: Use a statically allocated array for random bytes
hi Simon,
On Mon, 14 Mar 2022 at 03:53, Simon Glass wrote:
>
> Hi Sughosh,
>
> On Sun, 13 Mar 2022 at 08:49, Sughosh Ganu wrote:
> >
> > Use a statically allocated buffer on stack instead of using malloc for
> > reading the random bytes. Using a local array is faster than
> > allocating heap memory on every initiation of the command.
> >
> > Signed-off-by: Sughosh Ganu
> > ---
> >
> > Changes since V4:
> >
> > * New patch based on review comments from Simon to not use the malloc
> > call
> >
> > cmd/rng.c | 17 +++--
> > 1 file changed, 7 insertions(+), 10 deletions(-)
>
> It might be easier to put this patch before the other one.
>
> >
> > diff --git a/cmd/rng.c b/cmd/rng.c
> > index 2ddf27545f..81a23964b8 100644
> > --- a/cmd/rng.c
> > +++ b/cmd/rng.c
> > @@ -14,9 +14,9 @@
> > static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const
> > argv[])
> > {
> > size_t n;
> > - struct udevice *dev;
> > - void *buf;
> > + u8 buf[64];
> > int devnum;
> > + struct udevice *dev;
> > int ret = CMD_RET_SUCCESS;
> >
> > switch (argc) {
> > @@ -41,11 +41,10 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int
> > argc, char *const argv[])
> > return CMD_RET_FAILURE;
> > }
> >
> > - buf = malloc(n);
> > - if (!buf) {
> > - printf("Out of memory\n");
> > - return CMD_RET_FAILURE;
> > - }
> > + if (!n)
> > + return 0;
> > +
> > + n = min(n, sizeof(buf));
> >
> > if (dm_rng_read(dev, buf, n)) {
> > printf("Reading RNG failed\n");
>
> This looks like a Windows-style error. How about adding "(err=%d)",
> ret to this so we can see the error?
Again, this is not being changed by my patch. This is existing code
which is not being touched by the patch. These kinds of fixes should
be taken up separately. Thanks.
-sughosh
>
> > @@ -54,15 +53,13 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int
> > argc, char *const argv[])
> > print_hex_dump_bytes("", DUMP_PREFIX_OFFSET, buf, n);
> > }
> >
> > - free(buf);
> > -
> > return ret;
> > }
> >
> > #ifdef CONFIG_SYS_LONGHELP
> > static char rng_help_text[] =
> > "[dev [n]]\n"
> > - " - print n random bytes read from dev\n";
> > + " - print n random bytes(max 64) read from dev\n";
> > #endif
> >
> > U_BOOT_CMD(
> > --
> > 2.25.1
> >
>
> Regards,
> SImon
Re: [PATCH v5 7/9] cmd: rng: Use a statically allocated array for random bytes
Hi Sughosh,
On Sun, 13 Mar 2022 at 08:49, Sughosh Ganu wrote:
>
> Use a statically allocated buffer on stack instead of using malloc for
> reading the random bytes. Using a local array is faster than
> allocating heap memory on every initiation of the command.
>
> Signed-off-by: Sughosh Ganu
> ---
>
> Changes since V4:
>
> * New patch based on review comments from Simon to not use the malloc
> call
>
> cmd/rng.c | 17 +++--
> 1 file changed, 7 insertions(+), 10 deletions(-)
It might be easier to put this patch before the other one.
>
> diff --git a/cmd/rng.c b/cmd/rng.c
> index 2ddf27545f..81a23964b8 100644
> --- a/cmd/rng.c
> +++ b/cmd/rng.c
> @@ -14,9 +14,9 @@
> static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const
> argv[])
> {
> size_t n;
> - struct udevice *dev;
> - void *buf;
> + u8 buf[64];
> int devnum;
> + struct udevice *dev;
> int ret = CMD_RET_SUCCESS;
>
> switch (argc) {
> @@ -41,11 +41,10 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int
> argc, char *const argv[])
> return CMD_RET_FAILURE;
> }
>
> - buf = malloc(n);
> - if (!buf) {
> - printf("Out of memory\n");
> - return CMD_RET_FAILURE;
> - }
> + if (!n)
> + return 0;
> +
> + n = min(n, sizeof(buf));
>
> if (dm_rng_read(dev, buf, n)) {
> printf("Reading RNG failed\n");
This looks like a Windows-style error. How about adding "(err=%d)",
ret to this so we can see the error?
> @@ -54,15 +53,13 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int
> argc, char *const argv[])
> print_hex_dump_bytes("", DUMP_PREFIX_OFFSET, buf, n);
> }
>
> - free(buf);
> -
> return ret;
> }
>
> #ifdef CONFIG_SYS_LONGHELP
> static char rng_help_text[] =
> "[dev [n]]\n"
> - " - print n random bytes read from dev\n";
> + " - print n random bytes(max 64) read from dev\n";
> #endif
>
> U_BOOT_CMD(
> --
> 2.25.1
>
Regards,
SImon
[PATCH v5 7/9] cmd: rng: Use a statically allocated array for random bytes
Use a statically allocated buffer on stack instead of using malloc for
reading the random bytes. Using a local array is faster than
allocating heap memory on every initiation of the command.
Signed-off-by: Sughosh Ganu
---
Changes since V4:
* New patch based on review comments from Simon to not use the malloc
call
cmd/rng.c | 17 +++--
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/cmd/rng.c b/cmd/rng.c
index 2ddf27545f..81a23964b8 100644
--- a/cmd/rng.c
+++ b/cmd/rng.c
@@ -14,9 +14,9 @@
static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const
argv[])
{
size_t n;
- struct udevice *dev;
- void *buf;
+ u8 buf[64];
int devnum;
+ struct udevice *dev;
int ret = CMD_RET_SUCCESS;
switch (argc) {
@@ -41,11 +41,10 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int
argc, char *const argv[])
return CMD_RET_FAILURE;
}
- buf = malloc(n);
- if (!buf) {
- printf("Out of memory\n");
- return CMD_RET_FAILURE;
- }
+ if (!n)
+ return 0;
+
+ n = min(n, sizeof(buf));
if (dm_rng_read(dev, buf, n)) {
printf("Reading RNG failed\n");
@@ -54,15 +53,13 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int
argc, char *const argv[])
print_hex_dump_bytes("", DUMP_PREFIX_OFFSET, buf, n);
}
- free(buf);
-
return ret;
}
#ifdef CONFIG_SYS_LONGHELP
static char rng_help_text[] =
"[dev [n]]\n"
- " - print n random bytes read from dev\n";
+ " - print n random bytes(max 64) read from dev\n";
#endif
U_BOOT_CMD(
--
2.25.1
