| Message ID | 201102101753.09504.rjw@sisk.pl |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, Feb 10, 2011 at 08:53:09AM -0800, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rjw@sisk.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@sisk.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? > } > > 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? My concern is that some other part of the system might enable that flag after tg3_get_invariants() has run. If that happens and the device isn't really WOL capable bad things can occur. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@sisk.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@sisk.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. > > } > > > > 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. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@sisk.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@sisk.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@broadcom.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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); } static int __devinit tg3_issue_otp_command(struct tg3 *tp, u32 cmd)