Message-ID: <20140908155931.GP27864@e106497-lin.cambridge.arm.com>
Date:	Mon, 8 Sep 2014 16:59:31 +0100
From:	Liviu Dudau <Liviu.Dudau@....com>
To:	Rob Herring <robherring2@...il.com>
Cc:	Bjorn Helgaas <bhelgaas@...gle.com>, Arnd Bergmann <arnd@...db.de>,
	Rob Herring <robh+dt@...nel.org>,
	Jason Gunthorpe <jgunthorpe@...idianresearch.com>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Catalin Marinas <Catalin.Marinas@....com>,
	Will Deacon <Will.Deacon@....com>,
	Russell King <linux@....linux.org.uk>,
	linux-pci <linux-pci@...r.kernel.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Tanmay Inamdar <tinamdar@....com>,
	Grant Likely <grant.likely@...retlab.ca>,
	Sinan Kaya <okaya@...eaurora.org>,
	Jingoo Han <jg1.han@...sung.com>,
	Kukjin Kim <kgene.kim@...sung.com>,
	Suravee Suthikulanit <suravee.suthikulpanit@....com>,
	linux-arch <linux-arch@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Device Tree ML <devicetree@...r.kernel.org>,
	LAKML <linux-arm-kernel@...ts.infradead.org>,
	"grant.likely@...aro.org" <grant.likely@...aro.org>
Subject: Re: [PATCH v10 07/10] OF: Introduce helper function for getting PCI
 domain_nr

On Mon, Sep 08, 2014 at 04:27:36PM +0100, Rob Herring wrote:
> On Mon, Sep 8, 2014 at 9:54 AM, Liviu Dudau <Liviu.Dudau@....com> wrote:
> > On Mon, Sep 08, 2014 at 03:27:56PM +0100, Rob Herring wrote:
> >> On Mon, Sep 8, 2014 at 8:54 AM, Liviu Dudau <Liviu.Dudau@....com> wrote:
> >> > Add of_pci_get_domain_nr() to retrieve the PCI domain number
> >> > of a given device from DT. If the information is not present,
> >> > the function can be requested to allocate a new domain number.
> >> >
> >> > Cc: Bjorn Helgaas <bhelgaas@...gle.com>
> >> > Cc: Arnd Bergmann <arnd@...db.de>
> >> > Cc: Grant Likely <grant.likely@...aro.org>
> >> > Cc: Rob Herring <robh+dt@...nel.org>
> >> > Reviewed-by: Catalin Marinas <catalin.marinas@....com>
> >> > Signed-off-by: Liviu Dudau <Liviu.Dudau@....com>
> >> > ---
> >> >  drivers/of/of_pci.c    | 34 ++++++++++++++++++++++++++++++++++
> >> >  include/linux/of_pci.h |  7 +++++++
> >> >  2 files changed, 41 insertions(+)
> >> >
> >> > diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
> >> > index 8481996..a107edb 100644
> >> > --- a/drivers/of/of_pci.c
> >> > +++ b/drivers/of/of_pci.c
> >> > @@ -89,6 +89,40 @@ int of_pci_parse_bus_range(struct device_node *node, struct resource *res)
> >> >  }
> >> >  EXPORT_SYMBOL_GPL(of_pci_parse_bus_range);
> >> >
> >> > +static atomic_t of_domain_nr = ATOMIC_INIT(-1);
> >> > +
> >> > +/**
> >> > + * This function will try to obtain the host bridge domain number by
> >> > + * using of_alias_get_id() call with "pci-domain" as a stem. If that
> >> > + * fails, a local allocator will be used. The local allocator can
> >> > + * be requested to return a new domain_nr if the information is missing
> >> > + * from the device tree.
> >> > + *
> >> > + * @node: device tree node with the domain information
> >> > + * @allocate_if_missing: if DT lacks information about the domain nr,
> >> > + * allocate a new number.
> >> > + *
> >> > + * Returns the associated domain number from DT, or a new domain number
> >> > + * if DT information is missing and @allocate_if_missing is true. If
> >> > + * @allocate_if_missing is false then the last allocated domain number
> >> > + * will be returned.
> >> > + */
> >> > +int of_pci_get_domain_nr(struct device_node *node, bool allocate_if_missing)
> >> > +{
> >> > +       int domain;
> >> > +
> >> > +       domain = of_alias_get_id(node, "pci-domain");
> >> > +       if (domain == -ENODEV) {
> >> > +               if (allocate_if_missing)
> >> > +                       domain = atomic_inc_return(&of_domain_nr);
> >> > +               else
> >> > +                       domain = atomic_read(&of_domain_nr);
> >>
> >> This function seems a bit broken to me. It is overloaded with too many
> >> different outcomes. Think about how this would work if you have
> >> multiple PCI buses and a mixture of having pci-domain aliases or not.
> >> Aren't domain numbers global? Allocation should then start outside of
> >> the range of alias ids.
> >>
> >> Rob
> >>
> >
> > Rob,
> >
> > Would this version make more sense?
> 
> No.
> 
> > int of_pci_get_domain_nr(struct device_node *node, bool allocate_if_missing)
> > {
> >        int domain;
> >
> >        domain = of_alias_get_id(node, "pci-domain");
> >        if (domain == -ENODEV) {
> >                if (allocate_if_missing)
> >                        domain = atomic_inc_return(&of_domain_nr);
> >                else
> >                        domain = atomic_read(&of_domain_nr);
> >        } else {
> >                /* remember the largest value seen */
> >                int d = atomic_read(&of_domain_nr);
> >                atomic_set(&of_domain_nr, max(domain, d));
> >        }
> >
> >        return domain;
> > }
> > EXPORT_SYMBOL_GPL(of_pci_get_domain_nr);
> >
> > It would still create gaps and possible duplicates, but this is just a number
> > and trying to create a new root bus in an existing domain should fail. I have
> 
> Is failure okay in that case?

Well, you would get the failure at development time and hopefully fix the DT to
eliminate sparseness of domain numbers.

> 
> > no clue on how to generate unique values without parsing the DT and filling
> > a sparse array with values found there and then checking for allocated values
> 
> You really only need to know the maximum value and then start the
> non-DT allocations from there.
> 
> > on new requests. This function gets called quite a lot and I'm trying not to
> > make it too heavy weight.
> 
> Generally, nothing should be accessing the same DT value frequently.
> It should get cached somewhere.
> 

The problem appears for DTs that don't have the pci-domain info. Then the cached
value is left at the default non-valid value and attempts to rescan the DT will
be made every time the function is called.

> I don't really understand how domains are used so it's hard to provide
> a recommendation here. Do domains even belong in the DT?

ACPI calls them segments and the way Bjorn explained it to me at some moment was
that it was an attempt to split up a bus in different groups (or alternatively,
merge a few busses together). To be honest I haven't seen systems where the domain
is anything other than zero, but JasonG (or maybe Benjamin) were floating an
idea of using the domain number to identify physical slots.

> This function
> is just a weird mixture of data retrieval and allocation. I think you
> need to separate it into 2 functions.

It is meant to do allocation with the retrieval being a short-cut (or fine
control if you want).

I need to think a bit more for a better solution.

Best regards,
Liviu

> 
> Rob
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists