Re: Please revert 928bea964827d7824b548c1f8e06eccbbc4d0d7d

Yinghai Lu
2013-09-27T23:44:23Z

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