| Message ID | 20160922230806.GB1514@localhost (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/22/2016 07:08 PM, Bjorn Helgaas wrote: > On Wed, Sep 21, 2016 at 06:40:47PM -0400, Christopher Covington wrote: >> Hi Bjorn, >> >> On 09/21/2016 09:11 AM, Bjorn Helgaas wrote: >>> On Tue, Sep 20, 2016 at 09:15:14PM -0400, cov@codeaurora.org wrote: >> >>>>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c >>>>> index eb14f74..bb3b8ad 100644 >>>>> --- a/drivers/acpi/pci_mcfg.c >>>>> +++ b/drivers/acpi/pci_mcfg.c >>>>> @@ -42,86 +42,59 @@ struct mcfg_fixup { >>>>> struct resource cfgres; >>>>> }; >>>>> >>>>> -#define MCFG_DOM_ANY (-1) >>>> >>>> Did you delete this because there were no current users, because you'd >>>> prefer users just use "-1", or for some other reason? >>> >>> I removed it because there were no users of it and, more importantly, >>> the code doesn't implement support for it. >> >> It looks like a stale "First match against PCI topology <domain:bus>..." >> comment remains. > > Yep. I removed the comment since it's sort of obvious from the code. > I also renamed a few things and pulled the match out into a helper > function. > > I also changed the dmesg note: I think the actual resource and the > name of the pci_ecam_ops is more interesting than the table IDs (which > I think are already elsewhere in the dmesg log). It looks like the resource is already being printed from drivers/pci/ecam.c:102. > Here's the incremental diff, which I can't really test: Here's what it looks like for me: ACPI: PCI Root Bridge [PCI2] (domain 0002 [bus 00-1f]) acpi PNP0A08:02: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI] acpi PNP0A08:02: _OSC: platform does not support [PCIeHotplug] acpi PNP0A08:02: _OSC: OS now controls [PME AER PCIeCapability] acpi PNP0A08:02: MCFG quirk: ECAM space for [bus 00-1f] at [mem 0xa0000000000-0xa0001ffffff] with pci_3 acpi PNP0A08:02: ECAM at [mem 0xa0000000000-0xa0001ffffff] for [bus 00-1f] Remapped I/O 0x00000affffff0000 to [io 0x10000-0x1ffff window] PCI host bridge to bus 0002:00 > diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c > index 245b79f..0b36bc5 100644 > --- a/drivers/acpi/pci_mcfg.c > +++ b/drivers/acpi/pci_mcfg.c > @@ -36,7 +36,7 @@ struct mcfg_fixup { > char oem_id[ACPI_OEM_ID_SIZE + 1]; > char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1]; > u32 oem_revision; > - u16 seg; > + u16 segment; > struct resource bus_range; > struct pci_ecam_ops *ops; > struct resource cfgres; > @@ -102,30 +102,37 @@ static char mcfg_oem_id[ACPI_OEM_ID_SIZE]; > static char mcfg_oem_table_id[ACPI_OEM_TABLE_ID_SIZE]; > static u32 mcfg_oem_revision; > > -static void pci_mcfg_match_quirks(struct acpi_pci_root *root, > +static int pci_mcfg_quirk_matches(struct mcfg_fixup *f, u16 segment, > + struct resource *bus_range) > +{ > + if (!memcmp(f->oem_id, mcfg_oem_id, ACPI_OEM_ID_SIZE) && > + !memcmp(f->oem_table_id, mcfg_oem_table_id, > + ACPI_OEM_TABLE_ID_SIZE) && > + f->oem_revision == mcfg_oem_revision && > + f->segment == segment && > + resource_contains(&f->bus_range, bus_range)) > + return 1; > + > + return 0; > +} > + > +static void pci_mcfg_apply_quirks(struct acpi_pci_root *root, > struct resource *cfgres, > struct pci_ecam_ops **ecam_ops) > { > + u16 segment = root->segment; > + struct resource *bus_range = &root->secondary; > struct mcfg_fixup *f; > int i; > > - /* > - * First match against PCI topology <domain:bus> then use OEM ID, OEM > - * table ID, and OEM revision from MCFG table standard header. > - */ > for (i = 0, f = mcfg_quirks; i < ARRAY_SIZE(mcfg_quirks); i++, f++) { > - if (!memcmp(f->oem_id, mcfg_oem_id, ACPI_OEM_ID_SIZE) && > - !memcmp(f->oem_table_id, mcfg_oem_table_id, > - ACPI_OEM_TABLE_ID_SIZE) && > - f->oem_revision == mcfg_oem_revision && > - f->seg == root->segment && > - resource_contains(&f->bus_range, &root->secondary)) { > + if (pci_mcfg_quirk_matches(f, segment, bus_range)) { > if (f->cfgres.start) > *cfgres = f->cfgres; > if (f->ops) > *ecam_ops = f->ops; > - dev_info(&root->device->dev, "Applying PCI MCFG quirks for %s %s rev: %d\n", > - f->oem_id, f->oem_table_id, f->oem_revision); > + dev_info(&root->device->dev, "MCFG quirk: ECAM space for %pR at %pR with %ps\n", > + bus_range, cfgres, *ecam_ops); > return; > } > } > @@ -173,7 +180,7 @@ skip_lookup: > * MCFG does not have it. Invalid CFG start address means MCFG > * firmware bug or we need another quirk in array. > */ > - pci_mcfg_match_quirks(root, &res, &ops); > + pci_mcfg_apply_quirks(root, &res, &ops); > if (!res.start) > return -ENXIO; > >
On Fri, Sep 23, 2016 at 02:41:39PM -0400, Christopher Covington wrote: > On 09/22/2016 07:08 PM, Bjorn Helgaas wrote: > > On Wed, Sep 21, 2016 at 06:40:47PM -0400, Christopher Covington wrote: > >> Hi Bjorn, > >> > >> On 09/21/2016 09:11 AM, Bjorn Helgaas wrote: > >>> On Tue, Sep 20, 2016 at 09:15:14PM -0400, cov@codeaurora.org wrote: > >> > >>>>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c > >>>>> index eb14f74..bb3b8ad 100644 > >>>>> --- a/drivers/acpi/pci_mcfg.c > >>>>> +++ b/drivers/acpi/pci_mcfg.c > >>>>> @@ -42,86 +42,59 @@ struct mcfg_fixup { > >>>>> struct resource cfgres; > >>>>> }; > >>>>> > >>>>> -#define MCFG_DOM_ANY (-1) > >>>> > >>>> Did you delete this because there were no current users, because you'd > >>>> prefer users just use "-1", or for some other reason? > >>> > >>> I removed it because there were no users of it and, more importantly, > >>> the code doesn't implement support for it. > >> > >> It looks like a stale "First match against PCI topology <domain:bus>..." > >> comment remains. > > > > Yep. I removed the comment since it's sort of obvious from the code. > > I also renamed a few things and pulled the match out into a helper > > function. > > > > I also changed the dmesg note: I think the actual resource and the > > name of the pci_ecam_ops is more interesting than the table IDs (which > > I think are already elsewhere in the dmesg log). > > It looks like the resource is already being printed from > drivers/pci/ecam.c:102. Yes, but I want a hint that a quirk has overridden it because that's a clue that there's something wonky about the platform or the firmware. But I guess it'd be nice to mirror the format of the existing info (mem first, then bus range). > > Here's the incremental diff, which I can't really test: > > Here's what it looks like for me: > > ACPI: PCI Root Bridge [PCI2] (domain 0002 [bus 00-1f]) > acpi PNP0A08:02: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI] > acpi PNP0A08:02: _OSC: platform does not support [PCIeHotplug] > acpi PNP0A08:02: _OSC: OS now controls [PME AER PCIeCapability] > acpi PNP0A08:02: MCFG quirk: ECAM space for [bus 00-1f] at [mem 0xa0000000000-0xa0001ffffff] with pci_3 Is "pci_3" really the entire name? If not, what happened to the rest? I was hoping for a symbol we could grep for. > acpi PNP0A08:02: ECAM at [mem 0xa0000000000-0xa0001ffffff] for [bus 00-1f] > Remapped I/O 0x00000affffff0000 to [io 0x10000-0x1ffff window] > PCI host bridge to bus 0002:00 > > > diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c > > index 245b79f..0b36bc5 100644 > > --- a/drivers/acpi/pci_mcfg.c > > +++ b/drivers/acpi/pci_mcfg.c > > @@ -36,7 +36,7 @@ struct mcfg_fixup { > > char oem_id[ACPI_OEM_ID_SIZE + 1]; > > char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1]; > > u32 oem_revision; > > - u16 seg; > > + u16 segment; > > struct resource bus_range; > > struct pci_ecam_ops *ops; > > struct resource cfgres; > > @@ -102,30 +102,37 @@ static char mcfg_oem_id[ACPI_OEM_ID_SIZE]; > > static char mcfg_oem_table_id[ACPI_OEM_TABLE_ID_SIZE]; > > static u32 mcfg_oem_revision; > > > > -static void pci_mcfg_match_quirks(struct acpi_pci_root *root, > > +static int pci_mcfg_quirk_matches(struct mcfg_fixup *f, u16 segment, > > + struct resource *bus_range) > > +{ > > + if (!memcmp(f->oem_id, mcfg_oem_id, ACPI_OEM_ID_SIZE) && > > + !memcmp(f->oem_table_id, mcfg_oem_table_id, > > + ACPI_OEM_TABLE_ID_SIZE) && > > + f->oem_revision == mcfg_oem_revision && > > + f->segment == segment && > > + resource_contains(&f->bus_range, bus_range)) > > + return 1; > > + > > + return 0; > > +} > > + > > +static void pci_mcfg_apply_quirks(struct acpi_pci_root *root, > > struct resource *cfgres, > > struct pci_ecam_ops **ecam_ops) > > { > > + u16 segment = root->segment; > > + struct resource *bus_range = &root->secondary; > > struct mcfg_fixup *f; > > int i; > > > > - /* > > - * First match against PCI topology <domain:bus> then use OEM ID, OEM > > - * table ID, and OEM revision from MCFG table standard header. > > - */ > > for (i = 0, f = mcfg_quirks; i < ARRAY_SIZE(mcfg_quirks); i++, f++) { > > - if (!memcmp(f->oem_id, mcfg_oem_id, ACPI_OEM_ID_SIZE) && > > - !memcmp(f->oem_table_id, mcfg_oem_table_id, > > - ACPI_OEM_TABLE_ID_SIZE) && > > - f->oem_revision == mcfg_oem_revision && > > - f->seg == root->segment && > > - resource_contains(&f->bus_range, &root->secondary)) { > > + if (pci_mcfg_quirk_matches(f, segment, bus_range)) { > > if (f->cfgres.start) > > *cfgres = f->cfgres; > > if (f->ops) > > *ecam_ops = f->ops; > > - dev_info(&root->device->dev, "Applying PCI MCFG quirks for %s %s rev: %d\n", > > - f->oem_id, f->oem_table_id, f->oem_revision); > > + dev_info(&root->device->dev, "MCFG quirk: ECAM space for %pR at %pR with %ps\n", > > + bus_range, cfgres, *ecam_ops); > > return; > > } > > } > > @@ -173,7 +180,7 @@ skip_lookup: > > * MCFG does not have it. Invalid CFG start address means MCFG > > * firmware bug or we need another quirk in array. > > */ > > - pci_mcfg_match_quirks(root, &res, &ops); > > + pci_mcfg_apply_quirks(root, &res, &ops); > > if (!res.start) > > return -ENXIO; > > > > > > > -- > Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm > Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code > Aurora Forum, a Linux Foundation Collaborative Project.
On 09/23/2016 03:17 PM, Bjorn Helgaas wrote: > On Fri, Sep 23, 2016 at 02:41:39PM -0400, Christopher Covington wrote: >> On 09/22/2016 07:08 PM, Bjorn Helgaas wrote: >>> On Wed, Sep 21, 2016 at 06:40:47PM -0400, Christopher Covington wrote: >>>> Hi Bjorn, >>>> >>>> On 09/21/2016 09:11 AM, Bjorn Helgaas wrote: >>>>> On Tue, Sep 20, 2016 at 09:15:14PM -0400, cov@codeaurora.org wrote: >>>> >>>>>>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c >>>>>>> index eb14f74..bb3b8ad 100644 >>>>>>> --- a/drivers/acpi/pci_mcfg.c >>>>>>> +++ b/drivers/acpi/pci_mcfg.c >>>>>>> @@ -42,86 +42,59 @@ struct mcfg_fixup { >>>>>>> struct resource cfgres; >>>>>>> }; >>>>>>> >>>>>>> -#define MCFG_DOM_ANY (-1) >>>>>> >>>>>> Did you delete this because there were no current users, because you'd >>>>>> prefer users just use "-1", or for some other reason? >>>>> >>>>> I removed it because there were no users of it and, more importantly, >>>>> the code doesn't implement support for it. >>>> >>>> It looks like a stale "First match against PCI topology <domain:bus>..." >>>> comment remains. >>> >>> Yep. I removed the comment since it's sort of obvious from the code. >>> I also renamed a few things and pulled the match out into a helper >>> function. >>> >>> I also changed the dmesg note: I think the actual resource and the >>> name of the pci_ecam_ops is more interesting than the table IDs (which >>> I think are already elsewhere in the dmesg log). >> >> It looks like the resource is already being printed from >> drivers/pci/ecam.c:102. > > Yes, but I want a hint that a quirk has overridden it because that's a > clue that there's something wonky about the platform or the firmware. > > But I guess it'd be nice to mirror the format of the existing info > (mem first, then bus range). > >>> Here's the incremental diff, which I can't really test: >> >> Here's what it looks like for me: >> >> ACPI: PCI Root Bridge [PCI2] (domain 0002 [bus 00-1f]) >> acpi PNP0A08:02: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI] >> acpi PNP0A08:02: _OSC: platform does not support [PCIeHotplug] >> acpi PNP0A08:02: _OSC: OS now controls [PME AER PCIeCapability] >> acpi PNP0A08:02: MCFG quirk: ECAM space for [bus 00-1f] at [mem 0xa0000000000-0xa0001ffffff] with pci_3 > > Is "pci_3" really the entire name? If not, what happened to the rest? > I was hoping for a symbol we could grep for. The full name is pci_32b_ops. The print overflowed my tmux pane. >> acpi PNP0A08:02: ECAM at [mem 0xa0000000000-0xa0001ffffff] for [bus 00-1f] >> Remapped I/O 0x00000affffff0000 to [io 0x10000-0x1ffff window] >> PCI host bridge to bus 0002:00 Thanks, Cov
diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c index 245b79f..0b36bc5 100644 --- a/drivers/acpi/pci_mcfg.c +++ b/drivers/acpi/pci_mcfg.c @@ -36,7 +36,7 @@ struct mcfg_fixup { char oem_id[ACPI_OEM_ID_SIZE + 1]; char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1]; u32 oem_revision; - u16 seg; + u16 segment; struct resource bus_range; struct pci_ecam_ops *ops; struct resource cfgres; @@ -102,30 +102,37 @@ static char mcfg_oem_id[ACPI_OEM_ID_SIZE]; static char mcfg_oem_table_id[ACPI_OEM_TABLE_ID_SIZE]; static u32 mcfg_oem_revision; -static void pci_mcfg_match_quirks(struct acpi_pci_root *root, +static int pci_mcfg_quirk_matches(struct mcfg_fixup *f, u16 segment, + struct resource *bus_range) +{ + if (!memcmp(f->oem_id, mcfg_oem_id, ACPI_OEM_ID_SIZE) && + !memcmp(f->oem_table_id, mcfg_oem_table_id, + ACPI_OEM_TABLE_ID_SIZE) && + f->oem_revision == mcfg_oem_revision && + f->segment == segment && + resource_contains(&f->bus_range, bus_range)) + return 1; + + return 0; +} + +static void pci_mcfg_apply_quirks(struct acpi_pci_root *root, struct resource *cfgres, struct pci_ecam_ops **ecam_ops) { + u16 segment = root->segment; + struct resource *bus_range = &root->secondary; struct mcfg_fixup *f; int i; - /* - * First match against PCI topology <domain:bus> then use OEM ID, OEM - * table ID, and OEM revision from MCFG table standard header. - */ for (i = 0, f = mcfg_quirks; i < ARRAY_SIZE(mcfg_quirks); i++, f++) { - if (!memcmp(f->oem_id, mcfg_oem_id, ACPI_OEM_ID_SIZE) && - !memcmp(f->oem_table_id, mcfg_oem_table_id, - ACPI_OEM_TABLE_ID_SIZE) && - f->oem_revision == mcfg_oem_revision && - f->seg == root->segment && - resource_contains(&f->bus_range, &root->secondary)) { + if (pci_mcfg_quirk_matches(f, segment, bus_range)) { if (f->cfgres.start) *cfgres = f->cfgres; if (f->ops) *ecam_ops = f->ops; - dev_info(&root->device->dev, "Applying PCI MCFG quirks for %s %s rev: %d\n", - f->oem_id, f->oem_table_id, f->oem_revision); + dev_info(&root->device->dev, "MCFG quirk: ECAM space for %pR at %pR with %ps\n", + bus_range, cfgres, *ecam_ops); return; } } @@ -173,7 +180,7 @@ skip_lookup: * MCFG does not have it. Invalid CFG start address means MCFG * firmware bug or we need another quirk in array. */ - pci_mcfg_match_quirks(root, &res, &ops); + pci_mcfg_apply_quirks(root, &res, &ops); if (!res.start) return -ENXIO;