| Message ID | 1436962613-17359-5-git-send-email-marc.zyngier@arm.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Wed, Jul 15, 2015 at 01:16:38PM +0100, Marc Zyngier wrote: > In order to be able to populate the device msi_domain field, > add the necesary hooks to propagate the host bridge msi_domain s/necesary/necessary/ > across secondary busses to devices. > > So far, nobody populates the initial msi_domain. > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > drivers/pci/probe.c | 30 ++++++++++++++++++++++++++++++ > include/linux/pci.h | 1 + > 2 files changed, 31 insertions(+) > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index cefd636..376f6fa 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -661,6 +661,20 @@ static void pci_set_bus_speed(struct pci_bus *bus) > } > } > > +void __weak pcibios_set_host_bridge_msi_domain(struct pci_bus *bus) > +{ > +} > + > +static void pci_set_bus_msi_domain(struct pci_bus *bus) > +{ > + struct pci_dev *bridge = bus->self; > + > + if (!bridge) > + pcibios_set_host_bridge_msi_domain(bus); I would use pci_is_root_bus() here instead of testing "!bridge". > + else > + dev_set_msi_domain(&bus->dev, dev_get_msi_domain(&bridge->dev)); > +} > + > static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent, > struct pci_dev *bridge, int busnr) > { > @@ -714,6 +728,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent, > bridge->subordinate = child; > > add_dev: > + pci_set_bus_msi_domain(child); > ret = device_register(&child->dev); > WARN_ON(ret < 0); > > @@ -1544,6 +1559,17 @@ static void pci_init_capabilities(struct pci_dev *dev) > pci_enable_acs(dev); > } > > +static void pci_set_msi_domain(struct pci_dev *dev) > +{ > + /* > + * If no domain has been set through the pcibios callback, > + * inherit the default from the bus device. > + */ > + if (!dev_get_msi_domain(&dev->dev)) > + dev_set_msi_domain(&dev->dev, > + dev_get_msi_domain(&dev->bus->dev)); > +} > + > void pci_device_add(struct pci_dev *dev, struct pci_bus *bus) > { > int ret; > @@ -1585,6 +1611,9 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus) > ret = pcibios_add_device(dev); > WARN_ON(ret < 0); > > + /* Setup MSI irq domain */ > + pci_set_msi_domain(dev); > + > /* Notifier could use PCI capabilities */ > dev->match_driver = false; > ret = device_add(&dev->dev); > @@ -1975,6 +2004,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, > b->bridge = get_device(&bridge->dev); > device_enable_async_suspend(b->bridge); > pci_set_bus_of_node(b); > + pci_set_bus_msi_domain(b); > > if (!parent) > set_dev_node(b->bridge, pcibus_to_node(b)); > diff --git a/include/linux/pci.h b/include/linux/pci.h > index fbf245f..fe0825f 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -1644,6 +1644,7 @@ int pcibios_set_pcie_reset_state(struct pci_dev *dev, > int pcibios_add_device(struct pci_dev *dev); > void pcibios_release_device(struct pci_dev *dev); > void pcibios_penalize_isa_irq(int irq, int active); > +void pcibios_set_host_bridge_msi_domain(struct pci_bus *bus); > > #ifdef CONFIG_HIBERNATE_CALLBACKS > extern struct dev_pm_ops pcibios_pm_ops; > -- > 2.1.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 15, 2015 at 01:16:38PM +0100, Marc Zyngier wrote: > In order to be able to populate the device msi_domain field, > add the necesary hooks to propagate the host bridge msi_domain > across secondary busses to devices. > > So far, nobody populates the initial msi_domain. > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > drivers/pci/probe.c | 30 ++++++++++++++++++++++++++++++ > include/linux/pci.h | 1 + > 2 files changed, 31 insertions(+) > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index cefd636..376f6fa 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -661,6 +661,20 @@ static void pci_set_bus_speed(struct pci_bus *bus) > } > } > > +void __weak pcibios_set_host_bridge_msi_domain(struct pci_bus *bus) > +{ > +} I don't think there's anything in this series that requires this to be a weak function, is there? This is the only definition I see. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bjorn, On 21/07/15 22:26, Bjorn Helgaas wrote: > On Wed, Jul 15, 2015 at 01:16:38PM +0100, Marc Zyngier wrote: >> In order to be able to populate the device msi_domain field, >> add the necesary hooks to propagate the host bridge msi_domain >> across secondary busses to devices. >> >> So far, nobody populates the initial msi_domain. >> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >> --- >> drivers/pci/probe.c | 30 ++++++++++++++++++++++++++++++ >> include/linux/pci.h | 1 + >> 2 files changed, 31 insertions(+) >> >> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >> index cefd636..376f6fa 100644 >> --- a/drivers/pci/probe.c >> +++ b/drivers/pci/probe.c >> @@ -661,6 +661,20 @@ static void pci_set_bus_speed(struct pci_bus *bus) >> } >> } >> >> +void __weak pcibios_set_host_bridge_msi_domain(struct pci_bus *bus) >> +{ >> +} > > I don't think there's anything in this series that requires this to be a > weak function, is there? This is the only definition I see. It looks like all the pcibios_* functions so far have a weak attribute, and I've added it as a matter of consistency. I don't mind dropping it though. Thanks, M.
On Wed, Jul 22, 2015 at 9:42 AM, Marc Zyngier <marc.zyngier@arm.com> wrote: > Hi Bjorn, > > On 21/07/15 22:26, Bjorn Helgaas wrote: >> On Wed, Jul 15, 2015 at 01:16:38PM +0100, Marc Zyngier wrote: >>> In order to be able to populate the device msi_domain field, >>> add the necesary hooks to propagate the host bridge msi_domain >>> across secondary busses to devices. >>> >>> So far, nobody populates the initial msi_domain. >>> >>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >>> --- >>> drivers/pci/probe.c | 30 ++++++++++++++++++++++++++++++ >>> include/linux/pci.h | 1 + >>> 2 files changed, 31 insertions(+) >>> >>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >>> index cefd636..376f6fa 100644 >>> --- a/drivers/pci/probe.c >>> +++ b/drivers/pci/probe.c >>> @@ -661,6 +661,20 @@ static void pci_set_bus_speed(struct pci_bus *bus) >>> } >>> } >>> >>> +void __weak pcibios_set_host_bridge_msi_domain(struct pci_bus *bus) >>> +{ >>> +} >> >> I don't think there's anything in this series that requires this to be a >> weak function, is there? This is the only definition I see. > > It looks like all the pcibios_* functions so far have a weak attribute, > and I've added it as a matter of consistency. We've used pcibios_* names where we might need an arch-specific implementation. I'm not sure that's the case here -- do you envision an implementation under arch/* someday? If not, maybe it should just be a pci_* function instead of pcibios_*. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22/07/15 15:48, Bjorn Helgaas wrote: > On Wed, Jul 22, 2015 at 9:42 AM, Marc Zyngier <marc.zyngier@arm.com> wrote: >> Hi Bjorn, >> >> On 21/07/15 22:26, Bjorn Helgaas wrote: >>> On Wed, Jul 15, 2015 at 01:16:38PM +0100, Marc Zyngier wrote: >>>> In order to be able to populate the device msi_domain field, >>>> add the necesary hooks to propagate the host bridge msi_domain >>>> across secondary busses to devices. >>>> >>>> So far, nobody populates the initial msi_domain. >>>> >>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >>>> --- >>>> drivers/pci/probe.c | 30 ++++++++++++++++++++++++++++++ >>>> include/linux/pci.h | 1 + >>>> 2 files changed, 31 insertions(+) >>>> >>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >>>> index cefd636..376f6fa 100644 >>>> --- a/drivers/pci/probe.c >>>> +++ b/drivers/pci/probe.c >>>> @@ -661,6 +661,20 @@ static void pci_set_bus_speed(struct pci_bus *bus) >>>> } >>>> } >>>> >>>> +void __weak pcibios_set_host_bridge_msi_domain(struct pci_bus *bus) >>>> +{ >>>> +} >>> >>> I don't think there's anything in this series that requires this to be a >>> weak function, is there? This is the only definition I see. >> >> It looks like all the pcibios_* functions so far have a weak attribute, >> and I've added it as a matter of consistency. > > We've used pcibios_* names where we might need an arch-specific > implementation. I'm not sure that's the case here -- do you envision > an implementation under arch/* someday? If not, maybe it should just > be a pci_* function instead of pcibios_*. I could definitely see non-OF driven architectures wanting to override this. Or maybe we should turn it the other way around and make it call the various firmware interfaces (OF, ACPI...) until one of them succeeds. In which case your suggestion of making it a pci_* function makes a lot of sense. Thanks, M.
On Wed, Jul 22, 2015 at 03:54:14PM +0100, Marc Zyngier wrote: > On 22/07/15 15:48, Bjorn Helgaas wrote: > > On Wed, Jul 22, 2015 at 9:42 AM, Marc Zyngier <marc.zyngier@arm.com> wrote: > >> Hi Bjorn, > >> > >> On 21/07/15 22:26, Bjorn Helgaas wrote: > >>> On Wed, Jul 15, 2015 at 01:16:38PM +0100, Marc Zyngier wrote: > >>>> In order to be able to populate the device msi_domain field, > >>>> add the necesary hooks to propagate the host bridge msi_domain > >>>> across secondary busses to devices. > >>>> > >>>> So far, nobody populates the initial msi_domain. > >>>> > >>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > >>>> --- > >>>> drivers/pci/probe.c | 30 ++++++++++++++++++++++++++++++ > >>>> include/linux/pci.h | 1 + > >>>> 2 files changed, 31 insertions(+) > >>>> > >>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > >>>> index cefd636..376f6fa 100644 > >>>> --- a/drivers/pci/probe.c > >>>> +++ b/drivers/pci/probe.c > >>>> @@ -661,6 +661,20 @@ static void pci_set_bus_speed(struct pci_bus *bus) > >>>> } > >>>> } > >>>> > >>>> +void __weak pcibios_set_host_bridge_msi_domain(struct pci_bus *bus) > >>>> +{ > >>>> +} > >>> > >>> I don't think there's anything in this series that requires this to be a > >>> weak function, is there? This is the only definition I see. > >> > >> It looks like all the pcibios_* functions so far have a weak attribute, > >> and I've added it as a matter of consistency. > > > > We've used pcibios_* names where we might need an arch-specific > > implementation. I'm not sure that's the case here -- do you envision > > an implementation under arch/* someday? If not, maybe it should just > > be a pci_* function instead of pcibios_*. > > I could definitely see non-OF driven architectures wanting to override this. > > Or maybe we should turn it the other way around and make it call the > various firmware interfaces (OF, ACPI...) until one of them succeeds. In > which case your suggestion of making it a pci_* function makes a lot of > sense. Here's my advice, FWIW: - remove __weak - rename pcibios_set_host_bridge_msi_domain() to pci_host_bridge_msi_domain() and have it return a struct irq_domain * - pci_host_bridge_msi_domain() can call whatever firmware- or arch-specific code you need to look up the irq_domain - move the dev_set_msi_domain() call from pci_set_phb_of_msi_domain() to pci_set_bus_msi_domain() -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22/07/15 17:53, Bjorn Helgaas wrote: > On Wed, Jul 22, 2015 at 03:54:14PM +0100, Marc Zyngier wrote: >> On 22/07/15 15:48, Bjorn Helgaas wrote: >>> On Wed, Jul 22, 2015 at 9:42 AM, Marc Zyngier <marc.zyngier@arm.com> wrote: >>>> Hi Bjorn, >>>> >>>> On 21/07/15 22:26, Bjorn Helgaas wrote: >>>>> On Wed, Jul 15, 2015 at 01:16:38PM +0100, Marc Zyngier wrote: >>>>>> In order to be able to populate the device msi_domain field, >>>>>> add the necesary hooks to propagate the host bridge msi_domain >>>>>> across secondary busses to devices. >>>>>> >>>>>> So far, nobody populates the initial msi_domain. >>>>>> >>>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >>>>>> --- >>>>>> drivers/pci/probe.c | 30 ++++++++++++++++++++++++++++++ >>>>>> include/linux/pci.h | 1 + >>>>>> 2 files changed, 31 insertions(+) >>>>>> >>>>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >>>>>> index cefd636..376f6fa 100644 >>>>>> --- a/drivers/pci/probe.c >>>>>> +++ b/drivers/pci/probe.c >>>>>> @@ -661,6 +661,20 @@ static void pci_set_bus_speed(struct pci_bus *bus) >>>>>> } >>>>>> } >>>>>> >>>>>> +void __weak pcibios_set_host_bridge_msi_domain(struct pci_bus *bus) >>>>>> +{ >>>>>> +} >>>>> >>>>> I don't think there's anything in this series that requires this to be a >>>>> weak function, is there? This is the only definition I see. >>>> >>>> It looks like all the pcibios_* functions so far have a weak attribute, >>>> and I've added it as a matter of consistency. >>> >>> We've used pcibios_* names where we might need an arch-specific >>> implementation. I'm not sure that's the case here -- do you envision >>> an implementation under arch/* someday? If not, maybe it should just >>> be a pci_* function instead of pcibios_*. >> >> I could definitely see non-OF driven architectures wanting to override this. >> >> Or maybe we should turn it the other way around and make it call the >> various firmware interfaces (OF, ACPI...) until one of them succeeds. In >> which case your suggestion of making it a pci_* function makes a lot of >> sense. > > Here's my advice, FWIW: > > - remove __weak > > - rename pcibios_set_host_bridge_msi_domain() to > pci_host_bridge_msi_domain() and have it return a struct irq_domain * > > - pci_host_bridge_msi_domain() can call whatever firmware- or > arch-specific code you need to look up the irq_domain > > - move the dev_set_msi_domain() call from pci_set_phb_of_msi_domain() to > pci_set_bus_msi_domain() > Yes, seems to make sense. I end-up with something that looks much more readable (pci_set_bus_msi_domain looks fairly neat now). I'll post a new version shortly. Thanks, M.
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index cefd636..376f6fa 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -661,6 +661,20 @@ static void pci_set_bus_speed(struct pci_bus *bus) } } +void __weak pcibios_set_host_bridge_msi_domain(struct pci_bus *bus) +{ +} + +static void pci_set_bus_msi_domain(struct pci_bus *bus) +{ + struct pci_dev *bridge = bus->self; + + if (!bridge) + pcibios_set_host_bridge_msi_domain(bus); + else + dev_set_msi_domain(&bus->dev, dev_get_msi_domain(&bridge->dev)); +} + static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent, struct pci_dev *bridge, int busnr) { @@ -714,6 +728,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent, bridge->subordinate = child; add_dev: + pci_set_bus_msi_domain(child); ret = device_register(&child->dev); WARN_ON(ret < 0); @@ -1544,6 +1559,17 @@ static void pci_init_capabilities(struct pci_dev *dev) pci_enable_acs(dev); } +static void pci_set_msi_domain(struct pci_dev *dev) +{ + /* + * If no domain has been set through the pcibios callback, + * inherit the default from the bus device. + */ + if (!dev_get_msi_domain(&dev->dev)) + dev_set_msi_domain(&dev->dev, + dev_get_msi_domain(&dev->bus->dev)); +} + void pci_device_add(struct pci_dev *dev, struct pci_bus *bus) { int ret; @@ -1585,6 +1611,9 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus) ret = pcibios_add_device(dev); WARN_ON(ret < 0); + /* Setup MSI irq domain */ + pci_set_msi_domain(dev); + /* Notifier could use PCI capabilities */ dev->match_driver = false; ret = device_add(&dev->dev); @@ -1975,6 +2004,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, b->bridge = get_device(&bridge->dev); device_enable_async_suspend(b->bridge); pci_set_bus_of_node(b); + pci_set_bus_msi_domain(b); if (!parent) set_dev_node(b->bridge, pcibus_to_node(b)); diff --git a/include/linux/pci.h b/include/linux/pci.h index fbf245f..fe0825f 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1644,6 +1644,7 @@ int pcibios_set_pcie_reset_state(struct pci_dev *dev, int pcibios_add_device(struct pci_dev *dev); void pcibios_release_device(struct pci_dev *dev); void pcibios_penalize_isa_irq(int irq, int active); +void pcibios_set_host_bridge_msi_domain(struct pci_bus *bus); #ifdef CONFIG_HIBERNATE_CALLBACKS extern struct dev_pm_ops pcibios_pm_ops;
In order to be able to populate the device msi_domain field, add the necesary hooks to propagate the host bridge msi_domain across secondary busses to devices. So far, nobody populates the initial msi_domain. Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- drivers/pci/probe.c | 30 ++++++++++++++++++++++++++++++ include/linux/pci.h | 1 + 2 files changed, 31 insertions(+)