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]

Date:   Sat, 4 Feb 2017 17:35:03 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     João Paulo Rechi Vita <jprvita@...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 Wed, Feb 1, 2017 at 2:23 PM, João Paulo Rechi Vita <jprvita@...il.com> wrote:
> 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:

Fill commit message, btw.

>>> Signed-off-by: João Paulo Rechi Vita <jprvita@...lessm.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