On Sunday 14 June 2009, Andreas Mohr wrote:

Hi,

On Sun, Jun 14, 2009 at 12:28:15AM +0200, Rafael J. Wysocki wrote:

quoted

On Saturday 13 June 2009, Andreas Mohr wrote:

quoted

Hi all,

after having added non-MII PHY card support to e100, I noticed that
the suspend handler rejects power-management non-capable PCI cards,

Well, that means we have a bug somewhere in the PCI PM core.

I don't know. I had shortly investigated the same thing,
but it very much seemed that this is by design, pci_set_power_state()
is documented to reject non-PM cards (in power/pci.txt, and in
pci.c, too). Thus I didn't work in this area.

And from a cleanliness point of view pci_set_power_state()
acting on a non-PM card with no special non-PM ACPI support _should_
return an error status I guess.
(especially since docs say that pci_set_power_state() should
be used for PM cards)

quoted

quoted

causing a S2R request to immediately get back up to the desktop,
losing network access in the process (rtnl mutex deadlock).

That's bad.

Indeed, and I have no idea what the problem was.
rtnl_is_locked() always was false within suspend/resume,
thus it had to be a userspace-triggered effect sometime
_after_ (non-)resume happened
(probably due to the network controller being down and thus inoperable
after .suspend).

BTW, after that failed .suspend, .resume was not called. I assume this to
be correct behaviour.

quoted

quoted

 static int __e100_power_off(struct pci_dev *pdev, bool wake)
 {
+	/* some older devices don't support PCI PM
+	 * (e.g. mac_82557_D100_B combo card with 80c24 PHY)
+	 * - skip those! (they most likely won't support WoL either)
+	 */
+	if (!pci_find_capability(pdev, PCI_CAP_ID_PM))
+		return 0;

Devices without PCI_CAP_ID_PM may still be power-manageable by ACPI, so
returning 0 at this point is not a general solution.

Oh, interesting.

BTW, any idea why we have several drivers doing some seemingly useless

        /* Find power-management capability. */
        pm_cap = pci_find_capability(pdev, PCI_CAP_ID_PM);
        if (pm_cap == 0) {
                printk(KERN_ERR PFX "Cannot find PowerManagement capability, "
                       "aborting.\n");
                err = -EIO;
                goto err_out_free_res;
        }

?

- it's code bloat
- it needlessly rejects non-PM cards
- it annoys the hell out of users of a non-PM card

No idea.

quoted

quoted

+
 	if (wake) {
 		return pci_prepare_to_sleep(pdev);

pci_prepare_to_sleep() is supposed to return 0 for your device.  I'll have a
look at it.

No, wake is false for my card, thus that's not the branch to
investigate.

Ah.  The problem is, then, that we try to put the device into D3, which it
cannot do and error code is correctly returned from pci_set_power_state().

I would use the appended patch in that case and the patch sent previously
is necessary for the 'wake = true' case.

Thanks,
Rafael

---
 drivers/net/e100.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/net/e100.c
===================================================================

--- linux-2.6.orig/drivers/net/e100.c
+++ linux-2.6/drivers/net/e100.c

@@ -2763,8 +2763,9 @@ static int __e100_power_off(struct pci_d
 		return pci_prepare_to_sleep(pdev);
 	} else {
 		pci_wake_from_d3(pdev, false);
-		return pci_set_power_state(pdev, PCI_D3hot);
+		pci_set_power_state(pdev, PCI_D3hot);
 	}
+	return 0;
 }
 
 #ifdef CONFIG_PM

------------------------------------------------------------------------------

Crystal Reports - New Free Runtime and 30 Day Trial
Check out the new simplified licensing option that enables unlimited
royalty-free distribution of the report engine for externally facing 
server and web deployment.
http://p.sf.net/sfu/businessobjects