On Tue, Aug 1, 2017 at 8:43 AM, Pali Rohár <pali.rohar@xxxxxxxxx> wrote:
> On Tuesday 01 August 2017 08:37:26 Andy Lutomirski wrote:
>> When I converted dell-wmi to the new bus infrastructure, I left the
>> call to dell_wmi_check_descriptor_buffer() in dell_wmi_init(). This
>> could cause two problems:
>>
>> - An error message when loading the driver on a system without
>> dell-wmi. We'd try to read the event descriptor even if the WMI
>> GUID wasn't there.
>>
>> - A possible race if dell-wmi was loaded manually before wmi was
>> fully initialized.
>>
>> Fix it by moving the call to the probe function where it belongs.
>>
>> Fixes: bff589be59c5 ("platform/x86: dell-wmi: Convert to the WMI bus infrastructure")
>> Signed-off-by: Andy Lutomirski <luto@xxxxxxxxxx>
>> ---
>> drivers/platform/x86/dell-wmi.c | 12 +++++++-----
>> 1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
>> index f8978464df31..dad8f4afa17c 100644
>> --- a/drivers/platform/x86/dell-wmi.c
>> +++ b/drivers/platform/x86/dell-wmi.c
>> @@ -626,7 +626,7 @@ static void dell_wmi_input_destroy(struct wmi_device *wdev)
>> * WMI Interface Version 8 4 <version>
>> * WMI buffer length 12 4 4096
>> */
>> -static int __init dell_wmi_check_descriptor_buffer(void)
>> +static int dell_wmi_check_descriptor_buffer(void)
>> {
>> struct acpi_buffer out = { ACPI_ALLOCATE_BUFFER, NULL };
>> union acpi_object *obj;
>> @@ -717,9 +717,15 @@ static int dell_wmi_events_set_enabled(bool enable)
>>
>> static int dell_wmi_probe(struct wmi_device *wdev)
>> {
>> + int err;
>> +
>> struct dell_wmi_priv *priv = devm_kzalloc(
>> &wdev->dev, sizeof(struct dell_wmi_priv), GFP_KERNEL);
>>
>> + err = dell_wmi_check_descriptor_buffer();
>> + if (err)
>> + return err;
>> +
>> dev_set_drvdata(&wdev->dev, priv);
>>
>> return dell_wmi_input_setup(wdev);
>> @@ -749,10 +755,6 @@ static int __init dell_wmi_init(void)
>> {
>> int err;
>>
>> - err = dell_wmi_check_descriptor_buffer();
>> - if (err)
>> - return err;
>> -
>> dmi_check_system(dell_wmi_smbios_list);
>>
>> if (wmi_requires_smbios_request) {
>
> Hi! You should move also dell_wmi_events_set_enabled() into
> dell_wmi_probe() as there is no need to enable receiving events prior to
> creating input device.
I thought of that and intentionally didn't do it: I wanted to leave
enable and the disable paired properly, and there's nothing that
tracks the enabled state per device. Also, it's at least
theoretically possible to have more than one instance of dell-wmi in a
system (my laptop already has *two* wmi busses), and moving this code
to ->probe would break this.
(The current wmi.c code can't handle the same GUID on two busses, but
it could easily be added if anyone ever finds a laptop that does
that.)
