[Linux-kernel-mentees] [PATCH v1] ethernet: intel: e1000: Convert to dev_pm_ops
Bjorn Helgaas
helgaas at kernel.org
Sat Apr 11 00:23:10 UTC 2020
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
More information about the Linux-kernel-mentees
mailing list