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: <d219e8b4-f57e-a546-3794-6f6bc7030e9e@redhat.com>
Date:   Tue, 25 Jul 2023 17:07:45 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: WMI probe failure when reprobing
Content-Language: en-US, nl
To:     Armin Wolf <W_Armin@gmx.de>,
        "platform-driver-x86@vger.kernel.org" 
        <platform-driver-x86@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <1252c8fb-8d5f-98ad-b24a-5fabec2e1c8b@gmx.de>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <1252c8fb-8d5f-98ad-b24a-5fabec2e1c8b@gmx.de>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Armin,

On 7/22/23 02:09, Armin Wolf wrote:
> Hello,
> 
> i just noticed that under some circumstances, ACPI WMI devices might fail to reprobe
> when being manually unbound and later rebound.
> Example:
> 
> 1. ACPI WMI device #1 binds and registers WMI device with GUID
> "05901221-D566-11D1-B2F0-00A0C9062910", resulting in the device
> being named "05901221-D566-11D1-B2F0-00A0C9062910".
> 2. ACPI WMI device #2 binds and registers WMI device with GUID
> "05901221-D566-11D1-B2F0-00A0C9062910", resulting in the device
> being named "05901221-D566-11D1-B2F0-00A0C9062910-1".
> 3. ACPI WMI device #1 is manually unbound and later rebound,
> now the WMI device with GUID "05901221-D566-11D1-B2F0-00A0C9062910"
> is being named "05901221-D566-11D1-B2F0-00A0C9062910-1" too, since
> device naming depends on the number of GUIDs currently known to
> the WMI subsystem.
> 4. A WMI device named "05901221-D566-11D1-B2F0-00A0C9062910-1" already
> exists, causing the registration of the new WMI device to fail.
> 
> I thought about some possible ways to solve this naming issue:
> 
> - symlinks to simulate old WMI devices names, new WMI device names similar to "wmiX" with X being a global unique id
> - no symlinks, new WMI device names similar to "wmiX" with X being a global unique id
> - use global id instead of GUID number
> 
> The first approach has full sysfs backward compatibility but i do not know how to create symlinks inside the "devices"
> directory. The second approach is the easiest and cleanest one, but provides no sysfs backward compatibility. The last
> approach provides only limited sysfs backward compatibility and only for programs which can handle "<GUID>-X" WMI device
> names.
> 
> Currently, there is one single stable sysfs ABI entry concerning the WMI subsystem (for wmi-bmof), and two testing
> sysfs ABI entries (dell-wmi-privacy, sbl-fw-update). I do not know of any userspace programs relying on these ABIs,
> but i suspect there might be a couple of scripts which might be affected.
> 
> Which approach should i take to solve this problem?


The standard approach to get reliable unique ids in the kernel is to use
something like this:

#include <linux/idr.h>

static DEFINE_MUTEX(ida_lock);

struct guid_data {
	guid_t guid;
	struct ida ida;
	struct list_head list;
};

int guid_init() {
	ida_init(&guid_data->ida);
}

int wmi_create_device()
{
	int index;
	...
	mutex_lock(&ida_lock);
	index = ida_alloc(&guid_data->ida, GFP_KERNEL);
	mutex_unlock(&ida_lock);
	if (index < 0)
		return index;

	// store index for use on acpi_wmi_remove
	wmi_block->index = index;
	// use index to generate name, don't add -%d for index==0
	...
}

static void wmi_dev_release(struct device *dev)
{
        struct wmi_block *wblock = dev_to_wblock(dev);

	mutex_lock(&ida_lock);
	ida_free(&guid_data->ida, wblock->index);
	mutex_unlock(&ida_lock);	
        kfree(wblock);
}


This is going to need a linked-list of struct guid_data
structs and a new wmi_get_guid_data() function which
takes a new global mutex to protect the list and then
first walks that list looking for a guid match

If nothing is found kzalloc a new struct, init
the ida struct and add it to the list before releasing
the mutex protecting the list.

At the end of wmi_get_guid_data() return the found
or created struct guid_data or NULL on kzalloc error.

And in wmi_create_device() and wmi_dev_release()
use this to get a struct_guid_data matching the wblock
GUID so that we have 1 ida struct per GUID.

I would not worry about releasing the struct guid_data
if somehow the last wblock with that GUID disappears
chances are we are going to need it again soon and
the ida id-array will be empty then so we will start
with a clean-slate if we just re-use the old one
when a new wblock with the same GUID shows up again.

###

Not the prettiest with the need to have a new linked
lists of structs to get a per GUID ida, but it nicely
matches how most subsystems do this so I think it is
best.

I hope this small sketch of what a solution for this
could look like is useful.

Regards,

Hans