[PATCH 0/8] asus-wmi: Get rid of quirk_no_rfkill{,_wapf4}

[PATCH 0/8] asus-wmi: Get rid of quirk_no_rfkill{,_wapf4}

[João Paulo Rechi Vita]

This quirk avoids the conflicting usage of ASUS_WMI_DEVID_WLAN_LED (0x00010002)
by both asus-wireless and asus-wmi on machines where the BIOS can't set and
record the wlan status (wlan_ctrl_by_user in asus-wmi.c). At the moment we have
six models using that quirk upstream, but there are another fifteen for which
we have downstream patches, and will send upstream unless we find a more
dynamic solution. We also expect this list to continue growing.

This series makes use of ASUS_WMI_DSTS_USER_BIT plus the status of the ASHS
device to skip asus_wmi_rfkill_init(). Since asus-wmi now needs to know the ASHS
_HIDs, I have moved device_ids[] to a new header file, asus-wireless.h. I'm not
sure if there are any best practices in the kernel community against having one
module including headers from a different module, but I wanted to avoid
duplicating this list or creating an ordering dependency between the modules.
Any advice on alternatives here are welcome.

PS: I don't think asus-wireless.h needs to also be listed in MAINTAINERS,
please correct me if I'm wrong.

João Paulo Rechi Vita (8):
  Revert "asus-wmi: Add quirk_no_rfkill_wapf4 for the Asus X456UA"
  Revert "asus-wmi: Add quirk_no_rfkill_wapf4 for the Asus X456UF"
  Revert "asus-wmi: Add quirk_no_rfkill for the Asus Z550MA"
  Revert "asus-wmi: Add quirk_no_rfkill for the Asus U303LB"
  Revert "asus-wmi: Add quirk_no_rfkill for the Asus N552VW"
  Revert "asus-wmi: Create quirk for airplane_mode LED"
  asus-wireless: Export ids list
  asus-wmi: Don't register rfkill if ASHS and user bit are present

 drivers/platform/x86/asus-nb-wmi.c   | 49 ++----------------------------------
 drivers/platform/x86/asus-wireless.c | 15 +++++------
 drivers/platform/x86/asus-wireless.h | 10 ++++++++
 drivers/platform/x86/asus-wmi.c      | 21 ++++++++++++----
 drivers/platform/x86/asus-wmi.h      |  1 -
 5 files changed, 34 insertions(+), 62 deletions(-)
 create mode 100644 drivers/platform/x86/asus-wireless.h

-- 
2.11.0

[PATCH 1/8] Revert "asus-wmi: Add quirk_no_rfkill_wapf4 for the Asus X456UA"

[João Paulo Rechi Vita]

This reverts commit 56a37a72002b1eb01a1de391cf66383652784e78.

Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com>
---
 drivers/platform/x86/asus-nb-wmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/asus-nb-wmi.c b/drivers/platform/x86/asus-nb-wmi.c
index 5be4783e40d4..6079510eaa29 100644
--- a/drivers/platform/x86/asus-nb-wmi.c
+++ b/drivers/platform/x86/asus-nb-wmi.c
@@ -194,7 +194,7 @@ static const struct dmi_system_id asus_quirks[] = {
 			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
 			DMI_MATCH(DMI_PRODUCT_NAME, "X456UA"),
 		},
