Linux Kernel Mentees Archive on lore.kernel.org
 help / color / Atom feed
From: Vaibhav Gupta <vaibhav.varodek@gmail.com>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: "Brandeburg, Jesse" <jesse.brandeburg@intel.com>,
	"rjw@rjwysocki.net" <rjw@rjwysocki.net>,
	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@intel.com>,
	"linux-kernel-mentees@lists.linuxfoundation.org"
	<linux-kernel-mentees@lists.linuxfoundation.org>,
	Vaibhav Gupta <vaibhavgupta40@gmail.com>
Subject: Re: [Linux-kernel-mentees] [PATCH v1] ethernet: intel: e1000: Convert to dev_pm_ops
Date: Mon, 18 May 2020 21:00:14 +0530
Message-ID: <CAPBsFfAoHqvwR30fubkjfUkORGxOaotGJjmjy_HwfzzRj3b1PQ@mail.gmail.com> (raw)
In-Reply-To: <20200518152701.GA930338@bjorn-Precision-5520>

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

      reply index

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-10 12:44 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 message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAPBsFfAoHqvwR30fubkjfUkORGxOaotGJjmjy_HwfzzRj3b1PQ@mail.gmail.com \
    --to=vaibhav.varodek@gmail.com \
    --cc=helgaas@kernel.org \
    --cc=jeffrey.t.kirsher@intel.com \
    --cc=jesse.brandeburg@intel.com \
    --cc=linux-kernel-mentees@lists.linuxfoundation.org \
    --cc=rjw@rjwysocki.net \
    --cc=vaibhavgupta40@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux Kernel Mentees Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-kernel-mentees/0 linux-kernel-mentees/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-kernel-mentees linux-kernel-mentees/ https://lore.kernel.org/linux-kernel-mentees \
		linux-kernel-mentees@lists.linuxfoundation.org
	public-inbox-index linux-kernel-mentees

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.linuxfoundation.lists.linux-kernel-mentees


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git