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

From: "Matt Carlson" <[email-masked]>
Date: Thu, 10 Feb 2011 16:00:15 -0800

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 <[email-masked]>
> > > 
> > > 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 <[email-masked]>
> > > ---
> > >  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 <[email-masked]>

> > >  }
> > >  
> > >  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.