| Message ID | 20090722193400.GN8515@gospo.rdu.redhat.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
> [PATCH] ixgbe: remove unnecessary call to device_init_wakeup > > Calls to device_init_wakeup should not be necessary in drivers that use > device_set_wakeup_enable since pci_pm_init will set the can_wakeup flag > for the device when initialized. > > I can't test this since I don't have any of the 82599 KX4 interfaces > (the only ones capable of WOL), but I did instrument ixgbe_probe and > know that can_wakeup=1 when device_init_wakeup is removed. > > Signed-off-by: Andy Gospodarek <andy@greyhouse.net> > > --- > > ixgbe_main.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c > index 79f60e8..2f15abf 100644 > --- a/drivers/net/ixgbe/ixgbe_main.c > +++ b/drivers/net/ixgbe/ixgbe_main.c > @@ -5640,7 +5640,6 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev, > adapter->wol = 0; > break; > } > - device_init_wakeup(&adapter->pdev->dev, true); > device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol); > > /* pick up the PCI bus settings for reporting later */ I need to rebuild one of my boxes in the lab that I do WoL testing on for ixgbe devices. That shouldn't take me long, and I'll test this. If it works as expected, I'll send my ack. Cheers, -PJ Waskiewicz -- 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 Wed, 22 Jul 2009, Andy Gospodarek wrote: > On Tue, Jul 21, 2009 at 08:25:31PM -0700, Alexander Duyck wrote: > > My understanding was that the can_wakeup was supposed to be > > initialized by pci_pm_init or platform_pci_wakeup_init based on the > > pci-e capabilities. Is this not the case? It seems like this is > > where you should be looking to determine why the the can_wakeup isn't > > being initialized correctly. > > > > The patch below won't solve the problem either. The problem is that > > the can_wakeup value is being disabled when the EEPROM doesn't support > > WOL. > > The lack of EEPROM support for this was not the problem at all. > Ultimately it turns out that can_wakeup is set in pci_pm_init, so calls > to device_init_wakeup and device_set_wakeup_capable should not be needed > in any driver. > > > If you have to do this in the drivers, then my suggestion is to take a > > look at how ixgbe is doing it. You essentially need to initialize > > can_wakeup to true, and then set the should_wakeup attribute based on > > the EEPROM setting or via ethtool. This way you can still toggle the > > should_wakeup option without being blocked by the EEPROM disabling it. > > The first patch does exactly that (and ixgbe was part of my inspiration > for it). Since can_wakeup and should_wakeup are both set in > device_init_wakeup it's a suitable alternative in driver probe > functions, but probably not ideal when drivers check for > device_can_wakeup in their ethtool set_wol functions. > > Anyway, I knew I would find a bug or unnecessary code between e1000e and > friends or ixgbe, so it looks like ixgbe has an unnecessary call to > device_init_wakeup. Feel free to consider this patch as an alternative. > > > > [PATCH] ixgbe: remove unnecessary call to device_init_wakeup > > Calls to device_init_wakeup should not be necessary in drivers that use > device_set_wakeup_enable since pci_pm_init will set the can_wakeup flag > for the device when initialized. > > I can't test this since I don't have any of the 82599 KX4 interfaces > (the only ones capable of WOL), but I did instrument ixgbe_probe and > know that can_wakeup=1 when device_init_wakeup is removed. > > Signed-off-by: Andy Gospodarek <andy@greyhouse.net> > > --- > > ixgbe_main.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c > index 79f60e8..2f15abf 100644 > --- a/drivers/net/ixgbe/ixgbe_main.c > +++ b/drivers/net/ixgbe/ixgbe_main.c > @@ -5640,7 +5640,6 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev, > adapter->wol = 0; > break; > } > - device_init_wakeup(&adapter->pdev->dev, true); > device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol); > > /* pick up the PCI bus settings for reporting later */ Works just fine on two different 82599 mezz cards that support WoL. Thanks Andy for finding and fixing. Acked-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com> -- 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
From: "Waskiewicz Jr, Peter P" <peter.p.waskiewicz.jr@intel.com> Date: Wed, 22 Jul 2009 16:43:03 -0700 (Pacific Daylight Time) > On Wed, 22 Jul 2009, Andy Gospodarek wrote: > >> [PATCH] ixgbe: remove unnecessary call to device_init_wakeup >> >> Calls to device_init_wakeup should not be necessary in drivers that use >> device_set_wakeup_enable since pci_pm_init will set the can_wakeup flag >> for the device when initialized. >> >> I can't test this since I don't have any of the 82599 KX4 interfaces >> (the only ones capable of WOL), but I did instrument ixgbe_probe and >> know that can_wakeup=1 when device_init_wakeup is removed. >> >> Signed-off-by: Andy Gospodarek <andy@greyhouse.net> ... > Works just fine on two different 82599 mezz cards that support WoL. > Thanks Andy for finding and fixing. > > Acked-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com> Applied, 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
diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c index 79f60e8..2f15abf 100644 --- a/drivers/net/ixgbe/ixgbe_main.c +++ b/drivers/net/ixgbe/ixgbe_main.c @@ -5640,7 +5640,6 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev, adapter->wol = 0; break; } - device_init_wakeup(&adapter->pdev->dev, true); device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol); /* pick up the PCI bus settings for reporting later */