On Friday 26 September 2008, Bjorn Helgaas wrote:
> http://bugzilla.kernel.org/show_bug.cgi?id=11550
Sorry for having to revive this old thread. In November 2008 I reported
that this issue had been solved for me as a result of 1f98757776ea, but I
now find that was due to faulty testing. (I suspect that changing the BIOS
setting that affects this issue on my Toshiba laptop only takes effect
after a cold boot, not a normal reboot.)
The problem was that with the BIOS setting for "Device config" set to
"Setup by OS", I get 78 messages like:
pnp 00:08: io resource (0x2e-0x2f) overlaps 0000:00:1f.5 BAR 0 (0x0-0xff), disabling
pnp 00:08: io resource (0x2e-0x2f) overlaps 0000:00:1f.6 BAR 0 (0x0-0xff), disabling
If the BIOS setting is set to "All Devices", the problem does not occur.
The origin of these messages was bisected to:
commit aee3ad815dd291a7193ab01da0f1a30c84d00061
Author: Bjorn Helgaas <[email protected]>
Date: Fri Jun 27 16:56:57 2008 -0600
PNP: replace pnp_resource_table with dynamically allocated resources
Last analysis from Bjorn was:
> The problem seems to be that Frans has some PCI devices that are not
> configured by the BIOS, and their BARs contain zero. A PNP quirk
> checks for overlaps of PCI devices and PNP devices, and those zero-
> valued BARs of course conflict with the PNP motherboard devices that
> describe legacy hardware.
>
> Here's another approach based on section 3.5 of the PCI Firmware spec.
> It says:
>
> Since not all devices may be configured prior to the operating
> system handoff, the operating system needs to know whether a
> specific BAR register has been configured by firmware. The operating
> system makes the determination by checking the I/O Enable, and
> Memory Enable bits in the device's command register, and Expansion
> ROM BAR enable bits. If the enable bit is set, then the corresponding
> resource register has been configured.
>
> So instead of checking whether the BAR contains zero, the patch below
> checks the I/O, Mem, and ROM BAR enable bits to determine whether a
> BAR is enabled.
Below the then proposed patch from Bjorn, rediffed against 2.6.29-rc7.
I've verified that the patch still solves the issue for me. Attached
dmesg output for 2.6.29-rc7 without and with the patch.
Bjorn, could you please consider this patch for inclusion again?
Original thread: http://marc.info/?l=linux-kernel&m=122095745403793&w=4
TIA and sorry for the confusion,
FJP
diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index 32e8d88..e63f800 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -26,6 +26,28 @@
#include "pci.h"
+int pci_resource_enabled(struct pci_dev *dev, int bar)
+{
+ u16 command = 0;
+ u32 addr = 0;
+
+ pci_read_config_word(dev, PCI_COMMAND, &command);
+
+ if (pci_resource_flags(dev, bar) & IORESOURCE_IO)
+ return command & PCI_COMMAND_IO;
+
+ if (command & PCI_COMMAND_MEMORY) {
+ if (bar == PCI_ROM_RESOURCE) {
+ pci_read_config_dword(dev, dev->rom_base_reg, &addr);
+ return addr & PCI_ROM_ADDRESS_ENABLE;
+ }
+
+ return 1;
+ }
+
+ return 0;
+}
+
void pci_update_resource(struct pci_dev *dev, int resno)
{
struct pci_bus_region region;
diff --git a/drivers/pnp/quirks.c b/drivers/pnp/quirks.c
index 8473fe5..1f37988 100644
--- a/drivers/pnp/quirks.c
+++ b/drivers/pnp/quirks.c
@@ -247,6 +247,9 @@ static void quirk_system_pci_resources(struct pnp_dev *dev)
for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
unsigned long type;
+ if (!pci_resource_enabled(pdev, i))
+ continue;
+
type = pci_resource_flags(pdev, i) &
(IORESOURCE_IO | IORESOURCE_MEM);
if (!type || pci_resource_len(pdev, i) == 0)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index c927ae9..9848ac2 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -870,6 +870,8 @@ static inline int pci_proc_domain(struct pci_bus *bus)
}
#endif /* CONFIG_PCI_DOMAINS */
+extern int pci_resource_enabled(struct pci_dev *dev, int bar);
+
#else /* CONFIG_PCI is not enabled */
/*
@@ -1050,6 +1052,9 @@ static inline struct pci_dev *pci_get_bus_and_slot(unsigned int bus,
unsigned int devfn)
{ return NULL; }
+static inline int pci_resource_enabled(struct pci_dev *dev, int bar)
+{ return 0; }
+
#endif /* CONFIG_PCI */
/* Include architecture-dependent settings and functions */
On Wednesday 04 March 2009 01:17:15 pm Frans Pop wrote:
> On Friday 26 September 2008, Bjorn Helgaas wrote:
> > http://bugzilla.kernel.org/show_bug.cgi?id=11550
>
> Sorry for having to revive this old thread. In November 2008 I reported
> that this issue had been solved for me as a result of 1f98757776ea, but I
> now find that was due to faulty testing. (I suspect that changing the BIOS
> setting that affects this issue on my Toshiba laptop only takes effect
> after a cold boot, not a normal reboot.)
>
> The problem was that with the BIOS setting for "Device config" set to
> "Setup by OS", I get 78 messages like:
> pnp 00:08: io resource (0x2e-0x2f) overlaps 0000:00:1f.5 BAR 0 (0x0-0xff), disabling
> pnp 00:08: io resource (0x2e-0x2f) overlaps 0000:00:1f.6 BAR 0 (0x0-0xff), disabling
>
> If the BIOS setting is set to "All Devices", the problem does not occur.
>
> The origin of these messages was bisected to:
> commit aee3ad815dd291a7193ab01da0f1a30c84d00061
> Author: Bjorn Helgaas <[email protected]>
> Date: Fri Jun 27 16:56:57 2008 -0600
> PNP: replace pnp_resource_table with dynamically allocated resources
>
> Last analysis from Bjorn was:
> > The problem seems to be that Frans has some PCI devices that are not
> > configured by the BIOS, and their BARs contain zero. A PNP quirk
> > checks for overlaps of PCI devices and PNP devices, and those zero-
> > valued BARs of course conflict with the PNP motherboard devices that
> > describe legacy hardware.
> >
> > Here's another approach based on section 3.5 of the PCI Firmware spec.
> > It says:
> >
> > Since not all devices may be configured prior to the operating
> > system handoff, the operating system needs to know whether a
> > specific BAR register has been configured by firmware. The operating
> > system makes the determination by checking the I/O Enable, and
> > Memory Enable bits in the device's command register, and Expansion
> > ROM BAR enable bits. If the enable bit is set, then the corresponding
> > resource register has been configured.
> >
> > So instead of checking whether the BAR contains zero, the patch below
> > checks the I/O, Mem, and ROM BAR enable bits to determine whether a
> > BAR is enabled.
>
> Below the then proposed patch from Bjorn, rediffed against 2.6.29-rc7.
> I've verified that the patch still solves the issue for me. Attached
> dmesg output for 2.6.29-rc7 without and with the patch.
>
> Bjorn, could you please consider this patch for inclusion again?
>
> Original thread: http://marc.info/?l=linux-kernel&m=122095745403793&w=4
Seems like we do need something, but this patch is kind of a klunky
approach, so I'd like to come up with a better proposal. I don't
have any better ideas yet, though.
Bjorn
> diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
> index 32e8d88..e63f800 100644
> --- a/drivers/pci/setup-res.c
> +++ b/drivers/pci/setup-res.c
> @@ -26,6 +26,28 @@
> #include "pci.h"
>
>
> +int pci_resource_enabled(struct pci_dev *dev, int bar)
> +{
> + u16 command = 0;
> + u32 addr = 0;
> +
> + pci_read_config_word(dev, PCI_COMMAND, &command);
> +
> + if (pci_resource_flags(dev, bar) & IORESOURCE_IO)
> + return command & PCI_COMMAND_IO;
> +
> + if (command & PCI_COMMAND_MEMORY) {
> + if (bar == PCI_ROM_RESOURCE) {
> + pci_read_config_dword(dev, dev->rom_base_reg, &addr);
> + return addr & PCI_ROM_ADDRESS_ENABLE;
> + }
> +
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> void pci_update_resource(struct pci_dev *dev, int resno)
> {
> struct pci_bus_region region;
> diff --git a/drivers/pnp/quirks.c b/drivers/pnp/quirks.c
> index 8473fe5..1f37988 100644
> --- a/drivers/pnp/quirks.c
> +++ b/drivers/pnp/quirks.c
> @@ -247,6 +247,9 @@ static void quirk_system_pci_resources(struct pnp_dev *dev)
> for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> unsigned long type;
>
> + if (!pci_resource_enabled(pdev, i))
> + continue;
> +
> type = pci_resource_flags(pdev, i) &
> (IORESOURCE_IO | IORESOURCE_MEM);
> if (!type || pci_resource_len(pdev, i) == 0)
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index c927ae9..9848ac2 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -870,6 +870,8 @@ static inline int pci_proc_domain(struct pci_bus *bus)
> }
> #endif /* CONFIG_PCI_DOMAINS */
>
> +extern int pci_resource_enabled(struct pci_dev *dev, int bar);
> +
> #else /* CONFIG_PCI is not enabled */
>
> /*
> @@ -1050,6 +1052,9 @@ static inline struct pci_dev *pci_get_bus_and_slot(unsigned int bus,
> unsigned int devfn)
> { return NULL; }
>
> +static inline int pci_resource_enabled(struct pci_dev *dev, int bar)
> +{ return 0; }
> +
> #endif /* CONFIG_PCI */
>
> /* Include architecture-dependent settings and functions */
>
>
On Wed, 4 Mar 2009 14:53:51 -0700
Bjorn Helgaas <[email protected]> wrote:
> On Wednesday 04 March 2009 01:17:15 pm Frans Pop wrote:
> > Original thread:
> > http://marc.info/?l=linux-kernel&m=122095745403793&w=4
>
> Seems like we do need something, but this patch is kind of a klunky
> approach, so I'd like to come up with a better proposal. I don't
> have any better ideas yet, though.
Patch actually seems pretty reasonable to me, though like we discussed
at kernel summit last year, there are places where a 0 resource is
assumed to mean "not assigned". And clearly we need to do something
here... Anyone else have better ideas than Bjorn's patch below?
> > diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
> > index 32e8d88..e63f800 100644
> > --- a/drivers/pci/setup-res.c
> > +++ b/drivers/pci/setup-res.c
> > @@ -26,6 +26,28 @@
> > #include "pci.h"
> >
> >
> > +int pci_resource_enabled(struct pci_dev *dev, int bar)
> > +{
> > + u16 command = 0;
> > + u32 addr = 0;
> > +
> > + pci_read_config_word(dev, PCI_COMMAND, &command);
> > +
> > + if (pci_resource_flags(dev, bar) & IORESOURCE_IO)
> > + return command & PCI_COMMAND_IO;
> > +
> > + if (command & PCI_COMMAND_MEMORY) {
> > + if (bar == PCI_ROM_RESOURCE) {
> > + pci_read_config_dword(dev,
> > dev->rom_base_reg, &addr);
> > + return addr & PCI_ROM_ADDRESS_ENABLE;
> > + }
> > +
> > + return 1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > void pci_update_resource(struct pci_dev *dev, int resno)
> > {
> > struct pci_bus_region region;
> > diff --git a/drivers/pnp/quirks.c b/drivers/pnp/quirks.c
> > index 8473fe5..1f37988 100644
> > --- a/drivers/pnp/quirks.c
> > +++ b/drivers/pnp/quirks.c
> > @@ -247,6 +247,9 @@ static void quirk_system_pci_resources(struct
> > pnp_dev *dev) for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> > unsigned long type;
> >
> > + if (!pci_resource_enabled(pdev, i))
> > + continue;
> > +
> > type = pci_resource_flags(pdev, i) &
> > (IORESOURCE_IO |
> > IORESOURCE_MEM); if (!type || pci_resource_len(pdev, i) == 0)
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index c927ae9..9848ac2 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -870,6 +870,8 @@ static inline int pci_proc_domain(struct
> > pci_bus *bus) }
> > #endif /* CONFIG_PCI_DOMAINS */
> >
> > +extern int pci_resource_enabled(struct pci_dev *dev, int bar);
> > +
> > #else /* CONFIG_PCI is not enabled */
> >
> > /*
> > @@ -1050,6 +1052,9 @@ static inline struct pci_dev
> > *pci_get_bus_and_slot(unsigned int bus, unsigned int devfn)
> > { return NULL; }
> >
> > +static inline int pci_resource_enabled(struct pci_dev *dev, int
> > bar) +{ return 0; }
> > +
> > #endif /* CONFIG_PCI */
> >
> > /* Include architecture-dependent settings and functions */
> >
> >
>
>
>
--
Jesse Barnes, Intel Open Source Technology Center
On Thursday 19 March 2009 08:07:51 pm Jesse Barnes wrote:
> On Wed, 4 Mar 2009 14:53:51 -0700
> Bjorn Helgaas <[email protected]> wrote:
>
> > On Wednesday 04 March 2009 01:17:15 pm Frans Pop wrote:
> > > Original thread:
> > > http://marc.info/?l=linux-kernel&m=122095745403793&w=4
> >
> > Seems like we do need something, but this patch is kind of a klunky
> > approach, so I'd like to come up with a better proposal. I don't
> > have any better ideas yet, though.
>
> Patch actually seems pretty reasonable to me, though like we discussed
> at kernel summit last year, there are places where a 0 resource is
> assumed to mean "not assigned". And clearly we need to do something
> here... Anyone else have better ideas than Bjorn's patch below?
IIRC, Linus complained that it was ugly and slow to do all those
config space reads, and I have to agree with him. I'd like it
better if we had some sort of pci_dev "enabled" flag or if we could
make it so the pci_dev resources were invalid when the device is
disabled.
Bjorn
> > > diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
> > > index 32e8d88..e63f800 100644
> > > --- a/drivers/pci/setup-res.c
> > > +++ b/drivers/pci/setup-res.c
> > > @@ -26,6 +26,28 @@
> > > #include "pci.h"
> > >
> > >
> > > +int pci_resource_enabled(struct pci_dev *dev, int bar)
> > > +{
> > > + u16 command = 0;
> > > + u32 addr = 0;
> > > +
> > > + pci_read_config_word(dev, PCI_COMMAND, &command);
> > > +
> > > + if (pci_resource_flags(dev, bar) & IORESOURCE_IO)
> > > + return command & PCI_COMMAND_IO;
> > > +
> > > + if (command & PCI_COMMAND_MEMORY) {
> > > + if (bar == PCI_ROM_RESOURCE) {
> > > + pci_read_config_dword(dev,
> > > dev->rom_base_reg, &addr);
> > > + return addr & PCI_ROM_ADDRESS_ENABLE;
> > > + }
> > > +
> > > + return 1;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > void pci_update_resource(struct pci_dev *dev, int resno)
> > > {
> > > struct pci_bus_region region;
> > > diff --git a/drivers/pnp/quirks.c b/drivers/pnp/quirks.c
> > > index 8473fe5..1f37988 100644
> > > --- a/drivers/pnp/quirks.c
> > > +++ b/drivers/pnp/quirks.c
> > > @@ -247,6 +247,9 @@ static void quirk_system_pci_resources(struct
> > > pnp_dev *dev) for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> > > unsigned long type;
> > >
> > > + if (!pci_resource_enabled(pdev, i))
> > > + continue;
> > > +
> > > type = pci_resource_flags(pdev, i) &
> > > (IORESOURCE_IO |
> > > IORESOURCE_MEM); if (!type || pci_resource_len(pdev, i) == 0)
> > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > index c927ae9..9848ac2 100644
> > > --- a/include/linux/pci.h
> > > +++ b/include/linux/pci.h
> > > @@ -870,6 +870,8 @@ static inline int pci_proc_domain(struct
> > > pci_bus *bus) }
> > > #endif /* CONFIG_PCI_DOMAINS */
> > >
> > > +extern int pci_resource_enabled(struct pci_dev *dev, int bar);
> > > +
> > > #else /* CONFIG_PCI is not enabled */
> > >
> > > /*
> > > @@ -1050,6 +1052,9 @@ static inline struct pci_dev
> > > *pci_get_bus_and_slot(unsigned int bus, unsigned int devfn)
> > > { return NULL; }
> > >
> > > +static inline int pci_resource_enabled(struct pci_dev *dev, int
> > > bar) +{ return 0; }
> > > +
> > > #endif /* CONFIG_PCI */
> > >
> > > /* Include architecture-dependent settings and functions */
> > >
> > >
> >
> >
> >
>
>