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

[Bjorn Helgaas <helgaas@kernel.org>]

Rafael, do you have any thoughts on this?

On Fri, Apr 10, 2020 at 07:23:12PM -0500, Bjorn Helgaas wrote:
> 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.

I think it's OK to remove the pci_save_state() because I don't think
we ever needed it in the .shutdown() path, and we shouldn't need it in
the new generic .suspend() path.

But the commit log should probably mention the fact that it's being
removed from .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.

This part is a tangent.  Most NIC drivers don't have this code, so I
suspect it's not quite right, but you're not changing this part of
.shutdown(), so we can just let sleeping dogs lie for now.

Bjorn
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees