[PATCH 1/2] Revert "alx: remove WoL support"

[PATCH 1/2] Revert "alx: remove WoL support"

[AceLan Kao]

This reverts commit bc2bebe8de8ed4ba6482c9cc370b0dd72ffe8cd2.

There are still many people need this feature, so try adding it back.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=61651
Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
---
 drivers/net/ethernet/atheros/alx/ethtool.c |  36 +++++++
 drivers/net/ethernet/atheros/alx/hw.c      | 154 ++++++++++++++++++++++++++++-
 drivers/net/ethernet/atheros/alx/hw.h      |   5 +
 drivers/net/ethernet/atheros/alx/main.c    | 142 ++++++++++++++++++++++++--
 4 files changed, 326 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/atheros/alx/ethtool.c b/drivers/net/ethernet/atheros/alx/ethtool.c
index 2f4eabf..859e272 100644
--- a/drivers/net/ethernet/atheros/alx/ethtool.c
+++ b/drivers/net/ethernet/atheros/alx/ethtool.c
@@ -310,11 +310,47 @@ static int alx_get_sset_count(struct net_device *netdev, int sset)
 	}
 }
 
+static void alx_get_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
+{
+	struct alx_priv *alx = netdev_priv(netdev);
+	struct alx_hw *hw = &alx->hw;
+
+	wol->supported = WAKE_MAGIC | WAKE_PHY;
+	wol->wolopts = 0;
+
+	if (hw->sleep_ctrl & ALX_SLEEP_WOL_MAGIC)
+		wol->wolopts |= WAKE_MAGIC;
+	if (hw->sleep_ctrl & ALX_SLEEP_WOL_PHY)
+		wol->wolopts |= WAKE_PHY;
+}
+
+static int alx_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
+{
+	struct alx_priv *alx = netdev_priv(netdev);
+	struct alx_hw *hw = &alx->hw;
+
+	if (wol->wolopts & ~(WAKE_MAGIC | WAKE_PHY))
+		return -EOPNOTSUPP;
+
+	hw->sleep_ctrl = 0;
+
+	if (wol->wolopts & WAKE_MAGIC)
+		hw->sleep_ctrl |= ALX_SLEEP_WOL_MAGIC;
+	if (wol->wolopts & WAKE_PHY)
+		hw->sleep_ctrl |= ALX_SLEEP_WOL_PHY;
+
+	device_set_wakeup_enable(&alx->hw.pdev->dev, hw->sleep_ctrl);
+
+	return 0;
+}
+
 const struct ethtool_ops alx_ethtool_ops = {
 	.get_pauseparam	= alx_get_pauseparam,
 	.set_pauseparam	= alx_set_pauseparam,
 	.get_msglevel	= alx_get_msglevel,
 	.set_msglevel	= alx_set_msglevel,
+	.get_wol	= alx_get_wol,
+	.set_wol	= alx_set_wol,
 	.get_link	= ethtool_op_get_link,
 	.get_strings	= alx_get_strings,
 	.get_sset_count	= alx_get_sset_count,
diff --git a/drivers/net/ethernet/atheros/alx/hw.c b/drivers/net/ethernet/atheros/alx/hw.c
index 6ac40b0..f9bf612 100644
--- a/drivers/net/ethernet/atheros/alx/hw.c
+++ b/drivers/net/ethernet/atheros/alx/hw.c
@@ -332,6 +332,16 @@ void alx_set_macaddr(struct alx_hw *hw, const u8 *addr)
 	alx_write_mem32(hw, ALX_STAD1, val);
 }
 
+static void alx_enable_osc(struct alx_hw *hw)
+{
+	u32 val;
+
+	/* rising edge */
+	val = alx_read_mem32(hw, ALX_MISC);
+	alx_write_mem32(hw, ALX_MISC, val & ~ALX_MISC_INTNLOSC_OPEN);
+	alx_write_mem32(hw, ALX_MISC, val | ALX_MISC_INTNLOSC_OPEN);
+}
+
 static void alx_reset_osc(struct alx_hw *hw, u8 rev)
 {
 	u32 val, val2;
@@ -774,7 +784,6 @@ int alx_setup_speed_duplex(struct alx_hw *hw, u32 ethadv, u8 flowctrl)
 	return err;
 }
 
-
 void alx_post_phy_link(struct alx_hw *hw)
 {
 	u16 phy_val, len, agc;
@@ -848,6 +857,65 @@ void alx_post_phy_link(struct alx_hw *hw)
 	}
 }
 
