[PATCH 0/2] PCI: decode disable fixes

Since 2.6.36 (253d2e5498), we've disabled MEM & IO decoding while we size
BARs (except for host bridge devices). These patches tweak this in two
ways:

1) We only kept decoding disabled while sizing the low-order dword
of 64-bit BARs. I think we should keep it disabled while we do the
high-order dword also.

2) I think we should also disable decoding while updating 64-bit BARs,
e.g., when we move a device into an aperture of an upstream bridge. I'm
only proposing to disable decoding for 64-bit BAR updates because 32-bit
ones can be done atomically, while 64-bit updates require two writes and
the half-written state is a potential problem.

I'm considering these changes for the 3.6 merge window, which is
approaching fast, so let me know if you see issues with either of these.

---

Bjorn Helgaas (2):
PCI: leave MEM and IO decoding disabled during 64-bit BAR sizing, too
PCI: disable MEM decoding while updating 64-bit MEM BARs


drivers/pci/probe.c | 6 +++---
drivers/pci/setup-res.c | 18 ++++++++++++++++++
2 files changed, 21 insertions(+), 3 deletions(-)

--
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at [vger.kernel.org]
Please read the FAQ at [www.tux.org]

When we update 64-bit BARs, we have to perform two config writes. Between
the writes, the half-written BAR value could match a MEM access intended
for another device. This could result in corruption of this device (for
writes) or an unexpected response machine check (for reads).

To prevent this, disable MEM decoding while updating such BARs. This uses
the same safety test as 253d2e5498, which disables both MEM and IO while
sizing BARs, namely, we don't disable decoding for host bridge devices.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/setup-res.c | 18 ++++++++++++++++++
1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index eea85da..1a0e60e 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -30,6 +30,8 @@
void pci_update_resource(struct pci_dev *dev, int resno)
{
struct pci_bus_region region;
+ bool disable;
+ u16 cmd;
u32 new, check, mask;
int reg;
enum pci_bar_type type;
@@ -67,6 +69,18 @@ void pci_update_resource(struct pci_dev *dev, int resno)
new |= PCI_ROM_ADDRESS_ENABLE;
}

+ /*
+ * We can't update a 64-bit BAR atomically, so when possible,
+ * disable decoding so that a half-updated BAR won't conflict
+ * with another device.
+ */
+ disable = (res->flags & IORESOURCE_MEM_64) && !dev->mmio_always_on;
+ if (disable) {
+ pci_read_config_word(dev, PCI_COMMAND, &cmd);
+ pci_write_config_word(dev, PCI_COMMAND,
+ cmd & ~PCI_COMMAND_MEMORY);
+ }
+
pci_write_config_dword(dev, reg, new);
pci_read_config_dword(dev, reg, &check);

@@ -84,6 +98,10 @@ void pci_update_resource(struct pci_dev *dev, int resno)
"(high %#08x != %#08x)\n", resno, new, check);
}
}
+
+ if (disable)
+ pci_write_config_word(dev, PCI_COMMAND, cmd);
+
res->flags &= ~IORESOURCE_UNSET;
dev_dbg(&dev->dev, "BAR %d: set to %pR (PCI address [%#llx-%#llx])\n",
resno, res, (unsigned long long)region.start,

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at [vger.kernel.org]
Please read the FAQ at [www.tux.org]

After 253d2e5498, we disable MEM and IO decoding for most devices while we
size 32-bit BARs. However, we restore the original COMMAND register before
we size the upper 32 bits of 64-bit BARs, so we can still cause a conflict.

This patch waits to restore the original COMMAND register until we're
completely finished sizing the BAR.

Reference: [lkml.org]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/probe.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 658ac97..66b3a6f 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -152,9 +152,6 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
pci_read_config_dword(dev, pos, &sz);
pci_write_config_dword(dev, pos, l);

- if (!dev->mmio_always_on)
- pci_write_config_word(dev, PCI_COMMAND, orig_cmd);
-
/*
* All bits set in sz means the device isn't working properly.
* If the BAR isn't implemented, all bits must be 0. If it's a
@@ -239,6 +236,9 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
}

out:
+ if (!dev->mmio_always_on)
+ pci_write_config_word(dev, PCI_COMMAND, orig_cmd);
+
return (res->flags & IORESOURCE_MEM_64) ? 1 : 0;
fail:
res->flags = 0;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at [vger.kernel.org]
Please read the FAQ at [www.tux.org]

On Mon, 09 Jul 2012 12:20:18 -0600
Bjorn Helgaas <bhelgaas@google.com> wrote:

> After 253d2e5498, we disable MEM and IO decoding for most devices
> while we size 32-bit BARs. However, we restore the original COMMAND
> register before we size the upper 32 bits of 64-bit BARs, so we can
> still cause a conflict.
>
> This patch waits to restore the original COMMAND register until we're
> completely finished sizing the BAR.
agreed.

Thanks,

Jacob
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at [vger.kernel.org]
Please read the FAQ at [www.tux.org]

On Mon, Jul 9, 2012 at 12:20 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> Since 2.6.36 (253d2e5498), we've disabled MEM & IO decoding while we size
> BARs (except for host bridge devices). These patches tweak this in two
> ways:
>
> 1) We only kept decoding disabled while sizing the low-order dword
> of 64-bit BARs. I think we should keep it disabled while we do the
> high-order dword also.
>
> 2) I think we should also disable decoding while updating 64-bit BARs,
> e.g., when we move a device into an aperture of an upstream bridge. I'm
> only proposing to disable decoding for 64-bit BAR updates because 32-bit
> ones can be done atomically, while 64-bit updates require two writes and
> the half-written state is a potential problem.
>
> I'm considering these changes for the 3.6 merge window, which is
> approaching fast, so let me know if you see issues with either of these.
>
> ---
>
> Bjorn Helgaas (2):
> PCI: leave MEM and IO decoding disabled during 64-bit BAR sizing, too
> PCI: disable MEM decoding while updating 64-bit MEM BARs
>
>
> drivers/pci/probe.c | 6 +++---
> drivers/pci/setup-res.c | 18 ++++++++++++++++++
> 2 files changed, 21 insertions(+), 3 deletions(-)

I applied these to my "next" branch.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at [vger.kernel.org]
Please read the FAQ at [www.tux.org]

Hi,

On Mon, Jul 9, 2012 at 11:20 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> After 253d2e5498, we disable MEM and IO decoding for most devices while we
> size 32-bit BARs. However, we restore the original COMMAND register before
> we size the upper 32 bits of 64-bit BARs, so we can still cause a conflict.
>
> This patch waits to restore the original COMMAND register until we're
> completely finished sizing the BAR.
>
> Reference: [lkml.org]
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

This patch causes boot lockup on PA Semi hardware, since it disables
the bar on the UART that is used for console, and it has printks
between the old and the new re-enable location. If I boot with 'debug'
level for printk, I hit this. If I boot with just regular console
args, I don't.

I'm guessing any other platform that uses MMIO-based UART on PCI for
console will have similar issues. I can verify on Chrome OS x86
hardware tomorrow if legacy powerpc isn't important enough to care
about. :-)

I have no proposal for a fix for this. Can you please consider
reverting for 3.6 unless someone has a better idea?


-Olof
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at [vger.kernel.org]
Please read the FAQ at [www.tux.org]

On Thu, Aug 23, 2012 at 12:28:23AM -0700, Olof Johansson wrote:
> Hi,
>
> On Mon, Jul 9, 2012 at 11:20 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > After 253d2e5498, we disable MEM and IO decoding for most devices while we
> > size 32-bit BARs. However, we restore the original COMMAND register before
> > we size the upper 32 bits of 64-bit BARs, so we can still cause a conflict.
> >
> > This patch waits to restore the original COMMAND register until we're
> > completely finished sizing the BAR.
> >
> > Reference: [lkml.org]
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>
> This patch causes boot lockup on PA Semi hardware, since it disables
> the bar on the UART that is used for console, and it has printks
> between the old and the new re-enable location. If I boot with 'debug'
> level for printk, I hit this. If I boot with just regular console
> args, I don't.
>
> I'm guessing any other platform that uses MMIO-based UART on PCI for
> console will have similar issues. I can verify on Chrome OS x86
> hardware tomorrow if legacy powerpc isn't important enough to care
> about. :-)
>
> I have no proposal for a fix for this. Can you please consider
> reverting for 3.6 unless someone has a better idea?

Thanks a lot for finding and debugging this!

Can you try the patch below?


commit cfc29ece86d648e63fb46de81b2bf8e3e107672c
Author: Bjorn Helgaas <bhelgaas@google.com>
Date: Thu Aug 23 10:53:08 2012 -0600

PCI: Don't print anything while decoding is disabled

If we try to print to the console device while its decoding is disabled,
the system will hang.

Reported-by: Olof Johansson <olof@lixom.net>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 6c143b4..9f8a6b7 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -144,15 +144,13 @@ static inline unsigned long decode_bar(struct pci_dev *dev, u32 bar)
case PCI_BASE_ADDRESS_MEM_TYPE_32:
break;
case PCI_BASE_ADDRESS_MEM_TYPE_1M:
- dev_info(&dev->dev, "1M mem BAR treated as 32-bit BAR\n";
+ /* 1M mem BAR treated as 32-bit BAR */
break;
case PCI_BASE_ADDRESS_MEM_TYPE_64:
flags |= IORESOURCE_MEM_64;
break;
default:
- dev_warn(&dev->dev,
- "mem unknown type %x treated as 32-bit BAR\n",
- mem_type);
+ /* mem unknown type treated as 32-bit BAR */
break;
}
return flags;
@@ -173,9 +171,11 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
u32 l, sz, mask;
u16 orig_cmd;
struct pci_bus_region region;
+ bool bar_too_big = false, bar_disabled = false;

