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: <168cb996-513e-77a8-b895-ef0fdd0c1793@amd.com>
Date:   Fri, 2 Sep 2022 07:19:03 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.10.0
Subject: Re: [PATCH v2] platform/x86: wmi: Allow duplicate GUIDs for drivers
 that use struct wmi_driver
Content-Language: en-US
To:     Hans de Goede <hdegoede@redhat.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>
From:   Mario Limonciello <mario.limonciello@amd.com>
In-Reply-To: <3df6405a-924a-b0e6-c879-05b9151011e3@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?VjBra2liRWs1aWUzcmVUQ2pjaFdYUWs2bDAzY04vYVZTS3JRVVBCWFI5dnpR?=
 =?utf-8?B?cFJXUEptL1I2YXVZK1I1T0hNZmxETXVKTTFOeEIrTE5OUnEyQlB2cGhmSG05?=
 =?utf-8?B?ejlzQWs0YkVINzBwSGlOTlN5ckRYUUNqUFU1R1JjL0xOWm0vd3lXYWZOMlBu?=
 =?utf-8?B?dmlnSzVWQU8vUm05T0s5aEdQWm80bHdaVWJLYXZ0WWlSSjBzOVNvd1JwVGw5?=
 =?utf-8?B?T3BUMjU2RjRHaDl5MlJSWE1GRW40VHRMVlpGMHR2MVk0b2o3Q0NnTkhvUFRw?=
 =?utf-8?B?UnZ4SjZhOExhdmZ0bnpzOU93T0E2VG1NanUzQWhwd3lMWjYvb2RBZXNDaE04?=
 =?utf-8?B?NWxxUmJKeFNtWGlHZjkzbmZnZ3ZJZDFRNHh1Nlg0M1ZPRkx3ZlpnVnd1M1dp?=
 =?utf-8?B?T1NDcEpwclM0UDhUY1Q4bGJwYXhldXpWNmIwZklKNHlYNHhoOWxBOEpVTDNv?=
 =?utf-8?B?MHFpMEtWWTVjTUpFZHlRS2VvR1pMT0FSdXFiUS82clUyZC95eUVJRURic0sv?=
 =?utf-8?B?L0ZyN2pZdm5UbjJSWGhSQjBvNkxwWGpKaFVqRWl4QXZNWFBzUE5GMGo5QmtE?=
 =?utf-8?B?WWZWVVV6WVg0T3NqNVlZNFRma3dOMlRUODVCVkZobTdFT0Q1dlNTdEtLbnJq?=
 =?utf-8?B?dDd6VXVrcVRkZVplRGdnRzZxcFFaZDNtSTJ0NlFBdVU3RzBrZ2U4cUJXR285?=
 =?utf-8?B?SExReEM1QWs1aE1yRE9GczdCMmtkNXBqRmZUL0djRU4rVWhMaFlXbnphUG5m?=
 =?utf-8?B?Z3diY1cvRnFWZEU1d3FPUTlWVFpvYnM4cmoyZWRjSkRCNHFLR1NyU1Y3WndQ?=
 =?utf-8?B?L1VOK0pUblhvMk8zUGtuQS9WUjd0WHZoL09SYk12bHhka2J6QTZFeTJmcHAz?=
 =?utf-8?B?YmthVm5nYW94dWtDS2ttQ1R4Q2I1ZktnTitBcnZqa1EwM2k3ZEZ1bmlqMjgx?=
 =?utf-8?B?c3ZjZ1hTd0tPVVR4c0hkRVlkRTdkMnRsOHBCYkVBUGJnNDVSWjlIQ215TFB0?=
 =?utf-8?B?Ym9rUEYyOUhpRlU5ZGxBbU5UdUlsTTVCS0RHNWNIWktyeWtOTmJGdUU4UjNQ?=
 =?utf-8?B?c1A3bzdWRnU1dWNMdWdXWXV4NWFJN0VUWkpVWnpjQzl6WEZrQ0xVOUQxRFlj?=
 =?utf-8?B?ZkVDWjV3YWFSUnl1OGQyc3hXenFOR1RBd0RnNmp4RmtDb0E1aGNzcVoxUFlV?=
 =?utf-8?B?dHVXbERMOGlocGNNYS8rK25Za01RRU42bWFvTTRTbFFac3lxVStuaW44b2xL?=
 =?utf-8?B?aENNY2dDdDFiZ3lUTytCMm5rOGxXVk8wMGd4V2xuVkxsblRqbHpTQS90Zjlh?=
 =?utf-8?B?UnB3Q1JSTmJSSDU3WVRIMVFoZVhMY2ZTazduK0RSYnZLVlVaUm1wODBSUDRu?=
 =?utf-8?B?RFVmSWZqaTA1VG5DL0Rtb01SVnFXUFVUKzdWTng1M0IvTFk0NUMxSHRyQTdO?=
 =?utf-8?B?WklqbjFOYjhlK3NDd2Y1ZEFtUWJLYW9ZeS9Yc2MrVnhMM1VzdXJ4L2FHSkc0?=
 =?utf-8?B?YlNRUE1DVm5HUkNmdGEzenBydzJVYjJ5cC9TeUlhVnVISFJ6QXg3djJGbm5h?=
 =?utf-8?B?ZlFEQmYrd2RMT2dJdzRXWDVHeWVoZEIvVlFhdWlNL3N2Smp6T1RHV0MraE1G?=
 =?utf-8?B?VVBrcUtSWStyRHFPaTdqSTM5ajRqOGd4NGxuQlJMb0NaY0RQZjNXM1kyN2VR?=
 =?utf-8?B?WW5NbmdFVEtKWFltVEJmTGZTYVRtaXZmcityYk9pb1RNOXgzQWZCSmZKckVG?=
 =?utf-8?B?dHRRdmkrSjJGbUsvWWZoTWpEODArWTFvT2kybFErWkpFV0h1THVjaytVWHNw?=
 =?utf-8?B?R2pJdWNZdlU4MzZjMFJnaHZ5VEgxc0lqeW9kMWp2RVZqKzEyYmlsNXhNcC82?=
 =?utf-8?B?VHo0TnlyUXl6L09rWUtMV1JrOGlLTWpOdGdFU3A3TjdiMkNSc253TzRFN3lk?=
 =?utf-8?B?ckQ1ellBY3cxUzNPNEVnVUhrdml5NStGRXpqeTBHM1pDYmY5MnNJcC9lLytP?=
 =?utf-8?B?ZElSQnlZbHlHc215cmVUYnlsYWp2RVR5Z3NaVlUwUnFrN0JWRFBuUXRaOGtQ?=
 =?utf-8?B?L2JFWDR3YWl1SFgrc0RFVml0dWlBU0hwdWkrVDVUc2wxdUNxZjg0ajhtN3NN?=
 =?utf-8?B?NDZFOHY2eENnMHFtNjFZUXdTOFkwaHJpUm54MkxVMy9Xa1I3alJQSE9VRUNN?=
 =?utf-8?Q?Ozk/CKbAZ+G3XQOAUjC7Zp4iVbHBdwRWv1J2fI4sc1Ca?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d1873f18-4380-4831-030d-08da8cdd54c1
X-MS-Exchange-CrossTenant-AuthSource: MN0PR12MB6101.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Sep 2022 12:19:06.6911
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: mEoazACMz1Rjik0LcoRZKuHJt900DIS9oJfYtAp5AwX4b2AhiNby/Ns1cFnUcYKwmHXqWC39ICJibfPpNxD/KA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW3PR12MB4473
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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".