Re: [Xen-devel] [PATCH v6 2/8] xen/arm: make process_memory_node a device_tree_node_func

2019-08-19 Thread Julien Grall



On 8/17/19 1:48 AM, Stefano Stabellini wrote:

On Fri, 16 Aug 2019, Julien Grall wrote:
I think you are right, and honestly I was thinking about it while I
updated this patch. If I use -EINVAL, it would be the same return error
as the "invalid #address-cells or #size-cells". I just wanted to
double-check that it is OK for you.


We don't need to differentiate "invalid #{address, size}-cells" from 
"zero size", so using the same errno is fine.


Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v6 2/8] xen/arm: make process_memory_node a device_tree_node_func

2019-08-16 Thread Stefano Stabellini
On Fri, 16 Aug 2019, Julien Grall wrote:
> Hi,
> 
> On 16/08/2019 00:36, Stefano Stabellini wrote:
> > Change the signature of process_memory_node to match
> > device_tree_node_func. Thanks to this change, the next patch will be
> > able to use device_tree_for_each_node to call process_memory_node on all
> > the children of a provided node.
> > 
> > Return error if there is no reg property or if nr_banks is reached. Let
> > the caller deal with the error.
> 
> This sentence does not match the change below. Only 2 of the new error paths
> are described here.
> 
> > 
> > Add a printk when device tree parsing fails.
> > 
> > Signed-off-by: Stefano Stabellini 
> > ---
> > Changes in v6:
> > - fix out of space check
> > - bring back printk when address_cells or size_cells are not properly set
> > - return -EINVAL in that case (different from reg missing)
> > - add printk when parsing fails
> > - return -ENOENT when memory size is 0
> > 
> > Changes in v5:
> > - return -ENOENT if address_cells or size_cells are not properly set
> > 
> > Changes in v4:
> > - return error if there is no reg propery, remove printk
> > - return error if nr_banks is reached
> > 
> > Changes in v3:
> > - improve commit message
> > - check return value of process_memory_node
> > 
> > Changes in v2:
> > - new
> > ---
> >   xen/arch/arm/bootfdt.c | 29 ++---
> >   1 file changed, 18 insertions(+), 11 deletions(-)
> > 
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > index f1614ef7fc..9dc2c1352d 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -130,9 +130,10 @@ int __init device_tree_for_each_node(const void *fdt,
> > int node,
> >   return 0;
> >   }
> >   -static void __init process_memory_node(const void *fdt, int node,
> > -   const char *name,
> > -   u32 address_cells, u32 size_cells)
> > +static int __init process_memory_node(const void *fdt, int node,
> > +  const char *name, int depth,
> > +  u32 address_cells, u32 size_cells,
> > +  void *data)
> >   {
> >   const struct fdt_property *prop;
> >   int i;
> > @@ -145,15 +146,12 @@ static void __init process_memory_node(const void
> > *fdt, int node,
> >   {
> >   printk("fdt: node `%s': invalid #address-cells or #size-cells",
> >  name);
> > -return;
> > +return -EINVAL;
> >   }
> > prop = fdt_get_property(fdt, node, "reg", NULL);
> >   if ( !prop )
> > -{
> > -printk("fdt: node `%s': missing `reg' property\n", name);
> > -return;
> > -}
> > +return -ENOENT;
> > cell = (const __be32 *)prop->data;
> >   banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
> > @@ -162,11 +160,15 @@ static void __init process_memory_node(const void
> > *fdt, int node,
> >   {
> >   device_tree_get_reg(, address_cells, size_cells, ,
> > );
> >   if ( !size )
> > -continue;
> > +return -ENOENT;
> 
> I don't think we can treat the same way the lack of "regs" properties and a
> size of 0.
> 
> The former is expected as binding allow you to do it for reserved-memory. The
> latter is the user not writing the property correctly. So ignoring the latter
> will result to Xen potentially missing some reserved-regions (not great!).
> 
> So, similar to #address-cells/#size-cells discussion, we should return an
> error we are able to distinguish. Probably -EINVAL.

I think you are right, and honestly I was thinking about it while I
updated this patch. If I use -EINVAL, it would be the same return error
as the "invalid #address-cells or #size-cells". I just wanted to
double-check that it is OK for you.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v6 2/8] xen/arm: make process_memory_node a device_tree_node_func

2019-08-16 Thread Julien Grall

Hi,

On 16/08/2019 00:36, Stefano Stabellini wrote:

Change the signature of process_memory_node to match
device_tree_node_func. Thanks to this change, the next patch will be
able to use device_tree_for_each_node to call process_memory_node on all
the children of a provided node.

Return error if there is no reg property or if nr_banks is reached. Let
the caller deal with the error.


This sentence does not match the change below. Only 2 of the new error paths are 
described here.




Add a printk when device tree parsing fails.

Signed-off-by: Stefano Stabellini 
---
Changes in v6:
- fix out of space check
- bring back printk when address_cells or size_cells are not properly set
- return -EINVAL in that case (different from reg missing)
- add printk when parsing fails
- return -ENOENT when memory size is 0

Changes in v5:
- return -ENOENT if address_cells or size_cells are not properly set

Changes in v4:
- return error if there is no reg propery, remove printk
- return error if nr_banks is reached

Changes in v3:
- improve commit message
- check return value of process_memory_node

Changes in v2:
- new
---
  xen/arch/arm/bootfdt.c | 29 ++---
  1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index f1614ef7fc..9dc2c1352d 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -130,9 +130,10 @@ int __init device_tree_for_each_node(const void *fdt, int 
node,
  return 0;
  }
  
-static void __init process_memory_node(const void *fdt, int node,

-   const char *name,
-   u32 address_cells, u32 size_cells)
+static int __init process_memory_node(const void *fdt, int node,
+  const char *name, int depth,
+  u32 address_cells, u32 size_cells,
+  void *data)
  {
  const struct fdt_property *prop;
  int i;
@@ -145,15 +146,12 @@ static void __init process_memory_node(const void *fdt, 
int node,
  {
  printk("fdt: node `%s': invalid #address-cells or #size-cells",
 name);
-return;
+return -EINVAL;
  }
  
  prop = fdt_get_property(fdt, node, "reg", NULL);

  if ( !prop )
-{
-printk("fdt: node `%s': missing `reg' property\n", name);
-return;
-}
+return -ENOENT;
  
  cell = (const __be32 *)prop->data;

  banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
@@ -162,11 +160,15 @@ static void __init process_memory_node(const void *fdt, 
int node,
  {
  device_tree_get_reg(, address_cells, size_cells, , );
  if ( !size )
-continue;
+return -ENOENT;


I don't think we can treat the same way the lack of "regs" properties and a size 
of 0.


The former is expected as binding allow you to do it for reserved-memory. The 
latter is the user not writing the property correctly. So ignoring the latter 
will result to Xen potentially missing some reserved-regions (not great!).


So, similar to #address-cells/#size-cells discussion, we should return an error 
we are able to distinguish. Probably -EINVAL.


Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel