Please revert 928bea964827d7824b548c1f8e06eccbbc4d0d7d

Hi Linus, Yinghai !

Please consider reverting:

928bea964827d7824b548c1f8e06eccbbc4d0d7d
PCI: Delay enabling bridges until they're needed

(I'd suggest to revert now and maybe merge a better patch later)

This breaks PCI on the PowerPC "powernv" platform (which is booted via
kexec) and probably x86 as well under similar circumstances. It will
basically break PCIe if the bus master bit of the bridge isn't set at
boot (by the firmware for example, or because kexec'ing cleared it).

The reason is that the PCIe port driver will call pci_enable_device() on
the bridge (on everything under the sun actually), which will marked the
device enabled (but will not do a set_master).

Because of that, pci_enable_bridge() later on (called as a result of the
child device driver doing pci_enable_device) will see the bridge as
already enabled and will not call pci_set_master() on it.

Now, this could probably be fixed by simply doing pci_set_master() in
the PCIe port driver, but I find the whole logic very fragile (anything
that "enables" the bridge without setting master or for some reason
clears master will forever fail to re-enable it).

Maybe a better option is to unconditionally do pci_set_mater() in
pci_enable_bridge(), ie, move the call to before the enabled test.

However I am not too happy with that either. My experience with bridges
is that if bus master isn't set, they will also fail to report AER
errors and other similar upstream transactions. We might want to get
these reported properly even if no downstream device got successfully
enabled.

So I think the premises of the patches are flawed, at least on PCI
express, and we should stick to always enabling bridges (at least the
bus master bit on them).

Cheers,
Ben.

On Fri, Sep 27, 2013 at 1:28 AM, Benjamin Herrenschmidt
[off-list ref] wrote:

Hi Linus, Yinghai !

Please consider reverting:

928bea964827d7824b548c1f8e06eccbbc4d0d7d
PCI: Delay enabling bridges until they're needed

(I'd suggest to revert now and maybe merge a better patch later)

This breaks PCI on the PowerPC "powernv" platform (which is booted via
kexec) and probably x86 as well under similar circumstances. It will
basically break PCIe if the bus master bit of the bridge isn't set at
boot (by the firmware for example, or because kexec'ing cleared it).

The reason is that the PCIe port driver will call pci_enable_device() on
the bridge (on everything under the sun actually), which will marked the
device enabled (but will not do a set_master).

Because of that, pci_enable_bridge() later on (called as a result of the
child device driver doing pci_enable_device) will see the bridge as
already enabled and will not call pci_set_master() on it.

Now, this could probably be fixed by simply doing pci_set_master() in
the PCIe port driver, but I find the whole logic very fragile (anything
that "enables" the bridge without setting master or for some reason
clears master will forever fail to re-enable it).

Maybe a better option is to unconditionally do pci_set_mater() in
pci_enable_bridge(), ie, move the call to before the enabled test.

However I am not too happy with that either. My experience with bridges
is that if bus master isn't set, they will also fail to report AER
errors and other similar upstream transactions. We might want to get
these reported properly even if no downstream device got successfully
enabled.

So I think the premises of the patches are flawed, at least on PCI
express, and we should stick to always enabling bridges (at least the
bus master bit on them).

there is pci_set_master everywhere in pci drivers.

So i would like to use the first way that you suggest : call pci_set_master
PCIe port driver.

Thanks

Yinghai

On Fri, Sep 27, 2013 at 9:01 AM, Yinghai Lu [off-list ref] wrote:

So i would like to use the first way that you suggest : call pci_set_master
PCIe port driver.

So I have to say, that if we can fix this with just adding a single
new pci_set_master() call, we should do that before we decide to
revert.

If other, bigger issues then come up, we can decide to revert. But if
there's a one-liner fix, let's just do that first, ok?

Mind sending a patch?

             Linus

On Fri, Sep 27, 2013 at 9:01 AM, Yinghai Lu [off-list ref] wrote:

On Fri, Sep 27, 2013 at 1:28 AM, Benjamin Herrenschmidt
[off-list ref] wrote:

quoted

Hi Linus, Yinghai !

Please consider reverting:

928bea964827d7824b548c1f8e06eccbbc4d0d7d
PCI: Delay enabling bridges until they're needed

(I'd suggest to revert now and maybe merge a better patch later)

This breaks PCI on the PowerPC "powernv" platform (which is booted via
kexec) and probably x86 as well under similar circumstances. It will
basically break PCIe if the bus master bit of the bridge isn't set at
boot (by the firmware for example, or because kexec'ing cleared it).

The reason is that the PCIe port driver will call pci_enable_device() on
the bridge (on everything under the sun actually), which will marked the
device enabled (but will not do a set_master).

Because of that, pci_enable_bridge() later on (called as a result of the
child device driver doing pci_enable_device) will see the bridge as
already enabled and will not call pci_set_master() on it.

Ben,

looks like PCIe port driver does call pci_enable_device AND pci_set_master()

| int pcie_port_device_register(struct pci_dev *dev)
| {
|        int status, capabilities, i, nr_service;
|        int irqs[PCIE_PORT_DEVICE_MAXSERVICES];
|
|        /* Enable PCI Express port device */
|        status = pci_enable_device(dev);
|        if (status)
|                return status;
|
|        /* Get and check PCI Express port services */
|        capabilities = get_port_device_capability(dev);
|        if (!capabilities)
|                return 0;
|
|        pci_set_master(dev);

so how come that pci_set_master is not called for powerpc?

Can you send out lspci -vvxxx with current linus-tree and v3.11?

Yinghai

On Fri, 2013-09-27 at 10:10 -0700, Linus Torvalds wrote:

quoted

So i would like to use the first way that you suggest : call pci_set_master
PCIe port driver.

So I have to say, that if we can fix this with just adding a single
new pci_set_master() call, we should do that before we decide to
revert.

If other, bigger issues then come up, we can decide to revert. But if
there's a one-liner fix, let's just do that first, ok?

Mind sending a patch?

Wouldn't it be better to simply have pci_enable_device() always set bus
master on a bridge? I don't see any case where it makes sense to have
an enabled bridge without the master bit set on it...

Cheers,
Ben.

On Fri, Sep 27, 2013 at 2:46 PM, Benjamin Herrenschmidt
[off-list ref] wrote:

Wouldn't it be better to simply have pci_enable_device() always set bus
master on a bridge? I don't see any case where it makes sense to have
an enabled bridge without the master bit set on it...

Do you mean attached?

On Fri, 2013-09-27 at 14:54 -0700, Yinghai Lu wrote:

On Fri, Sep 27, 2013 at 2:46 PM, Benjamin Herrenschmidt
[off-list ref] wrote:

quoted

Wouldn't it be better to simply have pci_enable_device() always set bus
master on a bridge? I don't see any case where it makes sense to have
an enabled bridge without the master bit set on it...

Do you mean attached?

That's an option. I was thinking making pci_enable_device() itself
enable bus master on a bridge but yes, you approach should work.

I'm digging a bit more to figure out what went wrong in the
pcie port driver since that's interesting in its own right and I'll then
test your patch which I think is a more robust approach.

Cheers,
Ben.

On Fri, 2013-09-27 at 10:44 -0700, Yinghai Lu wrote:

|        /* Get and check PCI Express port services */
|        capabilities = get_port_device_capability(dev);
|        if (!capabilities)
|                return 0;
|
|        pci_set_master(dev);

so how come that pci_set_master is not called for powerpc?

Can you send out lspci -vvxxx with current linus-tree and v3.11?

Ah good point. It should have ... except that there are a number of ways
for get_port_device_capability() to fail and that should *not* leave the
bridge without the bus master capability set.

However I don't think that's what's happening here. I'll have to dig
more, will get back to you.

Cheers,
Ben.

On Fri, 2013-09-27 at 14:54 -0700, Yinghai Lu wrote:

On Fri, Sep 27, 2013 at 2:46 PM, Benjamin Herrenschmidt
[off-list ref] wrote:

quoted

Wouldn't it be better to simply have pci_enable_device() always set bus
master on a bridge? I don't see any case where it makes sense to have
an enabled bridge without the master bit set on it...

Do you mean attached?

So this patch works and fixes the problem. I think it makes the whole
thing more robust and should be applied.

I still don't know why the bridge doesn't get enabled properly without
it yes, tracking it down (the machine in question takes a LONG time to
reboot :-)

Cheers,
Ben.

On Fri, Sep 27, 2013 at 3:38 PM, Benjamin Herrenschmidt
[off-list ref] wrote:

On Fri, 2013-09-27 at 14:54 -0700, Yinghai Lu wrote:

quoted

On Fri, Sep 27, 2013 at 2:46 PM, Benjamin Herrenschmidt
[off-list ref] wrote:

quoted

Wouldn't it be better to simply have pci_enable_device() always set bus
master on a bridge? I don't see any case where it makes sense to have
an enabled bridge without the master bit set on it...

Do you mean attached?

So this patch works and fixes the problem. I think it makes the whole
thing more robust and should be applied.

good.

I still don't know why the bridge doesn't get enabled properly without
it yes, tracking it down (the machine in question takes a LONG time to
reboot :-)

ok, please if you are ok attached one instead. It will print some warning about
driver skipping pci_set_master, so we can catch more problem with drivers.

Thanks

Yinghai

On Fri, 2013-09-27 at 15:56 -0700, Yinghai Lu wrote:

ok, please if you are ok attached one instead. It will print some warning about
driver skipping pci_set_master, so we can catch more problem with drivers.

Except that the message is pretty cryptic :-) Especially since the
driver causing the message to be printed is not the one that did
the mistake in the first place, it's the next one coming up that
trips the warning.

In any case, the root cause is indeed the PCIe port driver:

We don't have ACPI, so pcie_port_platform_notify() isn't implemented,
and pcie_ports_auto is true, so we end up with capabilities set to 0.

Thus the port driver bails out before calling pci_set_master(). The fix
is to call pci_set_master() unconditionally. However that lead me to
find to a few interesting oddities in that port driver code:

 - If capabilities is 0, it returns after enabling the device and
doesn't disable it. But if it fails for any other reason later on (such
as failing to enable interrupts), it *will* disable the device. This is
inconsistent. In fact, if it had disabled the device as a result of the
0 capabilities, the problem would also not have happened (the subsequent
enable_bridge done by the e1000e driver would have done the right
thing). I've tested "fixing" that instead of moving the set_master call
and it fixes my problem as well.

 - In get_port_device_capability(), all capabilities are gated by a
combination of the bit in cap_mask and a corresponding HW check of
the presence of the relevant PCIe capability or similar... except
for the VC one, which is solely read from the HW capability. That means
that the platform pcie_port_platform_notify() has no ability to prevent
the VC capability (so if I have a broken bridge that advertises it but
my platform wants to block it, it can't).

 - I am quite nervous with the PCIe port driver disabling bridges. I
understand the intent but what if that bridge has some system device
behind it ? Something you don't even know about (used by ACPI, behind an
ISA bridge for example ?).

I think disabling bridges is a VERY risky proposition at all times
(including during PM) and I don't think the port driver should do it.

Maybe a more robust approach would be to detect one way or another that
the bridge was already enabled and only disable it if it wasn't or
something along those lines (ie ,restore it in the state it was)...

This is not my problem right now of course (in my case the bridge was
disabled to begin with) but I have a long experience with system stuff
hiding behind bridges and the code as it is written makes me a bit
nervous.

Cheers,
Ben.

[+ Rafael]

On Fri, Sep 27, 2013 at 4:19 PM, Benjamin Herrenschmidt
[off-list ref] wrote:

On Fri, 2013-09-27 at 15:56 -0700, Yinghai Lu wrote:

quoted

ok, please if you are ok attached one instead. It will print some warning about
driver skipping pci_set_master, so we can catch more problem with drivers.

Except that the message is pretty cryptic :-) Especially since the
driver causing the message to be printed is not the one that did
the mistake in the first place, it's the next one coming up that
trips the warning.

In any case, the root cause is indeed the PCIe port driver:

We don't have ACPI, so pcie_port_platform_notify() isn't implemented,
and pcie_ports_auto is true, so we end up with capabilities set to 0.

in
| commit fe31e69740eddc7316071ed5165fed6703c8cd12
| Author: Rafael J. Wysocki [off-list ref]
| Date:   Sun Dec 19 15:57:16 2010 +0100
|
|    PCI/PCIe: Clear Root PME Status bits early during system resume
|
|    I noticed that PCI Express PMEs don't work on my Toshiba Portege R500
|    after the system has been woken up from a sleep state by a PME
|    (through Wake-on-LAN).  After some investigation it turned out that
|    the BIOS didn't clear the Root PME Status bit in the root port that
|    received the wakeup PME and since the Requester ID was also set in
|    the port's Root Status register, any subsequent PMEs didn't trigger
|    interrupts.
|
|    This problem can be avoided by clearing the Root PME Status bits in
|    all PCI Express root ports during early resume.  For this purpose,
|    add an early resume routine to the PCIe port driver and make this
|    driver be always registered, even if pci_ports_disable is set (in
|    which case the driver's only function is to provide the early
|    resume callback).
|
|
|@@ -349,15 +349,18 @@ int pcie_port_device_register(struct pci_dev *dev)
|        int status, capabilities, i, nr_service;
|        int irqs[PCIE_PORT_DEVICE_MAXSERVICES];
|
|-       /* Get and check PCI Express port services */
|-       capabilities = get_port_device_capability(dev);
|-       if (!capabilities)
|-               return -ENODEV;
|-
|        /* Enable PCI Express port device */
|        status = pci_enable_device(dev);
|        if (status)
|                return status;
|+
|+       /* Get and check PCI Express port services */
|+       capabilities = get_port_device_capability(dev);
|+       if (!capabilities) {
|+               pcie_no_aspm();
|+               return 0;
|+       }
|+
|        pci_set_master(dev);
|        /*
|         * Initialize service irqs. Don't use service devices that

Thus the port driver bails out before calling pci_set_master(). The fix
is to call pci_set_master() unconditionally. However that lead me to
find to a few interesting oddities in that port driver code:

can we revert that partially change ? aka we should check get_port....
at first...

like attached.

Thanks

Yinghai

On Fri, 2013-09-27 at 16:44 -0700, Yinghai Lu wrote:

quoted

Thus the port driver bails out before calling pci_set_master(). The fix
is to call pci_set_master() unconditionally. However that lead me to
find to a few interesting oddities in that port driver code:

can we revert that partially change ? aka we should check get_port....
at first...

like attached.

In the meantime, can you properly submit the other one with the warning
to Linus ? It will make things more robust overall...

Also, please read my other comments. I think we are treading on very
fragile ground with that whole business of potentially disabling bridges
in the pcieport driver ...

Cheers,
Ben.

On Fri, Sep 27, 2013 at 8:05 PM, Benjamin Herrenschmidt
[off-list ref] wrote:

On Fri, 2013-09-27 at 16:44 -0700, Yinghai Lu wrote:

In the meantime, can you properly submit the other one with the warning
to Linus ? It will make things more robust overall...

https://patchwork.kernel.org/patch/2959121/

On Friday, September 27, 2013 04:44:20 PM Yinghai Lu wrote:

[+ Rafael]

On Fri, Sep 27, 2013 at 4:19 PM, Benjamin Herrenschmidt
[off-list ref] wrote:

quoted

On Fri, 2013-09-27 at 15:56 -0700, Yinghai Lu wrote:

quoted

ok, please if you are ok attached one instead. It will print some warning about
driver skipping pci_set_master, so we can catch more problem with drivers.

Except that the message is pretty cryptic :-) Especially since the
driver causing the message to be printed is not the one that did
the mistake in the first place, it's the next one coming up that
trips the warning.

In any case, the root cause is indeed the PCIe port driver:

We don't have ACPI, so pcie_port_platform_notify() isn't implemented,
and pcie_ports_auto is true, so we end up with capabilities set to 0.

in
| commit fe31e69740eddc7316071ed5165fed6703c8cd12
| Author: Rafael J. Wysocki [off-list ref]
| Date:   Sun Dec 19 15:57:16 2010 +0100
|
|    PCI/PCIe: Clear Root PME Status bits early during system resume
|
|    I noticed that PCI Express PMEs don't work on my Toshiba Portege R500
|    after the system has been woken up from a sleep state by a PME
|    (through Wake-on-LAN).  After some investigation it turned out that
|    the BIOS didn't clear the Root PME Status bit in the root port that
|    received the wakeup PME and since the Requester ID was also set in
|    the port's Root Status register, any subsequent PMEs didn't trigger
|    interrupts.
|
|    This problem can be avoided by clearing the Root PME Status bits in
|    all PCI Express root ports during early resume.  For this purpose,
|    add an early resume routine to the PCIe port driver and make this
|    driver be always registered, even if pci_ports_disable is set (in
|    which case the driver's only function is to provide the early
|    resume callback).
|
|
|@@ -349,15 +349,18 @@ int pcie_port_device_register(struct pci_dev *dev)
|        int status, capabilities, i, nr_service;
|        int irqs[PCIE_PORT_DEVICE_MAXSERVICES];
|
|-       /* Get and check PCI Express port services */
|-       capabilities = get_port_device_capability(dev);
|-       if (!capabilities)
|-               return -ENODEV;
|-
|        /* Enable PCI Express port device */
|        status = pci_enable_device(dev);
|        if (status)
|                return status;
|+
|+       /* Get and check PCI Express port services */
|+       capabilities = get_port_device_capability(dev);
|+       if (!capabilities) {
|+               pcie_no_aspm();
|+               return 0;
|+       }
|+
|        pci_set_master(dev);
|        /*
|         * Initialize service irqs. Don't use service devices that

quoted

Thus the port driver bails out before calling pci_set_master(). The fix
is to call pci_set_master() unconditionally. However that lead me to
find to a few interesting oddities in that port driver code:

can we revert that partially change ? aka we should check get_port....
at first...

like attached.

It looks like we can do something like this (just pasting your patch):

diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 31063ac..1ee6f16 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c

@@ -362,16 +362,16 @@ int pcie_port_device_register(struct pci_dev *dev)
 	int status, capabilities, i, nr_service;
 	int irqs[PCIE_PORT_DEVICE_MAXSERVICES];
 
-	/* Enable PCI Express port device */
-	status = pci_enable_device(dev);
-	if (status)
-		return status;
-
 	/* Get and check PCI Express port services */
 	capabilities = get_port_device_capability(dev);
 	if (!capabilities)
 		return 0;
 
+	/* Enable PCI Express port device */
+	status = pci_enable_device(dev);
+	if (status)
+		return status;
+
 	pci_set_master(dev);
 	/*
 	 * Initialize service irqs. Don't use service devices that

but I don't have that box with me to test whether or not it still works
correctly after this change.  I'll be back home on the next Saturday if
all goes well.

Thanks,
Rafael