Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
MIME-Version: 1.0
References: <CAJZ5v0j+L=a0UEiCEXUYDtvscaVF29FPro9FNupMkJ7do2eBGw@mail.gmail.com>
 <20230918124117.187-1-guojinhui.liam@bytedance.com>
In-Reply-To: <20230918124117.187-1-guojinhui.liam@bytedance.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Mon, 18 Sep 2023 17:09:27 +0200
Message-ID: <CAJZ5v0jWhC-poYDtemW36Nqm+0ADsG7=wBCUerWJWzQMv-zM2g@mail.gmail.com>
Subject: Re: [PATCH] driver core: platform: set numa_node before platform_add_device()
To:     Jinhui Guo <guojinhui.liam@bytedance.com>
Cc:     rafael@kernel.org, gregkh@linuxfoundation.org, lenb@kernel.org,
        linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org,
        lizefan.x@bytedance.com, lkp@intel.com, stable@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Mon, Sep 18, 2023 at 2:41 PM Jinhui Guo <guojinhui.liam@bytedance.com> wrote:
>
> On Mon, 18 Sep 2023 12:30:58 +0200, Rafael J. Wysocki wrote:
> > On Thu, Sep 14, 2023 at 11:32 PM Jinhui Guo
> > <guojinhui.liam@bytedance.com> wrote:
> > >
> > > platform_add_device()
> >
> > According to "git grep" this function is not present in 6.6-rc2.
> >
> > If you mean platform_device_add(), please update the patch subject and
> > changelog accordingly.
> >
>
> This is my mistake, the function name was written wrong.
> I will fix it in the next patch.
>
> > > creates the numa_node attribute of sysfs according
> > > to whether dev_to_node(dev) is equal to NUMA_NO_NODE. So set the numa node
> > > of device before creating numa_node attribute of sysfs.
> >
> > It would be good to also say that this needs to be done in
> > platform_device_register_full(), because that's where the platform
> > device object is allocated.
> >
>
> Thaks for your suggestion. I will modify my decription soon.
>
> > However, what about adding the NUMA node information to pdevinfo?  It
> > would be more straightforward to handle it then AFAICS.
> >
>
> I have tried three potential solutions to fix the bug:
> 1. The first one is what the current patch do.
>
> 2. Add a new function interface only for acpi_create_platform_device() call.
>    But the code will be a bit redundant.
>
> 3. Add an member "numa_node" in `struct platform_device_info`, just as what
>    `struct device` done:
>
> ```
> struct platform_device_info {
>         ...;
> #ifdef CONFIG_NUMA
>         int             numa_node;
> #endif
> ```
>
> But not all the call to platform_device_register_full() would set numa_node,
> and many of them use ` memset(&pdevinfo, 0, sizeof(pdevinfo));` to initialize
> `struct platform_device_info`. It could initialize numa_node to zero and
> result in wrong numa_node information in sysfs.

Well, platform_device_register_full() need not take that value as the
numa node number directly.  It may, for example, take the number from
pdevinfo, subtract 1 from it and use the result of that as the numa
node number, if not negative.