[Linux-kernel-mentees] [PATCH v1] ethernet: intel: e1000: Convert to dev_pm_ops
Vaibhav Gupta
vaibhav.varodek at gmail.com
Mon May 18 15:30:14 UTC 2020
On Mon, 18 May 2020 at 20:57, Bjorn Helgaas <helgaas at 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 at kernel.org> wrote:
> > > On Fri, May 01, 2020 at 09:19:00PM +0000, Kirsher, Jeffrey T wrote:
> > > > > -----Original Message-----
> > > > > From: Bjorn Helgaas <helgaas at kernel.org>
> > > > > Sent: Friday, May 1, 2020 13:58
> > > > > To: Vaibhav Gupta <vaibhavgupta40 at gmail.com>
> > > > > Cc: linux-kernel-mentees at lists.linuxfoundation.org; bjorn at helgaas.com;
> > > > > skhan at linuxfoundation.org; rjw at rjwysocki.net; Kirsher, Jeffrey T
> > > > > <jeffrey.t.kirsher at intel.com>; Brandeburg, Jesse
> > > > > <jesse.brandeburg at 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 at 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 at 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 at intel.com>
> Jesse Brandeburg <jesse.brandeburg at intel.com>
> David S. Miller <davem at davemloft.net>
> Rafael J. Wysocki <rjw at rjwysocki.net>
> intel-wired-lan at lists.osuosl.org
> netdev at vger.kernel.org
> linux-pm at vger.kernel.org
> linux-pci at 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 at lists.linuxfoundation.org
> > > https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
More information about the Linux-kernel-mentees
mailing list