[PATCH 0/3] Assorted fixes related to reserved memory

[PATCH 0/3] Assorted fixes related to reserved memory

[Atish Patra]

This series has few small assorted fixes related to reserved memory support
in RISC-V and EFI.

The series is rebased on top of the following series
http://patchwork.ozlabs.org/project/uboot/patch/1591767391-2669-2-git-send-email-bmeng.cn at gmail.com/

Changes from v1->v2:
1. Rebased on top of the Bin's series. Dropped the fix generic fdtdec code.
2. Added bootefi fix.

ption-prefix PATCH v2

Atish Patra (3):
riscv: Do not return error if reserved node already exists
riscv: Use optimized version of fdtdec_get_addr_size_no_parent
cmd: bootefi: Honor the address & size cells properties correctly

arch/riscv/lib/fdt_fixup.c | 4 ++--
cmd/bootefi.c              | 5 +++--
2 files changed, 5 insertions(+), 4 deletions(-)

--
2.24.0

[PATCH 1/3] riscv: Do not return error if reserved node already exists

[Atish Patra]

Not all errors are fatal. If a reserved memory node already exists in the
destination device tree, we can continue to boot without failing.

Signed-off-by: Atish Patra <atish.patra@wdc.com>
---
 arch/riscv/lib/fdt_fixup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/riscv/lib/fdt_fixup.c b/arch/riscv/lib/fdt_fixup.c
index 6db48ad04a56..91524d9a5ae9 100644
--- a/arch/riscv/lib/fdt_fixup.c
+++ b/arch/riscv/lib/fdt_fixup.c
@@ -62,7 +62,7 @@ int riscv_fdt_copy_resv_mem_node(const void *src, void *dst)
 		pmp_mem.end = addr + size - 1;
 		err = fdtdec_add_reserved_memory(dst, basename, &pmp_mem,
 						 &phandle);