mask = type ? PCI_ROM_ADDRESS_MASK : ~0;

+ /* No printks while decoding is disabled! */
if (!dev->mmio_always_on) {
pci_read_config_word(dev, PCI_COMMAND, &orig_cmd);
pci_write_config_word(dev, PCI_COMMAND,
@@ -240,8 +240,7 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
goto fail;

if ((sizeof(resource_size_t) < 8) && (sz64 > 0x100000000ULL)) {
- dev_err(&dev->dev, "reg %x: can't handle 64-bit BAR\n",
- pos);
+ bar_too_big = true;
goto fail;
}

@@ -252,12 +251,11 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
region.start = 0;
region.end = sz64;
pcibios_bus_to_resource(dev, res, &region);
+ bar_disabled = true;
} else {
region.start = l64;
region.end = l64 + sz64;
pcibios_bus_to_resource(dev, res, &region);
- dev_printk(KERN_DEBUG, &dev->dev, "reg %x: %pR\n",
- pos, res);
}
} else {
sz = pci_size(l, sz, mask);
@@ -268,18 +266,23 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
region.start = l;
region.end = l + sz;
pcibios_bus_to_resource(dev, res, &region);
-
- dev_printk(KERN_DEBUG, &dev->dev, "reg %x: %pR\n", pos, res);
}

- out:
+ goto out;
+
+
+fail:
+ res->flags = 0;
+out:
if (!dev->mmio_always_on)
pci_write_config_word(dev, PCI_COMMAND, orig_cmd);

+ if (bar_too_big)
+ dev_err(&dev->dev, "reg %x: can't handle 64-bit BAR\n", pos);
+ if (res->flags && !bar_disabled)
+ dev_printk(KERN_DEBUG, &dev->dev, "reg %x: %pR\n", pos, res);
+
return (res->flags & IORESOURCE_MEM_64) ? 1 : 0;
- fail:
- res->flags = 0;
- goto out;
}

static void pci_read_bases(struct pci_dev *dev, unsigned int howmany, int rom)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at [vger.kernel.org]
Please read the FAQ at [www.tux.org]

Hi,

On Thu, Aug 23, 2012 at 11:02 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Thu, Aug 23, 2012 at 12:28:23AM -0700, Olof Johansson wrote:
>> Hi,
>>
>> On Mon, Jul 9, 2012 at 11:20 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> > After 253d2e5498, we disable MEM and IO decoding for most devices while we
>> > size 32-bit BARs. However, we restore the original COMMAND register before
>> > we size the upper 32 bits of 64-bit BARs, so we can still cause a conflict.
>> >
>> > This patch waits to restore the original COMMAND register until we're
>> > completely finished sizing the BAR.
>> >
>> > Reference: [lkml.org]
>> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>
>> This patch causes boot lockup on PA Semi hardware, since it disables
>> the bar on the UART that is used for console, and it has printks
>> between the old and the new re-enable location. If I boot with 'debug'
>> level for printk, I hit this. If I boot with just regular console
>> args, I don't.
>>
>> I'm guessing any other platform that uses MMIO-based UART on PCI for
>> console will have similar issues. I can verify on Chrome OS x86
>> hardware tomorrow if legacy powerpc isn't important enough to care
>> about. :-)
>>
>> I have no proposal for a fix for this. Can you please consider
>> reverting for 3.6 unless someone has a better idea?
>
> Thanks a lot for finding and debugging this!
>
> Can you try the patch below?
>
>
> commit cfc29ece86d648e63fb46de81b2bf8e3e107672c
> Author: Bjorn Helgaas <bhelgaas@google.com>
> Date: Thu Aug 23 10:53:08 2012 -0600
>
> PCI: Don't print anything while decoding is disabled
>
> If we try to print to the console device while its decoding is disabled,
> the system will hang.
>
> Reported-by: Olof Johansson <olof@lixom.net>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Ah yes, of course. Teaches me to try to do anything productive late at
night. Tested fine here.

Acked-by: Olof Johansson <olof@lixom.net>


Thanks!

-Olof
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at [vger.kernel.org]
Please read the FAQ at [www.tux.org]