| Message ID | 1523273714-17264-2-git-send-email-acelan.kao@canonical.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [1/2] Revert "alx: remove WoL support" | expand |
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
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.
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.
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.
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.
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
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
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
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
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];
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(-)