+/* NOTE:
+ *    1. phy link must be established before calling this function
+ *    2. wol option (pattern,magic,link,etc.) is configed before call it.
+ */
+int alx_pre_suspend(struct alx_hw *hw, int speed, u8 duplex)
+{
+	u32 master, mac, phy, val;
+	int err = 0;
+
+	master = alx_read_mem32(hw, ALX_MASTER);
+	master &= ~ALX_MASTER_PCLKSEL_SRDS;
+	mac = hw->rx_ctrl;
+	/* 10/100 half */
+	ALX_SET_FIELD(mac, ALX_MAC_CTRL_SPEED,  ALX_MAC_CTRL_SPEED_10_100);
+	mac &= ~(ALX_MAC_CTRL_FULLD | ALX_MAC_CTRL_RX_EN | ALX_MAC_CTRL_TX_EN);
+
+	phy = alx_read_mem32(hw, ALX_PHY_CTRL);
+	phy &= ~(ALX_PHY_CTRL_DSPRST_OUT | ALX_PHY_CTRL_CLS);
+	phy |= ALX_PHY_CTRL_RST_ANALOG | ALX_PHY_CTRL_HIB_PULSE |
+	       ALX_PHY_CTRL_HIB_EN;
+
+	/* without any activity  */
+	if (!(hw->sleep_ctrl & ALX_SLEEP_ACTIVE)) {
+		err = alx_write_phy_reg(hw, ALX_MII_IER, 0);
+		if (err)
+			return err;
+		phy |= ALX_PHY_CTRL_IDDQ | ALX_PHY_CTRL_POWER_DOWN;
+	} else {
+		if (hw->sleep_ctrl & (ALX_SLEEP_WOL_MAGIC | ALX_SLEEP_CIFS))
+			mac |= ALX_MAC_CTRL_RX_EN | ALX_MAC_CTRL_BRD_EN;
+		if (hw->sleep_ctrl & ALX_SLEEP_CIFS)
+			mac |= ALX_MAC_CTRL_TX_EN;
+		if (duplex == DUPLEX_FULL)
+			mac |= ALX_MAC_CTRL_FULLD;
+		if (speed == SPEED_1000)
+			ALX_SET_FIELD(mac, ALX_MAC_CTRL_SPEED,
+				      ALX_MAC_CTRL_SPEED_1000);
+		phy |= ALX_PHY_CTRL_DSPRST_OUT;
+		err = alx_write_phy_ext(hw, ALX_MIIEXT_ANEG,
+					ALX_MIIEXT_S3DIG10,
+					ALX_MIIEXT_S3DIG10_SL);
+		if (err)
+			return err;
+	}
+
+	alx_enable_osc(hw);
+	hw->rx_ctrl = mac;
+	alx_write_mem32(hw, ALX_MASTER, master);
+	alx_write_mem32(hw, ALX_MAC_CTRL, mac);
+	alx_write_mem32(hw, ALX_PHY_CTRL, phy);
+
+	/* set val of PDLL D3PLLOFF */
+	val = alx_read_mem32(hw, ALX_PDLL_TRNS1);
+	val |= ALX_PDLL_TRNS1_D3PLLOFF_EN;
+	alx_write_mem32(hw, ALX_PDLL_TRNS1, val);
+
+	return 0;
+}
+
 bool alx_phy_configured(struct alx_hw *hw)
 {
 	u32 cfg, hw_cfg;
@@ -920,6 +988,26 @@ int alx_clear_phy_intr(struct alx_hw *hw)
 	return alx_read_phy_reg(hw, ALX_MII_ISR, &isr);
 }
 
+int alx_config_wol(struct alx_hw *hw)
+{
+	u32 wol = 0;
+	int err = 0;
+
+	/* turn on magic packet event */
+	if (hw->sleep_ctrl & ALX_SLEEP_WOL_MAGIC)
+		wol |= ALX_WOL0_MAGIC_EN | ALX_WOL0_PME_MAGIC_EN;
+
+	/* turn on link up event */
+	if (hw->sleep_ctrl & ALX_SLEEP_WOL_PHY) {
+		wol |=  ALX_WOL0_LINK_EN | ALX_WOL0_PME_LINK;
+		/* only link up can wake up */
+		err = alx_write_phy_reg(hw, ALX_MII_IER, ALX_IER_LINK_UP);
+	}
+	alx_write_mem32(hw, ALX_WOL0, wol);
+
+	return err;
+}
+
 void alx_disable_rss(struct alx_hw *hw)
 {
 	u32 ctrl = alx_read_mem32(hw, ALX_RXQ0);
@@ -1044,6 +1132,70 @@ void alx_mask_msix(struct alx_hw *hw, int index, bool mask)
 	alx_post_write(hw);
 }
 
