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: <CAOnJCULtHzQKJNE4OO_U2NMaW6pX38Pw7dLywGc9og1BuuAYNQ@mail.gmail.com>
 <20200830025438.GA9624@bjorn-Precision-5520>
In-Reply-To: <20200830025438.GA9624@bjorn-Precision-5520>
From:   Atish Patra <atishp@atishpatra.org>
Date:   Sun, 30 Aug 2020 00:22:24 -0700
Message-ID: <CAOnJCU+-=_Tjm1Wc8se1moTa7fJxzJJafiw=My7mPLBmszwccQ@mail.gmail.com>
Subject: Re: [RFC/RFT PATCH 3/6] arm64, numa: Move pcibus_to_node definition
 to generic numa code
To:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     Jonathan Cameron <Jonathan.Cameron@huawei.com>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Atish Patra <atish.patra@wdc.com>,
        Zong Li <zong.li@sifive.com>,
        linux-riscv <linux-riscv@lists.infradead.org>,
        Will Deacon <will@kernel.org>, linux-arch@vger.kernel.org,
        Rob Herring <robh@kernel.org>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        Ganapatrao Kulkarni <gkulkarni@cavium.com>,
        Steven Price <steven.price@arm.com>, linux-pci@vger.kernel.org,
        Greentime Hu <greentime.hu@sifive.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        Arnd Bergmann <arnd@arndb.de>,
        Anshuman Khandual <anshuman.khandual@arm.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        Nick Hu <nickhu@andestech.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Anup Patel <Anup.Patel@wdc.com>,
        "linux-kernel@vger.kernel.org List" <linux-kernel@vger.kernel.org>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Mike Rapoport <rppt@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sat, Aug 29, 2020 at 7:54 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Aug 28, 2020 at 06:11:50PM -0700, Atish Patra wrote:
> > On Fri, Aug 28, 2020 at 9:15 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Fri, Aug 28, 2020 at 10:48:30AM +0100, Jonathan Cameron wrote:
> > > > On Fri, 14 Aug 2020 14:47:22 -0700
> > > > Atish Patra <atish.patra@wdc.com> wrote:
> > > >
> > > > > pcibus_to_node is used only when numa is enabled and does not depend
> > > > > on ISA. Thus, it can be moved the generic numa implementation.
> > > > >
> > > > > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > > >
> > > > From a more general unification point of view, there seem to
> > > > be two ways architectures implement this.
> > > > Either
> > > >
> > > > bus->sysdata.node
> > > >
> > > > Or as here.
> > > > There are weird other options, but let us ignore those :)
> > > >
> > > > That is going to take a bit of unwinding should we
> > > > want to take this unification further and perhaps we want to think
> > > > about doing this in pci generic code rather than here?
> > > >
> > > > Perhaps this is one we are better keeping architecture specific for
> > > > now?
> > > >
> > > > +CC Bjorn and Linux-pci
> > > >
> > > >
> > > > > ---
> > > > >  arch/arm64/kernel/pci.c  | 10 ----------
> > > > >  drivers/base/arch_numa.c | 11 +++++++++++
> > > > >  2 files changed, 11 insertions(+), 10 deletions(-)
> > > > >
> > > > > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> > > > > index 1006ed2d7c60..07c122946c11 100644
> > > > > --- a/arch/arm64/kernel/pci.c
> > > > > +++ b/arch/arm64/kernel/pci.c
> > > > > @@ -54,16 +54,6 @@ int raw_pci_write(unsigned int domain, unsigned int bus,
> > > > >     return b->ops->write(b, devfn, reg, len, val);
> > > > >  }
> > > > >
> > > > > -#ifdef CONFIG_NUMA
> > > > > -
> > > > > -int pcibus_to_node(struct pci_bus *bus)
> > > > > -{
> > > > > -   return dev_to_node(&bus->dev);
> > > > > -}
> > > > > -EXPORT_SYMBOL(pcibus_to_node);
> > > > > -
> > > > > -#endif
> > > > > -
> > > > >  #ifdef CONFIG_ACPI
> > > > >
> > > > >  struct acpi_pci_generic_root_info {
> > > > > diff --git a/drivers/base/arch_numa.c b/drivers/base/arch_numa.c
> > > > > index 83341c807240..4ab1b20a615d 100644
> > > > > --- a/drivers/base/arch_numa.c
> > > > > +++ b/drivers/base/arch_numa.c
> > > > > @@ -11,6 +11,7 @@
> > > > >  #include <linux/acpi.h>
> > > > >  #include <linux/memblock.h>
> > > > >  #include <linux/module.h>
> > > > > +#include <linux/pci.h>
> > > > >  #include <linux/of.h>
> > > > >
> > > > >  #ifdef CONFIG_ARM64
> > > > > @@ -60,6 +61,16 @@ EXPORT_SYMBOL(cpumask_of_node);
> > > > >
> > > > >  #endif
> > > > >
> > > > > +#ifdef CONFIG_PCI
> > > > > +
> > > > > +int pcibus_to_node(struct pci_bus *bus)
> > > > > +{
> > > > > +   return dev_to_node(&bus->dev);
> > > > > +}
> > > > > +EXPORT_SYMBOL(pcibus_to_node);
> > > > > +
> > > > > +#endif
> > >
> > > I certainly agree that this should not be arch-specific, but I'm not
> > > really in favor of adding this PCI gunk in drivers/base.
> > >
> > > I think we can do better (eventually) by getting rid of
> > > pcibus_to_node() completely.  It's not used very much except by
> > > cpumask_of_pcibus(), which itself is hardly used at all.
> > >
> > I am a bit confused here. A quick grep suggested that pcibus_to_node()
> > is also called from generic pci probe,
> > controller and few drivers(block, infiniband) as well. Maybe I am
> > missing something here ?
>
> I didn't say it was *only* used by cpumask_of_pcibus().  13 of the 29
> calls are from cpumask_of_pcibus().
>
Ahh okay. Sorry I misunderstood that.

> As you point out, there are a few drivers that use it.  They typically
> have a pci_dev, so they do the equivalent of pcibus_to_node(pdev->bus).
> That seems silly; they should just do dev_to_node(&pdev->dev) instead.
>

That covers the use case for ARM64. There are other arch implementations
as well which retrieve node information from sysdata which seems to be
type casted to different structures on different arch.
What should be done for those ?

> I looked at this once, and it seems like there might have been a
> wrinkle like the pdev->dev node not being set correctly or something.
> If that's the case, I think it should be fixed.
>
> > We can move the pcibus_to_node to arch specific code for now if that's
> > what is preferred.
>
> Now I'm the one who's confused :)  Most arches, including arm64,
> already have arch-specific implementations of pcibus_to_node().  I
> didn't look at the rest of the series to see if there's a reason you
> need to move pcibus_to_node() from arch/arm64/kernel/pci.c to
> drivers//base/arch_numa.c.  If you don't need to, I would just leave
> it where it is.
>

The reason I moved it from arch/arm64/kernel/pci.c to drivers//base/arch_numa.c.
so that we don't have to define it for RISC-V. But it's just a single
line function and
we can define it in RISC-V as well. I will try that in v2.

> > > > >  static void numa_update_cpu(unsigned int cpu, bool remove)
> > > > >  {
> > > > >     int nid = cpu_to_node(cpu);


-- 
Regards,
Atish