Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210910124628.6261-1-justin.he@arm.com> <CAPcyv4ie_ZzEwrrKJEVrDP19UWAgSiW3GU9f99EqX0e6BPQDPA@mail.gmail.com>
 <AM6PR08MB4376FC35158104629C603197F7DA9@AM6PR08MB4376.eurprd08.prod.outlook.com>
 <CAPcyv4gyCHTcXUSLcsgnX8o0JUfpSNf8B=7zbfcZcvWFCUSCvw@mail.gmail.com> <AM6PR08MB4376877792291C237AE1AE30F7DB9@AM6PR08MB4376.eurprd08.prod.outlook.com>
In-Reply-To: <AM6PR08MB4376877792291C237AE1AE30F7DB9@AM6PR08MB4376.eurprd08.prod.outlook.com>
From:   Dan Williams <dan.j.williams@intel.com>
Date:   Wed, 15 Sep 2021 09:22:39 -0700
Message-ID: <CAPcyv4j6rusw7ixpjLsTK3NhJDD_poTp8dUKjQ45jn=vuqgncw@mail.gmail.com>
Subject: Re: [PATCH v2] device-dax: use fallback nid when numa node is invalid
To:     Justin He <Justin.He@arm.com>
Cc:     Vishal Verma <vishal.l.verma@intel.com>,
        Dave Jiang <dave.jiang@intel.com>,
        David Hildenbrand <david@redhat.com>,
        Linux NVDIMM <nvdimm@lists.linux.dev>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        nd <nd@arm.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Sep 14, 2021 at 11:51 PM Justin He <Justin.He@arm.com> wrote:
[..]
> > > diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> > > index fb775b967c52..d3a0cec635b1 100644
> > > --- a/drivers/acpi/nfit/core.c
> > > +++ b/drivers/acpi/nfit/core.c
> > > @@ -3005,15 +3005,8 @@ static int acpi_nfit_register_region(struct
> > > acpi_nfit_desc *acpi_desc,
> > >         ndr_desc->res = &res;
> > >         ndr_desc->provider_data = nfit_spa;
> > >         ndr_desc->attr_groups = acpi_nfit_region_attribute_groups;
> > > -       if (spa->flags & ACPI_NFIT_PROXIMITY_VALID) {
> > > -               ndr_desc->numa_node = acpi_map_pxm_to_online_node(
> > > -                                               spa->proximity_domain);
> > > -               ndr_desc->target_node = acpi_map_pxm_to_node(
> > > -                               spa->proximity_domain);
> > > -       } else {
> > > -               ndr_desc->numa_node = NUMA_NO_NODE;
> > > -               ndr_desc->target_node = NUMA_NO_NODE;
> > > -       }
> > > +       ndr_desc->numa_node = memory_add_physaddr_to_nid(spa->address);
> > > +       ndr_desc->target_node = phys_to_target_node(spa->address);
> > >
> > >         /*
> > >          * Persistence domain bits are hierarchical, if
> > > ===================================================
> > >
> > > Do you still suggest fixing like this?
> >
> > Are you saying that ACPI_NFIT_PROXIMITY_VALID is not set on your
> > platform, or that pxm_to_node() returns NUMA_NO_NODE?
> >
> Latter,  ACPI_NFIT_PROXIMITY_VALID is *set* in my case.
>
> > I would expect something like this:
> >
> > diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> > index a3ef6cce644c..95de7dc18ed8 100644
> > --- a/drivers/acpi/nfit/core.c
> > +++ b/drivers/acpi/nfit/core.c
> > @@ -3007,6 +3007,15 @@ static int acpi_nfit_register_region(struct
> > acpi_nfit_desc *acpi_desc,
> >                 ndr_desc->target_node = NUMA_NO_NODE;
> >         }
> >
> > +       /*
> > +        * Fallback to address based numa information if node lookup
> > +        * failed
> > +        */
> > +       if (ndr_desc->numa_node == NUMA_NO_NODE)
> > +               ndr_desc->numa_node = memory_add_physaddr_to_nid(spa-
> > >address);
> > +       if (ndr_desc->target_node == NUMA_NO_NODE)
> > +               phys_to_target_node(spa->address);
> > +
>
> Would it better to add a dev_info() here to report this node id changing?

Yes, given all the possibilities here, a dev_info() reporting the
final result of the node mapping is justifiable.