All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] x86, pci: Add quirk for unsizeable Broadwell EP bar
@ 2016-01-15 22:17 Andi Kleen
  2016-02-04 17:41 ` Bjorn Helgaas
  0 siblings, 1 reply; 9+ messages in thread
From: Andi Kleen @ 2016-01-15 22:17 UTC (permalink / raw)
  To: bhelgaas; +Cc: linux-pci, linux-kernel, x86, Andi Kleen

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.

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] x86, pci: Add quirk for unsizeable Broadwell EP bar
  2016-01-15 22:17 [PATCH] x86, pci: Add quirk for unsizeable Broadwell EP bar Andi Kleen
@ 2016-02-04 17:41 ` Bjorn Helgaas
  2016-02-04 18:54   ` Andi Kleen
  0 siblings, 1 reply; 9+ messages in thread
From: Bjorn Helgaas @ 2016-02-04 17:41 UTC (permalink / raw)
  To: Andi Kleen; +Cc: bhelgaas, linux-pci, linux-kernel, x86, Andi Kleen

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
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] x86, pci: Add quirk for unsizeable Broadwell EP bar
  2016-02-04 17:41 ` Bjorn Helgaas
@ 2016-02-04 18:54   ` Andi Kleen
  2016-02-05  1:57     ` Bjorn Helgaas
  0 siblings, 1 reply; 9+ messages in thread
From: Andi Kleen @ 2016-02-04 18:54 UTC (permalink / raw)
  To: Bjorn Helgaas; +Cc: Andi Kleen, bhelgaas, linux-pci, linux-kernel, x86

> 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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] x86, pci: Add quirk for unsizeable Broadwell EP bar
  2016-02-04 18:54   ` Andi Kleen
@ 2016-02-05  1:57     ` Bjorn Helgaas
  2016-02-05  3:36       ` Andi Kleen
  0 siblings, 1 reply; 9+ messages in thread
From: Bjorn Helgaas @ 2016-02-05  1:57 UTC (permalink / raw)
  To: Andi Kleen; +Cc: Andi Kleen, bhelgaas, linux-pci, linux-kernel, x86

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] x86, pci: Add quirk for unsizeable Broadwell EP bar
  2016-02-05  1:57     ` Bjorn Helgaas
@ 2016-02-05  3:36       ` Andi Kleen
  2016-02-05 16:34         ` Bjorn Helgaas
  0 siblings, 1 reply; 9+ messages in thread
From: Andi Kleen @ 2016-02-05  3:36 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Andi Kleen, Andi Kleen, bhelgaas, linux-pci, linux-kernel, x86

> > 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

-- 
ak@linux.intel.com -- Speaking for myself only.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] x86, pci: Add quirk for unsizeable Broadwell EP bar
  2016-02-05  3:36       ` Andi Kleen
@ 2016-02-05 16:34         ` Bjorn Helgaas
  2016-02-10 22:23           ` Andi Kleen
  0 siblings, 1 reply; 9+ messages in thread
From: Bjorn Helgaas @ 2016-02-05 16:34 UTC (permalink / raw)
  To: Andi Kleen; +Cc: Andi Kleen, bhelgaas, linux-pci, linux-kernel, x86

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] x86, pci: Add quirk for unsizeable Broadwell EP bar
  2016-02-05 16:34         ` Bjorn Helgaas
@ 2016-02-10 22:23           ` Andi Kleen
  2016-02-11 14:20             ` Bjorn Helgaas
  0 siblings, 1 reply; 9+ messages in thread
From: Andi Kleen @ 2016-02-10 22:23 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Andi Kleen, Andi Kleen, bhelgaas, linux-pci, linux-kernel, x86

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@linux.intel.com>

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;
 		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)			\

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] x86, pci: Add quirk for unsizeable Broadwell EP bar
  2016-02-10 22:23           ` Andi Kleen
@ 2016-02-11 14:20             ` Bjorn Helgaas
  0 siblings, 0 replies; 9+ messages in thread
From: Bjorn Helgaas @ 2016-02-11 14:20 UTC (permalink / raw)
  To: Andi Kleen; +Cc: Andi Kleen, bhelgaas, linux-pci, linux-kernel, x86

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@linux.intel.com>
> 
> 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)			\

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH] x86, pci: Add quirk for unsizeable Broadwell EP bar
@ 2016-02-17 23:45 Andi Kleen
  0 siblings, 0 replies; 9+ messages in thread
From: Andi Kleen @ 2016-02-17 23:45 UTC (permalink / raw)
  To: bhelgaas; +Cc: linux-kernel, x86, linux-pci, Andi Kleen

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.

Add a PCI quirk to shut off the messages.

v2: Handle all BARs, not just BAR0 (Chaohong Guo)
v3: Switch to patching bus ops
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/pci/fixup.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
index e585655..4fcb5d5 100644
--- a/arch/x86/pci/fixup.c
+++ b/arch/x86/pci/fixup.c
@@ -540,3 +540,48 @@ 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 inline bool bdwep_bad_bars(unsigned devfn, int where)
+{
+	return ((PCI_SLOT(devfn) == 0x12 && PCI_FUNC(devfn) == 0) ||
+		(PCI_SLOT(devfn) == 0x1e && PCI_FUNC(devfn) == 3)) &&
+		where >= 0x10 && where <= 0x24;
+}
+
+static int quirk_bdwep_bar_read(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 *value)
+{
+	if (bdwep_bad_bars(devfn, where)) {
+		*value = 0;
+		return 0;
+	}
+
+	return raw_pci_read(pci_domain_nr(bus), bus->number,
+			    devfn, where, size, value);
+}
+
+static int quirk_bdwep_bar_write(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 value)
+{
+	if (bdwep_bad_bars(devfn, where))
+		return 0;
+
+	return raw_pci_write(pci_domain_nr(bus), bus->number,
+			     devfn, where, size, value);
+}
+
+static struct pci_ops quirk_bdwep_bar_ops = {
+	.read = quirk_bdwep_bar_read,
+	.write = quirk_bdwep_bar_write,
+};
+
+static void pci_bdwep_bar(struct pci_dev *dev)
+{
+	pci_bus_set_ops(dev->bus, &quirk_bdwep_bar_ops);
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6fa0, pci_bdwep_bar);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6fc0, pci_bdwep_bar);
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2016-02-17 23:45 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-15 22:17 [PATCH] x86, pci: Add quirk for unsizeable Broadwell EP bar Andi Kleen
2016-02-04 17:41 ` Bjorn Helgaas
2016-02-04 18:54   ` Andi Kleen
2016-02-05  1:57     ` Bjorn Helgaas
2016-02-05  3:36       ` Andi Kleen
2016-02-05 16:34         ` Bjorn Helgaas
2016-02-10 22:23           ` Andi Kleen
2016-02-11 14:20             ` Bjorn Helgaas
2016-02-17 23:45 Andi Kleen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.