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

2018-05-13 Thread 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 :
> 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

2018-05-10 Thread 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

2018-05-09 Thread 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 :
> 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

2018-05-09 Thread 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

2018-05-08 Thread 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 :
> 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 :
>> 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 :
>>> From: Andrew Lunn 
>>> 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

2018-04-23 Thread 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 :
> 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 :
>> From: Andrew Lunn 
>> 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

2018-04-09 Thread 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 :
> From: Andrew Lunn 
> 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

2018-04-09 Thread David Miller
From: Andrew Lunn 
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

2018-04-09 Thread 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