[PATCH RESEND 0/3] e1000e: return runtime-pm back to work

[Konstantin Khlebnikov]

here some fixes for e1000e driver from:
https://lkml.org/lkml/2013/2/4/185
The rest patches from that patchset already committed.

---

Konstantin Khlebnikov (3):
e1000e: fix pci-device enable-counter balance
e1000e: fix runtime power management transitions
e1000e: fix accessing to suspended device


drivers/net/ethernet/intel/e1000e/ethtool.c | 13 ++++
drivers/net/ethernet/intel/e1000e/netdev.c | 82 +++++++--------------------
2 files changed, 34 insertions(+), 61 deletions(-)

--
Signature
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Konstantin Khlebnikov]

This patch fixes some annoying messages like 'Error reading PHY register' and
'Hardware Erorr' and saves several seconds on reboot.

Signed-off-by: Konstantin Khlebnikov <khleb...@openvz.org>
Acked-by: Rafael J. Wysocki <rafael.j...@intel.com>
Cc: e1000...@lists.sourceforge.net
Cc: Jeff Kirsher <jeffrey....@intel.com>
Cc: Bruce Allan <bruce....@intel.com>
---
drivers/net/ethernet/intel/e1000e/ethtool.c | 13 +++++++++++++
drivers/net/ethernet/intel/e1000e/netdev.c | 2 ++
2 files changed, 15 insertions(+)

diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c b/drivers/net/ethernet/intel/e1000e/ethtool.c
index 2c18137..f91a8f3 100644
--- a/drivers/net/ethernet/intel/e1000e/ethtool.c
+++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
@@ -36,6 +36,7 @@
#include <linux/delay.h>
#include <linux/vmalloc.h>
#include <linux/mdio.h>
+#include <linux/pm_runtime.h>

#include "e1000.h"

@@ -2229,7 +2230,19 @@ static int e1000e_get_ts_info(struct net_device *netdev,
return 0;
}

