Re: [RFC PATCH v3 15/24] ARM: NUMA: DT: Add CPU NUMA support

From: Stefano Stabellini <sstabellini@kernel.org>
To: Julien Grall <julien.grall@arm.com>
Cc: Tim Deegan <tim@xen.org>,
	kevin.tian@intel.com, Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	Vijay Kilari <vijay.kilari@gmail.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Dario Faggioli <dario.faggioli@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	"xen-devel@lists.xen.org" <xen-devel@lists.xen.org>,
	Jan Beulich <jbeulich@suse.com>,
	Vijaya Kumar K <Vijaya.Kumar@cavium.com>
Subject: Re: [RFC PATCH v3 15/24] ARM: NUMA: DT: Add CPU NUMA support
Date: Tue, 25 Jul 2017 11:48:54 -0700 (PDT)	[thread overview]
Message-ID: <alpine.DEB.2.10.1707251140360.22381@sstabellini-ThinkPad-X260> (raw)
In-Reply-To: <0c9c4897-47b2-dde5-250e-857f33564d27@arm.com>

On Tue, 25 Jul 2017, Julien Grall wrote:
> On 25/07/17 07:47, Vijay Kilari wrote:
> > > >  void numa_failed(void)
> > > >  {
> > > >      numa_off = true;
> > > >      init_dt_numa_distance();
> > > >      node_distance_fn = NULL;
> > > > +    init_cpu_to_node();
> > > > +}
> > > > +
> > > > +void __init numa_set_cpu_node(int cpu, unsigned int nid)
> > > > +{
> > > > +    if ( !node_isset(nid, processor_nodes_parsed) || nid >=
> > > > MAX_NUMNODES
> > > > )
> > > > +        nid = 0;
> > > 
> > > 
> > > This looks wrong to me. If the node-id is invalid, why would you blindly
> > > set
> > > to 0?
> > 
> > Generally this check will not pass. I will make this function return
> > error code in case
> > of wrong nid.
> 
> I don't really want to see error code and error handling everywhere in the
> initialization code. I would assume that if the NUMA bindings are wrong we
> should just crash Xen rather continuing with NUMA disabled.
> 
> Stefano do you have any opinion here?

Yes, I noticed that there is an overabundance of error checks in the
patches. I have pointed out in other cases that some of these checks are
duplicates.

I am OK with some checks but we should not do the same check over and
over.

To answer the question: do we need any checks at all?

I am fine with no checks on the device tree or ACPI bindings themselves.
I am also OK with some checks in few places to check that the
information passed by the firmware is in the right shape (for example we
check for the ACPI header length before accessing any ACPI tables). That
is good. But I am not OK with repeating the same check multiple times
uselessly or checking for conditions that cannot happen (like a NULL
pointer in the ACPI header case again).

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel