Re: [PATCH 2/2] platform/x86: Add driver for Yoga Tablet Mode switch

[Hans de Goede <hdegoede@redhat.com>]

Hi Andrew,

On 3/18/23 18:50, Andrew Kallmeyer wrote:
> On Fri, Mar 17, 2023 at 5:39 AM Armin Wolf <W_Armin@gmx.de> 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