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

[Bjorn Helgaas <helgaas@kernel.org>]

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