+static int e1000e_ethtool_begin(struct net_device *netdev)
+{
+ return pm_runtime_get_sync(netdev->dev.parent);
+}
+
+static void e1000e_ethtool_complete(struct net_device *netdev)
+{
+ pm_runtime_put_sync(netdev->dev.parent);
+}
+
static const struct ethtool_ops e1000_ethtool_ops = {
+ .begin = e1000e_ethtool_begin,
+ .complete = e1000e_ethtool_complete,
.get_settings = e1000_get_settings,
.set_settings = e1000_set_settings,
.get_drvinfo = e1000_get_drvinfo,
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 2954cc7..948b86ff 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -4663,6 +4663,7 @@ static void e1000_phy_read_status(struct e1000_adapter *adapter)
(adapter->hw.phy.media_type == e1000_media_type_copper)) {
int ret_val;

+ pm_runtime_get_sync(&adapter->pdev->dev);
ret_val = e1e_rphy(hw, MII_BMCR, &phy->bmcr);
ret_val |= e1e_rphy(hw, MII_BMSR, &phy->bmsr);
ret_val |= e1e_rphy(hw, MII_ADVERTISE, &phy->advertise);
@@ -4673,6 +4674,7 @@ static void e1000_phy_read_status(struct e1000_adapter *adapter)
ret_val |= e1e_rphy(hw, MII_ESTATUS, &phy->estatus);
if (ret_val)
e_warn("Error reading PHY register\n");
+ pm_runtime_put_sync(&adapter->pdev->dev);
} else {
/* Do not read PHY registers if link is not up
* Set values to typical power-on defaults

[Konstantin Khlebnikov]

This patch removes redundant and unbalanced pci_disable_device() from
__e1000_shutdown(). pci_clear_master() is enough, device can go into
suspended state with elevated enable_cnt.

Bug was introduced in commit 23606cf5d1192c2b17912cb2ef6e62f9b11de133
("e1000e / PCI / PM: Add basic runtime PM support (rev. 4)") in v2.6.35

Signed-off-by: Konstantin Khlebnikov <khleb...@openvz.org>
Acked-by: Rafael J. Wysocki <rafael.j...@intel.com>
Cc: e1000...@lists.sourceforge.net
Cc: Jeff Kirsher <jeffrey....@intel.com>
Cc: Bruce Allan <bruce....@intel.com>
---

drivers/net/ethernet/intel/e1000e/netdev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index a177b8b..1799021 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -5986,7 +5986,7 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool *enable_wake,
*/
e1000e_release_hw_control(adapter);

- pci_disable_device(pdev);
+ pci_clear_master(pdev);

return 0;

[Konstantin Khlebnikov]

This patch removes redundant actions from driver and fixes its interaction
with actions in pci-bus runtime power management code.

It removes pci_save_state() from __e1000_shutdown() for normal adapters,
PCI bus callbacks pci_pm_*() will do all this for us. Now __e1000_shutdown()
switches to D3-state only quad-port adapters, because they needs quirk for
clearing false-positive error from downsteam pci-e port.

pci_save_state() now called after clearing bus-master bit, thus __e1000_resume()
and e1000_io_slot_reset() must set it back after restoring configuration space.

This patch set get_link_status before calling pm_runtime_put() in e1000_open()
to allow e1000_idle() get real link status and schedule first runtime suspend.

This patch also enables wakeup for device if management mode is enabled
(like for WoL) as result pci_prepare_to_sleep() would setup wakeup without
special actions like custom 'enable_wakeup' sign.

Signed-off-by: Konstantin Khlebnikov <khleb...@openvz.org>
Acked-by: Rafael J. Wysocki <rafael.j...@intel.com>
Cc: e1000...@lists.sourceforge.net
Cc: Jeff Kirsher <jeffrey....@intel.com>
Cc: Bruce Allan <bruce....@intel.com>
---

drivers/net/ethernet/intel/e1000e/netdev.c | 78 ++++++----------------------
1 file changed, 18 insertions(+), 60 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 1799021..2954cc7 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -4303,6 +4303,7 @@ static int e1000_open(struct net_device *netdev)
netif_start_queue(netdev);

adapter->idle_check = true;
+ hw->mac.get_link_status = true;
pm_runtime_put(&pdev->dev);

/* fire a link status change interrupt to start the watchdog */
@@ -5887,8 +5888,7 @@ release:
return retval;
}

-static int __e1000_shutdown(struct pci_dev *pdev, bool *enable_wake,
- bool runtime)
+static int __e1000_shutdown(struct pci_dev *pdev, bool runtime)
{
struct net_device *netdev = pci_get_drvdata(pdev);
struct e1000_adapter *adapter = netdev_priv(netdev);
@@ -5912,10 +5912,6 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool *enable_wake,
}
e1000e_reset_interrupt_capability(adapter);

- retval = pci_save_state(pdev);
- if (retval)
- return retval;
-
status = er32(STATUS);
if (status & E1000_STATUS_LU)
wufc &= ~E1000_WUFC_LNKC;
@@ -5971,13 +5967,6 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool *enable_wake,
ew32(WUFC, 0);
}

- *enable_wake = !!wufc;
-
- /* make sure adapter isn't asleep if manageability is enabled */
- if ((adapter->flags & FLAG_MNG_PT_ENABLED) ||
- (hw->mac.ops.check_mng_mode(hw)))
- *enable_wake = true;
-
if (adapter->hw.phy.type == e1000_phy_igp_3)
e1000e_igp3_phy_powerdown_workaround_ich8lan(&adapter->hw);

@@ -5988,26 +5977,6 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool *enable_wake,

pci_clear_master(pdev);

