Hi Andrew, On 3/18/23 18:50, Andrew Kallmeyer wrote: > 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. Yes the notifier setup is somewhat involved. I believe posting the v2 which you have ready to post as is is fine. > 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. Assuming that with "this driver" you mean the Yoga Tablet Mode switch driver you are working on that already is a WMI BUS driver since it is a wmi_driver which attached itself to a wmi_device. The old way of doing things was with drivers which instantiated their own platform_device-s and then attached to those. These used the old specify a WMI object to operate on through GUID only methods like wmi_evaluate_method() everywhere. Regards, Hans
