Re: [PATCH] Make e100 suspend handler support PCI cards lacking PM capability

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Sunday 14 June 2009, Andreas Mohr wrote:
> Hi,
> 
> On Sun, Jun 14, 2009 at 12:28:15AM +0200, Rafael J. Wysocki wrote:
> > On Saturday 13 June 2009, Andreas Mohr wrote:
> > > 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)
> 
> > > 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.
> 
> > >  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.

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