| Message ID | 1452896279-22034-1-git-send-email-andi@firstfloor.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
Hi Andi, On Fri, Jan 15, 2016 at 02:17:59PM -0800, Andi Kleen wrote: > From: Andi Kleen <ak@linux.intel.com> > > The Home Agent and PCU PCI devices in Broadwell-EP have a BAR that returns a > non zero value when read, but is still not sizeable (because it doesn't > exist). This causes several [Firmware error] messages at boot. It does > not cause any functional problems, as the devices really have no BARs. Wow, this is a pretty egregious PCI spec violation. I'm sure there's some reason behind it, but it does irk me to have to put code in the core to work around such an obvious problem. It sounds like these devices have some device-specific register where BAR 0 is supposed to be? Setting IORESOURCE_PCI_FIXED doesn't seem like the right solution to me. Even if we set that, the core still believes this resource corresponds to some address space consumed by the device. I think we will still try to size the BAR and decode its type. I think it will still show up via lspci. That's all meaningless. How do you deal with this on Windows? I think you need to replace the config accessor with a special one that knows that this register is not a BAR, and they can return zero. Or maybe the accessor should hide these devices completely, i.e., return 0xffffffff for the vendor/device ID. Or maybe you even have a switch the BIOS can use to hide them from the OS. Bjorn > Add a PCI quirk to shut off the messages. > > Since the message is printed before the normal header fixup add EARLY > fixups. This requires changing the PCI probe code to not override > the resource flags unconditionally, so that the quirk can set flags. > > (I believe that's ok, they should be always zero before, but please double > check) > > Also don't print the invalid BAR message for FIXED BARs. > > Signed-off-by: Andi Kleen <ak@linux.intel.com> > --- > arch/x86/pci/fixup.c | 12 ++++++++++++ > drivers/pci/probe.c | 8 +++++--- > 2 files changed, 17 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c > index e585655..86bbdd6 100644 > --- a/arch/x86/pci/fixup.c > +++ b/arch/x86/pci/fixup.c > @@ -540,3 +540,15 @@ static void twinhead_reserve_killing_zone(struct pci_dev *dev) > } > } > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x27B9, twinhead_reserve_killing_zone); > + > +/* > + * Intel Broadwell EP. Prevent reading/updating BAR on Home Agent and PCU devices > + * which are not real BARs, but still return non-null. > + * This prevents a harmless warning message at boot. > + */ > +static void pci_bdwep_ha_bar(struct pci_dev *dev) > +{ > + dev->resource[0].flags |= IORESOURCE_PCI_FIXED; > +} > +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6fa0, pci_bdwep_ha_bar); > +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6fc0, pci_bdwep_ha_bar); > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index edb1984..f7926e8 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -214,7 +214,7 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type, > l = 0; > > if (type == pci_bar_unknown) { > - res->flags = decode_bar(dev, l); > + res->flags |= decode_bar(dev, l); > res->flags |= IORESOURCE_SIZEALIGN; > if (res->flags & IORESOURCE_IO) { > l64 = l & PCI_BASE_ADDRESS_IO_MASK; > @@ -251,8 +251,10 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type, > > sz64 = pci_size(l64, sz64, mask64); > if (!sz64) { > - dev_info(&dev->dev, FW_BUG "reg 0x%x: invalid BAR (can't size)\n", > - pos); > + /* Don't print this message for a fixed BAR */ > + if (!(res->flags & IORESOURCE_PCI_FIXED)) > + dev_info(&dev->dev, FW_BUG "reg 0x%x: invalid BAR (can't size)\n", > + pos); > goto fail; > } > > -- > 2.4.3 > -- 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
> It sounds like these devices have some device-specific register where > BAR 0 is supposed to be? Setting IORESOURCE_PCI_FIXED doesn't seem > like the right solution to me. Even if we set that, the core still There is no actually functional register on these locations that has any side effects. > believes this resource corresponds to some address space consumed by > the device. I think we will still try to size the BAR and decode its > type. I think it will still show up via lspci. That's all > meaningless. But would actually anything use it? > How do you deal with this on Windows? > > I think you need to replace the config accessor with a special one > that knows that this register is not a BAR, and they can return zero. > Or maybe the accessor should hide these devices completely, i.e., > return 0xffffffff for the vendor/device ID. Or maybe you even have a > switch the BIOS can use to hide them from the OS. In some cases we need the devices. -Andi -- 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 Thu, Feb 04, 2016 at 10:54:42AM -0800, Andi Kleen wrote: > > It sounds like these devices have some device-specific register where > > BAR 0 is supposed to be? Setting IORESOURCE_PCI_FIXED doesn't seem > > like the right solution to me. Even if we set that, the core still > > There is no actually functional register on these locations that has any side > effects. > > > believes this resource corresponds to some address space consumed by > > the device. I think we will still try to size the BAR and decode its > > type. I think it will still show up via lspci. That's all > > meaningless. > > But would actually anything use it? You mean, would anything actually use the lspci output? I don't know, but why would we want it to print garbage? And the kernel certainly uses the struct resource. Setting IORESOURCE_PCI_FIXED is not a way of saying "please ignore this resource." 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
> > But would actually anything use it? > > You mean, would anything actually use the lspci output? I don't know, > but why would we want it to print garbage? In he kernel. I don't think lspci is that interesting. > > And the kernel certainly uses the struct resource. Setting > IORESOURCE_PCI_FIXED is not a way of saying "please ignore this > resource." There is already another quirk that uses the same technique to handle a bad bar. I also didn't notice any bad side effects. Again what would it be used for? I looked into the new device ops you asked for, but it is fairly complicated as the ops are not per device but per bus, and there can be many copies of this device, and the pci_dev is not passed, so it needs complicated pattern matching on the devfn. Doing it like the existing quirk is much simpler, and seems to work just fine. -Andi
On Fri, Feb 05, 2016 at 04:36:17AM +0100, Andi Kleen wrote: > > > But would actually anything use it? > > > > You mean, would anything actually use the lspci output? I don't know, > > but why would we want it to print garbage? > > In he kernel. I don't think lspci is that interesting. > > > > And the kernel certainly uses the struct resource. Setting > > IORESOURCE_PCI_FIXED is not a way of saying "please ignore this > > resource." > > There is already another quirk that uses the same technique to handle > a bad bar. I also didn't notice any bad side effects. Again what would it be > used for? I suppose you mean pci_siemens_interrupt_controller(), added by 73a74ed3a6f8 ("PCI: i386: fixup for Siemens Nixdorf AG FSC Multiprocessor Interrupt Controllers")? Here are the problems I see: - We still try to size the register as though it were a real BAR, which means we write 0xffffffff to it. This register isn't a BAR, so we have no idea what effect this will have on the device. In your case, you might know it's safe, but it's not safe in general - We read from the register and interpret the low bits as a BAR type. We have no idea what data we'll get from a non-BAR register, so we're essentially putting junk in the struct resource flags. - We try to figure out the size of the BAR. Depending on what data we get, we may compute some arbitrary size. You might know that we happen to compute a zero size or something else safe, but that depends on your device and is not safe in general. I don't want to have to worry about your device settings in that register being magically compatible with the pci_size() computation. - I think it's possible that the data we read from this register would lead us to conclude this is a BAR that consumes memory space at some adddress. Or I/O space at some address. We rely on this information to perform PCI address space allocation. > I looked into the new device ops you asked for, but it is fairly > complicated as the ops are not per device but per bus, and there > can be many copies of this device, and the pci_dev is not passed, > so it needs complicated pattern matching on the devfn. > > Doing it like the existing quirk is much simpler, and seems to work > just fine. It's simple and seems to work in this case, but it doesn't lead to a consistent model of the system. It may be a hassle to work around this via pci_ops, but, well, if you [Intel] want to take advantage of all the generic PCI code, you sort of have to play within the constraints of the generic PCI model. Maybe there's some other way to do a quirk that doesn't involve using IORESOURCE_PCI_FIXED. I just object to the fact that we run so much code that thinks this is a real BAR, and we have no idea what data we're getting back, so we don't know what that struct resource will end up looking like. What I *want* is for that resource to remain zeroed out, just like any other resource corresponding to an unimplemented BAR. 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
diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c index e585655..86bbdd6 100644 --- a/arch/x86/pci/fixup.c +++ b/arch/x86/pci/fixup.c @@ -540,3 +540,15 @@ static void twinhead_reserve_killing_zone(struct pci_dev *dev) } } DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x27B9, twinhead_reserve_killing_zone); + +/* + * Intel Broadwell EP. Prevent reading/updating BAR on Home Agent and PCU devices + * which are not real BARs, but still return non-null. + * This prevents a harmless warning message at boot. + */ +static void pci_bdwep_ha_bar(struct pci_dev *dev) +{ + dev->resource[0].flags |= IORESOURCE_PCI_FIXED; +} +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6fa0, pci_bdwep_ha_bar); +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6fc0, pci_bdwep_ha_bar); diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index edb1984..f7926e8 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -214,7 +214,7 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type, l = 0; if (type == pci_bar_unknown) { - res->flags = decode_bar(dev, l); + res->flags |= decode_bar(dev, l); res->flags |= IORESOURCE_SIZEALIGN; if (res->flags & IORESOURCE_IO) { l64 = l & PCI_BASE_ADDRESS_IO_MASK; @@ -251,8 +251,10 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type, sz64 = pci_size(l64, sz64, mask64); if (!sz64) { - dev_info(&dev->dev, FW_BUG "reg 0x%x: invalid BAR (can't size)\n", - pos); + /* Don't print this message for a fixed BAR */ + if (!(res->flags & IORESOURCE_PCI_FIXED)) + dev_info(&dev->dev, FW_BUG "reg 0x%x: invalid BAR (can't size)\n", + pos); goto fail; }