- return 0;
-}
-
-static void e1000_power_off(struct pci_dev *pdev, bool sleep, bool wake)
-{
- if (sleep && wake) {
- pci_prepare_to_sleep(pdev);
- return;
- }
-
- pci_wake_from_d3(pdev, wake);
- pci_set_power_state(pdev, PCI_D3hot);
-}
-
-static void e1000_complete_shutdown(struct pci_dev *pdev, bool sleep,
- bool wake)
-{
- struct net_device *netdev = pci_get_drvdata(pdev);
- struct e1000_adapter *adapter = netdev_priv(netdev);
-
/* The pci-e switch on some quad port adapters will report a
* correctable error when the MAC transitions from D0 to D3. To
* prevent this we need to mask off the correctable errors on the
@@ -6021,12 +5990,13 @@ static void e1000_complete_shutdown(struct pci_dev *pdev, bool sleep,
pcie_capability_write_word(us_dev, PCI_EXP_DEVCTL,
(devctl & ~PCI_EXP_DEVCTL_CERE));

- e1000_power_off(pdev, sleep, wake);
+ pci_save_state(pdev);
+ pci_prepare_to_sleep(pdev);

pcie_capability_write_word(us_dev, PCI_EXP_DEVCTL, devctl);
- } else {
- e1000_power_off(pdev, sleep, wake);
}
+
+ return 0;
}

#ifdef CONFIG_PCIEASPM
@@ -6084,9 +6054,7 @@ static int __e1000_resume(struct pci_dev *pdev)
if (aspm_disable_flag)
e1000e_disable_aspm(pdev, aspm_disable_flag);

- pci_set_power_state(pdev, PCI_D0);
- pci_restore_state(pdev);
- pci_save_state(pdev);
+ pci_set_master(pdev);

e1000e_set_interrupt_capability(adapter);
if (netif_running(netdev)) {
@@ -6152,14 +6120,8 @@ static int __e1000_resume(struct pci_dev *pdev)
static int e1000_suspend(struct device *dev)
{
struct pci_dev *pdev = to_pci_dev(dev);
- int retval;
- bool wake;
-
- retval = __e1000_shutdown(pdev, &wake, false);
- if (!retval)
- e1000_complete_shutdown(pdev, true, wake);

- return retval;
+ return __e1000_shutdown(pdev, false);
}

static int e1000_resume(struct device *dev)
@@ -6182,13 +6144,10 @@ static int e1000_runtime_suspend(struct device *dev)
struct net_device *netdev = pci_get_drvdata(pdev);
struct e1000_adapter *adapter = netdev_priv(netdev);

- if (e1000e_pm_ready(adapter)) {
- bool wake;
-
- __e1000_shutdown(pdev, &wake, true);
- }
+ if (!e1000e_pm_ready(adapter))
+ return 0;

- return 0;
+ return __e1000_shutdown(pdev, true);
}

static int e1000_idle(struct device *dev)
@@ -6226,12 +6185,7 @@ static int e1000_runtime_resume(struct device *dev)

static void e1000_shutdown(struct pci_dev *pdev)
{
- bool wake = false;
-
- __e1000_shutdown(pdev, &wake, false);
-
- if (system_state == SYSTEM_POWER_OFF)
- e1000_complete_shutdown(pdev, false, wake);
+ __e1000_shutdown(pdev, false);
}

#ifdef CONFIG_NET_POLL_CONTROLLER
@@ -6352,9 +6306,9 @@ static pci_ers_result_t e1000_io_slot_reset(struct pci_dev *pdev)
"Cannot re-enable PCI device after reset.\n");
result = PCI_ERS_RESULT_DISCONNECT;
} else {
- pci_set_master(pdev);
pdev->state_saved = true;
pci_restore_state(pdev);
+ pci_set_master(pdev);

pci_enable_wake(pdev, PCI_D3hot, 0);
pci_enable_wake(pdev, PCI_D3cold, 0);
@@ -6783,7 +6737,11 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)

/* initialize the wol settings based on the eeprom settings */
adapter->wol = adapter->eeprom_wol;
- device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol);
+
+ /* make sure adapter isn't asleep if manageability is enabled */
+ if (adapter->wol || (adapter->flags & FLAG_MNG_PT_ENABLED) ||
+ (hw->mac.ops.check_mng_mode(hw)))
+ device_wakeup_enable(&pdev->dev);

