On Wed, Nov 12, 2014 at 10:39:17AM +0000, Arnd Bergmann wrote:
> On Monday 10 November 2014 16:41:46 Lorenzo Pieralisi wrote:
> > The current logic used for PCI domain assignment in arm64
> > pci_bus_assign_domain_nr() is flawed in that, depending on the host
> > controllers configuration for a platform and the respective initialization
> > sequence, core code may end up allocating PCI domain numbers from both DT and
> > the core code generic domain counter, which would result in PCI domain
> > allocation aliases/errors.
> >
> > This patch fixes the logic behind the PCI domain number assignment and
> > moves the resulting code to generic PCI core code so that the same domain
> > allocation logic is used on all platforms selecting
> >
> > CONFIG_PCI_DOMAINS_GENERIC
> >
> > instead of resorting to an arch specific implementation that might end up
> > duplicating the PCI domain assignment logic wrongly.
> >
> > Cc: Arnd Bergmann <arnd@xxxxxxxx>
> > Cc: Liviu Dudau <liviu.dudau@xxxxxxx>
> > Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> > Cc: Rob Herring <robh+dt@xxxxxxxxxx>
> > Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
>
> The general approach seems good to me,
>
> Acked-by: Arnd Bergmann <arnd@xxxxxxxx>
>
> I would suggest one simplification:
>
> > +
> > +#ifdef CONFIG_PCI_DOMAINS_GENERIC
> > +
> > +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > +{
> > + static int use_dt_domains = -1;
> > + int domain = of_get_pci_domain_nr(parent->of_node);
> > +
> > + /*
> > + * Check DT domain and use_dt_domains values.
> > + *
> > + * If DT domain property is valid (domain >= 0) and
> > + * use_dt_domains != 0, the DT assignment is valid since this means
> > + * we have not previously allocated a domain number by using
> > + * pci_get_new_domain_nr(); we should also update use_dt_domains to
> > + * 1, to indicate that we have just assigned a domain number from
> > + * DT.
> > + *
> > + * If DT domain property value is not valid (ie domain < 0), and we
> > + * have not previously assigned a domain number from DT
> > + * (use_dt_domains != 1) we should assign a domain number by
> > + * using the:
> > + *
> > + * pci_get_new_domain_nr()
> > + *
> > + * API and update the use_dt_domains value to keep track of method we
> > + * are using to assign domain numbers (use_dt_domains = 0).
> > + *
> > + * All other combinations imply we have a platform that is trying
> > + * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
> > + * which is a recipe for domain mishandling and it is prevented by
> > + * invalidating the domain value (domain = -1) and printing a
> > + * corresponding error.
> > + */
> > + if (domain >= 0 && use_dt_domains) {
> > + use_dt_domains = 1;
> > + } else if (domain < 0 && use_dt_domains != 1) {
> > + use_dt_domains = 0;
> > + domain = pci_get_new_domain_nr();
> > + } else {
> > + dev_err(parent, "Node %s has inconsistent \"linux,pci-domain\" property in DT\n",
> > + parent->of_node->full_name);
> > + domain = -1;
> > + }
> > +
> > + bus->domain_nr = domain;
> > +}
> > +#endif
> > #endif
> >
>
> Since this is now in the file in which it gets called, you can mark the
> function itself as 'static' and remove the extern declaration and inline
> wrapper from the header file. You can also avoid the #ifdef by doing
It is not, it is in driver/pci/pci.c, it is called in probe.c.
Maybe I can move the function to probe.c, but this would leave the
domain handling in two separate files.
I can't remove the #ifdeffery in that domain_nr in pci_bus is #ifdeffed
too, unless I remove that #ifdef and I compile it in all the time.
Thanks all for the review,
Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
