| Message ID | 1453209083-3358-11-git-send-email-tn@semihalf.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Tomasz,
[auto build test ERROR on v4.4-rc8]
[cannot apply to tip/irq/core next-20160119]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
url: https://github.com/0day-ci/linux/commits/Tomasz-Nowicki/Introduce-ACPI-world-to-GICv3-ITS-irqchip/20160119-211652
config: arm64-allmodconfig (attached as .config)
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm64
All errors (new ones prefixed by >>):
drivers/irqchip/irq-gic-v3-its-pci-msi.c: In function 'its_pci_msi_parse_madt':
>> drivers/irqchip/irq-gic-v3-its-pci-msi.c:168:2: error: implicit declaration of function 'pci_msi_register_fwnode_provider' [-Werror=implicit-function-declaration]
pci_msi_register_fwnode_provider(&iort_find_pci_domain_token);
^
cc1: some warnings being treated as errors
vim +/pci_msi_register_fwnode_provider +168 drivers/irqchip/irq-gic-v3-its-pci-msi.c
162 return 0;
163 }
164
165 if (its_pci_msi_init_one(domain_handle))
166 return 0;
167
> 168 pci_msi_register_fwnode_provider(&iort_find_pci_domain_token);
169 pr_info("PCI/MSI: ITS@0x%lx domain created\n",
170 (long)its_entry->base_address);
171 return 0;
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 19/01/16 13:11, Tomasz Nowicki wrote: > After refactoring DT code, we let ACPI to build ITS PCI MSI domain > and do requester ID to device ID translation using IORT table. > > We have now full PCI MSI domain stack, thus we can enable ITS initialization > from GICv3 core driver for ACPI scenario. > > Signed-off-by: Tomasz Nowicki <tn@semihalf.com> > --- > drivers/irqchip/irq-gic-v3-its-pci-msi.c | 44 +++++++++++++++++++++++++++++++- > drivers/irqchip/irq-gic-v3.c | 3 +-- > drivers/pci/msi.c | 3 +++ > 3 files changed, 47 insertions(+), 3 deletions(-) > > diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c > index 06165cb..7f0a958 100644 > --- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c > +++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c > @@ -15,6 +15,8 @@ > * along with this program. If not, see <http://www.gnu.org/licenses/>. > */ > > +#include <linux/acpi.h> > +#include <linux/iort.h> > #include <linux/msi.h> > #include <linux/of.h> > #include <linux/of_irq.h> > @@ -143,10 +145,50 @@ static int __init its_pci_of_msi_init(void) > return 0; > } > > +#ifdef CONFIG_ACPI > + > +static int __init > +its_pci_msi_parse_madt(struct acpi_subtable_header *header, > + const unsigned long end) > +{ > + struct acpi_madt_generic_translator *its_entry; > + struct fwnode_handle *domain_handle; > + > + its_entry = (struct acpi_madt_generic_translator *)header; > + domain_handle = iort_find_its_domain_token(its_entry->translation_id); > + if (!domain_handle) { > + pr_err("ITS@0x%lx: Unable to locate ITS domain handle\n", > + (long)its_entry->base_address); > + return 0; > + } > + > + if (its_pci_msi_init_one(domain_handle)) > + return 0; > + > + pci_msi_register_fwnode_provider(&iort_find_pci_domain_token); I'm a bit worried by this. You are registering this for each and every ITS that gets probed (useless, but why not). But also, you're using a hook that is designed to work at the bus level, without caring for the actual PCI devices. That's fine for something like GICv2m, which exposes a single domain, but I can't picture how this works when you have devices sitting behind a single RC that talk to different ITSs. My understanding is that IORT was behaving in a similar way the msi-map property works, so I'm a bit puzzled here. Can you please shed some light on that? Thanks, M.
+ Charles On 10.02.2016 13:02, Marc Zyngier wrote: > On 19/01/16 13:11, Tomasz Nowicki wrote: >> After refactoring DT code, we let ACPI to build ITS PCI MSI domain >> and do requester ID to device ID translation using IORT table. >> >> We have now full PCI MSI domain stack, thus we can enable ITS initialization >> from GICv3 core driver for ACPI scenario. >> >> Signed-off-by: Tomasz Nowicki <tn@semihalf.com> >> --- >> drivers/irqchip/irq-gic-v3-its-pci-msi.c | 44 +++++++++++++++++++++++++++++++- >> drivers/irqchip/irq-gic-v3.c | 3 +-- >> drivers/pci/msi.c | 3 +++ >> 3 files changed, 47 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c >> index 06165cb..7f0a958 100644 >> --- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c >> +++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c >> @@ -15,6 +15,8 @@ >> * along with this program. If not, see <http://www.gnu.org/licenses/>. >> */ >> >> +#include <linux/acpi.h> >> +#include <linux/iort.h> >> #include <linux/msi.h> >> #include <linux/of.h> >> #include <linux/of_irq.h> >> @@ -143,10 +145,50 @@ static int __init its_pci_of_msi_init(void) >> return 0; >> } >> >> +#ifdef CONFIG_ACPI >> + >> +static int __init >> +its_pci_msi_parse_madt(struct acpi_subtable_header *header, >> + const unsigned long end) >> +{ >> + struct acpi_madt_generic_translator *its_entry; >> + struct fwnode_handle *domain_handle; >> + >> + its_entry = (struct acpi_madt_generic_translator *)header; >> + domain_handle = iort_find_its_domain_token(its_entry->translation_id); >> + if (!domain_handle) { >> + pr_err("ITS@0x%lx: Unable to locate ITS domain handle\n", >> + (long)its_entry->base_address); >> + return 0; >> + } >> + >> + if (its_pci_msi_init_one(domain_handle)) >> + return 0; >> + >> + pci_msi_register_fwnode_provider(&iort_find_pci_domain_token); > > I'm a bit worried by this. You are registering this for each and every > ITS that gets probed (useless, but why not). But also, you're using a > hook that is designed to work at the bus level, without caring for the > actual PCI devices. That's fine for something like GICv2m, which exposes > a single domain, but I can't picture how this works when you have > devices sitting behind a single RC that talk to different ITSs. > > My understanding is that IORT was behaving in a similar way the msi-map > property works, so I'm a bit puzzled here. > > Can you please shed some light on that? > I see your point now. It is possible to describe such case in IORT, for example: ******************************************** RC0 node: --------------- Mapping 0: <input ID range> -> <output ID range> <0:100> -> <0:100> parent -> ITS0 --------------- Mapping 1: <input ID range> -> <output ID range> <101:200> -> <101:200> parent -> ITS1 --------------- ******************************************** So for this scenario I cannot use pci_host_bridge_acpi_msi_domain() to find IRQ domain based on bus device (unless there is only one ITS bound to e.g. RC), I should rather add ACPI implementation to pci_msi_get_device_domain on per-device MSI basis. Do you agree? BTW. I should have put IORT specification link to changelog: http://infocenter.arm.com/help/topic/com.arm.doc.den0049a/DEN0049A_IO_Remapping_Table.pdf Thanks, Tomasz
On 12/02/16 12:26, Tomasz Nowicki wrote: > + Charles > > On 10.02.2016 13:02, Marc Zyngier wrote: >> On 19/01/16 13:11, Tomasz Nowicki wrote: >>> After refactoring DT code, we let ACPI to build ITS PCI MSI domain >>> and do requester ID to device ID translation using IORT table. >>> >>> We have now full PCI MSI domain stack, thus we can enable ITS initialization >>> from GICv3 core driver for ACPI scenario. >>> >>> Signed-off-by: Tomasz Nowicki <tn@semihalf.com> >>> --- >>> drivers/irqchip/irq-gic-v3-its-pci-msi.c | 44 +++++++++++++++++++++++++++++++- >>> drivers/irqchip/irq-gic-v3.c | 3 +-- >>> drivers/pci/msi.c | 3 +++ >>> 3 files changed, 47 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c >>> index 06165cb..7f0a958 100644 >>> --- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c >>> +++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c >>> @@ -15,6 +15,8 @@ >>> * along with this program. If not, see <http://www.gnu.org/licenses/>. >>> */ >>> >>> +#include <linux/acpi.h> >>> +#include <linux/iort.h> >>> #include <linux/msi.h> >>> #include <linux/of.h> >>> #include <linux/of_irq.h> >>> @@ -143,10 +145,50 @@ static int __init its_pci_of_msi_init(void) >>> return 0; >>> } >>> >>> +#ifdef CONFIG_ACPI >>> + >>> +static int __init >>> +its_pci_msi_parse_madt(struct acpi_subtable_header *header, >>> + const unsigned long end) >>> +{ >>> + struct acpi_madt_generic_translator *its_entry; >>> + struct fwnode_handle *domain_handle; >>> + >>> + its_entry = (struct acpi_madt_generic_translator *)header; >>> + domain_handle = iort_find_its_domain_token(its_entry->translation_id); >>> + if (!domain_handle) { >>> + pr_err("ITS@0x%lx: Unable to locate ITS domain handle\n", >>> + (long)its_entry->base_address); >>> + return 0; >>> + } >>> + >>> + if (its_pci_msi_init_one(domain_handle)) >>> + return 0; >>> + >>> + pci_msi_register_fwnode_provider(&iort_find_pci_domain_token); >> >> I'm a bit worried by this. You are registering this for each and every >> ITS that gets probed (useless, but why not). But also, you're using a >> hook that is designed to work at the bus level, without caring for the >> actual PCI devices. That's fine for something like GICv2m, which exposes >> a single domain, but I can't picture how this works when you have >> devices sitting behind a single RC that talk to different ITSs. >> >> My understanding is that IORT was behaving in a similar way the msi-map >> property works, so I'm a bit puzzled here. >> >> Can you please shed some light on that? >> > > I see your point now. It is possible to describe such case in IORT, for > example: > > ******************************************** > RC0 node: > --------------- > Mapping 0: > <input ID range> -> <output ID range> > <0:100> -> <0:100> > parent -> ITS0 > --------------- > Mapping 1: > <input ID range> -> <output ID range> > <101:200> -> <101:200> > parent -> ITS1 > --------------- > ******************************************** > > So for this scenario I cannot use pci_host_bridge_acpi_msi_domain() to > find IRQ domain based on bus device (unless there is only one ITS bound > to e.g. RC), I should rather add ACPI implementation to > pci_msi_get_device_domain on per-device MSI basis. Do you agree? That's what I was angling for, having had that in mind when writing that particular piece of code (see 54fa97eeb for the rational). You should also split the plugging of IORT into the MSI layer from the ITS driver (last hunk of this patch). Thanks, M.
diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c index 06165cb..7f0a958 100644 --- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c +++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c @@ -15,6 +15,8 @@ * along with this program. If not, see <http://www.gnu.org/licenses/>. */ +#include <linux/acpi.h> +#include <linux/iort.h> #include <linux/msi.h> #include <linux/of.h> #include <linux/of_irq.h> @@ -143,10 +145,50 @@ static int __init its_pci_of_msi_init(void) return 0; } +#ifdef CONFIG_ACPI + +static int __init +its_pci_msi_parse_madt(struct acpi_subtable_header *header, + const unsigned long end) +{ + struct acpi_madt_generic_translator *its_entry; + struct fwnode_handle *domain_handle; + + its_entry = (struct acpi_madt_generic_translator *)header; + domain_handle = iort_find_its_domain_token(its_entry->translation_id); + if (!domain_handle) { + pr_err("ITS@0x%lx: Unable to locate ITS domain handle\n", + (long)its_entry->base_address); + return 0; + } + + if (its_pci_msi_init_one(domain_handle)) + return 0; + + pci_msi_register_fwnode_provider(&iort_find_pci_domain_token); + pr_info("PCI/MSI: ITS@0x%lx domain created\n", + (long)its_entry->base_address); + return 0; +} + +static int __init its_pci_acpi_msi_init(void) +{ + acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR, + its_pci_msi_parse_madt, 0); + return 0; +} +#else +inline static int __init its_pci_acpi_msi_init(void) +{ + return 0; +} +#endif + static int __init its_pci_msi_init(void) { its_pci_of_msi_init(); + its_pci_acpi_msi_init(); + return 0; } - early_initcall(its_pci_msi_init); diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c index 995b7251..fee635e 100644 --- a/drivers/irqchip/irq-gic-v3.c +++ b/drivers/irqchip/irq-gic-v3.c @@ -871,8 +871,7 @@ static int __init gic_init_bases(void __iomem *dist_base, set_handle_irq(gic_handle_irq); - if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) && gic_dist_supports_lpis() && - to_of_node(handle)) /* Temp hack to prevent ITS init for ACPI */ + if (IS_ENABLED(CONFIG_ARM_GIC_V3_ITS) && gic_dist_supports_lpis()) its_init(handle, &gic_data.rdists, gic_data.domain); gic_smp_init(); diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c index 7eaa4c8..6ced37b 100644 --- a/drivers/pci/msi.c +++ b/drivers/pci/msi.c @@ -18,6 +18,7 @@ #include <linux/smp.h> #include <linux/errno.h> #include <linux/io.h> +#include <linux/iort.h> #include <linux/slab.h> #include <linux/irqdomain.h> #include <linux/of_irq.h> @@ -1366,6 +1367,8 @@ u32 pci_msi_domain_get_msi_rid(struct irq_domain *domain, struct pci_dev *pdev) of_node = irq_domain_get_of_node(domain); if (of_node) rid = of_msi_map_rid(&pdev->dev, of_node, rid); + else + iort_find_pci_id(pdev, rid, &rid); return rid; }
After refactoring DT code, we let ACPI to build ITS PCI MSI domain and do requester ID to device ID translation using IORT table. We have now full PCI MSI domain stack, thus we can enable ITS initialization from GICv3 core driver for ACPI scenario. Signed-off-by: Tomasz Nowicki <tn@semihalf.com> --- drivers/irqchip/irq-gic-v3-its-pci-msi.c | 44 +++++++++++++++++++++++++++++++- drivers/irqchip/irq-gic-v3.c | 3 +-- drivers/pci/msi.c | 3 +++ 3 files changed, 47 insertions(+), 3 deletions(-)