* [Linux-kernel-mentees] [PATCH v1] ethernet: intel: e1000: Convert to dev_pm_ops @ 2020-04-10 12:44 Vaibhav Gupta 2020-04-11 0:15 ` Bjorn Helgaas ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Vaibhav Gupta @ 2020-04-10 12:44 UTC (permalink / raw) To: linux-kernel-mentees, bjorn, skhan, rjw; +Cc: Vaibhav Gupta Convert the legacy callback .suspend() and .resume() to the generic ones. Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com> --- drivers/net/ethernet/intel/e1000/e1000_main.c | 47 +++++-------------- 1 file changed, 12 insertions(+), 35 deletions(-) diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c index 2bced34c19ba..09a6ef46be96 100644 --- a/drivers/net/ethernet/intel/e1000/e1000_main.c +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c @@ -152,8 +152,8 @@ static int e1000_vlan_rx_kill_vid(struct net_device *netdev, static void e1000_restore_vlan(struct e1000_adapter *adapter); #ifdef CONFIG_PM -static int e1000_suspend(struct pci_dev *pdev, pm_message_t state); -static int e1000_resume(struct pci_dev *pdev); +static int e1000_suspend(struct device *dev); +static int e1000_resume(struct device *dev); #endif static void e1000_shutdown(struct pci_dev *pdev); @@ -179,16 +179,16 @@ static const struct pci_error_handlers e1000_err_handler = { .resume = e1000_io_resume, }; +static SIMPLE_DEV_PM_OPS(e1000_pm_ops, e1000_suspend, e1000_resume); + static struct pci_driver e1000_driver = { .name = e1000_driver_name, .id_table = e1000_pci_tbl, .probe = e1000_probe, .remove = e1000_remove, -#ifdef CONFIG_PM - /* Power Management Hooks */ - .suspend = e1000_suspend, - .resume = e1000_resume, -#endif + .driver = { + .pm = &e1000_pm_ops, + }, .shutdown = e1000_shutdown, .err_handler = &e1000_err_handler }; @@ -5052,9 +5052,6 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool *enable_wake) struct e1000_hw *hw = &adapter->hw; u32 ctrl, ctrl_ext, rctl, status; u32 wufc = adapter->wol; -#ifdef CONFIG_PM - int retval = 0; -#endif netif_device_detach(netdev); @@ -5068,12 +5065,6 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool *enable_wake) e1000_down(adapter); } -#ifdef CONFIG_PM - retval = pci_save_state(pdev); - if (retval) - return retval; -#endif - status = er32(STATUS); if (status & E1000_STATUS_LU) wufc &= ~E1000_WUFC_LNKC; @@ -5135,36 +5126,22 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool *enable_wake) } #ifdef CONFIG_PM -static int e1000_suspend(struct pci_dev *pdev, pm_message_t state) +static int e1000_suspend(struct device *dev) { - int retval; + struct pci_dev *pdev = to_pci_dev(dev); bool wake; - retval = __e1000_shutdown(pdev, &wake); - if (retval) - return retval; - - if (wake) { - pci_prepare_to_sleep(pdev); - } else { - pci_wake_from_d3(pdev, false); - pci_set_power_state(pdev, PCI_D3hot); - } - - return 0; + return __e1000_shutdown(pdev, &wake); } -static int e1000_resume(struct pci_dev *pdev) +static int e1000_resume(struct device *dev) { + struct pci_dev *pdev = to_pci_dev(dev); struct net_device *netdev = pci_get_drvdata(pdev); struct e1000_adapter *adapter = netdev_priv(netdev); struct e1000_hw *hw = &adapter->hw; u32 err; - pci_set_power_state(pdev, PCI_D0); - pci_restore_state(pdev); - pci_save_state(pdev); - if (adapter->need_ioport) err = pci_enable_device(pdev); else -- 2.26.0 _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v1] ethernet: intel: e1000: Convert to dev_pm_ops 2020-04-10 12:44 [Linux-kernel-mentees] [PATCH v1] ethernet: intel: e1000: Convert to dev_pm_ops Vaibhav Gupta @ 2020-04-11 0:15 ` Bjorn Helgaas 2020-04-11 14:03 ` Vaibhav Gupta 2020-04-11 0:23 ` Bjorn Helgaas 2020-05-01 20:58 ` Bjorn Helgaas 2 siblings, 1 reply; 11+ messages in thread From: Bjorn Helgaas @ 2020-04-11 0:15 UTC (permalink / raw) To: Vaibhav Gupta; +Cc: rjw, linux-kernel-mentees Hi Vaibhav, Thanks for the patch. Minor procedural nits: 1) Use "git log --oneline drivers/net/ethernet/intel/e1000/e1000_main.c" and match the prefix to the convention for the file. The e1000-specific commits all start with "e1000: " 2) The subject should say something about the overall goal and why we want this, so "dev_pm_ops" is a little too low-level. I like "Convert to generic power management", e.g., e1000: Convert to generic power management When you eventually post this to netdev, I think you should post several patches as a series, e.g., maybe all the Intel drivers together. In this case it's not because they depend on each other, but because the patches will be similar and the same set of people will review them, so it's convenient for them to look at them all at once. On Fri, Apr 10, 2020 at 06:14:19PM +0530, Vaibhav Gupta wrote: > Convert the legacy callback .suspend() and .resume() > to the generic ones. This should mention "power management" somehow. I think 77b84bb306fd ("xen-platform: Convert to generic power management") is a good template (of course, since I wrote it :)). I have other questions, so no need to repost this until we work through those a little bit. Bjorn _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v1] ethernet: intel: e1000: Convert to dev_pm_ops 2020-04-11 0:15 ` Bjorn Helgaas @ 2020-04-11 14:03 ` Vaibhav Gupta 0 siblings, 0 replies; 11+ messages in thread From: Vaibhav Gupta @ 2020-04-11 14:03 UTC (permalink / raw) To: Bjorn Helgaas; +Cc: linux-kernel-mentees, Rafael J. Wysocki, Vaibhav Gupta > 1) Use "git log --oneline > drivers/net/ethernet/intel/e1000/e1000_main.c" and match the prefix to > the convention for the file. The e1000-specific commits all start > with "e1000: " Okay, will do that! > > 2) The subject should say something about the overall goal and why we > want this, so "dev_pm_ops" is a little too low-level. I like "Convert > to generic power management", e.g., > > e1000: Convert to generic power management Okay! > > When you eventually post this to netdev, I think you should post > several patches as a series, e.g., maybe all the Intel drivers > together. In this case it's not because they depend on each other, > but because the patches will be similar and the same set of people > will review them, so it's convenient for them to look at them all at > once. Yes, when I will send it to netdev, will send the patch-set for whole intel family. This patch is for RFC. > This should mention "power management" somehow. I think 77b84bb306fd > ("xen-platform: Convert to generic power management") is a good > template (of course, since I wrote it :)). Sure, I will follow that. > > I have other questions, so no need to repost this until we work > through those a little bit. > Okay! Thanks! --Vaibhav Gupta _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v1] ethernet: intel: e1000: Convert to dev_pm_ops 2020-04-10 12:44 [Linux-kernel-mentees] [PATCH v1] ethernet: intel: e1000: Convert to dev_pm_ops Vaibhav Gupta 2020-04-11 0:15 ` Bjorn Helgaas @ 2020-04-11 0:23 ` Bjorn Helgaas 2020-04-23 0:24 ` Bjorn Helgaas 2020-05-01 20:58 ` Bjorn Helgaas 2 siblings, 1 reply; 11+ messages in thread From: Bjorn Helgaas @ 2020-04-11 0:23 UTC (permalink / raw) To: Vaibhav Gupta, rjw; +Cc: linux-kernel-mentees On Fri, Apr 10, 2020 at 06:14:19PM +0530, Vaibhav Gupta wrote: > Convert the legacy callback .suspend() and .resume() > to the generic ones. > drivers/net/ethernet/intel/e1000/e1000_main.c | 47 +++++-------------- > @@ -5068,12 +5065,6 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool *enable_wake) > e1000_down(adapter); > } > > -#ifdef CONFIG_PM > - retval = pci_save_state(pdev); > - if (retval) > - return retval; > -#endif __e1000_shutdown() is used in both by both e1000_suspend() and e1000_shutdown(). Suspend is obviously for power management, but I don't understand the connection between shutdown and PM. Why would something in the shutdown path would depend on CONFIG_PM? If we're in the shutdown path, we're either going to power off the whole machine or reboot (or kexec a new kernel). In any event, I don't think the pci_save_state() is needed, so removing it shouldn't hurt shutdown. But I'm curious about this common idiom in e1000 and other network drivers: e1000_shutdown() { ... if (system_state == SYSTEM_POWER_OFF) { pci_wake_from_d3(pdev, wake); pci_set_power_state(pdev, PCI_D3hot); } } The pci_wake_from_d3() part sort of makes sense -- we want to configure the device for wake-on-lan. But all the drivers that do this are network drivers. USB, keyboard, etc drivers also need that functionality, but none of them use pci_wake_from_d3(). Why? And why do we need the pci_set_power_state()? Seems like if we're going to power off the whole machine we wouldn't need to set devices to D3hot. Almost all the pci_set_power_state(pdev, PCI_D3hot) calls are from legacy suspend functions (which makes sense to me) or network driver shutdown functions, and I don't understand why network drivers do this differently than others. Bjorn _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v1] ethernet: intel: e1000: Convert to dev_pm_ops 2020-04-11 0:23 ` Bjorn Helgaas @ 2020-04-23 0:24 ` Bjorn Helgaas 0 siblings, 0 replies; 11+ messages in thread From: Bjorn Helgaas @ 2020-04-23 0:24 UTC (permalink / raw) To: Vaibhav Gupta, rjw; +Cc: linux-kernel-mentees Rafael, do you have any thoughts on this? On Fri, Apr 10, 2020 at 07:23:12PM -0500, Bjorn Helgaas wrote: > On Fri, Apr 10, 2020 at 06:14:19PM +0530, Vaibhav Gupta wrote: > > Convert the legacy callback .suspend() and .resume() > > to the generic ones. > > > drivers/net/ethernet/intel/e1000/e1000_main.c | 47 +++++-------------- > > > @@ -5068,12 +5065,6 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool *enable_wake) > > e1000_down(adapter); > > } > > > > -#ifdef CONFIG_PM > > - retval = pci_save_state(pdev); > > - if (retval) > > - return retval; > > -#endif > > __e1000_shutdown() is used in both by both e1000_suspend() and > e1000_shutdown(). Suspend is obviously for power management, but I > don't understand the connection between shutdown and PM. Why would > something in the shutdown path would depend on CONFIG_PM? > > If we're in the shutdown path, we're either going to power off the > whole machine or reboot (or kexec a new kernel). In any event, I > don't think the pci_save_state() is needed, so removing it shouldn't > hurt shutdown. I think it's OK to remove the pci_save_state() because I don't think we ever needed it in the .shutdown() path, and we shouldn't need it in the new generic .suspend() path. But the commit log should probably mention the fact that it's being removed from .shutdown(). > But I'm curious about this common idiom in e1000 and other network > drivers: > > e1000_shutdown() > { > ... > if (system_state == SYSTEM_POWER_OFF) { > pci_wake_from_d3(pdev, wake); > pci_set_power_state(pdev, PCI_D3hot); > } > } > > The pci_wake_from_d3() part sort of makes sense -- we want to > configure the device for wake-on-lan. But all the drivers that do > this are network drivers. USB, keyboard, etc drivers also need that > functionality, but none of them use pci_wake_from_d3(). Why? > > And why do we need the pci_set_power_state()? Seems like if we're > going to power off the whole machine we wouldn't need to set devices > to D3hot. Almost all the pci_set_power_state(pdev, PCI_D3hot) calls > are from legacy suspend functions (which makes sense to me) or network > driver shutdown functions, and I don't understand why network drivers > do this differently than others. This part is a tangent. Most NIC drivers don't have this code, so I suspect it's not quite right, but you're not changing this part of .shutdown(), so we can just let sleeping dogs lie for now. Bjorn _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v1] ethernet: intel: e1000: Convert to dev_pm_ops 2020-04-10 12:44 [Linux-kernel-mentees] [PATCH v1] ethernet: intel: e1000: Convert to dev_pm_ops Vaibhav Gupta 2020-04-11 0:15 ` Bjorn Helgaas 2020-04-11 0:23 ` Bjorn Helgaas @ 2020-05-01 20:58 ` Bjorn Helgaas 2020-05-01 21:19 ` Kirsher, Jeffrey T 2 siblings, 1 reply; 11+ messages in thread From: Bjorn Helgaas @ 2020-05-01 20:58 UTC (permalink / raw) To: Vaibhav Gupta; +Cc: rjw, Jesse Brandeburg, Jeff Kirsher, linux-kernel-mentees [+cc Jeff, Jesse] On Fri, Apr 10, 2020 at 06:14:19PM +0530, Vaibhav Gupta wrote: > Convert the legacy callback .suspend() and .resume() > to the generic ones. > > Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com> > --- > drivers/net/ethernet/intel/e1000/e1000_main.c | 47 +++++-------------- > 1 file changed, 12 insertions(+), 35 deletions(-) > > diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c > index 2bced34c19ba..09a6ef46be96 100644 > --- a/drivers/net/ethernet/intel/e1000/e1000_main.c > +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c > @@ -152,8 +152,8 @@ static int e1000_vlan_rx_kill_vid(struct net_device *netdev, > static void e1000_restore_vlan(struct e1000_adapter *adapter); > > #ifdef CONFIG_PM > -static int e1000_suspend(struct pci_dev *pdev, pm_message_t state); > -static int e1000_resume(struct pci_dev *pdev); > +static int e1000_suspend(struct device *dev); > +static int e1000_resume(struct device *dev); > #endif > static void e1000_shutdown(struct pci_dev *pdev); > > @@ -179,16 +179,16 @@ static const struct pci_error_handlers e1000_err_handler = { > .resume = e1000_io_resume, > }; > > +static SIMPLE_DEV_PM_OPS(e1000_pm_ops, e1000_suspend, e1000_resume); > + > static struct pci_driver e1000_driver = { > .name = e1000_driver_name, > .id_table = e1000_pci_tbl, > .probe = e1000_probe, > .remove = e1000_remove, > -#ifdef CONFIG_PM > - /* Power Management Hooks */ > - .suspend = e1000_suspend, > - .resume = e1000_resume, > -#endif > + .driver = { > + .pm = &e1000_pm_ops, > + }, > .shutdown = e1000_shutdown, > .err_handler = &e1000_err_handler > }; > @@ -5052,9 +5052,6 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool *enable_wake) > struct e1000_hw *hw = &adapter->hw; > u32 ctrl, ctrl_ext, rctl, status; > u32 wufc = adapter->wol; > -#ifdef CONFIG_PM > - int retval = 0; > -#endif > > netif_device_detach(netdev); > > @@ -5068,12 +5065,6 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool *enable_wake) > e1000_down(adapter); > } > > -#ifdef CONFIG_PM > - retval = pci_save_state(pdev); > - if (retval) > - return retval; > -#endif > - > status = er32(STATUS); > if (status & E1000_STATUS_LU) > wufc &= ~E1000_WUFC_LNKC; > @@ -5135,36 +5126,22 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool *enable_wake) > } > > #ifdef CONFIG_PM > -static int e1000_suspend(struct pci_dev *pdev, pm_message_t state) > +static int e1000_suspend(struct device *dev) > { > - int retval; > + struct pci_dev *pdev = to_pci_dev(dev); > bool wake; > > - retval = __e1000_shutdown(pdev, &wake); > - if (retval) > - return retval; > - > - if (wake) { > - pci_prepare_to_sleep(pdev); > - } else { > - pci_wake_from_d3(pdev, false); I think there's a case where this changes the behavior because we normally set the device wakeup enable to adapter->wol, but the "wake" returned from __e1000_shutdown() is sometimes different. e1000_probe adapter->wol = adapter->eeprom_wol; # assume adapter->wol == 1 device_set_wakeup_enable(adapter->wol); Existing code: e1000_suspend __e1000_shutdown(&wake) # assume returns wake == 0 pci_wake_from_d3(false) pci_enable_wake(PCI_D3hot, false) # <-- compare New code using generic PM ops: pci_pm_suspend e1000_suspend __e1000_shutdown(&wake) # returns wake == 0 (ignored) pci_pm_suspend_noirq pci_prepare_to_sleep wakeup = device_may_wakeup() # returns 1 pci_enable_wake(PCI_D3hot, true) # <-- different! I sort of suspect that __e1000_shutdown() should call device_set_wakeup_enable() when it updates the chip's wake-on-lan registers, but the driver maintainers would know better. > - pci_set_power_state(pdev, PCI_D3hot); > - } > - > - return 0; > + return __e1000_shutdown(pdev, &wake); > } > > -static int e1000_resume(struct pci_dev *pdev) > +static int e1000_resume(struct device *dev) > { > + struct pci_dev *pdev = to_pci_dev(dev); > struct net_device *netdev = pci_get_drvdata(pdev); > struct e1000_adapter *adapter = netdev_priv(netdev); > struct e1000_hw *hw = &adapter->hw; > u32 err; > > - pci_set_power_state(pdev, PCI_D0); > - pci_restore_state(pdev); > - pci_save_state(pdev); > - > if (adapter->need_ioport) > err = pci_enable_device(pdev); > else > -- > 2.26.0 > _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v1] ethernet: intel: e1000: Convert to dev_pm_ops 2020-05-01 20:58 ` Bjorn Helgaas @ 2020-05-01 21:19 ` Kirsher, Jeffrey T 2020-05-01 21:45 ` Bjorn Helgaas 0 siblings, 1 reply; 11+ messages in thread From: Kirsher, Jeffrey T @ 2020-05-01 21:19 UTC (permalink / raw) To: Bjorn Helgaas, Vaibhav Gupta; +Cc: rjw, linux-kernel-mentees, Brandeburg, Jesse > -----Original Message----- > From: Bjorn Helgaas <helgaas@kernel.org> > Sent: Friday, May 1, 2020 13:58 > To: Vaibhav Gupta <vaibhavgupta40@gmail.com> > Cc: linux-kernel-mentees@lists.linuxfoundation.org; bjorn@helgaas.com; > skhan@linuxfoundation.org; rjw@rjwysocki.net; Kirsher, Jeffrey T > <jeffrey.t.kirsher@intel.com>; Brandeburg, Jesse > <jesse.brandeburg@intel.com> > Subject: Re: [Linux-kernel-mentees] [PATCH v1] ethernet: intel: e1000: Convert > to dev_pm_ops > > [+cc Jeff, Jesse] > > On Fri, Apr 10, 2020 at 06:14:19PM +0530, Vaibhav Gupta wrote: > > Convert the legacy callback .suspend() and .resume() to the generic > > ones. > > > > Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com> > > --- > > drivers/net/ethernet/intel/e1000/e1000_main.c | 47 > > +++++-------------- > > 1 file changed, 12 insertions(+), 35 deletions(-) [Kirsher, Jeffrey T] Was there a reason why this patch was not at least CC'd to intel-wired-lan@lists.osuosl.org mailing list for all Intel wired LAN driver changes? Thank you Bjorn for at least adding Jesse and myself to the thread. On top of the potential issues Bjorn pointed out, this could cause regression issues that can not fully be validated since this driver is really old and all supported devices may no longer be available to test against. > > > > diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c > > b/drivers/net/ethernet/intel/e1000/e1000_main.c > > index 2bced34c19ba..09a6ef46be96 100644 > > --- a/drivers/net/ethernet/intel/e1000/e1000_main.c > > +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c > > @@ -152,8 +152,8 @@ static int e1000_vlan_rx_kill_vid(struct > > net_device *netdev, static void e1000_restore_vlan(struct > > e1000_adapter *adapter); > > > > #ifdef CONFIG_PM > > -static int e1000_suspend(struct pci_dev *pdev, pm_message_t state); > > -static int e1000_resume(struct pci_dev *pdev); > > +static int e1000_suspend(struct device *dev); static int > > +e1000_resume(struct device *dev); > > #endif > > static void e1000_shutdown(struct pci_dev *pdev); > > > > @@ -179,16 +179,16 @@ static const struct pci_error_handlers > e1000_err_handler = { > > .resume = e1000_io_resume, > > }; > > > > +static SIMPLE_DEV_PM_OPS(e1000_pm_ops, e1000_suspend, > e1000_resume); > > + > > static struct pci_driver e1000_driver = { > > .name = e1000_driver_name, > > .id_table = e1000_pci_tbl, > > .probe = e1000_probe, > > .remove = e1000_remove, > > -#ifdef CONFIG_PM > > - /* Power Management Hooks */ > > - .suspend = e1000_suspend, > > - .resume = e1000_resume, > > -#endif > > + .driver = { > > + .pm = &e1000_pm_ops, > > + }, > > .shutdown = e1000_shutdown, > > .err_handler = &e1000_err_handler > > }; > > @@ -5052,9 +5052,6 @@ static int __e1000_shutdown(struct pci_dev *pdev, > bool *enable_wake) > > struct e1000_hw *hw = &adapter->hw; > > u32 ctrl, ctrl_ext, rctl, status; > > u32 wufc = adapter->wol; > > -#ifdef CONFIG_PM > > - int retval = 0; > > -#endif > > > > netif_device_detach(netdev); > > > > @@ -5068,12 +5065,6 @@ static int __e1000_shutdown(struct pci_dev > *pdev, bool *enable_wake) > > e1000_down(adapter); > > } > > > > -#ifdef CONFIG_PM > > - retval = pci_save_state(pdev); > > - if (retval) > > - return retval; > > -#endif > > - > > status = er32(STATUS); > > if (status & E1000_STATUS_LU) > > wufc &= ~E1000_WUFC_LNKC; > > @@ -5135,36 +5126,22 @@ static int __e1000_shutdown(struct pci_dev > > *pdev, bool *enable_wake) } > > > > #ifdef CONFIG_PM > > -static int e1000_suspend(struct pci_dev *pdev, pm_message_t state) > > +static int e1000_suspend(struct device *dev) > > { > > - int retval; > > + struct pci_dev *pdev = to_pci_dev(dev); > > bool wake; > > > > - retval = __e1000_shutdown(pdev, &wake); > > - if (retval) > > - return retval; > > - > > - if (wake) { > > - pci_prepare_to_sleep(pdev); > > - } else { > > - pci_wake_from_d3(pdev, false); > > I think there's a case where this changes the behavior because we normally set > the device wakeup enable to adapter->wol, but the "wake" > returned from __e1000_shutdown() is sometimes different. > > e1000_probe > adapter->wol = adapter->eeprom_wol; # assume adapter->wol == 1 > device_set_wakeup_enable(adapter->wol); > > Existing code: > e1000_suspend > __e1000_shutdown(&wake) # assume returns wake == 0 > pci_wake_from_d3(false) > pci_enable_wake(PCI_D3hot, false) # <-- compare > > New code using generic PM ops: > pci_pm_suspend > e1000_suspend > __e1000_shutdown(&wake) # returns wake == 0 (ignored) > pci_pm_suspend_noirq > pci_prepare_to_sleep > wakeup = device_may_wakeup() # returns 1 > pci_enable_wake(PCI_D3hot, true) # <-- different! > > I sort of suspect that __e1000_shutdown() should call > device_set_wakeup_enable() when it updates the chip's wake-on-lan registers, > but the driver maintainers would know better. > > > - pci_set_power_state(pdev, PCI_D3hot); > > - } > > - > > - return 0; > > + return __e1000_shutdown(pdev, &wake); > > } > > > > -static int e1000_resume(struct pci_dev *pdev) > > +static int e1000_resume(struct device *dev) > > { > > + struct pci_dev *pdev = to_pci_dev(dev); > > struct net_device *netdev = pci_get_drvdata(pdev); > > struct e1000_adapter *adapter = netdev_priv(netdev); > > struct e1000_hw *hw = &adapter->hw; > > u32 err; > > > > - pci_set_power_state(pdev, PCI_D0); > > - pci_restore_state(pdev); > > - pci_save_state(pdev); > > - > > if (adapter->need_ioport) > > err = pci_enable_device(pdev); > > else > > -- > > 2.26.0 > > _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v1] ethernet: intel: e1000: Convert to dev_pm_ops 2020-05-01 21:19 ` Kirsher, Jeffrey T @ 2020-05-01 21:45 ` Bjorn Helgaas 2020-05-18 15:07 ` Vaibhav Gupta 0 siblings, 1 reply; 11+ messages in thread From: Bjorn Helgaas @ 2020-05-01 21:45 UTC (permalink / raw) To: Kirsher, Jeffrey T Cc: Vaibhav Gupta, rjw, Brandeburg, Jesse, linux-kernel-mentees On Fri, May 01, 2020 at 09:19:00PM +0000, Kirsher, Jeffrey T wrote: > > -----Original Message----- > > From: Bjorn Helgaas <helgaas@kernel.org> > > Sent: Friday, May 1, 2020 13:58 > > To: Vaibhav Gupta <vaibhavgupta40@gmail.com> > > Cc: linux-kernel-mentees@lists.linuxfoundation.org; bjorn@helgaas.com; > > skhan@linuxfoundation.org; rjw@rjwysocki.net; Kirsher, Jeffrey T > > <jeffrey.t.kirsher@intel.com>; Brandeburg, Jesse > > <jesse.brandeburg@intel.com> > > Subject: Re: [Linux-kernel-mentees] [PATCH v1] ethernet: intel: e1000: Convert > > to dev_pm_ops > > > > [+cc Jeff, Jesse] > > > > On Fri, Apr 10, 2020 at 06:14:19PM +0530, Vaibhav Gupta wrote: > > > Convert the legacy callback .suspend() and .resume() to the generic > > > ones. > > > > > > Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com> > > > --- > > > drivers/net/ethernet/intel/e1000/e1000_main.c | 47 > > > +++++-------------- > > > 1 file changed, 12 insertions(+), 35 deletions(-) > [Kirsher, Jeffrey T] > > Was there a reason why this patch was not at least CC'd to > intel-wired-lan@lists.osuosl.org mailing list for all Intel wired > LAN driver changes? Thank you Bjorn for at least adding Jesse and > myself to the thread. Don't worry, this is potentially part of a mentoring project, and we're just trying to shake out trivial issues first before throwing Vaibhav straight into the lions' den, so to speak. > On top of the potential issues Bjorn pointed out, this could cause > regression issues that can not fully be validated since this driver > is really old and all supported devices may no longer be available > to test against. Understood. The problem we're trying to solve is that the PCI core carries quite a bit of code to support both the legacy power management and the "new" (now 10 year-old) generic scheme. That extra code is its own source of bugs since it's hard to keep both paths up to date and tested. > > > diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c > > > b/drivers/net/ethernet/intel/e1000/e1000_main.c > > > index 2bced34c19ba..09a6ef46be96 100644 > > > --- a/drivers/net/ethernet/intel/e1000/e1000_main.c > > > +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c > > > @@ -152,8 +152,8 @@ static int e1000_vlan_rx_kill_vid(struct > > > net_device *netdev, static void e1000_restore_vlan(struct > > > e1000_adapter *adapter); > > > > > > #ifdef CONFIG_PM > > > -static int e1000_suspend(struct pci_dev *pdev, pm_message_t state); > > > -static int e1000_resume(struct pci_dev *pdev); > > > +static int e1000_suspend(struct device *dev); static int > > > +e1000_resume(struct device *dev); > > > #endif > > > static void e1000_shutdown(struct pci_dev *pdev); > > > > > > @@ -179,16 +179,16 @@ static const struct pci_error_handlers > > e1000_err_handler = { > > > .resume = e1000_io_resume, > > > }; > > > > > > +static SIMPLE_DEV_PM_OPS(e1000_pm_ops, e1000_suspend, > > e1000_resume); > > > + > > > static struct pci_driver e1000_driver = { > > > .name = e1000_driver_name, > > > .id_table = e1000_pci_tbl, > > > .probe = e1000_probe, > > > .remove = e1000_remove, > > > -#ifdef CONFIG_PM > > > - /* Power Management Hooks */ > > > - .suspend = e1000_suspend, > > > - .resume = e1000_resume, > > > -#endif > > > + .driver = { > > > + .pm = &e1000_pm_ops, > > > + }, > > > .shutdown = e1000_shutdown, > > > .err_handler = &e1000_err_handler > > > }; > > > @@ -5052,9 +5052,6 @@ static int __e1000_shutdown(struct pci_dev *pdev, > > bool *enable_wake) > > > struct e1000_hw *hw = &adapter->hw; > > > u32 ctrl, ctrl_ext, rctl, status; > > > u32 wufc = adapter->wol; > > > -#ifdef CONFIG_PM > > > - int retval = 0; > > > -#endif > > > > > > netif_device_detach(netdev); > > > > > > @@ -5068,12 +5065,6 @@ static int __e1000_shutdown(struct pci_dev > > *pdev, bool *enable_wake) > > > e1000_down(adapter); > > > } > > > > > > -#ifdef CONFIG_PM > > > - retval = pci_save_state(pdev); > > > - if (retval) > > > - return retval; > > > -#endif > > > - > > > status = er32(STATUS); > > > if (status & E1000_STATUS_LU) > > > wufc &= ~E1000_WUFC_LNKC; > > > @@ -5135,36 +5126,22 @@ static int __e1000_shutdown(struct pci_dev > > > *pdev, bool *enable_wake) } > > > > > > #ifdef CONFIG_PM > > > -static int e1000_suspend(struct pci_dev *pdev, pm_message_t state) > > > +static int e1000_suspend(struct device *dev) > > > { > > > - int retval; > > > + struct pci_dev *pdev = to_pci_dev(dev); > > > bool wake; > > > > > > - retval = __e1000_shutdown(pdev, &wake); > > > - if (retval) > > > - return retval; > > > - > > > - if (wake) { > > > - pci_prepare_to_sleep(pdev); > > > - } else { > > > - pci_wake_from_d3(pdev, false); > > > > I think there's a case where this changes the behavior because we normally set > > the device wakeup enable to adapter->wol, but the "wake" > > returned from __e1000_shutdown() is sometimes different. > > > > e1000_probe > > adapter->wol = adapter->eeprom_wol; # assume adapter->wol == 1 > > device_set_wakeup_enable(adapter->wol); > > > > Existing code: > > e1000_suspend > > __e1000_shutdown(&wake) # assume returns wake == 0 > > pci_wake_from_d3(false) > > pci_enable_wake(PCI_D3hot, false) # <-- compare > > > > New code using generic PM ops: > > pci_pm_suspend > > e1000_suspend > > __e1000_shutdown(&wake) # returns wake == 0 (ignored) > > pci_pm_suspend_noirq > > pci_prepare_to_sleep > > wakeup = device_may_wakeup() # returns 1 > > pci_enable_wake(PCI_D3hot, true) # <-- different! > > > > I sort of suspect that __e1000_shutdown() should call > > device_set_wakeup_enable() when it updates the chip's wake-on-lan > > registers, but the driver maintainers would know better. If you have any ideas about this, or maybe patterns in newer Intel drivers that could be followed here, we'd love to hear them. If you think it's impossible to update e1000 to generic power management, it'd be useful to know that, too, so we can move on to other drivers. > > > - pci_set_power_state(pdev, PCI_D3hot); > > > - } > > > - > > > - return 0; > > > + return __e1000_shutdown(pdev, &wake); > > > } > > > > > > -static int e1000_resume(struct pci_dev *pdev) > > > +static int e1000_resume(struct device *dev) > > > { > > > + struct pci_dev *pdev = to_pci_dev(dev); > > > struct net_device *netdev = pci_get_drvdata(pdev); > > > struct e1000_adapter *adapter = netdev_priv(netdev); > > > struct e1000_hw *hw = &adapter->hw; > > > u32 err; > > > > > > - pci_set_power_state(pdev, PCI_D0); > > > - pci_restore_state(pdev); > > > - pci_save_state(pdev); > > > - > > > if (adapter->need_ioport) > > > err = pci_enable_device(pdev); > > > else > > > -- > > > 2.26.0 > > > _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v1] ethernet: intel: e1000: Convert to dev_pm_ops 2020-05-01 21:45 ` Bjorn Helgaas @ 2020-05-18 15:07 ` Vaibhav Gupta 2020-05-18 15:27 ` Bjorn Helgaas 0 siblings, 1 reply; 11+ messages in thread From: Vaibhav Gupta @ 2020-05-18 15:07 UTC (permalink / raw) To: Bjorn Helgaas Cc: Brandeburg, Jesse, rjw, Kirsher, Jeffrey T, linux-kernel-mentees, Vaibhav Gupta On Sat, 2 May 2020 at 03:15, Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Fri, May 01, 2020 at 09:19:00PM +0000, Kirsher, Jeffrey T wrote: > > > -----Original Message----- > > > From: Bjorn Helgaas <helgaas@kernel.org> > > > Sent: Friday, May 1, 2020 13:58 > > > To: Vaibhav Gupta <vaibhavgupta40@gmail.com> > > > Cc: linux-kernel-mentees@lists.linuxfoundation.org; bjorn@helgaas.com; > > > skhan@linuxfoundation.org; rjw@rjwysocki.net; Kirsher, Jeffrey T > > > <jeffrey.t.kirsher@intel.com>; Brandeburg, Jesse > > > <jesse.brandeburg@intel.com> > > > Subject: Re: [Linux-kernel-mentees] [PATCH v1] ethernet: intel: e1000: Convert > > > to dev_pm_ops > > > > > > [+cc Jeff, Jesse] > > > > > > On Fri, Apr 10, 2020 at 06:14:19PM +0530, Vaibhav Gupta wrote: > > > > Convert the legacy callback .suspend() and .resume() to the generic > > > > ones. > > > > > > > > Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com> > > > > --- > > > > drivers/net/ethernet/intel/e1000/e1000_main.c | 47 > > > > +++++-------------- > > > > 1 file changed, 12 insertions(+), 35 deletions(-) > > [Kirsher, Jeffrey T] > > > > Was there a reason why this patch was not at least CC'd to > > intel-wired-lan@lists.osuosl.org mailing list for all Intel wired > > LAN driver changes? Thank you Bjorn for at least adding Jesse and > > myself to the thread. > > Don't worry, this is potentially part of a mentoring project, and > we're just trying to shake out trivial issues first before throwing > Vaibhav straight into the lions' den, so to speak. > > > On top of the potential issues Bjorn pointed out, this could cause > > regression issues that can not fully be validated since this driver > > is really old and all supported devices may no longer be available > > to test against. > > Understood. The problem we're trying to solve is that the PCI > core carries quite a bit of code to support both the legacy power > management and the "new" (now 10 year-old) generic scheme. That > extra code is its own source of bugs since it's hard to keep both > paths up to date and tested. > > > > > diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c > > > > b/drivers/net/ethernet/intel/e1000/e1000_main.c > > > > index 2bced34c19ba..09a6ef46be96 100644 > > > > --- a/drivers/net/ethernet/intel/e1000/e1000_main.c > > > > +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c > > > > @@ -152,8 +152,8 @@ static int e1000_vlan_rx_kill_vid(struct > > > > net_device *netdev, static void e1000_restore_vlan(struct > > > > e1000_adapter *adapter); > > > > > > > > #ifdef CONFIG_PM > > > > -static int e1000_suspend(struct pci_dev *pdev, pm_message_t state); > > > > -static int e1000_resume(struct pci_dev *pdev); > > > > +static int e1000_suspend(struct device *dev); static int > > > > +e1000_resume(struct device *dev); > > > > #endif > > > > static void e1000_shutdown(struct pci_dev *pdev); > > > > > > > > @@ -179,16 +179,16 @@ static const struct pci_error_handlers > > > e1000_err_handler = { > > > > .resume = e1000_io_resume, > > > > }; > > > > > > > > +static SIMPLE_DEV_PM_OPS(e1000_pm_ops, e1000_suspend, > > > e1000_resume); > > > > + > > > > static struct pci_driver e1000_driver = { > > > > .name = e1000_driver_name, > > > > .id_table = e1000_pci_tbl, > > > > .probe = e1000_probe, > > > > .remove = e1000_remove, > > > > -#ifdef CONFIG_PM > > > > - /* Power Management Hooks */ > > > > - .suspend = e1000_suspend, > > > > - .resume = e1000_resume, > > > > -#endif > > > > + .driver = { > > > > + .pm = &e1000_pm_ops, > > > > + }, > > > > .shutdown = e1000_shutdown, > > > > .err_handler = &e1000_err_handler > > > > }; > > > > @@ -5052,9 +5052,6 @@ static int __e1000_shutdown(struct pci_dev *pdev, > > > bool *enable_wake) > > > > struct e1000_hw *hw = &adapter->hw; > > > > u32 ctrl, ctrl_ext, rctl, status; > > > > u32 wufc = adapter->wol; > > > > -#ifdef CONFIG_PM > > > > - int retval = 0; > > > > -#endif > > > > > > > > netif_device_detach(netdev); > > > > > > > > @@ -5068,12 +5065,6 @@ static int __e1000_shutdown(struct pci_dev > > > *pdev, bool *enable_wake) > > > > e1000_down(adapter); > > > > } > > > > > > > > -#ifdef CONFIG_PM > > > > - retval = pci_save_state(pdev); > > > > - if (retval) > > > > - return retval; > > > > -#endif > > > > - > > > > status = er32(STATUS); > > > > if (status & E1000_STATUS_LU) > > > > wufc &= ~E1000_WUFC_LNKC; > > > > @@ -5135,36 +5126,22 @@ static int __e1000_shutdown(struct pci_dev > > > > *pdev, bool *enable_wake) } > > > > > > > > #ifdef CONFIG_PM > > > > -static int e1000_suspend(struct pci_dev *pdev, pm_message_t state) > > > > +static int e1000_suspend(struct device *dev) > > > > { > > > > - int retval; > > > > + struct pci_dev *pdev = to_pci_dev(dev); > > > > bool wake; > > > > > > > > - retval = __e1000_shutdown(pdev, &wake); > > > > - if (retval) > > > > - return retval; > > > > - > > > > - if (wake) { > > > > - pci_prepare_to_sleep(pdev); > > > > - } else { > > > > - pci_wake_from_d3(pdev, false); > > > > > > I think there's a case where this changes the behavior because we normally set > > > the device wakeup enable to adapter->wol, but the "wake" > > > returned from __e1000_shutdown() is sometimes different. > > > > > > e1000_probe > > > adapter->wol = adapter->eeprom_wol; # assume adapter->wol == 1 > > > device_set_wakeup_enable(adapter->wol); > > > > > > Existing code: > > > e1000_suspend > > > __e1000_shutdown(&wake) # assume returns wake == 0 > > > pci_wake_from_d3(false) > > > pci_enable_wake(PCI_D3hot, false) # <-- compare > > > > > > New code using generic PM ops: > > > pci_pm_suspend > > > e1000_suspend > > > __e1000_shutdown(&wake) # returns wake == 0 (ignored) > > > pci_pm_suspend_noirq > > > pci_prepare_to_sleep > > > wakeup = device_may_wakeup() # returns 1 > > > pci_enable_wake(PCI_D3hot, true) # <-- different! > > > > > > I sort of suspect that __e1000_shutdown() should call > > > device_set_wakeup_enable() when it updates the chip's wake-on-lan > > > registers, but the driver maintainers would know better. > > If you have any ideas about this, or maybe patterns in newer Intel > drivers that could be followed here, we'd love to hear them. > > If you think it's impossible to update e1000 to generic power > management, it'd be useful to know that, too, so we can move on to > other drivers. Is it safe to assume that we should leave e1000 for now? As the thread seems to be discontinued. --Vaibhav Gupta > > > > > - pci_set_power_state(pdev, PCI_D3hot); > > > > - } > > > > - > > > > - return 0; > > > > + return __e1000_shutdown(pdev, &wake); > > > > } > > > > > > > > -static int e1000_resume(struct pci_dev *pdev) > > > > +static int e1000_resume(struct device *dev) > > > > { > > > > + struct pci_dev *pdev = to_pci_dev(dev); > > > > struct net_device *netdev = pci_get_drvdata(pdev); > > > > struct e1000_adapter *adapter = netdev_priv(netdev); > > > > struct e1000_hw *hw = &adapter->hw; > > > > u32 err; > > > > > > > > - pci_set_power_state(pdev, PCI_D0); > > > > - pci_restore_state(pdev); > > > > - pci_save_state(pdev); > > > > - > > > > if (adapter->need_ioport) > > > > err = pci_enable_device(pdev); > > > > else > > > > -- > > > > 2.26.0 > > > > > _______________________________________________ > Linux-kernel-mentees mailing list > Linux-kernel-mentees@lists.linuxfoundation.org > https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v1] ethernet: intel: e1000: Convert to dev_pm_ops 2020-05-18 15:07 ` Vaibhav Gupta @ 2020-05-18 15:27 ` Bjorn Helgaas 2020-05-18 15:30 ` Vaibhav Gupta 0 siblings, 1 reply; 11+ messages in thread From: Bjorn Helgaas @ 2020-05-18 15:27 UTC (permalink / raw) To: Vaibhav Gupta Cc: Brandeburg, Jesse, rjw, Kirsher, Jeffrey T, linux-kernel-mentees, Vaibhav Gupta On Mon, May 18, 2020 at 08:37:13PM +0530, Vaibhav Gupta wrote: > On Sat, 2 May 2020 at 03:15, Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Fri, May 01, 2020 at 09:19:00PM +0000, Kirsher, Jeffrey T wrote: > > > > -----Original Message----- > > > > From: Bjorn Helgaas <helgaas@kernel.org> > > > > Sent: Friday, May 1, 2020 13:58 > > > > To: Vaibhav Gupta <vaibhavgupta40@gmail.com> > > > > Cc: linux-kernel-mentees@lists.linuxfoundation.org; bjorn@helgaas.com; > > > > skhan@linuxfoundation.org; rjw@rjwysocki.net; Kirsher, Jeffrey T > > > > <jeffrey.t.kirsher@intel.com>; Brandeburg, Jesse > > > > <jesse.brandeburg@intel.com> > > > > Subject: Re: [Linux-kernel-mentees] [PATCH v1] ethernet: intel: e1000: Convert > > > > to dev_pm_ops > > > > > > > > [+cc Jeff, Jesse] > > > > > > > > On Fri, Apr 10, 2020 at 06:14:19PM +0530, Vaibhav Gupta wrote: > > > > > Convert the legacy callback .suspend() and .resume() to the generic > > > > > ones. > > > > > > > > > > Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com> > > > > > --- > > > > > drivers/net/ethernet/intel/e1000/e1000_main.c | 47 > > > > > +++++-------------- > > > > > 1 file changed, 12 insertions(+), 35 deletions(-) > > > [Kirsher, Jeffrey T] > > > > > > Was there a reason why this patch was not at least CC'd to > > > intel-wired-lan@lists.osuosl.org mailing list for all Intel wired > > > LAN driver changes? Thank you Bjorn for at least adding Jesse and > > > myself to the thread. > > > > Don't worry, this is potentially part of a mentoring project, and > > we're just trying to shake out trivial issues first before throwing > > Vaibhav straight into the lions' den, so to speak. > > > > > On top of the potential issues Bjorn pointed out, this could cause > > > regression issues that can not fully be validated since this driver > > > is really old and all supported devices may no longer be available > > > to test against. > > > > Understood. The problem we're trying to solve is that the PCI > > core carries quite a bit of code to support both the legacy power > > management and the "new" (now 10 year-old) generic scheme. That > > extra code is its own source of bugs since it's hard to keep both > > paths up to date and tested. > > > > > > > diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c > > > > > b/drivers/net/ethernet/intel/e1000/e1000_main.c > > > > > index 2bced34c19ba..09a6ef46be96 100644 > > > > > --- a/drivers/net/ethernet/intel/e1000/e1000_main.c > > > > > +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c > > > > > @@ -152,8 +152,8 @@ static int e1000_vlan_rx_kill_vid(struct > > > > > net_device *netdev, static void e1000_restore_vlan(struct > > > > > e1000_adapter *adapter); > > > > > > > > > > #ifdef CONFIG_PM > > > > > -static int e1000_suspend(struct pci_dev *pdev, pm_message_t state); > > > > > -static int e1000_resume(struct pci_dev *pdev); > > > > > +static int e1000_suspend(struct device *dev); static int > > > > > +e1000_resume(struct device *dev); > > > > > #endif > > > > > static void e1000_shutdown(struct pci_dev *pdev); > > > > > > > > > > @@ -179,16 +179,16 @@ static const struct pci_error_handlers > > > > e1000_err_handler = { > > > > > .resume = e1000_io_resume, > > > > > }; > > > > > > > > > > +static SIMPLE_DEV_PM_OPS(e1000_pm_ops, e1000_suspend, > > > > e1000_resume); > > > > > + > > > > > static struct pci_driver e1000_driver = { > > > > > .name = e1000_driver_name, > > > > > .id_table = e1000_pci_tbl, > > > > > .probe = e1000_probe, > > > > > .remove = e1000_remove, > > > > > -#ifdef CONFIG_PM > > > > > - /* Power Management Hooks */ > > > > > - .suspend = e1000_suspend, > > > > > - .resume = e1000_resume, > > > > > -#endif > > > > > + .driver = { > > > > > + .pm = &e1000_pm_ops, > > > > > + }, > > > > > .shutdown = e1000_shutdown, > > > > > .err_handler = &e1000_err_handler > > > > > }; > > > > > @@ -5052,9 +5052,6 @@ static int __e1000_shutdown(struct pci_dev *pdev, > > > > bool *enable_wake) > > > > > struct e1000_hw *hw = &adapter->hw; > > > > > u32 ctrl, ctrl_ext, rctl, status; > > > > > u32 wufc = adapter->wol; > > > > > -#ifdef CONFIG_PM > > > > > - int retval = 0; > > > > > -#endif > > > > > > > > > > netif_device_detach(netdev); > > > > > > > > > > @@ -5068,12 +5065,6 @@ static int __e1000_shutdown(struct pci_dev > > > > *pdev, bool *enable_wake) > > > > > e1000_down(adapter); > > > > > } > > > > > > > > > > -#ifdef CONFIG_PM > > > > > - retval = pci_save_state(pdev); > > > > > - if (retval) > > > > > - return retval; > > > > > -#endif > > > > > - > > > > > status = er32(STATUS); > > > > > if (status & E1000_STATUS_LU) > > > > > wufc &= ~E1000_WUFC_LNKC; > > > > > @@ -5135,36 +5126,22 @@ static int __e1000_shutdown(struct pci_dev > > > > > *pdev, bool *enable_wake) } > > > > > > > > > > #ifdef CONFIG_PM > > > > > -static int e1000_suspend(struct pci_dev *pdev, pm_message_t state) > > > > > +static int e1000_suspend(struct device *dev) > > > > > { > > > > > - int retval; > > > > > + struct pci_dev *pdev = to_pci_dev(dev); > > > > > bool wake; > > > > > > > > > > - retval = __e1000_shutdown(pdev, &wake); > > > > > - if (retval) > > > > > - return retval; > > > > > - > > > > > - if (wake) { > > > > > - pci_prepare_to_sleep(pdev); > > > > > - } else { > > > > > - pci_wake_from_d3(pdev, false); > > > > > > > > I think there's a case where this changes the behavior because we normally set > > > > the device wakeup enable to adapter->wol, but the "wake" > > > > returned from __e1000_shutdown() is sometimes different. > > > > > > > > e1000_probe > > > > adapter->wol = adapter->eeprom_wol; # assume adapter->wol == 1 > > > > device_set_wakeup_enable(adapter->wol); > > > > > > > > Existing code: > > > > e1000_suspend > > > > __e1000_shutdown(&wake) # assume returns wake == 0 > > > > pci_wake_from_d3(false) > > > > pci_enable_wake(PCI_D3hot, false) # <-- compare > > > > > > > > New code using generic PM ops: > > > > pci_pm_suspend > > > > e1000_suspend > > > > __e1000_shutdown(&wake) # returns wake == 0 (ignored) > > > > pci_pm_suspend_noirq > > > > pci_prepare_to_sleep > > > > wakeup = device_may_wakeup() # returns 1 > > > > pci_enable_wake(PCI_D3hot, true) # <-- different! > > > > > > > > I sort of suspect that __e1000_shutdown() should call > > > > device_set_wakeup_enable() when it updates the chip's wake-on-lan > > > > registers, but the driver maintainers would know better. > > > > If you have any ideas about this, or maybe patterns in newer Intel > > drivers that could be followed here, we'd love to hear them. > > > > If you think it's impossible to update e1000 to generic power > > management, it'd be useful to know that, too, so we can move on to > > other drivers. > > Is it safe to assume that we should leave e1000 for now? As > the thread seems to be discontinued. My suggestion: update the patch to reflect my feedback above, and post what we believe to be the correct thing to the appropriate upstream lists: Jeff Kirsher <jeffrey.t.kirsher@intel.com> Jesse Brandeburg <jesse.brandeburg@intel.com> David S. Miller <davem@davemloft.net> Rafael J. Wysocki <rjw@rjwysocki.net> intel-wired-lan@lists.osuosl.org netdev@vger.kernel.org linux-pm@vger.kernel.org linux-pci@vger.kernel.org The maintainers may want to defer it for now because it's old hardware that's hard to test, and that's OK. But at least we'll have the patch available, and if/when we get to the point of removing legacy PM from the PCI core, we can revisit this. What we learn about wake-on-LAN here will probably apply to other drivers as well. > > > > > - pci_set_power_state(pdev, PCI_D3hot); > > > > > - } > > > > > - > > > > > - return 0; > > > > > + return __e1000_shutdown(pdev, &wake); > > > > > } > > > > > > > > > > -static int e1000_resume(struct pci_dev *pdev) > > > > > +static int e1000_resume(struct device *dev) > > > > > { > > > > > + struct pci_dev *pdev = to_pci_dev(dev); > > > > > struct net_device *netdev = pci_get_drvdata(pdev); > > > > > struct e1000_adapter *adapter = netdev_priv(netdev); > > > > > struct e1000_hw *hw = &adapter->hw; > > > > > u32 err; > > > > > > > > > > - pci_set_power_state(pdev, PCI_D0); > > > > > - pci_restore_state(pdev); > > > > > - pci_save_state(pdev); > > > > > - > > > > > if (adapter->need_ioport) > > > > > err = pci_enable_device(pdev); > > > > > else > > > > > -- > > > > > 2.26.0 > > > > > > > _______________________________________________ > > Linux-kernel-mentees mailing list > > Linux-kernel-mentees@lists.linuxfoundation.org > > https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v1] ethernet: intel: e1000: Convert to dev_pm_ops 2020-05-18 15:27 ` Bjorn Helgaas @ 2020-05-18 15:30 ` Vaibhav Gupta 0 siblings, 0 replies; 11+ messages in thread From: Vaibhav Gupta @ 2020-05-18 15:30 UTC (permalink / raw) To: Bjorn Helgaas Cc: Brandeburg, Jesse, rjw, Kirsher, Jeffrey T, linux-kernel-mentees, Vaibhav Gupta On Mon, 18 May 2020 at 20:57, Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Mon, May 18, 2020 at 08:37:13PM +0530, Vaibhav Gupta wrote: > > On Sat, 2 May 2020 at 03:15, Bjorn Helgaas <helgaas@kernel.org> wrote: > > > On Fri, May 01, 2020 at 09:19:00PM +0000, Kirsher, Jeffrey T wrote: > > > > > -----Original Message----- > > > > > From: Bjorn Helgaas <helgaas@kernel.org> > > > > > Sent: Friday, May 1, 2020 13:58 > > > > > To: Vaibhav Gupta <vaibhavgupta40@gmail.com> > > > > > Cc: linux-kernel-mentees@lists.linuxfoundation.org; bjorn@helgaas.com; > > > > > skhan@linuxfoundation.org; rjw@rjwysocki.net; Kirsher, Jeffrey T > > > > > <jeffrey.t.kirsher@intel.com>; Brandeburg, Jesse > > > > > <jesse.brandeburg@intel.com> > > > > > Subject: Re: [Linux-kernel-mentees] [PATCH v1] ethernet: intel: e1000: Convert > > > > > to dev_pm_ops > > > > > > > > > > [+cc Jeff, Jesse] > > > > > > > > > > On Fri, Apr 10, 2020 at 06:14:19PM +0530, Vaibhav Gupta wrote: > > > > > > Convert the legacy callback .suspend() and .resume() to the generic > > > > > > ones. > > > > > > > > > > > > Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com> > > > > > > --- > > > > > > drivers/net/ethernet/intel/e1000/e1000_main.c | 47 > > > > > > +++++-------------- > > > > > > 1 file changed, 12 insertions(+), 35 deletions(-) > > > > [Kirsher, Jeffrey T] > > > > > > > > Was there a reason why this patch was not at least CC'd to > > > > intel-wired-lan@lists.osuosl.org mailing list for all Intel wired > > > > LAN driver changes? Thank you Bjorn for at least adding Jesse and > > > > myself to the thread. > > > > > > Don't worry, this is potentially part of a mentoring project, and > > > we're just trying to shake out trivial issues first before throwing > > > Vaibhav straight into the lions' den, so to speak. > > > > > > > On top of the potential issues Bjorn pointed out, this could cause > > > > regression issues that can not fully be validated since this driver > > > > is really old and all supported devices may no longer be available > > > > to test against. > > > > > > Understood. The problem we're trying to solve is that the PCI > > > core carries quite a bit of code to support both the legacy power > > > management and the "new" (now 10 year-old) generic scheme. That > > > extra code is its own source of bugs since it's hard to keep both > > > paths up to date and tested. > > > > > > > > > diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c > > > > > > b/drivers/net/ethernet/intel/e1000/e1000_main.c > > > > > > index 2bced34c19ba..09a6ef46be96 100644 > > > > > > --- a/drivers/net/ethernet/intel/e1000/e1000_main.c > > > > > > +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c > > > > > > @@ -152,8 +152,8 @@ static int e1000_vlan_rx_kill_vid(struct > > > > > > net_device *netdev, static void e1000_restore_vlan(struct > > > > > > e1000_adapter *adapter); > > > > > > > > > > > > #ifdef CONFIG_PM > > > > > > -static int e1000_suspend(struct pci_dev *pdev, pm_message_t state); > > > > > > -static int e1000_resume(struct pci_dev *pdev); > > > > > > +static int e1000_suspend(struct device *dev); static int > > > > > > +e1000_resume(struct device *dev); > > > > > > #endif > > > > > > static void e1000_shutdown(struct pci_dev *pdev); > > > > > > > > > > > > @@ -179,16 +179,16 @@ static const struct pci_error_handlers > > > > > e1000_err_handler = { > > > > > > .resume = e1000_io_resume, > > > > > > }; > > > > > > > > > > > > +static SIMPLE_DEV_PM_OPS(e1000_pm_ops, e1000_suspend, > > > > > e1000_resume); > > > > > > + > > > > > > static struct pci_driver e1000_driver = { > > > > > > .name = e1000_driver_name, > > > > > > .id_table = e1000_pci_tbl, > > > > > > .probe = e1000_probe, > > > > > > .remove = e1000_remove, > > > > > > -#ifdef CONFIG_PM > > > > > > - /* Power Management Hooks */ > > > > > > - .suspend = e1000_suspend, > > > > > > - .resume = e1000_resume, > > > > > > -#endif > > > > > > + .driver = { > > > > > > + .pm = &e1000_pm_ops, > > > > > > + }, > > > > > > .shutdown = e1000_shutdown, > > > > > > .err_handler = &e1000_err_handler > > > > > > }; > > > > > > @@ -5052,9 +5052,6 @@ static int __e1000_shutdown(struct pci_dev *pdev, > > > > > bool *enable_wake) > > > > > > struct e1000_hw *hw = &adapter->hw; > > > > > > u32 ctrl, ctrl_ext, rctl, status; > > > > > > u32 wufc = adapter->wol; > > > > > > -#ifdef CONFIG_PM > > > > > > - int retval = 0; > > > > > > -#endif > > > > > > > > > > > > netif_device_detach(netdev); > > > > > > > > > > > > @@ -5068,12 +5065,6 @@ static int __e1000_shutdown(struct pci_dev > > > > > *pdev, bool *enable_wake) > > > > > > e1000_down(adapter); > > > > > > } > > > > > > > > > > > > -#ifdef CONFIG_PM > > > > > > - retval = pci_save_state(pdev); > > > > > > - if (retval) > > > > > > - return retval; > > > > > > -#endif > > > > > > - > > > > > > status = er32(STATUS); > > > > > > if (status & E1000_STATUS_LU) > > > > > > wufc &= ~E1000_WUFC_LNKC; > > > > > > @@ -5135,36 +5126,22 @@ static int __e1000_shutdown(struct pci_dev > > > > > > *pdev, bool *enable_wake) } > > > > > > > > > > > > #ifdef CONFIG_PM > > > > > > -static int e1000_suspend(struct pci_dev *pdev, pm_message_t state) > > > > > > +static int e1000_suspend(struct device *dev) > > > > > > { > > > > > > - int retval; > > > > > > + struct pci_dev *pdev = to_pci_dev(dev); > > > > > > bool wake; > > > > > > > > > > > > - retval = __e1000_shutdown(pdev, &wake); > > > > > > - if (retval) > > > > > > - return retval; > > > > > > - > > > > > > - if (wake) { > > > > > > - pci_prepare_to_sleep(pdev); > > > > > > - } else { > > > > > > - pci_wake_from_d3(pdev, false); > > > > > > > > > > I think there's a case where this changes the behavior because we normally set > > > > > the device wakeup enable to adapter->wol, but the "wake" > > > > > returned from __e1000_shutdown() is sometimes different. > > > > > > > > > > e1000_probe > > > > > adapter->wol = adapter->eeprom_wol; # assume adapter->wol == 1 > > > > > device_set_wakeup_enable(adapter->wol); > > > > > > > > > > Existing code: > > > > > e1000_suspend > > > > > __e1000_shutdown(&wake) # assume returns wake == 0 > > > > > pci_wake_from_d3(false) > > > > > pci_enable_wake(PCI_D3hot, false) # <-- compare > > > > > > > > > > New code using generic PM ops: > > > > > pci_pm_suspend > > > > > e1000_suspend > > > > > __e1000_shutdown(&wake) # returns wake == 0 (ignored) > > > > > pci_pm_suspend_noirq > > > > > pci_prepare_to_sleep > > > > > wakeup = device_may_wakeup() # returns 1 > > > > > pci_enable_wake(PCI_D3hot, true) # <-- different! > > > > > > > > > > I sort of suspect that __e1000_shutdown() should call > > > > > device_set_wakeup_enable() when it updates the chip's wake-on-lan > > > > > registers, but the driver maintainers would know better. > > > > > > If you have any ideas about this, or maybe patterns in newer Intel > > > drivers that could be followed here, we'd love to hear them. > > > > > > If you think it's impossible to update e1000 to generic power > > > management, it'd be useful to know that, too, so we can move on to > > > other drivers. > > > > Is it safe to assume that we should leave e1000 for now? As > > the thread seems to be discontinued. > > My suggestion: update the patch to reflect my feedback above, and post > what we believe to be the correct thing to the appropriate upstream > lists: > > Jeff Kirsher <jeffrey.t.kirsher@intel.com> > Jesse Brandeburg <jesse.brandeburg@intel.com> > David S. Miller <davem@davemloft.net> > Rafael J. Wysocki <rjw@rjwysocki.net> > intel-wired-lan@lists.osuosl.org > netdev@vger.kernel.org > linux-pm@vger.kernel.org > linux-pci@vger.kernel.org > > The maintainers may want to defer it for now because it's old hardware > that's hard to test, and that's OK. But at least we'll have the patch > available, and if/when we get to the point of removing legacy PM from > the PCI core, we can revisit this. > > What we learn about wake-on-LAN here will probably apply to other > drivers as well. Sure! Thanks ! --Vaibhav Gupta > > > > > > > - pci_set_power_state(pdev, PCI_D3hot); > > > > > > - } > > > > > > - > > > > > > - return 0; > > > > > > + return __e1000_shutdown(pdev, &wake); > > > > > > } > > > > > > > > > > > > -static int e1000_resume(struct pci_dev *pdev) > > > > > > +static int e1000_resume(struct device *dev) > > > > > > { > > > > > > + struct pci_dev *pdev = to_pci_dev(dev); > > > > > > struct net_device *netdev = pci_get_drvdata(pdev); > > > > > > struct e1000_adapter *adapter = netdev_priv(netdev); > > > > > > struct e1000_hw *hw = &adapter->hw; > > > > > > u32 err; > > > > > > > > > > > > - pci_set_power_state(pdev, PCI_D0); > > > > > > - pci_restore_state(pdev); > > > > > > - pci_save_state(pdev); > > > > > > - > > > > > > if (adapter->need_ioport) > > > > > > err = pci_enable_device(pdev); > > > > > > else > > > > > > -- > > > > > > 2.26.0 > > > > > > > > > _______________________________________________ > > > Linux-kernel-mentees mailing list > > > Linux-kernel-mentees@lists.linuxfoundation.org > > > https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-05-18 15:31 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-10 12:44 [Linux-kernel-mentees] [PATCH v1] ethernet: intel: e1000: Convert to dev_pm_ops Vaibhav Gupta 2020-04-11 0:15 ` Bjorn Helgaas 2020-04-11 14:03 ` Vaibhav Gupta 2020-04-11 0:23 ` Bjorn Helgaas 2020-04-23 0:24 ` Bjorn Helgaas 2020-05-01 20:58 ` Bjorn Helgaas 2020-05-01 21:19 ` Kirsher, Jeffrey T 2020-05-01 21:45 ` Bjorn Helgaas 2020-05-18 15:07 ` Vaibhav Gupta 2020-05-18 15:27 ` Bjorn Helgaas 2020-05-18 15:30 ` Vaibhav Gupta
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).