netdev - Re: [net-next 11/12] e1000e: Fix EEE in S5 w/ Runtime PM enabled

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140726151005.GA20445@electric-eye.fr.zoreil.com>
Date:	Sat, 26 Jul 2014 17:10:05 +0200
From:	Francois Romieu <romieu@...zoreil.com>
To:	Jeff Kirsher <jeffrey.t.kirsher@...el.com>
Cc:	davem@...emloft.net, David Ertman <david.m.ertman@...el.com>,
	netdev@...r.kernel.org, nhorman@...hat.com, sassmann@...hat.com,
	"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>
Subject: Re: [net-next 11/12] e1000e: Fix EEE in S5 w/ Runtime PM enabled

(Rafael Cced:)

Jeff Kirsher <jeffrey.t.kirsher@...el.com> :
> From: David Ertman <david.m.ertman@...el.com>
> 
> The process of shutting down the system causes a call to the close PM
> callback.  The reset in close causes a loss of link, and the resultant
> LSC interrupt causes the Runtime PM idle callback to be called.  The
> check for link (while link is down) in the idle callback is wiping the
> information about the EEE ability of the link partner. The information
> is still gone when the PHY is powered back up in the shutdown flow. This
> causes EEE in S5 to fail when Runtime PM is active.
> 
> Save the link partner's EEE ability in the idle callback so that a Runtime
> PM event will not cause a loss of this information.

Why does e1000_check_for_copper_link_ich8lan unconditionally wipe the
information about the EEE ability of the link partner instead of performing
something similar to the eee_lp_ability detection part e1000_set_eee_pchlan ?

[...]
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index fe3e42a..1ce0d74 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -6357,9 +6357,14 @@ static int e1000e_pm_runtime_idle(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);
> +	u16 eee_lp;
>  
> -	if (!e1000e_has_link(adapter))
> +	eee_lp = adapter->hw.dev_spec.ich8lan.eee_lp_ability;
> +
> +	if (!e1000e_has_link(adapter)) {
> +		adapter->hw.dev_spec.ich8lan.eee_lp_ability = eee_lp;
>  		pm_schedule_suspend(dev, 5 * MSEC_PER_SEC);
> +	}
>  
>  	return -EBUSY;

/me wonders...

commit 63eb48f151b5f1d8dba35d6176d0d7c9643b33af
Author: David Ertman <davidx.m.ertman@...el.com>
Date:   Fri Feb 14 07:16:46 2014 +0000

    e1000e Refactor of Runtime Power Management
    
    Fix issues with:
    RuntimePM causing the device to repeatedly flip between suspend and resume
    with the interface administratively downed.
    Having RuntimePM enabled interfering with the functionality of Energy
    Efficient Ethernet.
    
    Added checks to disallow functions that should not be executed if the
    device is currently runtime suspended
    
    Make runtime_idle callback to use same deterministic behavior as the igb
    driver.

pm_schedule_suspend is already called from the link change thread
e1000_watchdog_task. You may consider removing everything from runtime_idle.

-- 
Ueimor
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html