On Wed, Feb 10, 2016 at 11:23:33PM +0100, Andi Kleen wrote:
> On Fri, Feb 05, 2016 at 10:34:27AM -0600, Bjorn Helgaas wrote:
> > 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:
>
> All good points. I changed the patch to use a new flag
> IORESOURCE_PCI_IGNORE that skips the whole bar process
> (except for the BAR size, which are ok in my case at least).
>
> Also fixed to handle all BARs.
>
> Does that look better?
>
> ------
> x86, pci: Add quirk for unsizeable Broadwell EP bar
>
> 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.
>
> 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)
>
> I used a new resource flag that causes the BAR to be ignored
>
> v2: Handle all BARs, not just BAR0 (Chaohong Guo)
> Switch over to using IORESOURCE_PCI_IGNORE over FIXED
> Signed-off-by: Andi Kleen <ak@xxxxxxxxxxxxxxx>
>
> diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
> index e585655..837acb7 100644
> --- a/arch/x86/pci/fixup.c
> +++ b/arch/x86/pci/fixup.c
> @@ -540,3 +540,18 @@ 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 BARs 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)
> +{
> + int i;
> +
> + for (i = 0; i < 5; i++)
> + dev->resource[i].flags |= IORESOURCE_PCI_IGNORE;
> +}
> +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 6d7ab9b..7c12e6d 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -214,7 +214,9 @@ 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);
> + if (res->flags & IORESOURCE_PCI_IGNORE)
> + goto out;
Sorry, I really don't like this either. For one thing, if these
registers are not BARs, I don't want to even read them. I don't want
the possible side-effects from reading the register, and I don't want
to worry about what happens when we feed garbage to decode_bar().
But more importantly, I don't want to define a whole new IORESOURCE_*
flag just for this, and I don't want places that use resources to have
to check for the new flag.
There's lots of code that assumes "res->flags == 0" means "this
resource does not correspond to a BAR". For examples, look at the
output of this:
git grep -e 'if (!r.*->flags'
These Broadwell-EP resources don't correspond to valid BARs, so they
should look just like the resources we have for unimplemented BARs. I
think the cleanest way to accomplish this is to make the registers
look like unimplemented BARs, i.e., by making pci_read_config_dword()
read zero.
I think it should be pretty easy to define your own pci_ops that
special-cases these registers on these devices and falls back to the
existing raw_pci_read() for everything else, and you could probably
install those ops with an early quirk.
We already do something very similar for quirk_pcie_aspm_ops.
> res->flags |= IORESOURCE_SIZEALIGN;
> if (res->flags & IORESOURCE_IO) {
> l64 = l & PCI_BASE_ADDRESS_IO_MASK;
> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> index 24bea08..5262102 100644
> --- a/include/linux/ioport.h
> +++ b/include/linux/ioport.h
> @@ -104,7 +104,7 @@ struct resource {
>
> /* PCI control bits. Shares IORESOURCE_BITS with above PCI ROM. */
> #define IORESOURCE_PCI_FIXED (1<<4) /* Do not move resource */
> -
> +#define IORESOURCE_PCI_IGNORE (1<<5) /* Ignore resource */
>
> /* helpers to define resources */
> #define DEFINE_RES_NAMED(_start, _size, _name, _flags) \
--
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
