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

[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

[PATCH RESEND 1/3] e1000e: fix pci-device enable-counter balance

[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 <khlebnikov@openvz.org>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: e1000-devel@lists.sourceforge.net
Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Cc: Bruce Allan <bruce.w.allan@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;
 }

[PATCH RESEND 2/3] e1000e: fix runtime power management transitions

[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 <khlebnikov@openvz.org>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: e1000-devel@lists.sourceforge.net
Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Cc: Bruce Allan <bruce.w.allan@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);

[PATCH RESEND 3/3] e1000e: fix accessing to suspended device

[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 <khlebnikov@openvz.org>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: e1000-devel@lists.sourceforge.net
Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Cc: Bruce Allan <bruce.w.allan@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

Re: [E1000-devel] [PATCH RESEND 3/3] e1000e: fix accessing to suspended device

[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 netdev@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.

>
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: e1000-devel@lists.sourceforge.net
> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Cc: Bruce Allan <bruce.w.allan@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,

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-devel@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
>

Re: [E1000-devel] [PATCH RESEND 3/3] e1000e: fix accessing to suspended device

[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 netdev@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.

>
>>
>> Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
>> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Cc: e1000-devel@lists.sourceforge.net
>> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>> Cc: Bruce Allan <bruce.w.allan@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,
>
> 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-devel@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
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

Re: [E1000-devel] [PATCH RESEND 3/3] e1000e: fix accessing to suspended device

[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 netdev@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.

Re: [E1000-devel] [PATCH RESEND 3/3] e1000e: fix accessing to suspended device

[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 netdev@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.

Re: [PATCH RESEND 3/3] e1000e: fix accessing to suspended device

[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 netdev@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.


------------------------------------------------------------------------------
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-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired

Re: [PATCH RESEND 1/3] e1000e: fix pci-device enable-counter balance

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 846 bytes --]

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 <khlebnikov@openvz.org>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: e1000-devel@lists.sourceforge.net
> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Cc: Bruce Allan <bruce.w.allan@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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH RESEND 2/3] e1000e: fix runtime power management transitions

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 1543 bytes --]

On Mon, 2013-02-25 at 09:19 +0400, Konstantin Khlebnikov wrote:
> 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 <khlebnikov@openvz.org>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: e1000-devel@lists.sourceforge.net
> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Cc: Bruce Allan <bruce.w.allan@intel.com>
> ---
>  drivers/net/ethernet/intel/e1000e/netdev.c |   78
> ++++++----------------------
>  1 file changed, 18 insertions(+), 60 deletions(-) 

I have added this patch to my e1000e patch queue.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH RESEND 3/3] e1000e: fix accessing to suspended device

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 709 bytes --]

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 <khlebnikov@openvz.org>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: e1000-devel@lists.sourceforge.net
> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Cc: Bruce Allan <bruce.w.allan@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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]