Re: [Linux-kernel-mentees] [PATCH v1] ethernet: intel: e1000: Convert to dev_pm_ops

["Kirsher, Jeffrey T" <jeffrey.t.kirsher@intel.com>]

> -----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