netdev - Re: [PATCH 1/3] tg3: Avoid setting power.can_wakeup for devices that cannot wake up

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]

Date:	Thu, 10 Feb 2011 16:00:15 -0800
From:	"Matt Carlson" <mcarlson@...adcom.com>
To:	"Rafael J. Wysocki" <rjw@...k.pl>
cc:	"Matthew Carlson" <mcarlson@...adcom.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"David Miller" <davem@...emloft.net>,
	"Michael Chan" <mchan@...adcom.com>,
	"Linux PM mailing list" <linux-pm@...ts.linux-foundation.org>,
	"Thomas Fjellstrom" <thomas@...llstrom.ca>,
	"Jay Cliburn" <jcliburn@...il.com>,
	"Chris Snook" <chris.snook@...il.com>,
	"Jie Yang" <jie.yang@...eros.com>
Subject: Re: [PATCH 1/3] tg3: Avoid setting power.can_wakeup for devices
 that cannot wake up

On Thu, Feb 10, 2011 at 01:08:42PM -0800, Rafael J. Wysocki wrote:
> On Thursday, February 10, 2011, Matt Carlson wrote:
> > On Thu, Feb 10, 2011 at 08:53:09AM -0800, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rjw@...k.pl>
> > > 
> > > The tg3 driver uses device_init_wakeup() in such a way that the
> > > device's power.can_wakeup flag may be set even though the PCI
> > > subsystem cleared it before, in which case the device cannot wake
> > > up the system from sleep states.  Modify the driver to only change
> > > the power.can_wakeup flag if the device is not capable of generating
> > > wakeup signals.
> > > 
> > > Signed-off-by: Rafael J. Wysocki <rjw@...k.pl>
> > > ---
> > >  drivers/net/tg3.c |    6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > Index: linux-2.6/drivers/net/tg3.c
> > > ===================================================================
> > > --- linux-2.6.orig/drivers/net/tg3.c
> > > +++ linux-2.6/drivers/net/tg3.c
> > > @@ -12403,9 +12403,11 @@ static void __devinit tg3_get_eeprom_hw_
> > >  			tp->tg3_flags3 |= TG3_FLG3_RGMII_EXT_IBND_TX_EN;
> > >  	}
> > >  done:
> > > -	device_init_wakeup(&tp->pdev->dev, tp->tg3_flags & TG3_FLAG_WOL_CAP);
> > > -	device_set_wakeup_enable(&tp->pdev->dev,
> > > +	if (tp->tg3_flags & TG3_FLAG_WOL_CAP)
> > > +		device_set_wakeup_enable(&tp->pdev->dev,
> > >  				 tp->tg3_flags & TG3_FLAG_WOL_ENABLE);
> > > +	else
> > > +		device_set_wakeup_capable(&tp->pdev->dev, false);
> > 
> > I did this because I couldn't see where 'can_wakeup' gets set.  I don't
> > see a call to device_init_wakeup() that would be relevant to tg3
> > devices.  I do see a couple calls to device_set_wakeup_capable() in
> > acpi/glue.c and acpi/scan.c.  Is that the place?
> 
> No, it's pci_pm_init() or platform_pci_wakeup_init() and they both use
> device_set_wakeup_capable() rather tha device_init_wakeup(), which is just a
> combination of device_set_wakeup_capable() and device_set_wakeup_enable()
> anyway.
> 
> And there's no why reason PCI drivers should use device_pm_init() at all.

O.K.

Acked-by: Matt Carlson <mcarlson@...adcom.com>

> > >  }
> > >  
> > >  static int __devinit tg3_issue_otp_command(struct tg3 *tp, u32 cmd)
> > 
> > This is something I was always curious about too.  The TG3_FLAG_WOL_CAP
> > tracks whether or not the device can handle WOL.  Is it safe to do away
> > with this flag and lean on the 'can_wakeup' flag instead?
> 
> I don't think so.  power.can_wakeup only tracks the capability to generate
> wakeup signals from the PCI perspective and it is set by PCI if the device
> appears to be able to generate wakeup signals.
> 
> So, I think the driver should work as in the $subject patch - reset the
> power.can_wakeup flag if TG3_FLAG_WOL_CAP is unset and don't touch it
> otherwise.

That makes sense.  Thanks.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html