Please revert 928bea964827d7824b548c1f8e06eccbbc4d0d7d

Please revert 928bea964827d7824b548c1f8e06eccbbc4d0d7d

[Benjamin Herrenschmidt]

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.

Re: Please revert 928bea964827d7824b548c1f8e06eccbbc4d0d7d

[Yinghai Lu]

On Fri, Sep 27, 2013 at 1:28 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> 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

Re: Please revert 928bea964827d7824b548c1f8e06eccbbc4d0d7d

[Linus Torvalds]

On Fri, Sep 27, 2013 at 9:01 AM, Yinghai Lu <yinghai@kernel.org> 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

Re: Please revert 928bea964827d7824b548c1f8e06eccbbc4d0d7d

[Benjamin Herrenschmidt]

On Fri, 2013-09-27 at 10:10 -0700, Linus Torvalds 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?

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.

Re: Please revert 928bea964827d7824b548c1f8e06eccbbc4d0d7d

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 316 bytes --]

On Fri, Sep 27, 2013 at 2:46 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> 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?

[-- Attachment #2: pci_set_master_again.patch --]
[-- Type: application/octet-stream, Size: 510 bytes --]

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e8ccf6c..4eff99b 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1156,11 +1156,13 @@ static void pci_enable_bridge(struct pci_dev *dev)
 	pci_enable_bridge(dev->bus->self);
 
 	if (pci_is_enabled(dev))
-		return;
+		goto out; /* some other driver could skip pci_set_master ! */
 	retval = pci_enable_device(dev);
 	if (retval)
 		dev_err(&dev->dev, "Error enabling bridge (%d), continuing\n",
 			retval);
+
+out:
 	pci_set_master(dev);
 }
 

Re: Please revert 928bea964827d7824b548c1f8e06eccbbc4d0d7d

[Benjamin Herrenschmidt]

On Fri, 2013-09-27 at 14:54 -0700, Yinghai Lu wrote:
> On Fri, Sep 27, 2013 at 2:46 PM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> 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?

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.

Re: Please revert 928bea964827d7824b548c1f8e06eccbbc4d0d7d

[Benjamin Herrenschmidt]

On Fri, 2013-09-27 at 14:54 -0700, Yinghai Lu wrote:
> On Fri, Sep 27, 2013 at 2:46 PM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> 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?

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.

Re: Please revert 928bea964827d7824b548c1f8e06eccbbc4d0d7d

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 942 bytes --]

On Fri, Sep 27, 2013 at 3:38 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Fri, 2013-09-27 at 14:54 -0700, Yinghai Lu wrote:
>> On Fri, Sep 27, 2013 at 2:46 PM, Benjamin Herrenschmidt
>> <benh@kernel.crashing.org> 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?
>
> 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

[-- Attachment #2: pci_set_master_again_v2.patch --]
[-- Type: application/octet-stream, Size: 1420 bytes --]

Subject: [PATCH] PCI: Workaround missing pci_set_master in pci drivers

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

break PCI on powerpc.  The reason is that the PCIe port driver will
call pci_enable_device() on the bridge, so device enabled (but skip
pci_set_master somehow).

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.

Fixed by add checking in pci_enable_bridge, and call pci_set_master
if driver skip that.

Reported-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/pci/pci.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/pci/pci.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci.c
+++ linux-2.6/drivers/pci/pci.c
@@ -1156,8 +1156,14 @@ static void pci_enable_bridge(struct pci
 
 	pci_enable_bridge(dev->bus->self);
 
-	if (pci_is_enabled(dev))
+	if (pci_is_enabled(dev)) {
+		if (!dev->is_busmaster) {
+			dev_warn(&dev->dev, "driver skip pci_set_master, fix it!\n");
+			pci_set_master(dev);
+		}
 		return;
+	}
+
 	retval = pci_enable_device(dev);
 	if (retval)
 		dev_err(&dev->dev, "Error enabling bridge (%d), continuing\n",

Re: Please revert 928bea964827d7824b548c1f8e06eccbbc4d0d7d

[Benjamin Herrenschmidt]

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.

Re: Please revert 928bea964827d7824b548c1f8e06eccbbc4d0d7d

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 2949 bytes --]

[+ Rafael]

On Fri, Sep 27, 2013 at 4:19 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> 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.

in
| commit fe31e69740eddc7316071ed5165fed6703c8cd12
| Author: Rafael J. Wysocki <rjw@sisk.pl>
| 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

[-- Attachment #2: fix_pci_set_master_port_pcie.patch --]
[-- Type: application/octet-stream, Size: 784 bytes --]

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

Re: Please revert 928bea964827d7824b548c1f8e06eccbbc4d0d7d

[Benjamin Herrenschmidt]

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

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

[PATCH] PCI: Workaround missing pci_set_master in pci drivers

[Yinghai Lu]

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

break PCI on powerpc.  The reason is that the PCIe port driver will
call pci_enable_device() on the bridge, so device enabled (but skip
pci_set_master because pcie_port_auto and no acpi on powerpc ).

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.

Fixed by add checking in pci_enable_bridge, and call pci_set_master
if driver skip that.
That will make the code more robot and wade off problem for missing
pci_set_master in drivers.

Reported-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/pci/pci.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/pci/pci.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci.c
+++ linux-2.6/drivers/pci/pci.c
@@ -1156,8 +1156,14 @@ static void pci_enable_bridge(struct pci
 
 	pci_enable_bridge(dev->bus->self);
 
-	if (pci_is_enabled(dev))
+	if (pci_is_enabled(dev)) {
+		if (!dev->is_busmaster) {
+			dev_warn(&dev->dev, "driver skip pci_set_master, fix it!\n");
+			pci_set_master(dev);
+		}
 		return;
+	}
+
 	retval = pci_enable_device(dev);
 	if (retval)
 		dev_err(&dev->dev, "Error enabling bridge (%d), continuing\n",

Re: [PATCH] PCI: Workaround missing pci_set_master in pci drivers

[Theodore Ts'o]

On Sat, Sep 28, 2013 at 01:13:07PM -0700, Yinghai Lu wrote:
> Fixed by add checking in pci_enable_bridge, and call pci_set_master
> if driver skip that.
> That will make the code more robot and wade off problem for missing
> pci_set_master in drivers.

Petty spelling nit; feel free to ignore unless you need to respin the
patch anyway....

s/robot/robost/

Cheers,

						- Ted

Re: [PATCH] PCI: Workaround missing pci_set_master in pci drivers

[Linus Torvalds]

On Sun, Sep 29, 2013 at 3:41 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> On Sat, Sep 28, 2013 at 01:13:07PM -0700, Yinghai Lu wrote:
>> Fixed by add checking in pci_enable_bridge, and call pci_set_master
>> if driver skip that.
>> That will make the code more robot and wade off problem for missing
>> pci_set_master in drivers.
>
> Petty spelling nit; feel free to ignore unless you need to respin the
> patch anyway....
>
> s/robot/robost/

That's an oddly spelled nitpick "correction".

If you really want to fix it, it's "robust" and "ward off problems".
But it's too late now, and the wrong spelling and word choice is in
the git tree and released in the -rc3 I just pushed out.

           Linus

Re: [PATCH] PCI: Workaround missing pci_set_master in pci drivers

[Theodore Ts'o]

On Sun, Sep 29, 2013 at 03:46:33PM -0700, Linus Torvalds wrote:
> On Sun, Sep 29, 2013 at 3:41 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> > On Sat, Sep 28, 2013 at 01:13:07PM -0700, Yinghai Lu wrote:
> >> Fixed by add checking in pci_enable_bridge, and call pci_set_master
> >> if driver skip that.
> >> That will make the code more robot and wade off problem for missing
> >> pci_set_master in drivers.
> >
> > Petty spelling nit; feel free to ignore unless you need to respin the
> > patch anyway....
> >
> > s/robot/robost/
> 
> That's an oddly spelled nitpick "correction".

Sorry, typed too quickly.  :-(

						- Ted

Re: [PATCH] PCI: Workaround missing pci_set_master in pci drivers

[Bjorn Helgaas]

On Sat, Sep 28, 2013 at 01:13:07PM -0700, Yinghai Lu wrote:
> BenH found:
> | 928bea964827d7824b548c1f8e06eccbbc4d0d7d
> | PCI: Delay enabling bridges until they're needed
> 
> break PCI on powerpc.  The reason is that the PCIe port driver will
> call pci_enable_device() on the bridge, so device enabled (but skip
> pci_set_master because pcie_port_auto and no acpi on powerpc ).
> 
> 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.
> 
> Fixed by add checking in pci_enable_bridge, and call pci_set_master
> if driver skip that.
> That will make the code more robot and wade off problem for missing
> pci_set_master in drivers.
> 
> Reported-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> 
> ---
>  drivers/pci/pci.c |    8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6/drivers/pci/pci.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/pci.c
> +++ linux-2.6/drivers/pci/pci.c
> @@ -1156,8 +1156,14 @@ static void pci_enable_bridge(struct pci
>  
>  	pci_enable_bridge(dev->bus->self);
>  
> -	if (pci_is_enabled(dev))
> +	if (pci_is_enabled(dev)) {
> +		if (!dev->is_busmaster) {
> +			dev_warn(&dev->dev, "driver skip pci_set_master, fix it!\n");

I know this is already in Linus' tree, but if we're going to enable
bus mastering here, what's the point of the warning?  If somebody
fixes the driver by adding a pci_set_master() call there, does that
improve something?

Bjorn

> +			pci_set_master(dev);
> +		}
>  		return;
> +	}
> +
>  	retval = pci_enable_device(dev);
>  	if (retval)
>  		dev_err(&dev->dev, "Error enabling bridge (%d), continuing\n",

Re: [PATCH] PCI: Workaround missing pci_set_master in pci drivers

[Yinghai Lu]

On Thu, Oct 3, 2013 at 3:06 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Sat, Sep 28, 2013 at 01:13:07PM -0700, Yinghai Lu wrote:
>> @@ -1156,8 +1156,14 @@ static void pci_enable_bridge(struct pci
>>
>>       pci_enable_bridge(dev->bus->self);
>>
>> -     if (pci_is_enabled(dev))
>> +     if (pci_is_enabled(dev)) {
>> +             if (!dev->is_busmaster) {
>> +                     dev_warn(&dev->dev, "driver skip pci_set_master, fix it!\n");
>
> I know this is already in Linus' tree, but if we're going to enable
> bus mastering here, what's the point of the warning?  If somebody
> fixes the driver by adding a pci_set_master() call there, does that
> improve something?

Help us to catch other offender and fix them.

Yinghai

Re: [PATCH] PCI: Workaround missing pci_set_master in pci drivers

[Bjorn Helgaas]

On Thu, Oct 3, 2013 at 5:35 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Thu, Oct 3, 2013 at 3:06 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Sat, Sep 28, 2013 at 01:13:07PM -0700, Yinghai Lu wrote:
>>> @@ -1156,8 +1156,14 @@ static void pci_enable_bridge(struct pci
>>>
>>>       pci_enable_bridge(dev->bus->self);
>>>
>>> -     if (pci_is_enabled(dev))
>>> +     if (pci_is_enabled(dev)) {
>>> +             if (!dev->is_busmaster) {
>>> +                     dev_warn(&dev->dev, "driver skip pci_set_master, fix it!\n");
>>
>> I know this is already in Linus' tree, but if we're going to enable
>> bus mastering here, what's the point of the warning?  If somebody
>> fixes the driver by adding a pci_set_master() call there, does that
>> improve something?
>
> Help us to catch other offender and fix them.

What is improved by doing it in the driver instead of here?

Re: [PATCH] PCI: Workaround missing pci_set_master in pci drivers

[Paul Bolle]

On Fri, 2013-10-04 at 09:55 -0600, Bjorn Helgaas wrote:
> On Thu, Oct 3, 2013 at 5:35 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> > On Thu, Oct 3, 2013 at 3:06 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> >> On Sat, Sep 28, 2013 at 01:13:07PM -0700, Yinghai Lu wrote:
> >>> @@ -1156,8 +1156,14 @@ static void pci_enable_bridge(struct pci
> >>>
> >>>       pci_enable_bridge(dev->bus->self);
> >>>
> >>> -     if (pci_is_enabled(dev))
> >>> +     if (pci_is_enabled(dev)) {
> >>> +             if (!dev->is_busmaster) {
> >>> +                     dev_warn(&dev->dev, "driver skip pci_set_master, fix it!\n");
> >>
> >> I know this is already in Linus' tree, but if we're going to enable
> >> bus mastering here, what's the point of the warning?  If somebody
> >> fixes the driver by adding a pci_set_master() call there, does that
> >> improve something?
> >
> > Help us to catch other offender and fix them.
> 
> What is improved by doing it in the driver instead of here?

After booting v3.12 for the first time on a laptop I noticed two new
warnings: 
    <4>[    4.427959] pcieport 0000:00:1c.4: driver skip pci_set_master, fix it!
    <4>[    4.448630] pcieport 0000:00:1c.1: driver skip pci_set_master, fix it!

These warnings aren't entirely clear, but luckily they are easily
greppable. It turns out they can be traced back to this patch.

So some further grepping, looking at the code, etc. suggests these
warnings could be silenced by calling pci_set_master() before calling
pci_enable_device(). Ie, reverse the current order of those calls in
drivers/pci/pcie/portdrv_core.c:pcie_port_device_register(). Is that
correct? But what should then be done if pci_enable_device() fails?

And Bjorn's question - what's the point of this warning if
pci_set_master() will be called anyway - also came up when I looked at
that code segment for the first time. But I'm not familiar with the PCI
code.


Paul Bolle

Re: [PATCH] PCI: Workaround missing pci_set_master in pci drivers

[Bjorn Helgaas]

On Mon, Nov 04, 2013 at 01:44:08PM +0100, Paul Bolle wrote:
> On Fri, 2013-10-04 at 09:55 -0600, Bjorn Helgaas wrote:
> > On Thu, Oct 3, 2013 at 5:35 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> > > On Thu, Oct 3, 2013 at 3:06 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > >> On Sat, Sep 28, 2013 at 01:13:07PM -0700, Yinghai Lu wrote:
> > >>> @@ -1156,8 +1156,14 @@ static void pci_enable_bridge(struct pci
> > >>>
> > >>>       pci_enable_bridge(dev->bus->self);
> > >>>
> > >>> -     if (pci_is_enabled(dev))
> > >>> +     if (pci_is_enabled(dev)) {
> > >>> +             if (!dev->is_busmaster) {
> > >>> +                     dev_warn(&dev->dev, "driver skip pci_set_master, fix it!\n");
> > >>
> > >> I know this is already in Linus' tree, but if we're going to enable
> > >> bus mastering here, what's the point of the warning?  If somebody
> > >> fixes the driver by adding a pci_set_master() call there, does that
> > >> improve something?
> > >
> > > Help us to catch other offender and fix them.
> > 
> > What is improved by doing it in the driver instead of here?
> 
> After booting v3.12 for the first time on a laptop I noticed two new
> warnings: 
>     <4>[    4.427959] pcieport 0000:00:1c.4: driver skip pci_set_master, fix it!
>     <4>[    4.448630] pcieport 0000:00:1c.1: driver skip pci_set_master, fix it!
> 
> These warnings aren't entirely clear, but luckily they are easily
> greppable. It turns out they can be traced back to this patch.
> 
> So some further grepping, looking at the code, etc. suggests these
> warnings could be silenced by calling pci_set_master() before calling
> pci_enable_device(). Ie, reverse the current order of those calls in
> drivers/pci/pcie/portdrv_core.c:pcie_port_device_register(). Is that
> correct? But what should then be done if pci_enable_device() fails?
> 
> And Bjorn's question - what's the point of this warning if
> pci_set_master() will be called anyway - also came up when I looked at
> that code segment for the first time. But I'm not familiar with the PCI
> code.

Unless somebody can point out a problem with doing the pci_set_master()
inside pci_enable_bridges(), I plan to merge the following patch to
remove the warning.


PCI: Drop warning about drivers that don't use pci_set_master()

From: Bjorn Helgaas <bhelgaas@google.com>

f41f064cf4 ("PCI: Workaround missing pci_set_master in pci drivers") made
pci_enable_bridge() turn on bus mastering if the driver hadn't done so
already.  It also added a warning in this case.  But there's no reason to
warn about it unless it's actually a problem to enable bus mastering here.

This patch drops the warning because I'm not aware of any such problem.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
CC: Paul Bolle <pebolle@tiscali.nl>
---
 drivers/pci/pci.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 7a92d81..ac40f90 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1156,10 +1156,8 @@ static void pci_enable_bridge(struct pci_dev *dev)
 	pci_enable_bridge(dev->bus->self);
 
 	if (pci_is_enabled(dev)) {
-		if (!dev->is_busmaster) {
-			dev_warn(&dev->dev, "driver skip pci_set_master, fix it!\n");
+		if (!dev->is_busmaster)
 			pci_set_master(dev);
-		}
 		return;
 	}
 

Re: Please revert 928bea964827d7824b548c1f8e06eccbbc4d0d7d

[Yinghai Lu]

On Fri, Sep 27, 2013 at 8:05 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> 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/

Re: Please revert 928bea964827d7824b548c1f8e06eccbbc4d0d7d

[Rafael J. Wysocki]

On Friday, September 27, 2013 04:44:20 PM Yinghai Lu wrote:
> [+ Rafael]
> 
> On Fri, Sep 27, 2013 at 4:19 PM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > 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.
> 
> in
> | commit fe31e69740eddc7316071ed5165fed6703c8cd12
> | Author: Rafael J. Wysocki <rjw@sisk.pl>
> | 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.

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

Re: Please revert 928bea964827d7824b548c1f8e06eccbbc4d0d7d

[Yinghai Lu]

On Fri, Sep 27, 2013 at 9:01 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Fri, Sep 27, 2013 at 1:28 AM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> 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.

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

Re: Please revert 928bea964827d7824b548c1f8e06eccbbc4d0d7d

[Benjamin Herrenschmidt]

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.