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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+A7VXU3zNm2qHaM2hnaJ0GamrL5c0zCKC-MLgPZ+9M_DyKKkQ@mail.gmail.com>
Date:   Wed, 1 Feb 2017 07:23:45 -0500
From:   João Paulo Rechi Vita <jprvita@...il.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Corentin Chary <corentin.chary@...il.com>,
        Darren Hart <dvhart@...radead.org>,
        Platform Driver <platform-driver-x86@...r.kernel.org>,
        acpi4asus-user <acpi4asus-user@...ts.sourceforge.net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        linux@...lessm.com,
        João Paulo Rechi Vita <jprvita@...lessm.com>
Subject: Re: [PATCH 7/8] asus-wireless: Export ids list

On 27 January 2017 at 10:36, Andy Shevchenko <andy.shevchenko@...il.com> wrote:
> On Thu, Jan 26, 2017 at 5:30 PM, João Paulo Rechi Vita
> <jprvita@...il.com> wrote:
>> Signed-off-by: João Paulo Rechi Vita <jprvita@...lessm.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