+int alx_select_powersaving_speed(struct alx_hw *hw, int *speed, u8 *duplex)
+{
+	int i, err;
+	u16 lpa;
+
+	err = alx_read_phy_link(hw);
+	if (err)
+		return err;
+
+	if (hw->link_speed == SPEED_UNKNOWN) {
+		*speed = SPEED_UNKNOWN;
+		*duplex = DUPLEX_UNKNOWN;
+		return 0;
+	}
+
+	err = alx_read_phy_reg(hw, MII_LPA, &lpa);
+	if (err)
+		return err;
+
+	if (!(lpa & LPA_LPACK)) {
+		*speed = hw->link_speed;
+		return 0;
+	}
+
+	if (lpa & LPA_10FULL) {
+		*speed = SPEED_10;
+		*duplex = DUPLEX_FULL;
+	} else if (lpa & LPA_10HALF) {
+		*speed = SPEED_10;
+		*duplex = DUPLEX_HALF;
+	} else if (lpa & LPA_100FULL) {
+		*speed = SPEED_100;
+		*duplex = DUPLEX_FULL;
+	} else {
+		*speed = SPEED_100;
+		*duplex = DUPLEX_HALF;
+	}
+
+	if (*speed == hw->link_speed && *duplex == hw->duplex)
+		return 0;
+	err = alx_write_phy_reg(hw, ALX_MII_IER, 0);
+	if (err)
+		return err;
+	err = alx_setup_speed_duplex(hw, alx_speed_to_ethadv(*speed, *duplex) |
+					ADVERTISED_Autoneg, ALX_FC_ANEG |
+					ALX_FC_RX | ALX_FC_TX);
+	if (err)
+		return err;
+
+	/* wait for linkup */
+	for (i = 0; i < ALX_MAX_SETUP_LNK_CYCLE; i++) {
+		msleep(100);
+
+		err = alx_read_phy_link(hw);
+		if (err < 0)
+			return err;
+		if (hw->link_speed != SPEED_UNKNOWN)
+			break;
+	}
+	if (i == ALX_MAX_SETUP_LNK_CYCLE)
+		return -ETIMEDOUT;
+
+	return 0;
+}
 
 bool alx_get_phy_info(struct alx_hw *hw)
 {
diff --git a/drivers/net/ethernet/atheros/alx/hw.h b/drivers/net/ethernet/atheros/alx/hw.h
index e42d7e0..a7fb6c8 100644
--- a/drivers/net/ethernet/atheros/alx/hw.h
+++ b/drivers/net/ethernet/atheros/alx/hw.h
@@ -487,6 +487,8 @@ struct alx_hw {
 	u8 flowctrl;
 	u32 adv_cfg;
 
+	u32 sleep_ctrl;
+
 	spinlock_t mdio_lock;
 	struct mdio_if_info mdio;
 	u16 phy_id[2];
@@ -549,12 +551,14 @@ void alx_reset_pcie(struct alx_hw *hw);
 void alx_enable_aspm(struct alx_hw *hw, bool l0s_en, bool l1_en);
 int alx_setup_speed_duplex(struct alx_hw *hw, u32 ethadv, u8 flowctrl);
 void alx_post_phy_link(struct alx_hw *hw);
+int alx_pre_suspend(struct alx_hw *hw, int speed, u8 duplex);
 int alx_read_phy_reg(struct alx_hw *hw, u16 reg, u16 *phy_data);
 int alx_write_phy_reg(struct alx_hw *hw, u16 reg, u16 phy_data);
 int alx_read_phy_ext(struct alx_hw *hw, u8 dev, u16 reg, u16 *pdata);
 int alx_write_phy_ext(struct alx_hw *hw, u8 dev, u16 reg, u16 data);
 int alx_read_phy_link(struct alx_hw *hw);
 int alx_clear_phy_intr(struct alx_hw *hw);
+int alx_config_wol(struct alx_hw *hw);
 void alx_cfg_mac_flowcontrol(struct alx_hw *hw, u8 fc);
 void alx_start_mac(struct alx_hw *hw);
 int alx_reset_mac(struct alx_hw *hw);
@@ -563,6 +567,7 @@ bool alx_phy_configured(struct alx_hw *hw);
 void alx_configure_basic(struct alx_hw *hw);
 void alx_mask_msix(struct alx_hw *hw, int index, bool mask);
 void alx_disable_rss(struct alx_hw *hw);
+int alx_select_powersaving_speed(struct alx_hw *hw, int *speed, u8 *duplex);
 bool alx_get_phy_info(struct alx_hw *hw);
 void alx_update_hw_stats(struct alx_hw *hw);
 
diff --git a/drivers/net/ethernet/atheros/alx/main.c b/drivers/net/ethernet/atheros/alx/main.c
index 567ee54..c0e2bb2 100644
--- a/drivers/net/ethernet/atheros/alx/main.c
+++ b/drivers/net/ethernet/atheros/alx/main.c
@@ -1070,6 +1070,7 @@ static int alx_init_sw(struct alx_priv *alx)
 	alx->dev->max_mtu = ALX_MAX_FRAME_LEN(ALX_MAX_FRAME_SIZE);
 	alx->tx_ringsz = 256;
 	alx->rx_ringsz = 512;
+	hw->sleep_ctrl = ALX_SLEEP_WOL_MAGIC | ALX_SLEEP_WOL_PHY;
 	hw->imt = 200;
 	alx->int_mask = ALX_ISR_MISC;
 	hw->dma_chnl = hw->max_dma_chnl;
@@ -1346,6 +1347,66 @@ static int alx_stop(struct net_device *netdev)
 	return 0;
 }
 
+static int __alx_shutdown(struct pci_dev *pdev, bool *wol_en)
+{
+	struct alx_priv *alx = pci_get_drvdata(pdev);
+	struct net_device *netdev = alx->dev;
+	struct alx_hw *hw = &alx->hw;
+	int err, speed;
+	u8 duplex;
+
+	netif_device_detach(netdev);
+
+	if (netif_running(netdev))
+		__alx_stop(alx);
+
+#ifdef CONFIG_PM_SLEEP
+	err = pci_save_state(pdev);
+	if (err)
+		return err;
+#endif
+
+	err = alx_select_powersaving_speed(hw, &speed, &duplex);
+	if (err)
+		return err;
+	err = alx_clear_phy_intr(hw);
+	if (err)
+		return err;
+	err = alx_pre_suspend(hw, speed, duplex);
+	if (err)
+		return err;
+	err = alx_config_wol(hw);
+	if (err)
+		return err;
+
+	*wol_en = false;
+	if (hw->sleep_ctrl & ALX_SLEEP_ACTIVE) {
+		netif_info(alx, wol, netdev,
+			   "wol: ctrl=%X, speed=%X\n",
+			   hw->sleep_ctrl, speed);
+		device_set_wakeup_enable(&pdev->dev, true);
+		*wol_en = true;
+	}
+
+	pci_disable_device(pdev);
+
+	return 0;
+}
+
+static void alx_shutdown(struct pci_dev *pdev)
+{
+	int err;
+	bool wol_en;
+
+	err = __alx_shutdown(pdev, &wol_en);
+	if (!err) {
+		pci_wake_from_d3(pdev, wol_en);
+		pci_set_power_state(pdev, PCI_D3hot);
+	} else {
+		dev_err(&pdev->dev, "shutdown fail %d\n", err);
+	}
+}
+
 static void alx_link_check(struct work_struct *work)
 {
 	struct alx_priv *alx;
@@ -1841,6 +1902,8 @@ static int alx_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		goto out_unmap;
 	}
 
+	device_set_wakeup_enable(&pdev->dev, hw->sleep_ctrl);
+
 	netdev_info(netdev,
 		    "Qualcomm Atheros AR816x/AR817x Ethernet [%pM]\n",
 		    netdev->dev_addr);
@@ -1883,12 +1946,22 @@ static void alx_remove(struct pci_dev *pdev)
 static int alx_suspend(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
-	struct alx_priv *alx = pci_get_drvdata(pdev);
+	int err;
+	bool wol_en;
+
+	err = __alx_shutdown(pdev, &wol_en);
+	if (err) {
+		dev_err(&pdev->dev, "shutdown fail in suspend %d\n", err);
+		return err;
+	}
+
+	if (wol_en) {
+		pci_prepare_to_sleep(pdev);
+	} else {
+		pci_wake_from_d3(pdev, false);
+		pci_set_power_state(pdev, PCI_D3hot);
+	}
 
-	if (!netif_running(alx->dev))
-		return 0;
-	netif_device_detach(alx->dev);
-	__alx_stop(alx);
 	return 0;
 }
 
@@ -1896,23 +1969,69 @@ static int alx_resume(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct alx_priv *alx = pci_get_drvdata(pdev);
+	struct net_device *netdev = alx->dev;
 	struct alx_hw *hw = &alx->hw;
+	int err;
+
+	pci_set_power_state(pdev, PCI_D0);
+	pci_restore_state(pdev);
+	pci_save_state(pdev);
+
+	pci_enable_wake(pdev, PCI_D3hot, 0);
+	pci_enable_wake(pdev, PCI_D3cold, 0);
 
+	hw->link_speed = SPEED_UNKNOWN;
+	alx->int_mask = ALX_ISR_MISC;
+
+	alx_reset_pcie(hw);
 	alx_reset_phy(hw);
 
-	if (!netif_running(alx->dev))
-		return 0;
-	netif_device_attach(alx->dev);
-	return __alx_open(alx, true);
+	pci_set_power_state(pdev, PCI_D0);
+	pci_restore_state(pdev);
+	pci_save_state(pdev);
+
+	pci_enable_wake(pdev, PCI_D3hot, 0);
+	pci_enable_wake(pdev, PCI_D3cold, 0);
+
+	hw->link_speed = SPEED_UNKNOWN;
+	alx->int_mask = ALX_ISR_MISC;
+
+	alx_reset_pcie(hw);
+	alx_reset_phy(hw);
+
+	err = alx_reset_mac(hw);
+	if (err) {
+		netif_err(alx, hw, alx->dev,
+			  "resume:reset_mac fail %d\n", err);
+		return -EIO;
+	}
+
+	err = alx_setup_speed_duplex(hw, hw->adv_cfg, hw->flowctrl);
+	if (err) {
+		netif_err(alx, hw, alx->dev,
+			  "resume:setup_speed_duplex fail %d\n", err);
+		return -EIO;
+	}
+
+	if (netif_running(netdev)) {
+		err = __alx_open(alx, true);
+		if (err)
+			return err;
+	}
+
+	netif_device_attach(netdev);
+
+	return err;
 }
+#endif
 
+#ifdef CONFIG_PM_SLEEP
 static SIMPLE_DEV_PM_OPS(alx_pm_ops, alx_suspend, alx_resume);
 #define ALX_PM_OPS      (&alx_pm_ops)
 #else
 #define ALX_PM_OPS      NULL
 #endif
 
-
 static pci_ers_result_t alx_pci_error_detected(struct pci_dev *pdev,
 					       pci_channel_state_t state)
 {
@@ -1955,6 +2074,8 @@ static pci_ers_result_t alx_pci_error_slot_reset(struct pci_dev *pdev)
 	}
 
 	pci_set_master(pdev);
+	pci_enable_wake(pdev, PCI_D3hot, 0);
+	pci_enable_wake(pdev, PCI_D3cold, 0);
 
 	alx_reset_pcie(hw);
 	if (!alx_reset_mac(hw))
@@ -2011,6 +2132,7 @@ static struct pci_driver alx_driver = {
 	.id_table    = alx_pci_tbl,
 	.probe       = alx_probe,
 	.remove      = alx_remove,
+	.shutdown    = alx_shutdown,
 	.err_handler = &alx_err_handlers,
 	.driver.pm   = ALX_PM_OPS,
 };
-- 
2.7.4

[PATCH 2/2] alx: add disable_wol paramenter

[AceLan Kao]

The WoL feature was reported broken and will lead to
the system resume immediately after suspending.
This symptom is not happening on every system, so adding
disable_wol option and disable WoL by default to prevent the issue from
happening again.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=61651
Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
---
 drivers/net/ethernet/atheros/alx/ethtool.c | 7 ++++++-
 drivers/net/ethernet/atheros/alx/main.c    | 5 +++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/atheros/alx/ethtool.c b/drivers/net/ethernet/atheros/alx/ethtool.c
index 859e272..e50129d 100644
--- a/drivers/net/ethernet/atheros/alx/ethtool.c
+++ b/drivers/net/ethernet/atheros/alx/ethtool.c
@@ -46,6 +46,8 @@
 #include "reg.h"
 #include "hw.h"
 
+extern const bool disable_wol;
+
 /* The order of these strings must match the order of the fields in
  * struct alx_hw_stats
  * See hw.h
@@ -315,6 +317,9 @@ static void alx_get_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
 	struct alx_priv *alx = netdev_priv(netdev);
 	struct alx_hw *hw = &alx->hw;
 
+	if (disable_wol)
+		return;
+
 	wol->supported = WAKE_MAGIC | WAKE_PHY;
 	wol->wolopts = 0;
 
@@ -329,7 +334,7 @@ static int alx_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
 	struct alx_priv *alx = netdev_priv(netdev);
 	struct alx_hw *hw = &alx->hw;
 
-	if (wol->wolopts & ~(WAKE_MAGIC | WAKE_PHY))
+	if (disable_wol || (wol->wolopts & ~(WAKE_MAGIC | WAKE_PHY)))
 		return -EOPNOTSUPP;
 
 	hw->sleep_ctrl = 0;
diff --git a/drivers/net/ethernet/atheros/alx/main.c b/drivers/net/ethernet/atheros/alx/main.c
index c0e2bb2..c27fdf7 100644
--- a/drivers/net/ethernet/atheros/alx/main.c
+++ b/drivers/net/ethernet/atheros/alx/main.c
@@ -51,6 +51,11 @@
 
 const char alx_drv_name[] = "alx";
 
+/* disable WoL by default */
+bool disable_wol = 1;
+module_param(disable_wol, bool, 0);
+MODULE_PARM_DESC(disable_wol, "Disable Wake on Lan feature");
+
 static void alx_free_txbuf(struct alx_tx_queue *txq, int entry)
 {
 	struct alx_buffer *txb = &txq->bufs[entry];
-- 
2.7.4

Re: [PATCH 2/2] alx: add disable_wol paramenter

[Andrew Lunn]

On Mon, Apr 09, 2018 at 07:35:14PM +0800, AceLan Kao wrote:
> The WoL feature was reported broken and will lead to
> the system resume immediately after suspending.
> This symptom is not happening on every system, so adding
> disable_wol option and disable WoL by default to prevent the issue from
> happening again.

>  const char alx_drv_name[] = "alx";
>  
> +/* disable WoL by default */
> +bool disable_wol = 1;
> +module_param(disable_wol, bool, 0);
> +MODULE_PARM_DESC(disable_wol, "Disable Wake on Lan feature");
> +

Hi AceLan

This seems like you are papering over the cracks. And module
parameters are not liked.

Please try to find the real problem.

       Andrew

Re: [PATCH 2/2] alx: add disable_wol paramenter

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Mon, 9 Apr 2018 14:39:10 +0200

> On Mon, Apr 09, 2018 at 07:35:14PM +0800, AceLan Kao wrote:
>> The WoL feature was reported broken and will lead to
>> the system resume immediately after suspending.
>> This symptom is not happening on every system, so adding
>> disable_wol option and disable WoL by default to prevent the issue from
>> happening again.
> 
>>  const char alx_drv_name[] = "alx";
>>  
>> +/* disable WoL by default */
>> +bool disable_wol = 1;
>> +module_param(disable_wol, bool, 0);
>> +MODULE_PARM_DESC(disable_wol, "Disable Wake on Lan feature");
>> +
> 
> Hi AceLan
> 
> This seems like you are papering over the cracks. And module
> parameters are not liked.
> 
> Please try to find the real problem.

Agreed.

Re: [PATCH 2/2] alx: add disable_wol paramenter

[AceLan Kao]

The problem is I don't have a machine with that wakeup issue, and I
need WoL feature.
Instead of spreading "alx with WoL" dkms package everywhere, I would
like to see it's supported in the driver and is disabled by default.

Moreover, the wakeup issue may come from old Atheros chips, or result
from buggy BIOS.
With the WoL has been removed from the driver, no one will report
issue about that, and we don't have any chance to find a fix for it.

Adding this feature back is not covering a paper on the issue, it
makes people have a chance to examine this feature.

2018-04-09 22:50 GMT+08:00 David Miller <davem@davemloft.net>:
> From: Andrew Lunn <andrew@lunn.ch>
> Date: Mon, 9 Apr 2018 14:39:10 +0200
>
>> On Mon, Apr 09, 2018 at 07:35:14PM +0800, AceLan Kao wrote:
>>> The WoL feature was reported broken and will lead to
>>> the system resume immediately after suspending.
>>> This symptom is not happening on every system, so adding
>>> disable_wol option and disable WoL by default to prevent the issue from
>>> happening again.
>>
>>>  const char alx_drv_name[] = "alx";
>>>
>>> +/* disable WoL by default */
>>> +bool disable_wol = 1;
>>> +module_param(disable_wol, bool, 0);
>>> +MODULE_PARM_DESC(disable_wol, "Disable Wake on Lan feature");
>>> +
>>
>> Hi AceLan
>>
>> This seems like you are papering over the cracks. And module
>> parameters are not liked.
>>
>> Please try to find the real problem.
>
> Agreed.

Re: [PATCH 2/2] alx: add disable_wol paramenter

[AceLan Kao]

Hi,

May I know the final decision of this patch?
Thanks.

Best regards,
AceLan Kao.

2018-04-10 10:40 GMT+08:00 AceLan Kao <acelan.kao@canonical.com>:
> The problem is I don't have a machine with that wakeup issue, and I
> need WoL feature.
> Instead of spreading "alx with WoL" dkms package everywhere, I would
> like to see it's supported in the driver and is disabled by default.
>
> Moreover, the wakeup issue may come from old Atheros chips, or result
> from buggy BIOS.
> With the WoL has been removed from the driver, no one will report
> issue about that, and we don't have any chance to find a fix for it.
>
> Adding this feature back is not covering a paper on the issue, it
> makes people have a chance to examine this feature.
>
> 2018-04-09 22:50 GMT+08:00 David Miller <davem@davemloft.net>:
>> From: Andrew Lunn <andrew@lunn.ch>
>> Date: Mon, 9 Apr 2018 14:39:10 +0200
>>
>>> On Mon, Apr 09, 2018 at 07:35:14PM +0800, AceLan Kao wrote:
>>>> The WoL feature was reported broken and will lead to
>>>> the system resume immediately after suspending.
>>>> This symptom is not happening on every system, so adding
>>>> disable_wol option and disable WoL by default to prevent the issue from
>>>> happening again.
>>>
>>>>  const char alx_drv_name[] = "alx";
>>>>
>>>> +/* disable WoL by default */
>>>> +bool disable_wol = 1;
>>>> +module_param(disable_wol, bool, 0);
>>>> +MODULE_PARM_DESC(disable_wol, "Disable Wake on Lan feature");
>>>> +
>>>
>>> Hi AceLan
>>>
>>> This seems like you are papering over the cracks. And module
>>> parameters are not liked.
>>>
>>> Please try to find the real problem.
>>
>> Agreed.

Re: [PATCH 2/2] alx: add disable_wol paramenter

[AceLan Kao]

Hi,

I didn't get any response around one month.
I'm still here hoping you can consider accepting the WoL patch.

Without that patch, people have no chance to bump into the issue and
have no chance to fix it.
Moreover, it leads to the dkms package be spreaded around, and it'll
become a more annoying issue when UEFI secure boot is enabled[1].
Please re-consider it to enable WoL again and set it to disable by default,
so that we/user have a chance to examine the feature and have a chance
to find out a read fix for it.
Thanks.

1. https://bugzilla.kernel.org/show_bug.cgi?id=61651

Best regards,
AceLan Kao.

2018-04-24 11:45 GMT+08:00 AceLan Kao <acelan.kao@canonical.com>:
> Hi,
>
> May I know the final decision of this patch?
> Thanks.
>
> Best regards,
> AceLan Kao.
>
> 2018-04-10 10:40 GMT+08:00 AceLan Kao <acelan.kao@canonical.com>:
>> The problem is I don't have a machine with that wakeup issue, and I
>> need WoL feature.
>> Instead of spreading "alx with WoL" dkms package everywhere, I would
>> like to see it's supported in the driver and is disabled by default.
>>
>> Moreover, the wakeup issue may come from old Atheros chips, or result
>> from buggy BIOS.
>> With the WoL has been removed from the driver, no one will report
>> issue about that, and we don't have any chance to find a fix for it.
>>
>> Adding this feature back is not covering a paper on the issue, it
>> makes people have a chance to examine this feature.
>>
>> 2018-04-09 22:50 GMT+08:00 David Miller <davem@davemloft.net>:
>>> From: Andrew Lunn <andrew@lunn.ch>
>>> Date: Mon, 9 Apr 2018 14:39:10 +0200
>>>
>>>> On Mon, Apr 09, 2018 at 07:35:14PM +0800, AceLan Kao wrote:
>>>>> The WoL feature was reported broken and will lead to
>>>>> the system resume immediately after suspending.
>>>>> This symptom is not happening on every system, so adding
>>>>> disable_wol option and disable WoL by default to prevent the issue from
>>>>> happening again.
>>>>
>>>>>  const char alx_drv_name[] = "alx";
>>>>>
>>>>> +/* disable WoL by default */
>>>>> +bool disable_wol = 1;
>>>>> +module_param(disable_wol, bool, 0);
>>>>> +MODULE_PARM_DESC(disable_wol, "Disable Wake on Lan feature");
>>>>> +
>>>>
>>>> Hi AceLan
>>>>
>>>> This seems like you are papering over the cracks. And module
>>>> parameters are not liked.
>>>>
>>>> Please try to find the real problem.
>>>
>>> Agreed.

Re: [PATCH 2/2] alx: add disable_wol paramenter

[Andrew Lunn]

On Wed, May 09, 2018 at 10:40:13AM +0800, AceLan Kao wrote:
> Hi,
> 
> I didn't get any response around one month.

I didn't notice you posting an results of searching for the true
problem? Please can you point me at an email archive.

	 Andrew

Re: [PATCH 2/2] alx: add disable_wol paramenter

[AceLan Kao]

Hi Andrew,

We have some machines using Qualcomm Atheros Killer E2400 Gigabit
Ethernet Controller,
but none of them has the unintentional wake up issue.
We're willing to fix it if we encountered the issue, but before we can
do it, we need this feature is supported by the driver.

Taking the feature has been removed for 5 years into account, I doubt
if we still can reproduce this issue,
but again, to verify this issue we need to add back this feature first.
Set WoL disabled by default won't introduce any regression but give
users and developers a chance to fix it.

Best regards,
AceLan Kao.

2018-05-09 21:45 GMT+08:00 Andrew Lunn <andrew@lunn.ch>:
> On Wed, May 09, 2018 at 10:40:13AM +0800, AceLan Kao wrote:
>> Hi,
>>
>> I didn't get any response around one month.
>
> I didn't notice you posting an results of searching for the true
> problem? Please can you point me at an email archive.
>
>          Andrew

Re: [PATCH 2/2] alx: add disable_wol paramenter

[Andrew Lunn]

On Thu, May 10, 2018 at 01:58:24PM +0800, AceLan Kao wrote:
> Hi Andrew,
> 
> We have some machines using Qualcomm Atheros Killer E2400 Gigabit
> Ethernet Controller,
> but none of them has the unintentional wake up issue.
> We're willing to fix it if we encountered the issue, but before we can
> do it, we need this feature is supported by the driver.
> 
> Taking the feature has been removed for 5 years into account, I doubt
> if we still can reproduce this issue,
> but again, to verify this issue we need to add back this feature first.
> Set WoL disabled by default won't introduce any regression but give
> users and developers a chance to fix it.

The main problem here is the module parameter. That is not going to be
accepted.

Can you argue the cure is worse than the disease? Is WoL not working
considered by a lot of people as being a bug? Double wake up is also a
bug, but not many people care, it does not cause any data corruption,
etc. So can you argue overall we have a less buggy system, but still
buggy, if WoL is enabled?

If you can write a convincing Change Message arguing the case, a patch
simply re-enabling WoL might be accepted.

But you also need to take on the responsibility to help debug the
failed shutdowns in order to get to the bottom of this problem.

       Andrew

Re: [PATCH 2/2] alx: add disable_wol paramenter

[AceLan Kao]

Okay, I'll submit a new patch with some more description of why we
need this feature.
Thanks.

2018-05-10 20:34 GMT+08:00 Andrew Lunn <andrew@lunn.ch>:
> On Thu, May 10, 2018 at 01:58:24PM +0800, AceLan Kao wrote:
>> Hi Andrew,
>>
>> We have some machines using Qualcomm Atheros Killer E2400 Gigabit
>> Ethernet Controller,
>> but none of them has the unintentional wake up issue.
>> We're willing to fix it if we encountered the issue, but before we can
>> do it, we need this feature is supported by the driver.
>>
>> Taking the feature has been removed for 5 years into account, I doubt
>> if we still can reproduce this issue,
>> but again, to verify this issue we need to add back this feature first.
>> Set WoL disabled by default won't introduce any regression but give
>> users and developers a chance to fix it.
>
> The main problem here is the module parameter. That is not going to be
> accepted.
>
> Can you argue the cure is worse than the disease? Is WoL not working
> considered by a lot of people as being a bug? Double wake up is also a
> bug, but not many people care, it does not cause any data corruption,
> etc. So can you argue overall we have a less buggy system, but still
> buggy, if WoL is enabled?
>
> If you can write a convincing Change Message arguing the case, a patch
> simply re-enabling WoL might be accepted.
>
> But you also need to take on the responsibility to help debug the
> failed shutdowns in order to get to the bottom of this problem.
>
>        Andrew