/* save off EEPROM version number */
e1000_read_nvm(&adapter->hw, 5, 1, &adapter->eeprom_vers);

[Waskiewicz Jr, Peter P]

On 2/24/2013 9:19 PM, Konstantin Khlebnikov wrote:
> This patch fixes some annoying messages like 'Error reading PHY register' and
> 'Hardware Erorr' and saves several seconds on reboot.

Any networking-related patches should also include net...@vger.kernel.org.

I'm also a bit confused how the changes below match the patch
description. Elaborating a bit more how the changes suppress the
messages might be a good thing.

What do the ethtool additions have to do with this patch? The patch
description really doesn't seem to cover why these are here.

> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index 2954cc7..948b86ff 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -4663,6 +4663,7 @@ static void e1000_phy_read_status(struct e1000_adapter *adapter)
> (adapter->hw.phy.media_type == e1000_media_type_copper)) {
> int ret_val;
>
> + pm_runtime_get_sync(&adapter->pdev->dev);
> ret_val = e1e_rphy(hw, MII_BMCR, &phy->bmcr);
> ret_val |= e1e_rphy(hw, MII_BMSR, &phy->bmsr);
> ret_val |= e1e_rphy(hw, MII_ADVERTISE, &phy->advertise);
> @@ -4673,6 +4674,7 @@ static void e1000_phy_read_status(struct e1000_adapter *adapter)
> ret_val |= e1e_rphy(hw, MII_ESTATUS, &phy->estatus);
> if (ret_val)
> e_warn("Error reading PHY register\n");
> + pm_runtime_put_sync(&adapter->pdev->dev);
> } else {
> /* Do not read PHY registers if link is not up
> * Set values to typical power-on defaults
>
>

> ------------------------------------------------------------------------------
> Everyone hates slow websites. So do we.
> Make your web apps faster with AppDynamics
> Download AppDynamics Lite for free today:
> http://p.sf.net/sfu/appdyn_d2d_feb
> _______________________________________________
> E1000-devel mailing list
> E1000...@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/e1000-devel
> To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

[Konstantin Khlebnikov]

Waskiewicz Jr, Peter P wrote:
> On 2/24/2013 9:19 PM, Konstantin Khlebnikov wrote:
>> This patch fixes some annoying messages like 'Error reading PHY register' and
>> 'Hardware Erorr' and saves several seconds on reboot.
>
> Any networking-related patches should also include net...@vger.kernel.org.

Yeah, I forgot about this, since I came here from PCI-bus side, not from the network =)

>
> I'm also a bit confused how the changes below match the patch description.
> Elaborating a bit more how the changes suppress the messages might be a good thing.

Patch eliminates reason of these errors -- now driver will wake up
the device before accessing to its registers.

[Ben Hutchings]

On Tue, 2013-02-26 at 14:03 +0400, Konstantin Khlebnikov wrote:
> Waskiewicz Jr, Peter P wrote:
> > On 2/24/2013 9:19 PM, Konstantin Khlebnikov wrote:
> >> This patch fixes some annoying messages like 'Error reading PHY register' and
> >> 'Hardware Erorr' and saves several seconds on reboot.
> >
> > Any networking-related patches should also include net...@vger.kernel.org.
>
> Yeah, I forgot about this, since I came here from PCI-bus side, not from the network =)
>
> >
> > I'm also a bit confused how the changes below match the patch description.
> > Elaborating a bit more how the changes suppress the messages might be a good thing.
>
> Patch eliminates reason of these errors -- now driver will wake up
> the device before accessing to its registers.

[...]

But e1000e calls netif_device_detach() when entering run-time suspend.
That currently prevents any ethtool operations from running until it
resumes.

(This is definitely a misfeature of either the ethtool core or e1000e.
It is absolutely ridiculous that I can't check the PHY settings of an
e1000e - or even driver information! - when the link is down.)

Ben.

--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[Konstantin Khlebnikov]

Ben Hutchings wrote:
> On Tue, 2013-02-26 at 14:03 +0400, Konstantin Khlebnikov wrote:
>> Waskiewicz Jr, Peter P wrote:
>>> On 2/24/2013 9:19 PM, Konstantin Khlebnikov wrote:
>>>> This patch fixes some annoying messages like 'Error reading PHY register' and
>>>> 'Hardware Erorr' and saves several seconds on reboot.
>>>
>>> Any networking-related patches should also include net...@vger.kernel.org.
>>
>> Yeah, I forgot about this, since I came here from PCI-bus side, not from the network =)
>>
>>>
>>> I'm also a bit confused how the changes below match the patch description.
>> > Elaborating a bit more how the changes suppress the messages might be a good thing.
>>
>> Patch eliminates reason of these errors -- now driver will wake up
>> the device before accessing to its registers.
> [...]
>
> But e1000e calls netif_device_detach() when entering run-time suspend.
> That currently prevents any ethtool operations from running until it
> resumes.

This seems racy. dev_ethtool() runs under rtnl-lock, but suspend-resume
run asynchronously. Runtime-suspend shoudn't call netif_device_detach(),
logically device isn't dead it just take a nap for a while..

>
> (This is definitely a misfeature of either the ethtool core or e1000e.
> It is absolutely ridiculous that I can't check the PHY settings of an
> e1000e - or even driver information! - when the link is down.)

Ok link is down, but device still alive and I not see anything ridiculous
in checking its registers.

[Jeff Kirsher]

I have added this patch to my e1000e patch queue.

[Jeff Kirsher]

On Mon, 2013-02-25 at 09:19 +0400, Konstantin Khlebnikov wrote:

> This patch fixes some annoying messages like 'Error reading PHY
> register' and
> 'Hardware Erorr' and saves several seconds on reboot.
>
> Signed-off-by: Konstantin Khlebnikov <khleb...@openvz.org>
> Acked-by: Rafael J. Wysocki <rafael.j...@intel.com>
> Cc: e1000...@lists.sourceforge.net
> Cc: Jeff Kirsher <jeffrey....@intel.com>
> Cc: Bruce Allan <bruce....@intel.com>
> ---
> drivers/net/ethernet/intel/e1000e/ethtool.c | 13 +++++++++++++
> drivers/net/ethernet/intel/e1000e/netdev.c | 2 ++
> 2 files changed, 15 insertions(+)

I have added this patch as well to my e1000e queue of patches.

[Jeff Kirsher]

On Mon, 2013-02-25 at 09:19 +0400, Konstantin Khlebnikov wrote:

> This patch removes redundant and unbalanced pci_disable_device() from
> __e1000_shutdown(). pci_clear_master() is enough, device can go into
> suspended state with elevated enable_cnt.
>
> Bug was introduced in commit 23606cf5d1192c2b17912cb2ef6e62f9b11de133
> ("e1000e / PCI / PM: Add basic runtime PM support (rev. 4)") in
> v2.6.35
>
> Signed-off-by: Konstantin Khlebnikov <khleb...@openvz.org>
> Acked-by: Rafael J. Wysocki <rafael.j...@intel.com>
> Cc: e1000...@lists.sourceforge.net
> Cc: Jeff Kirsher <jeffrey....@intel.com>
> Cc: Bruce Allan <bruce....@intel.com>
> ---
> drivers/net/ethernet/intel/e1000e/netdev.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)

I have added the patch to my queue of e1000e patches.