-		.driver_data = &quirk_no_rfkill_wapf4,
+		.driver_data = &quirk_asus_wapf4,
 	},
 	{
 		.callback = dmi_matched,
-- 
2.11.0

[PATCH 2/8] Revert "asus-wmi: Add quirk_no_rfkill_wapf4 for the Asus X456UF"

[João Paulo Rechi Vita]

This reverts commit a961a285b479977fa21f12f71cd62f28adaba17c.

Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com>
---
 drivers/platform/x86/asus-nb-wmi.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/platform/x86/asus-nb-wmi.c b/drivers/platform/x86/asus-nb-wmi.c
index 6079510eaa29..5bf1dd225788 100644
--- a/drivers/platform/x86/asus-nb-wmi.c
+++ b/drivers/platform/x86/asus-nb-wmi.c
@@ -107,11 +107,6 @@ static struct quirk_entry quirk_no_rfkill = {
 	.no_rfkill = true,
 };
 
-static struct quirk_entry quirk_no_rfkill_wapf4 = {
-	.wapf = 4,
-	.no_rfkill = true,
-};
-
 static struct quirk_entry quirk_asus_ux303ub = {
 	.wmi_backlight_native = true,
 };
@@ -203,7 +198,7 @@ static const struct dmi_system_id asus_quirks[] = {
 			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
 			DMI_MATCH(DMI_PRODUCT_NAME, "X456UF"),
 		},
-		.driver_data = &quirk_no_rfkill_wapf4,
+		.driver_data = &quirk_asus_wapf4,
 	},
 	{
 		.callback = dmi_matched,
-- 
2.11.0

[PATCH 3/8] Revert "asus-wmi: Add quirk_no_rfkill for the Asus Z550MA"

[João Paulo Rechi Vita]

This reverts commit 6b7ff2af5286a8c6bec7ff5f4df62e3506c1674e.

Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com>
---
 drivers/platform/x86/asus-nb-wmi.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/platform/x86/asus-nb-wmi.c b/drivers/platform/x86/asus-nb-wmi.c
index 5bf1dd225788..9d686d642934 100644
--- a/drivers/platform/x86/asus-nb-wmi.c
+++ b/drivers/platform/x86/asus-nb-wmi.c
@@ -391,15 +391,6 @@ static const struct dmi_system_id asus_quirks[] = {
 	},
 	{
 		.callback = dmi_matched,
-		.ident = "ASUSTeK COMPUTER INC. Z550MA",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
-			DMI_MATCH(DMI_PRODUCT_NAME, "Z550MA"),
-		},
-		.driver_data = &quirk_no_rfkill,
-	},
-	{
-		.callback = dmi_matched,
 		.ident = "ASUSTeK COMPUTER INC. UX303UB",
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
-- 
2.11.0

[PATCH 4/8] Revert "asus-wmi: Add quirk_no_rfkill for the Asus U303LB"

[João Paulo Rechi Vita]

This reverts commit 02db9ff7af18f7ace60f430cbbaa1d846b350916.

Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com>
---
 drivers/platform/x86/asus-nb-wmi.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/platform/x86/asus-nb-wmi.c b/drivers/platform/x86/asus-nb-wmi.c
index 9d686d642934..5e088fbef5ad 100644
--- a/drivers/platform/x86/asus-nb-wmi.c
+++ b/drivers/platform/x86/asus-nb-wmi.c
@@ -382,15 +382,6 @@ static const struct dmi_system_id asus_quirks[] = {
 	},
 	{
 		.callback = dmi_matched,
-		.ident = "ASUSTeK COMPUTER INC. U303LB",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
-			DMI_MATCH(DMI_PRODUCT_NAME, "U303LB"),
-		},
-		.driver_data = &quirk_no_rfkill,
-	},
-	{
-		.callback = dmi_matched,
 		.ident = "ASUSTeK COMPUTER INC. UX303UB",
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
-- 
2.11.0

[PATCH 5/8] Revert "asus-wmi: Add quirk_no_rfkill for the Asus N552VW"

[João Paulo Rechi Vita]

This reverts commit 2d735244b798f0c8bf93ace1facfafdc1f7a4e6e.

Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com>
---
 drivers/platform/x86/asus-nb-wmi.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/platform/x86/asus-nb-wmi.c b/drivers/platform/x86/asus-nb-wmi.c
index 5e088fbef5ad..3a2fb29b1bc4 100644
--- a/drivers/platform/x86/asus-nb-wmi.c
+++ b/drivers/platform/x86/asus-nb-wmi.c
@@ -373,15 +373,6 @@ static const struct dmi_system_id asus_quirks[] = {
 	},
 	{
 		.callback = dmi_matched,
-		.ident = "ASUSTeK COMPUTER INC. N552VW",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
-			DMI_MATCH(DMI_PRODUCT_NAME, "N552VW"),
-		},
-		.driver_data = &quirk_no_rfkill,
-	},
-	{
-		.callback = dmi_matched,
 		.ident = "ASUSTeK COMPUTER INC. UX303UB",
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
-- 
2.11.0

[PATCH 6/8] Revert "asus-wmi: Create quirk for airplane_mode LED"

[João Paulo Rechi Vita]

This reverts commit a977e59c0c67c9d492bb16677ce66d67cae0ebd8.

Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com>
---
 drivers/platform/x86/asus-nb-wmi.c | 13 -------------
 drivers/platform/x86/asus-wmi.c    |  8 +++-----
 drivers/platform/x86/asus-wmi.h    |  1 -
 3 files changed, 3 insertions(+), 19 deletions(-)

diff --git a/drivers/platform/x86/asus-nb-wmi.c b/drivers/platform/x86/asus-nb-wmi.c
index 3a2fb29b1bc4..dea98ffb6f60 100644
--- a/drivers/platform/x86/asus-nb-wmi.c
+++ b/drivers/platform/x86/asus-nb-wmi.c
@@ -103,10 +103,6 @@ static struct quirk_entry quirk_asus_x200ca = {
 	.wapf = 2,
 };
 
-static struct quirk_entry quirk_no_rfkill = {
-	.no_rfkill = true,
-};
-
 static struct quirk_entry quirk_asus_ux303ub = {
 	.wmi_backlight_native = true,
 };
@@ -364,15 +360,6 @@ static const struct dmi_system_id asus_quirks[] = {
 	},
 	{
 		.callback = dmi_matched,
-		.ident = "ASUSTeK COMPUTER INC. X555UB",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
-			DMI_MATCH(DMI_PRODUCT_NAME, "X555UB"),
-		},
-		.driver_data = &quirk_no_rfkill,
-	},
-	{
-		.callback = dmi_matched,
 		.ident = "ASUSTeK COMPUTER INC. UX303UB",
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 43cb680adbb4..da3b12c131ce 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -2095,11 +2095,9 @@ static int asus_wmi_add(struct platform_device *pdev)
 	if (err)
 		goto fail_leds;
 
-	if (!asus->driver->quirks->no_rfkill) {
-		err = asus_wmi_rfkill_init(asus);
-		if (err)
-			goto fail_rfkill;
-	}
+	err = asus_wmi_rfkill_init(asus);
+	if (err)
+		goto fail_rfkill;
 
 	/* Some Asus desktop boards export an acpi-video backlight interface,
 	   stop this from showing up */
diff --git a/drivers/platform/x86/asus-wmi.h b/drivers/platform/x86/asus-wmi.h
index fdff626c3b51..c9589d9342bb 100644
--- a/drivers/platform/x86/asus-wmi.h
+++ b/drivers/platform/x86/asus-wmi.h
@@ -39,7 +39,6 @@ struct key_entry;
 struct asus_wmi;
 
 struct quirk_entry {
-	bool no_rfkill;
 	bool hotplug_wireless;
 	bool scalar_panel_brightness;
 	bool store_backlight_power;
-- 
2.11.0

[PATCH 7/8] asus-wireless: Export ids list

[João Paulo Rechi Vita]

Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com>
---
 drivers/platform/x86/asus-wireless.c | 15 ++++++---------
 drivers/platform/x86/asus-wireless.h | 10 ++++++++++
 2 files changed, 16 insertions(+), 9 deletions(-)
 create mode 100644 drivers/platform/x86/asus-wireless.h

diff --git a/drivers/platform/x86/asus-wireless.c b/drivers/platform/x86/asus-wireless.c
index 5a238fad35fb..fa28613688b4 100644
--- a/drivers/platform/x86/asus-wireless.c
+++ b/drivers/platform/x86/asus-wireless.c
@@ -17,6 +17,8 @@
 #include <linux/pci_ids.h>
 #include <linux/leds.h>
 
+#include "asus-wireless.h"
+
 #define ASUS_WIRELESS_LED_STATUS 0x2
 
 struct hswc_params {
@@ -40,12 +42,7 @@ static const struct hswc_params id_params[] = {
 	{ 0x5, 0x4 },
 };
 
-static const struct acpi_device_id device_ids[] = {
-	{"ATK4001", 0},
-	{"ATK4002", 0},
-	{"", 0},
-};
-MODULE_DEVICE_TABLE(acpi, device_ids);
+MODULE_DEVICE_TABLE(acpi, asus_wireless_ids);
 
 static u64 asus_wireless_method(acpi_handle handle, const char *method,
 				int param)
@@ -130,8 +127,8 @@ static int asus_wireless_add(struct acpi_device *adev)
 	adev->driver_data = data;
 
 	hid = acpi_device_hid(adev);
-	for (i = 0; strcmp(device_ids[i].id, ""); i++) {
-		if (!strcmp(device_ids[i].id, hid)) {
+	for (i = 0; strcmp(asus_wireless_ids[i].id, ""); i++) {
+		if (!strcmp(asus_wireless_ids[i].id, hid)) {
 			data->hswc_params = &id_params[i];
 			break;
 		}
@@ -179,7 +176,7 @@ static int asus_wireless_remove(struct acpi_device *adev)
 static struct acpi_driver asus_wireless_driver = {
 	.name = "Asus Wireless Radio Control Driver",
 	.class = "hotkey",
-	.ids = device_ids,
+	.ids = asus_wireless_ids,
 	.ops = {
 		.add = asus_wireless_add,
 		.remove = asus_wireless_remove,
diff --git a/drivers/platform/x86/asus-wireless.h b/drivers/platform/x86/asus-wireless.h
new file mode 100644
index 000000000000..10a345863da6
--- /dev/null
+++ b/drivers/platform/x86/asus-wireless.h
@@ -0,0 +1,10 @@
+#ifndef _ASUS_WIRELESS_H_
+#define _ASUS_WIRELESS_H_
+
+const struct acpi_device_id asus_wireless_ids[] = {
+	{"ATK4001", 0},
+	{"ATK4002", 0},
+	{"", 0},
+};
+
+#endif /* !_ASUS_WIRELESS_H_ */
-- 
2.11.0

[PATCH 8/8] asus-wmi: Don't register rfkill if ASHS and user bit are present

[João Paulo Rechi Vita]

Some Asus laptops that have an airplane-mode indicator LED, also have
the WMI WLAN user bit set, and the following bits in their DSDT:

    Scope (_SB)
    {
      (...)
      Device (ATKD)
      {
        (...)
        Method (WMNB, 3, Serialized)
        {
          (...)
          If (LEqual (IIA0, 0x00010002))
          {
            OWGD (IIA1)
            Return (One)
          }
        }
      }
    }

So when asus-wmi uses ASUS_WMI_DEVID_WLAN_LED (0x00010002) to store the
wlan state, it drives the airplane-mode indicator LED (through the call
to OWGD) in an inverted fashion: the LED is ON when airplane mode is OFF
(since wlan is ON), and vice-versa.

This skips registering RFKill switches at all for these laptops, to
allow the asus-wireless driver to drive the airplane mode LED correctly
through the ASHS ACPI device. Relying on the presence of ASHS and
ASUS_WMI_DSTS_USER_BIT avoids adding DMI-based quirks for at least 21
different laptops.

Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com>
---
 drivers/platform/x86/asus-wmi.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index da3b12c131ce..645204865a3f 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -49,6 +49,7 @@
 #include <linux/dmi.h>
 #include <acpi/video.h>
 
+#include "asus-wireless.h"
 #include "asus-wmi.h"
 
 MODULE_AUTHOR("Corentin Chary <corentin.chary@gmail.com>, "
@@ -2051,6 +2052,16 @@ static int asus_wmi_fan_init(struct asus_wmi *asus)
 	return 0;
 }
 
+static bool ashs_present(void)
+{
+	int i;
+
+	for (i = 0; strcmp(asus_wireless_ids[i].id, ""); i++)
+		if (acpi_dev_found(asus_wireless_ids[i].id))
+			return true;
+	return false;
+}
+
 /*
  * WMI Driver
  */
@@ -2095,9 +2106,15 @@ static int asus_wmi_add(struct platform_device *pdev)
 	if (err)
 		goto fail_leds;
 
-	err = asus_wmi_rfkill_init(asus);
-	if (err)
-		goto fail_rfkill;
+	asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_WLAN, &result);
+	if (result & (ASUS_WMI_DSTS_PRESENCE_BIT | ASUS_WMI_DSTS_USER_BIT))
+		asus->driver->wlan_ctrl_by_user = 1;
+
+	if (!(asus->driver->wlan_ctrl_by_user && ashs_present())) {
+		err = asus_wmi_rfkill_init(asus);
+		if (err)
+			goto fail_rfkill;
+	}
 
 	/* Some Asus desktop boards export an acpi-video backlight interface,
 	   stop this from showing up */
@@ -2132,10 +2149,6 @@ static int asus_wmi_add(struct platform_device *pdev)
 	if (err)
 		goto fail_debugfs;
 
-	asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_WLAN, &result);
-	if (result & (ASUS_WMI_DSTS_PRESENCE_BIT | ASUS_WMI_DSTS_USER_BIT))
-		asus->driver->wlan_ctrl_by_user = 1;
-
 	return 0;
 
 fail_debugfs:
-- 
2.11.0

Re: [PATCH 8/8] asus-wmi: Don't register rfkill if ASHS and user bit are present

[Joe Perches]

On Thu, 2017-01-26 at 10:30 -0500, João Paulo Rechi Vita wrote:
> Some Asus laptops that have an airplane-mode indicator LED, also have
> the WMI WLAN user bit set, and the following bits in their DSDT:

style trivia:

> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
[]
> @@ -2051,6 +2052,16 @@ static int asus_wmi_fan_init(struct asus_wmi *asus)
>  	return 0;
>  }
>  
> +static bool ashs_present(void)
> +{
> +	int i;
> +
> +	for (i = 0; strcmp(asus_wireless_ids[i].id, ""); i++)
> +		if (acpi_dev_found(asus_wireless_ids[i].id))
> +			return true;

The for loop would commonly have braces because of the
internal if.

Re: [PATCH 7/8] asus-wireless: Export ids list

[Andy Shevchenko]

On Thu, Jan 26, 2017 at 5:30 PM, João Paulo Rechi Vita
<jprvita@gmail.com> wrote:
> Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com>
> ---
>  drivers/platform/x86/asus-wireless.c | 15 ++++++---------
>  drivers/platform/x86/asus-wireless.h | 10 ++++++++++
>  2 files changed, 16 insertions(+), 9 deletions(-)
>  create mode 100644 drivers/platform/x86/asus-wireless.h
>
> diff --git a/drivers/platform/x86/asus-wireless.c b/drivers/platform/x86/asus-wireless.c
> index 5a238fad35fb..fa28613688b4 100644
> --- a/drivers/platform/x86/asus-wireless.c
> +++ b/drivers/platform/x86/asus-wireless.c
> @@ -17,6 +17,8 @@
>  #include <linux/pci_ids.h>
>  #include <linux/leds.h>
>
> +#include "asus-wireless.h"
> +
>  #define ASUS_WIRELESS_LED_STATUS 0x2
>
>  struct hswc_params {
> @@ -40,12 +42,7 @@ static const struct hswc_params id_params[] = {
>         { 0x5, 0x4 },
>  };
>
> -static const struct acpi_device_id device_ids[] = {
> -       {"ATK4001", 0},
> -       {"ATK4002", 0},
> -       {"", 0},
> -};
> -MODULE_DEVICE_TABLE(acpi, device_ids);
> +MODULE_DEVICE_TABLE(acpi, asus_wireless_ids);
>

No, Don't do this.

>  static u64 asus_wireless_method(acpi_handle handle, const char *method,
>                                 int param)
> @@ -130,8 +127,8 @@ static int asus_wireless_add(struct acpi_device *adev)
>         adev->driver_data = data;
>
>         hid = acpi_device_hid(adev);
> -       for (i = 0; strcmp(device_ids[i].id, ""); i++) {

This is wrong.

> -               if (!strcmp(device_ids[i].id, hid)) {
> +       for (i = 0; strcmp(asus_wireless_ids[i].id, ""); i++) {

This is too.

Potential infinite loop.

On top of that seems you just introduced this by previous patches and
changing here.
Often it means you need to reconsider how you actually split the
series on logical pieces.

> +               if (!strcmp(asus_wireless_ids[i].id, hid)) {
>                         data->hswc_params = &id_params[i];
>                         break;
>                 }
> @@ -179,7 +176,7 @@ static int asus_wireless_remove(struct acpi_device *adev)
>  static struct acpi_driver asus_wireless_driver = {
>         .name = "Asus Wireless Radio Control Driver",
>         .class = "hotkey",
> -       .ids = device_ids,
> +       .ids = asus_wireless_ids,
>         .ops = {
>                 .add = asus_wireless_add,
>                 .remove = asus_wireless_remove,
> diff --git a/drivers/platform/x86/asus-wireless.h b/drivers/platform/x86/asus-wireless.h
> new file mode 100644
> index 000000000000..10a345863da6
> --- /dev/null
> +++ b/drivers/platform/x86/asus-wireless.h
> @@ -0,0 +1,10 @@
> +#ifndef _ASUS_WIRELESS_H_
> +#define _ASUS_WIRELESS_H_
> +
> +const struct acpi_device_id asus_wireless_ids[] = {
> +       {"ATK4001", 0},
> +       {"ATK4002", 0},
> +       {"", 0},
> +};
> +
> +#endif /* !_ASUS_WIRELESS_H_ */
> --
> 2.11.0
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/8] asus-wmi: Get rid of quirk_no_rfkill{,_wapf4}

[Andy Shevchenko]

On Thu, Jan 26, 2017 at 5:30 PM, João Paulo Rechi Vita
<jprvita@gmail.com> wrote:
> This quirk avoids the conflicting usage of ASUS_WMI_DEVID_WLAN_LED (0x00010002)
> by both asus-wireless and asus-wmi on machines where the BIOS can't set and
> record the wlan status (wlan_ctrl_by_user in asus-wmi.c). At the moment we have
> six models using that quirk upstream, but there are another fifteen for which
> we have downstream patches, and will send upstream unless we find a more
> dynamic solution. We also expect this list to continue growing.
>
> This series makes use of ASUS_WMI_DSTS_USER_BIT plus the status of the ASHS
> device to skip asus_wmi_rfkill_init(). Since asus-wmi now needs to know the ASHS
> _HIDs, I have moved device_ids[] to a new header file, asus-wireless.h. I'm not
> sure if there are any best practices in the kernel community against having one
> module including headers from a different module, but I wanted to avoid
> duplicating this list or creating an ordering dependency between the modules.
> Any advice on alternatives here are welcome.
>
> PS: I don't think asus-wireless.h needs to also be listed in MAINTAINERS,
> please correct me if I'm wrong.
>
> João Paulo Rechi Vita (8):

>   Revert "asus-wmi: Add quirk_no_rfkill_wapf4 for the Asus X456UA"
>   Revert "asus-wmi: Add quirk_no_rfkill_wapf4 for the Asus X456UF"
>   Revert "asus-wmi: Add quirk_no_rfkill for the Asus Z550MA"
>   Revert "asus-wmi: Add quirk_no_rfkill for the Asus U303LB"
>   Revert "asus-wmi: Add quirk_no_rfkill for the Asus N552VW"
>   Revert "asus-wmi: Create quirk for airplane_mode LED"

Don't do this.
Each of them will break the working case -> means regression on user experience.

>   asus-wireless: Export ids list
>   asus-wmi: Don't register rfkill if ASHS and user bit are present
>
>  drivers/platform/x86/asus-nb-wmi.c   | 49 ++----------------------------------
>  drivers/platform/x86/asus-wireless.c | 15 +++++------
>  drivers/platform/x86/asus-wireless.h | 10 ++++++++
>  drivers/platform/x86/asus-wmi.c      | 21 ++++++++++++----
>  drivers/platform/x86/asus-wmi.h      |  1 -
>  5 files changed, 34 insertions(+), 62 deletions(-)
>  create mode 100644 drivers/platform/x86/asus-wireless.h
>
> --
> 2.11.0
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 8/8] asus-wmi: Don't register rfkill if ASHS and user bit are present

[João Paulo Rechi Vita]

On 26 January 2017 at 17:45, Joe Perches <joe@perches.com> wrote:
> On Thu, 2017-01-26 at 10:30 -0500, João Paulo Rechi Vita wrote:
>> Some Asus laptops that have an airplane-mode indicator LED, also have
>> the WMI WLAN user bit set, and the following bits in their DSDT:
>
> style trivia:
>
>> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> []
>> @@ -2051,6 +2052,16 @@ static int asus_wmi_fan_init(struct asus_wmi *asus)
>>       return 0;
>>  }
>>
>> +static bool ashs_present(void)
>> +{
>> +     int i;
>> +
>> +     for (i = 0; strcmp(asus_wireless_ids[i].id, ""); i++)
>> +             if (acpi_dev_found(asus_wireless_ids[i].id))
>> +                     return true;
>
> The for loop would commonly have braces because of the
> internal if.
>

Thanks, I'm fixing it for the next revision.

--
João Paulo Rechi Vita
http://about.me/jprvita

Re: [PATCH 8/8] asus-wmi: Don't register rfkill if ASHS and user bit are present

[João Paulo Rechi Vita]

On 26 January 2017 at 17:45, Joe Perches <joe@perches.com> wrote:
> On Thu, 2017-01-26 at 10:30 -0500, João Paulo Rechi Vita wrote:
>> Some Asus laptops that have an airplane-mode indicator LED, also have
>> the WMI WLAN user bit set, and the following bits in their DSDT:
>
> style trivia:
>
>> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> []
>> @@ -2051,6 +2052,16 @@ static int asus_wmi_fan_init(struct asus_wmi *asus)
>>       return 0;
>>  }
>>
>> +static bool ashs_present(void)
>> +{
>> +     int i;
>> +
>> +     for (i = 0; strcmp(asus_wireless_ids[i].id, ""); i++)
>> +             if (acpi_dev_found(asus_wireless_ids[i].id))
>> +                     return true;
>
> The for loop would commonly have braces because of the
> internal if.
>

Thanks, I'm fixing it for the next revision.

Re: [PATCH 0/8] asus-wmi: Get rid of quirk_no_rfkill{,_wapf4}

[João Paulo Rechi Vita]

On 27 January 2017 at 10:37, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Thu, Jan 26, 2017 at 5:30 PM, João Paulo Rechi Vita
> <jprvita@gmail.com> wrote:
>> This quirk avoids the conflicting usage of ASUS_WMI_DEVID_WLAN_LED (0x00010002)
>> by both asus-wireless and asus-wmi on machines where the BIOS can't set and
>> record the wlan status (wlan_ctrl_by_user in asus-wmi.c). At the moment we have
>> six models using that quirk upstream, but there are another fifteen for which
>> we have downstream patches, and will send upstream unless we find a more
>> dynamic solution. We also expect this list to continue growing.
>>
>> This series makes use of ASUS_WMI_DSTS_USER_BIT plus the status of the ASHS
>> device to skip asus_wmi_rfkill_init(). Since asus-wmi now needs to know the ASHS
>> _HIDs, I have moved device_ids[] to a new header file, asus-wireless.h. I'm not
>> sure if there are any best practices in the kernel community against having one
>> module including headers from a different module, but I wanted to avoid
>> duplicating this list or creating an ordering dependency between the modules.
>> Any advice on alternatives here are welcome.
>>
>> PS: I don't think asus-wireless.h needs to also be listed in MAINTAINERS,
>> please correct me if I'm wrong.
>>
>> João Paulo Rechi Vita (8):
>
>>   Revert "asus-wmi: Add quirk_no_rfkill_wapf4 for the Asus X456UA"
>>   Revert "asus-wmi: Add quirk_no_rfkill_wapf4 for the Asus X456UF"
>>   Revert "asus-wmi: Add quirk_no_rfkill for the Asus Z550MA"
>>   Revert "asus-wmi: Add quirk_no_rfkill for the Asus U303LB"
>>   Revert "asus-wmi: Add quirk_no_rfkill for the Asus N552VW"
>>   Revert "asus-wmi: Create quirk for airplane_mode LED"
>
> Don't do this.
> Each of them will break the working case -> means regression on user experience.
>

They are being substituted by a generic fix implemented by the
following two  commits, so the user experience will not change at all
after the whole series get merged.

>>   asus-wireless: Export ids list
>>   asus-wmi: Don't register rfkill if ASHS and user bit are present
>>

Or your point is that these two should come before the reverts? I
thought patchsets were looked at as an unit from the user experience /
functionally perspective. Can you please advise on what is the correct
approach here?

Thanks again for your review on this series.

--
João Paulo Rechi Vita
http://about.me/jprvita

Re: [PATCH 0/8] asus-wmi: Get rid of quirk_no_rfkill{,_wapf4}

[João Paulo Rechi Vita]

On 27 January 2017 at 10:37, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Thu, Jan 26, 2017 at 5:30 PM, João Paulo Rechi Vita
> <jprvita@gmail.com> wrote:
>> This quirk avoids the conflicting usage of ASUS_WMI_DEVID_WLAN_LED (0x00010002)
>> by both asus-wireless and asus-wmi on machines where the BIOS can't set and
>> record the wlan status (wlan_ctrl_by_user in asus-wmi.c). At the moment we have
>> six models using that quirk upstream, but there are another fifteen for which
>> we have downstream patches, and will send upstream unless we find a more
>> dynamic solution. We also expect this list to continue growing.
>>
>> This series makes use of ASUS_WMI_DSTS_USER_BIT plus the status of the ASHS
>> device to skip asus_wmi_rfkill_init(). Since asus-wmi now needs to know the ASHS
>> _HIDs, I have moved device_ids[] to a new header file, asus-wireless.h. I'm not
>> sure if there are any best practices in the kernel community against having one
>> module including headers from a different module, but I wanted to avoid
>> duplicating this list or creating an ordering dependency between the modules.
>> Any advice on alternatives here are welcome.
>>
>> PS: I don't think asus-wireless.h needs to also be listed in MAINTAINERS,
>> please correct me if I'm wrong.
>>
>> João Paulo Rechi Vita (8):
>
>>   Revert "asus-wmi: Add quirk_no_rfkill_wapf4 for the Asus X456UA"
>>   Revert "asus-wmi: Add quirk_no_rfkill_wapf4 for the Asus X456UF"
>>   Revert "asus-wmi: Add quirk_no_rfkill for the Asus Z550MA"
>>   Revert "asus-wmi: Add quirk_no_rfkill for the Asus U303LB"
>>   Revert "asus-wmi: Add quirk_no_rfkill for the Asus N552VW"
>>   Revert "asus-wmi: Create quirk for airplane_mode LED"
>
> Don't do this.
> Each of them will break the working case -> means regression on user experience.
>

They are being substituted by a generic fix implemented by the
following two  commits, so the user experience will not change at all
after the whole series get merged.

>>   asus-wireless: Export ids list
>>   asus-wmi: Don't register rfkill if ASHS and user bit are present
>>

Or your point is that these two should come before the reverts? I
thought patchsets were looked at as an unit from the user experience /
functionally perspective. Can you please advise on what is the correct
approach here?

Thanks again for your review on this series.

Re: [PATCH 7/8] asus-wireless: Export ids list

[João Paulo Rechi Vita]

On 27 January 2017 at 10:36, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Thu, Jan 26, 2017 at 5:30 PM, João Paulo Rechi Vita
> <jprvita@gmail.com> wrote:
>> Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com>
>> ---
>>  drivers/platform/x86/asus-wireless.c | 15 ++++++---------
>>  drivers/platform/x86/asus-wireless.h | 10 ++++++++++
>>  2 files changed, 16 insertions(+), 9 deletions(-)
>>  create mode 100644 drivers/platform/x86/asus-wireless.h
>>
>> diff --git a/drivers/platform/x86/asus-wireless.c b/drivers/platform/x86/asus-wireless.c
>> index 5a238fad35fb..fa28613688b4 100644
>> --- a/drivers/platform/x86/asus-wireless.c
>> +++ b/drivers/platform/x86/asus-wireless.c
>> @@ -17,6 +17,8 @@
>>  #include <linux/pci_ids.h>
>>  #include <linux/leds.h>
>>
>> +#include "asus-wireless.h"
>> +
>>  #define ASUS_WIRELESS_LED_STATUS 0x2
>>
>>  struct hswc_params {
>> @@ -40,12 +42,7 @@ static const struct hswc_params id_params[] = {
>>         { 0x5, 0x4 },
>>  };
>>
>> -static const struct acpi_device_id device_ids[] = {
>> -       {"ATK4001", 0},
>> -       {"ATK4002", 0},
>> -       {"", 0},
>> -};
>> -MODULE_DEVICE_TABLE(acpi, device_ids);
>> +MODULE_DEVICE_TABLE(acpi, asus_wireless_ids);
>>
>
> No, Don't do this.
>

Why?

>>  static u64 asus_wireless_method(acpi_handle handle, const char *method,
>>                                 int param)
>> @@ -130,8 +127,8 @@ static int asus_wireless_add(struct acpi_device *adev)
>>         adev->driver_data = data;
>>
>>         hid = acpi_device_hid(adev);
>> -       for (i = 0; strcmp(device_ids[i].id, ""); i++) {
>
> This is wrong.
>
>> -               if (!strcmp(device_ids[i].id, hid)) {
>> +       for (i = 0; strcmp(asus_wireless_ids[i].id, ""); i++) {
>
> This is too.
>
> Potential infinite loop.
>
> On top of that seems you just introduced this by previous patches and
> changing here.
> Often it means you need to reconsider how you actually split the
> series on logical pieces.
>

Can you please elaborate a bit more? All this change does is to change
the name of the variable being iterated in the loop. As you said, the
loop was introduced in a previous series, and you didn't spot any
problems there. I don't think it makes sense to also rename the
variable in that other series, since I'm only renaming it because I'm
moving it to a header so it can be used by asus-wmi.c as well.

>> +               if (!strcmp(asus_wireless_ids[i].id, hid)) {
>>                         data->hswc_params = &id_params[i];
>>                         break;
>>                 }
>> @@ -179,7 +176,7 @@ static int asus_wireless_remove(struct acpi_device *adev)
>>  static struct acpi_driver asus_wireless_driver = {
>>         .name = "Asus Wireless Radio Control Driver",
>>         .class = "hotkey",
>> -       .ids = device_ids,
>> +       .ids = asus_wireless_ids,
>>         .ops = {
>>                 .add = asus_wireless_add,
>>                 .remove = asus_wireless_remove,
>> diff --git a/drivers/platform/x86/asus-wireless.h b/drivers/platform/x86/asus-wireless.h
>> new file mode 100644
>> index 000000000000..10a345863da6
>> --- /dev/null
>> +++ b/drivers/platform/x86/asus-wireless.h
>> @@ -0,0 +1,10 @@
>> +#ifndef _ASUS_WIRELESS_H_
>> +#define _ASUS_WIRELESS_H_
>> +
>> +const struct acpi_device_id asus_wireless_ids[] = {
>> +       {"ATK4001", 0},
>> +       {"ATK4002", 0},
>> +       {"", 0},
>> +};
>> +
>> +#endif /* !_ASUS_WIRELESS_H_ */
>> --
>> 2.11.0
>>
>
>
>
> --
> With Best Regards,
> Andy Shevchenko


--
João Paulo Rechi Vita
http://about.me/jprvita

Re: [PATCH 7/8] asus-wireless: Export ids list

[João Paulo Rechi Vita]

On 27 January 2017 at 10:36, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Thu, Jan 26, 2017 at 5:30 PM, João Paulo Rechi Vita
> <jprvita@gmail.com> wrote:
>> Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com>
>> ---
>>  drivers/platform/x86/asus-wireless.c | 15 ++++++---------
>>  drivers/platform/x86/asus-wireless.h | 10 ++++++++++
>>  2 files changed, 16 insertions(+), 9 deletions(-)
>>  create mode 100644 drivers/platform/x86/asus-wireless.h
>>
>> diff --git a/drivers/platform/x86/asus-wireless.c b/drivers/platform/x86/asus-wireless.c
>> index 5a238fad35fb..fa28613688b4 100644
>> --- a/drivers/platform/x86/asus-wireless.c
>> +++ b/drivers/platform/x86/asus-wireless.c
>> @@ -17,6 +17,8 @@
>>  #include <linux/pci_ids.h>
>>  #include <linux/leds.h>
>>
>> +#include "asus-wireless.h"
>> +
>>  #define ASUS_WIRELESS_LED_STATUS 0x2
>>
>>  struct hswc_params {
>> @@ -40,12 +42,7 @@ static const struct hswc_params id_params[] = {
>>         { 0x5, 0x4 },
>>  };
>>
>> -static const struct acpi_device_id device_ids[] = {
>> -       {"ATK4001", 0},
>> -       {"ATK4002", 0},
>> -       {"", 0},
>> -};
>> -MODULE_DEVICE_TABLE(acpi, device_ids);
>> +MODULE_DEVICE_TABLE(acpi, asus_wireless_ids);
>>
>
> No, Don't do this.
>

Why?

>>  static u64 asus_wireless_method(acpi_handle handle, const char *method,
>>                                 int param)
>> @@ -130,8 +127,8 @@ static int asus_wireless_add(struct acpi_device *adev)
>>         adev->driver_data = data;
>>
>>         hid = acpi_device_hid(adev);
>> -       for (i = 0; strcmp(device_ids[i].id, ""); i++) {
>
> This is wrong.
>
>> -               if (!strcmp(device_ids[i].id, hid)) {
>> +       for (i = 0; strcmp(asus_wireless_ids[i].id, ""); i++) {
>
> This is too.
>
> Potential infinite loop.
>
> On top of that seems you just introduced this by previous patches and
> changing here.
> Often it means you need to reconsider how you actually split the
> series on logical pieces.
>

Can you please elaborate a bit more? All this change does is to change
the name of the variable being iterated in the loop. As you said, the
loop was introduced in a previous series, and you didn't spot any
problems there. I don't think it makes sense to also rename the
variable in that other series, since I'm only renaming it because I'm
moving it to a header so it can be used by asus-wmi.c as well.

>> +               if (!strcmp(asus_wireless_ids[i].id, hid)) {
>>                         data->hswc_params = &id_params[i];
>>                         break;
>>                 }
>> @@ -179,7 +176,7 @@ static int asus_wireless_remove(struct acpi_device *adev)
>>  static struct acpi_driver asus_wireless_driver = {
>>         .name = "Asus Wireless Radio Control Driver",
>>         .class = "hotkey",
>> -       .ids = device_ids,
>> +       .ids = asus_wireless_ids,
>>         .ops = {
>>                 .add = asus_wireless_add,
>>                 .remove = asus_wireless_remove,
>> diff --git a/drivers/platform/x86/asus-wireless.h b/drivers/platform/x86/asus-wireless.h
>> new file mode 100644
>> index 000000000000..10a345863da6
>> --- /dev/null
>> +++ b/drivers/platform/x86/asus-wireless.h
>> @@ -0,0 +1,10 @@
>> +#ifndef _ASUS_WIRELESS_H_
>> +#define _ASUS_WIRELESS_H_
>> +
>> +const struct acpi_device_id asus_wireless_ids[] = {
>> +       {"ATK4001", 0},
>> +       {"ATK4002", 0},
>> +       {"", 0},
>> +};
>> +
>> +#endif /* !_ASUS_WIRELESS_H_ */
>> --
>> 2.11.0
>>
>
>
>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH 0/8] asus-wmi: Get rid of quirk_no_rfkill{,_wapf4}

[Andy Shevchenko]

On Wed, Feb 1, 2017 at 2:21 PM, João Paulo Rechi Vita <jprvita@gmail.com> wrote:
> On 27 January 2017 at 10:37, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>> On Thu, Jan 26, 2017 at 5:30 PM, João Paulo Rechi Vita
>> <jprvita@gmail.com> wrote:

>>>   Revert "asus-wmi: Add quirk_no_rfkill_wapf4 for the Asus X456UA"
>>>   Revert "asus-wmi: Add quirk_no_rfkill_wapf4 for the Asus X456UF"
>>>   Revert "asus-wmi: Add quirk_no_rfkill for the Asus Z550MA"
>>>   Revert "asus-wmi: Add quirk_no_rfkill for the Asus U303LB"
>>>   Revert "asus-wmi: Add quirk_no_rfkill for the Asus N552VW"
>>>   Revert "asus-wmi: Create quirk for airplane_mode LED"
>>
>> Don't do this.
>> Each of them will break the working case -> means regression on user experience.
>>
>
> They are being substituted by a generic fix implemented by the
> following two  commits, so the user experience will not change at all
> after the whole series get merged.
>
>>>   asus-wireless: Export ids list
>>>   asus-wmi: Don't register rfkill if ASHS and user bit are present
>>>
>
> Or your point is that these two should come before the reverts? I
> thought patchsets were looked at as an unit from the user experience /
> functionally perspective. Can you please advise on what is the correct
> approach here?

You prepare entire new infrastructure and then you have two options:
1. Remove old stuff and apply new use in one patch
2. Replace old calls by new ones in one patch, and remove not used
anymore old stuff in the other patch.

Choose whatever suits better.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 7/8] asus-wireless: Export ids list

[Andy Shevchenko]

On Wed, Feb 1, 2017 at 2:23 PM, João Paulo Rechi Vita <jprvita@gmail.com> wrote:
> On 27 January 2017 at 10:36, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>> On Thu, Jan 26, 2017 at 5:30 PM, João Paulo Rechi Vita
>> <jprvita@gmail.com> wrote:

Fill commit message, btw.

>>> Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com>

>>> -static const struct acpi_device_id device_ids[] = {
>>> -       {"ATK4001", 0},
>>> -       {"ATK4002", 0},
>>> -       {"", 0},
>>> -};
>>> -MODULE_DEVICE_TABLE(acpi, device_ids);
>>> +MODULE_DEVICE_TABLE(acpi, asus_wireless_ids);
>>>
>>
>> No, Don't do this.
>>
>
> Why?

Table is a property of certain driver. You make it visible to parts
that non need it.
Moreover, you may here the list itself non-explicit, which reduces
readability and understandability worse.

If you would like to maintain a list of devices in two
(semi-)independent modules, it would be not good looking in any case,
either you make a hard dependency (if they already are it's okay to
just export a function which helps you to find an ID in the list), or
copy it in both modules.

I need to check this as well.

>>>  static u64 asus_wireless_method(acpi_handle handle, const char *method,
>>>                                 int param)
>>> @@ -130,8 +127,8 @@ static int asus_wireless_add(struct acpi_device *adev)
>>>         adev->driver_data = data;
>>>
>>>         hid = acpi_device_hid(adev);
>>> -       for (i = 0; strcmp(device_ids[i].id, ""); i++) {
>>
>> This is wrong.
>>
>>> -               if (!strcmp(device_ids[i].id, hid)) {
>>> +       for (i = 0; strcmp(asus_wireless_ids[i].id, ""); i++) {
>>
>> This is too.
>>
>> Potential infinite loop.
>>
>> On top of that seems you just introduced this by previous patches and
>> changing here.
>> Often it means you need to reconsider how you actually split the
>> series on logical pieces.
>>
>
> Can you please elaborate a bit more?

The original code relies on "" in the first parameter which basically
can be NULL. This fragile.
But this is part subject to change in a sequential patch.

> All this change does is to change
> the name of the variable being iterated in the loop. As you said, the
> loop was introduced in a previous series, and you didn't spot any
> problems there.

If I didn't spot it doesn't mean there is no issues, right? ;)

> I don't think it makes sense to also rename the
> variable in that other series, since I'm only renaming it because I'm
> moving it to a header so it can be used by asus-wmi.c as well.

The main problem with the above is introduction something that you are
changing soon later.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 7/8] asus-wireless: Export ids list

[João Paulo Rechi Vita]

On 4 February 2017 at 10:35, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Wed, Feb 1, 2017 at 2:23 PM, João Paulo Rechi Vita <jprvita@gmail.com> wrote:
>> On 27 January 2017 at 10:36, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>> On Thu, Jan 26, 2017 at 5:30 PM, João Paulo Rechi Vita
>>> <jprvita@gmail.com> wrote:
>
> Fill commit message, btw.
>
>>>> Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com>
>
>>>> -static const struct acpi_device_id device_ids[] = {
>>>> -       {"ATK4001", 0},
>>>> -       {"ATK4002", 0},
>>>> -       {"", 0},
>>>> -};
>>>> -MODULE_DEVICE_TABLE(acpi, device_ids);
>>>> +MODULE_DEVICE_TABLE(acpi, asus_wireless_ids);
>>>>
>>>
>>> No, Don't do this.
>>>
>>
>> Why?
>
> Table is a property of certain driver. You make it visible to parts
> that non need it.
> Moreover, you may here the list itself non-explicit, which reduces
> readability and understandability worse.
>
> If you would like to maintain a list of devices in two
> (semi-)independent modules, it would be not good looking in any case,
> either you make a hard dependency (if they already are it's okay to
> just export a function which helps you to find an ID in the list), or
> copy it in both modules.
>
> I need to check this as well.
>

Currently these modules do not depend on each other. There are
machines which only need asus-wmi, and not asus-wireless, and there
may be machines in the future which will only need asus-wireless and
not asus-wmi (not really seen any in that situation, but it is a
possibility), so I don't think adding a dependency here is the right
thing.

Duplicating code is not good, so I was trying to avoid it, but maybe
there isn't really any better way in this case.

>>>>  static u64 asus_wireless_method(acpi_handle handle, const char *method,
>>>>                                 int param)
>>>> @@ -130,8 +127,8 @@ static int asus_wireless_add(struct acpi_device *adev)
>>>>         adev->driver_data = data;
>>>>
>>>>         hid = acpi_device_hid(adev);
>>>> -       for (i = 0; strcmp(device_ids[i].id, ""); i++) {
>>>
>>> This is wrong.
>>>
>>>> -               if (!strcmp(device_ids[i].id, hid)) {
>>>> +       for (i = 0; strcmp(asus_wireless_ids[i].id, ""); i++) {
>>>
>>> This is too.
>>>
>>> Potential infinite loop.
>>>
>>> On top of that seems you just introduced this by previous patches and
>>> changing here.
>>> Often it means you need to reconsider how you actually split the
>>> series on logical pieces.
>>>
>>
>> Can you please elaborate a bit more?
>
> The original code relies on "" in the first parameter which basically
> can be NULL. This fragile.
> But this is part subject to change in a sequential patch.
>
>> All this change does is to change
>> the name of the variable being iterated in the loop. As you said, the
>> loop was introduced in a previous series, and you didn't spot any
>> problems there.
>
> If I didn't spot it doesn't mean there is no issues, right? ;)
>
>> I don't think it makes sense to also rename the
>> variable in that other series, since I'm only renaming it because I'm
>> moving it to a header so it can be used by asus-wmi.c as well.
>
> The main problem with the above is introduction something that you are
> changing soon later.
>

Ok, I should probably have sent this as RFC, as it was actually the
case. But since I'm not going to export the ids list anymore, this
patch will be dropped from the next revision.

--
João Paulo Rechi Vita
http://about.me/jprvita

Re: [PATCH 7/8] asus-wireless: Export ids list

[João Paulo Rechi Vita]

On 4 February 2017 at 10:35, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Wed, Feb 1, 2017 at 2:23 PM, João Paulo Rechi Vita <jprvita@gmail.com> wrote:
>> On 27 January 2017 at 10:36, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>> On Thu, Jan 26, 2017 at 5:30 PM, João Paulo Rechi Vita
>>> <jprvita@gmail.com> wrote:
>
> Fill commit message, btw.
>
>>>> Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com>
>
>>>> -static const struct acpi_device_id device_ids[] = {
>>>> -       {"ATK4001", 0},
>>>> -       {"ATK4002", 0},
>>>> -       {"", 0},
>>>> -};
>>>> -MODULE_DEVICE_TABLE(acpi, device_ids);
>>>> +MODULE_DEVICE_TABLE(acpi, asus_wireless_ids);
>>>>
>>>
>>> No, Don't do this.
>>>
>>
>> Why?
>
> Table is a property of certain driver. You make it visible to parts
> that non need it.
> Moreover, you may here the list itself non-explicit, which reduces
> readability and understandability worse.
>
> If you would like to maintain a list of devices in two
> (semi-)independent modules, it would be not good looking in any case,
> either you make a hard dependency (if they already are it's okay to
> just export a function which helps you to find an ID in the list), or
> copy it in both modules.
>
> I need to check this as well.
>

Currently these modules do not depend on each other. There are
machines which only need asus-wmi, and not asus-wireless, and there
may be machines in the future which will only need asus-wireless and
not asus-wmi (not really seen any in that situation, but it is a
possibility), so I don't think adding a dependency here is the right
thing.

Duplicating code is not good, so I was trying to avoid it, but maybe
there isn't really any better way in this case.

>>>>  static u64 asus_wireless_method(acpi_handle handle, const char *method,
>>>>                                 int param)
>>>> @@ -130,8 +127,8 @@ static int asus_wireless_add(struct acpi_device *adev)
>>>>         adev->driver_data = data;
>>>>
>>>>         hid = acpi_device_hid(adev);
>>>> -       for (i = 0; strcmp(device_ids[i].id, ""); i++) {
>>>
>>> This is wrong.
>>>
>>>> -               if (!strcmp(device_ids[i].id, hid)) {
>>>> +       for (i = 0; strcmp(asus_wireless_ids[i].id, ""); i++) {
>>>
>>> This is too.
>>>
>>> Potential infinite loop.
>>>
>>> On top of that seems you just introduced this by previous patches and
>>> changing here.
>>> Often it means you need to reconsider how you actually split the
>>> series on logical pieces.
>>>
>>
>> Can you please elaborate a bit more?
>
> The original code relies on "" in the first parameter which basically
> can be NULL. This fragile.
> But this is part subject to change in a sequential patch.
>
>> All this change does is to change
>> the name of the variable being iterated in the loop. As you said, the
>> loop was introduced in a previous series, and you didn't spot any
>> problems there.
>
> If I didn't spot it doesn't mean there is no issues, right? ;)
>
>> I don't think it makes sense to also rename the
>> variable in that other series, since I'm only renaming it because I'm
>> moving it to a header so it can be used by asus-wmi.c as well.
>
> The main problem with the above is introduction something that you are
> changing soon later.
>

Ok, I should probably have sent this as RFC, as it was actually the
case. But since I'm not going to export the ids list anymore, this
patch will be dropped from the next revision.