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;
MIME-Version: 1.0
References: <20220829201500.6341-1-mario.limonciello@amd.com>
In-Reply-To: <20220829201500.6341-1-mario.limonciello@amd.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Thu, 1 Sep 2022 20:16:31 +0300
Message-ID: <CAHp75Vc3v4a6=ZJnOYYFGHEomExxopuUy8axDL=M2tbxHqtXqQ@mail.gmail.com>
Subject: Re: [PATCH v2] platform/x86: wmi: Allow duplicate GUIDs for drivers
 that use struct wmi_driver
To:     Mario Limonciello <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>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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.

-- 
With Best Regards,
Andy Shevchenko