Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <0033b40f-f583-c3a4-ce87-73cbb362537b@redhat.com>
Date:   Sat, 3 Sep 2022 13:12:31 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.12.0
Subject: Re: [PATCH v2] platform/x86: wmi: Allow duplicate GUIDs for drivers
 that use struct wmi_driver
Content-Language: en-US
To:     Mario Limonciello <mario.limonciello@amd.com>
Cc:     Mark Gross <markgross@kernel.org>,
        Platform Driver <platform-driver-x86@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Andy Shevchenko <andy.shevchenko@gmail.com>
References: <20220829201500.6341-1-mario.limonciello@amd.com>
 <CAHp75Vc3v4a6=ZJnOYYFGHEomExxopuUy8axDL=M2tbxHqtXqQ@mail.gmail.com>
 <MN0PR12MB6101441AA29CE815DCAD26AEE27B9@MN0PR12MB6101.namprd12.prod.outlook.com>
 <3df6405a-924a-b0e6-c879-05b9151011e3@redhat.com>
 <168cb996-513e-77a8-b895-ef0fdd0c1793@amd.com>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <168cb996-513e-77a8-b895-ef0fdd0c1793@amd.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi,

On 9/2/22 14:19, Mario Limonciello wrote:
> On 9/2/22 03:07, Hans de Goede wrote:
>> Hi,
>>
>> On 9/1/22 23:39, Limonciello, Mario wrote:
>>> [Public]
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Andy Shevchenko <andy.shevchenko@gmail.com>
>>>> Sent: Thursday, September 1, 2022 12:17
>>>> To: Limonciello, Mario <Mario.Limonciello@amd.com>
>>>> Cc: Hans de Goede <hdegoede@redhat.com>; Mark Gross
>>>> <markgross@kernel.org>; Platform Driver <platform-driver-
>>>> x86@vger.kernel.org>; Linux Kernel Mailing List <linux-
>>>> kernel@vger.kernel.org>
>>>> Subject: Re: [PATCH v2] platform/x86: wmi: Allow duplicate GUIDs for drivers
>>>> that use struct wmi_driver
>>>>
>>>> On Mon, Aug 29, 2022 at 11:20 PM Mario Limonciello
>>>> <mario.limonciello@amd.com> wrote:
>>>>>
>>>>> The WMI subsystem in the kernel currently tracks WMI devices by
>>>>> a GUID string not by ACPI device.  The GUID used by the `wmi-bmof`
>>>>> module however is available from many devices on nearly every machine.
>>>>>
>>>>> This originally was though to be a bug, but as it happens on most
>>>>
>>>> thought
>>>>
>>>>> machines it is a design mistake.  It has been fixed by tying an ACPI
>>>>> device to the driver with struct wmi_driver. So drivers that have
>>>>> moved over to struct wmi_driver can actually support multiple
>>>>> instantiations of a GUID without any problem.
>>>>>
>>>>> Add an allow list into wmi.c for GUIDs that the drivers that are known
>>>>> to use struct wmi_driver.  The list is populated with `wmi-bmof` right
>>>>> now. The additional instances of that in sysfs with be suffixed with -%d
>>>>
>>>> ...
>>>>
>>>>> +/* allow duplicate GUIDs as these device drivers use struct wmi_driver */
>>>>> +static const char * const allow_duplicates[] = {
>>>>> +       "05901221-D566-11D1-B2F0-00A0C9062910", /* wmi-bmof */
>>>>> +       NULL,
>>>>
>>>> No comma for the terminator.
>>>>
>>>>> +};
>>>>
>>>> ...
>>>>
>>>>> +static int guid_count(const guid_t *guid)
>>>>> +{
>>>>> +       struct wmi_block *wblock;
>>>>> +       int count = 0;
>>>>> +
>>>>> +       list_for_each_entry(wblock, &wmi_block_list, list) {
>>>>> +               if (guid_equal(&wblock->gblock.guid, guid))
>>>>> +                       count++;
>>>>> +       }
>>>>> +
>>>>> +       return count;
>>>>> +}
>>>>
>>>> I haven't deeply checked the code, but this kind of approach is
>>>> fragile and proven to be error prone as shown in practice. The
>>>> scenario is (again, not sure if it's possible, need a comment in the
>>>> code if it's not possible) removing an entry from the list in the
>>>> middle and trying to add it again. you will see the duplicate count
>>>> values. That's why in the general case we use IDA or similar
>>>> approaches.
>>>
>>> It shouldn't be possible to add/remove from the list, they're fixed
>>> lists that were parsed from _WDG.
>>>
>>> Hans - since you already took this into your review queue, can you
>>> land fixes for the 3 things Andy pointed out before it goes to -next
>>> or do you want me to do a manual follow up for them?
>>
>> I can do a local fix and squash it into the original commit.
>>
>>> 1) Spelling error in commit message
>>> 2) Remove comma on terminator
>>
>> Ack, will fix.
>>
>>> 3) Add a comment why guid_count is safe (if you agree with me it is)
>>
>> I agree it is safe.
>>
>> Can you suggest some wording for the comment please ?
>>
>> Regards,
>>
>> Hans
>>
> 
> Maybe something like "_WDG is a static list that is only parsed at startup, it's safe to count entries without extra protection".

Ok, that works for me. I've added that as a comment as
well as squashed in the other 2 suggestions by Andy.

Regards,

Hans