On Fri, Mar 17, 2023 at 5:39 AM Armin Wolf <W_Armin@xxxxxx> wrote: > > Am 16.03.23 um 10:00 schrieb Hans de Goede: >> >> So I really so no need to make the code needlessly complicated with 2 sub-drivers which then notify each other. Let keep things KISS and keep this as is, so for the next version only change >> the method_id from 0xab to 0x01. > > I think that using wmi_evaluate_method() is deprecated and we should try to minimize its usage whenever possible. As for the handling of the WMI GUIDs, i believe that > just using the first matching WMI device is not a stable solution. We simply do not know if Lenovo considers both WMI GUIDs singletons or not. This means they could > for example decide to have multiple independent data sources for tablet mode events. The chances for this are indeed small, but it will still create a problem for users > using such machines. By having two drivers and maybe a global notifier call chain, we would enable the driver to handle such "unlikely situations" correctly. > > This would also allow the driver to work on machines missing the WMI event GUID. In such a case, userspace could then just poll the data WMI GUID for input, but > this is just an idea. Hi Armin, would it work to add the second GUID to the existing wmi_driver wmi_device_id array? Then I could save the wmi_device in the driver data on probe. Later when I get the notification on the other GUID I would just call wmidev_evaluate_method on the saved pointer out of the private data. I would just need a way to distinguish the two wmi_device structs. Seems like the notifier setup wouldn't be needed and it could stay as one module for one feature. I have the code ready to mail a v2 patch series with the remove function added and the fixed method id and the input triggering on probe, but still using wmi_evaluate_method. Without having much kernel experience, I sort of agree with Hans that it would be best to be simpler and not have two modules for one feature, the notifier setup looks somewhat involved. However if we can do something like the above idea, that doesn't seem to make it much more complicated to avoid the deprecated API and I can mail that out instead. So let me know what you think. >>> Drivers can be instantiated multiple times, and each time their probe callback is invoked, >>> and many older WMI drivers cannot do this, so the allowlist exists. >>> The section "State Container" in Documentation/driver-api/driver-model/design-patterns.rst >>> explains how to write drivers which can be instantiated multiple times. >>> >>> If your driver is not a singleton, i.e. it can safely be instantiated multiple times, then >>> you can add its WMI GUID to the allowlist. >> I'm not sure about adding this to the allowlist, using the new API is good (and nice and clean) but this is still expected to be a singleton. > > The allowlist is dealing with drivers not jet converted to the WMI bus model. The allowlist should ideally disappear once the conversion has been > completed, something which would become difficult if WMI drivers would continue to rely on the older GUID singleton behavior which is not compliant > with the ACPI WMI spec AFAIK. If we know that our WMI GUID is a singleton (which we do not), then we should handle this inside our driver, not inside > the WMI probing code. Would it not work to convert this driver to the WMI bus model now? I wasn't able to find anything about this bus model code. If I'm understanding Documentation/driver-api/driver-model/design-patterns.rst correctly then I think this driver would be okay to have the same GUID probed multiple times. Each device would get its own private data allocated for it and that would be cleaned up which each device is removed. Although, it does look like you would get double input events. I'm not sure if that's the correct behavior without access to a machine with two of these switch devices to test on. - Andrew
