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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <200906141831.05349.rjw@sisk.pl>
Date:	Sun, 14 Jun 2009 18:31:04 +0200
From:	"Rafael J. Wysocki" <rjw@...k.pl>
To:	andi@...as.de
Cc:	akpm@...ux-foundation.org, e1000-devel@...ts.sourceforge.net,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Make e100 suspend handler support PCI cards lacking PM capability

On Sunday 14 June 2009, Rafael J. Wysocki wrote:
> 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.

Or perhaps better this one (functionally equivalent).

---
 drivers/net/e100.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Index: linux-2.6/drivers/net/e100.c
===================================================================
--- linux-2.6.orig/drivers/net/e100.c
+++ linux-2.6/drivers/net/e100.c
@@ -2759,12 +2759,13 @@ static void __e100_shutdown(struct pci_d
 
 static int __e100_power_off(struct pci_dev *pdev, bool wake)
 {
-	if (wake) {
+	if (wake)
 		return pci_prepare_to_sleep(pdev);
-	} else {
-		pci_wake_from_d3(pdev, false);
-		return pci_set_power_state(pdev, PCI_D3hot);
-	}
+
+	pci_wake_from_d3(pdev, false);
+	pci_set_power_state(pdev, PCI_D3hot);
+
+	return 0;
 }
 
 #ifdef CONFIG_PM
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/