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

[Bjorn Helgaas <helgaas@kernel.org>]

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