-		if (err < 0) {
+		if (err < 0 && err != FDT_ERR_EXISTS) {
 			printf("failed to add reserved memory: %d\n", err);
 			return err;
 		}
-- 
2.24.0

[PATCH 2/3] riscv: Use optimized version of fdtdec_get_addr_size_no_parent

[Atish Patra]

fdtdec_get_addr_size_no_parent is not an optimized version if parent
node is already available with the caller.

Use fdtdec_get_addr_size_auto_parent to read the "reg" property

Signed-off-by: Atish Patra <atish.patra@wdc.com>
---
 arch/riscv/lib/fdt_fixup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/riscv/lib/fdt_fixup.c b/arch/riscv/lib/fdt_fixup.c
index 91524d9a5ae9..00b84dccbef0 100644
--- a/arch/riscv/lib/fdt_fixup.c
+++ b/arch/riscv/lib/fdt_fixup.c
@@ -44,7 +44,7 @@ int riscv_fdt_copy_resv_mem_node(const void *src, void *dst)
 	fdt_for_each_subnode(node, src, offset) {
 		name = fdt_get_name(src, node, NULL);
 
-		addr = fdtdec_get_addr_size_auto_noparent(src, node,
+		addr = fdtdec_get_addr_size_auto_parent(src, offset, node,
 							  "reg", 0, &size,
 							  false);
 		if (addr == FDT_ADDR_T_NONE) {
-- 
2.24.0

[PATCH 3/3] cmd: bootefi: Honor the address & size cells properties correctly

[Atish Patra]

fdtdec_get_addr_size reads the uses a fixed value for address & size
cell properties which may not be correct always.

Use the auto variant of the function which automatically reads
 #address-cells & #size-cells from parent and uses to read the "reg"
property.

Signed-off-by: Atish Patra <atish.patra@wdc.com>
---
 cmd/bootefi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 0f6d0f77507c..5f3fcce597de 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -190,8 +190,9 @@ static void efi_carve_out_dt_rsv(void *fdt)
 		subnode = fdt_first_subnode(fdt, nodeoffset);
 		while (subnode >= 0) {
 			/* check if this subnode has a reg property */
-			addr = fdtdec_get_addr_size(fdt, subnode, "reg",
-						    (fdt_size_t *)&size);
+			addr = fdtdec_get_addr_size_auto_parent(fdt, nodeoffset,
+						subnode, "reg", 0,
+						(fdt_size_t *)&size, false);
 			/*
 			 * The /reserved-memory node may have children with
 			 * a size instead of a reg property.
-- 
2.24.0

[PATCH 3/3] cmd: bootefi: Honor the address & size cells properties correctly

[Heinrich Schuchardt]

On 6/19/20 3:51 AM, Atish Patra wrote:
> fdtdec_get_addr_size reads the uses a fixed value for address & size
> cell properties which may not be correct always.
>
> Use the auto variant of the function which automatically reads
>   #address-cells & #size-cells from parent and uses to read the "reg"
> property.
>
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> ---
>   cmd/bootefi.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 0f6d0f77507c..5f3fcce597de 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -190,8 +190,9 @@ static void efi_carve_out_dt_rsv(void *fdt)
>   		subnode = fdt_first_subnode(fdt, nodeoffset);
>   		while (subnode >= 0) {
>   			/* check if this subnode has a reg property */
> -			addr = fdtdec_get_addr_size(fdt, subnode, "reg",
> -						    (fdt_size_t *)&size);
> +			addr = fdtdec_get_addr_size_auto_parent(fdt, nodeoffset,
> +						subnode, "reg", 0,
> +						(fdt_size_t *)&size, false);

On qemu_arm_defconfig: sizeof(fdt_size_t) = 4, sizeof(u64) = 8. So after
the call the upper four bytes of size will be random bytes from the stack.

Best regards

Heinrich

>   			/*
>   			 * The /reserved-memory node may have children with
>   			 * a size instead of a reg property.
>

[PATCH 3/3] cmd: bootefi: Honor the address & size cells properties correctly

[Atish Patra]

On Thu, Jun 18, 2020 at 11:51 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 6/19/20 3:51 AM, Atish Patra wrote:
> > fdtdec_get_addr_size reads the uses a fixed value for address & size
> > cell properties which may not be correct always.
> >
> > Use the auto variant of the function which automatically reads
> >   #address-cells & #size-cells from parent and uses to read the "reg"
> > property.
> >
> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > ---
> >   cmd/bootefi.c | 5 +++--
> >   1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > index 0f6d0f77507c..5f3fcce597de 100644
> > --- a/cmd/bootefi.c
> > +++ b/cmd/bootefi.c
> > @@ -190,8 +190,9 @@ static void efi_carve_out_dt_rsv(void *fdt)
> >               subnode = fdt_first_subnode(fdt, nodeoffset);
> >               while (subnode >= 0) {
> >                       /* check if this subnode has a reg property */
> > -                     addr = fdtdec_get_addr_size(fdt, subnode, "reg",
> > -                                                 (fdt_size_t *)&size);
> > +                     addr = fdtdec_get_addr_size_auto_parent(fdt, nodeoffset,
> > +                                             subnode, "reg", 0,
> > +                                             (fdt_size_t *)&size, false);
>
> On qemu_arm_defconfig: sizeof(fdt_size_t) = 4, sizeof(u64) = 8. So after
> the call the upper four bytes of size will be random bytes from the stack.
>

Ah yes. Thanks for catching that and the quick fix.

> Best regards
>
> Heinrich
>
> >                       /*
> >                        * The /reserved-memory node may have children with
> >                        * a size instead of a reg property.
> >
>


-- 
Regards,
Atish

[PATCH 1/3] riscv: Do not return error if reserved node already exists

[Bin Meng]

Hi Atish,

On Fri, Jun 19, 2020 at 9:52 AM Atish Patra <atish.patra@wdc.com> wrote:
>
> Not all errors are fatal. If a reserved memory node already exists in the
> destination device tree, we can continue to boot without failing.
>
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> ---
>  arch/riscv/lib/fdt_fixup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/riscv/lib/fdt_fixup.c b/arch/riscv/lib/fdt_fixup.c
> index 6db48ad04a56..91524d9a5ae9 100644
> --- a/arch/riscv/lib/fdt_fixup.c
> +++ b/arch/riscv/lib/fdt_fixup.c
> @@ -62,7 +62,7 @@ int riscv_fdt_copy_resv_mem_node(const void *src, void *dst)
>                 pmp_mem.end = addr + size - 1;
>                 err = fdtdec_add_reserved_memory(dst, basename, &pmp_mem,
>                                                  &phandle);
> -               if (err < 0) {
> +               if (err < 0 && err != FDT_ERR_EXISTS) {

This FDT_ERR_EXISTS error code is possibly returned by:

    node = fdt_add_subnode(blob, parent, name);
    if (node < 0)
        return node;

But if it exists, I believe fdtdec_add_reserved_memory() already
returns 0 because they are likely to have the same range of memory
block start address and size, no?

>                         printf("failed to add reserved memory: %d\n", err);
>                         return err;
>                 }
> --

Regards,
Bin

[PATCH 2/3] riscv: Use optimized version of fdtdec_get_addr_size_no_parent

[Bin Meng]

On Fri, Jun 19, 2020 at 9:52 AM Atish Patra <atish.patra@wdc.com> wrote:
>
> fdtdec_get_addr_size_no_parent is not an optimized version if parent
> node is already available with the caller.
>
> Use fdtdec_get_addr_size_auto_parent to read the "reg" property
>
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> ---
>  arch/riscv/lib/fdt_fixup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/riscv/lib/fdt_fixup.c b/arch/riscv/lib/fdt_fixup.c
> index 91524d9a5ae9..00b84dccbef0 100644
> --- a/arch/riscv/lib/fdt_fixup.c
> +++ b/arch/riscv/lib/fdt_fixup.c
> @@ -44,7 +44,7 @@ int riscv_fdt_copy_resv_mem_node(const void *src, void *dst)
>         fdt_for_each_subnode(node, src, offset) {
>                 name = fdt_get_name(src, node, NULL);
>
> -               addr = fdtdec_get_addr_size_auto_noparent(src, node,
> +               addr = fdtdec_get_addr_size_auto_parent(src, offset, node,
>                                                           "reg", 0, &size,
>                                                           false);

nits: please make above 2 lines indent to the (

>                 if (addr == FDT_ADDR_T_NONE) {
> --

Reviewed-by: Bin Meng <bin.meng@windriver.com>

[PATCH 1/3] riscv: Do not return error if reserved node already exists

[Atish Patra]

On Tue, Jun 23, 2020 at 12:24 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Atish,
>
> On Fri, Jun 19, 2020 at 9:52 AM Atish Patra <atish.patra@wdc.com> wrote:
> >
> > Not all errors are fatal. If a reserved memory node already exists in the
> > destination device tree, we can continue to boot without failing.
> >
> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > ---
> >  arch/riscv/lib/fdt_fixup.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/riscv/lib/fdt_fixup.c b/arch/riscv/lib/fdt_fixup.c
> > index 6db48ad04a56..91524d9a5ae9 100644
> > --- a/arch/riscv/lib/fdt_fixup.c
> > +++ b/arch/riscv/lib/fdt_fixup.c
> > @@ -62,7 +62,7 @@ int riscv_fdt_copy_resv_mem_node(const void *src, void *dst)
> >                 pmp_mem.end = addr + size - 1;
> >                 err = fdtdec_add_reserved_memory(dst, basename, &pmp_mem,
> >                                                  &phandle);
> > -               if (err < 0) {
> > +               if (err < 0 && err != FDT_ERR_EXISTS) {
>
> This FDT_ERR_EXISTS error code is possibly returned by:
>
>     node = fdt_add_subnode(blob, parent, name);
>     if (node < 0)
>         return node;
>
> But if it exists, I believe fdtdec_add_reserved_memory() already
> returns 0 because they are likely to have the same range of memory
> block start address and size, no?
>

Currently, yes. As libfdt and fdtdec_add_reserved_memory external to
this code, functionality
can change without modifying fdt_fixup.c knowingly or unknowingly.
FDT_ERR_EXISTS is harmless error in this context and we shouldn't
cause boot failure because of that.
That's why I add that exclusion.

> >                         printf("failed to add reserved memory: %d\n", err);
> >                         return err;
> >                 }
> > --
>
> Regards,
> Bin



-- 
Regards,
Atish

[PATCH 1/3] riscv: Do not return error if reserved node already exists

[Bin Meng]

Hi Atish,

On Wed, Jun 24, 2020 at 2:17 AM Atish Patra <atishp@atishpatra.org> wrote:
>
> On Tue, Jun 23, 2020 at 12:24 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Atish,
> >
> > On Fri, Jun 19, 2020 at 9:52 AM Atish Patra <atish.patra@wdc.com> wrote:
> > >
> > > Not all errors are fatal. If a reserved memory node already exists in the
> > > destination device tree, we can continue to boot without failing.
> > >
> > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > ---
> > >  arch/riscv/lib/fdt_fixup.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/arch/riscv/lib/fdt_fixup.c b/arch/riscv/lib/fdt_fixup.c
> > > index 6db48ad04a56..91524d9a5ae9 100644
> > > --- a/arch/riscv/lib/fdt_fixup.c
> > > +++ b/arch/riscv/lib/fdt_fixup.c
> > > @@ -62,7 +62,7 @@ int riscv_fdt_copy_resv_mem_node(const void *src, void *dst)
> > >                 pmp_mem.end = addr + size - 1;
> > >                 err = fdtdec_add_reserved_memory(dst, basename, &pmp_mem,
> > >                                                  &phandle);
> > > -               if (err < 0) {
> > > +               if (err < 0 && err != FDT_ERR_EXISTS) {
> >
> > This FDT_ERR_EXISTS error code is possibly returned by:
> >
> >     node = fdt_add_subnode(blob, parent, name);
> >     if (node < 0)
> >         return node;
> >
> > But if it exists, I believe fdtdec_add_reserved_memory() already
> > returns 0 because they are likely to have the same range of memory
> > block start address and size, no?
> >
>
> Currently, yes. As libfdt and fdtdec_add_reserved_memory external to
> this code, functionality
> can change without modifying fdt_fixup.c knowingly or unknowingly.
> FDT_ERR_EXISTS is harmless error in this context and we shouldn't
> cause boot failure because of that.
> That's why I add that exclusion.

Okay. But the error should be checked against -FDT_ERR_EXISTS.

Regards,
Bin

[PATCH 1/3] riscv: Do not return error if reserved node already exists

[Atish Patra]

On Tue, Jun 23, 2020 at 5:51 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Atish,
>
> On Wed, Jun 24, 2020 at 2:17 AM Atish Patra <atishp@atishpatra.org> wrote:
> >
> > On Tue, Jun 23, 2020 at 12:24 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > Hi Atish,
> > >
> > > On Fri, Jun 19, 2020 at 9:52 AM Atish Patra <atish.patra@wdc.com> wrote:
> > > >
> > > > Not all errors are fatal. If a reserved memory node already exists in the
> > > > destination device tree, we can continue to boot without failing.
> > > >
> > > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > > ---
> > > >  arch/riscv/lib/fdt_fixup.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/riscv/lib/fdt_fixup.c b/arch/riscv/lib/fdt_fixup.c
> > > > index 6db48ad04a56..91524d9a5ae9 100644
> > > > --- a/arch/riscv/lib/fdt_fixup.c
> > > > +++ b/arch/riscv/lib/fdt_fixup.c
> > > > @@ -62,7 +62,7 @@ int riscv_fdt_copy_resv_mem_node(const void *src, void *dst)
> > > >                 pmp_mem.end = addr + size - 1;
> > > >                 err = fdtdec_add_reserved_memory(dst, basename, &pmp_mem,
> > > >                                                  &phandle);
> > > > -               if (err < 0) {
> > > > +               if (err < 0 && err != FDT_ERR_EXISTS) {
> > >
> > > This FDT_ERR_EXISTS error code is possibly returned by:
> > >
> > >     node = fdt_add_subnode(blob, parent, name);
> > >     if (node < 0)
> > >         return node;
> > >
> > > But if it exists, I believe fdtdec_add_reserved_memory() already
> > > returns 0 because they are likely to have the same range of memory
> > > block start address and size, no?
> > >
> >
> > Currently, yes. As libfdt and fdtdec_add_reserved_memory external to
> > this code, functionality
> > can change without modifying fdt_fixup.c knowingly or unknowingly.
> > FDT_ERR_EXISTS is harmless error in this context and we shouldn't
> > cause boot failure because of that.
> > That's why I add that exclusion.
>
> Okay. But the error should be checked against -FDT_ERR_EXISTS.
>

My bad. I will fix it and send the next version.

> Regards,
> Bin



-- 
Regards,
Atish