On Tue, Aug 1, 2017 at 2:16 PM, Pali Rohár <pali.rohar@xxxxxxxxx> wrote:
> On Tuesday 01 August 2017 13:59:56 Darren Hart wrote:
>> On Tue, Aug 01, 2017 at 10:45:46PM +0200, Pali Rohár wrote:
>> > On Tuesday 01 August 2017 11:08:26 Andy Lutomirski wrote:
>> > > 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.)
>> >
>> > Yes, thanks for clarification, it makes sense.
>> >
>> > Just one hypothetical situation, but as we know we should not trust what
>> > vendors put into ACPI DSDT...
>> >
>> > Before whole wmi bus patches were introduced, function
>> > dell_wmi_events_set_enabled() was called only after every check passed:
>> >
>> > 1) WMI GUID exists
>> >
>> > 2) WMI descriptor buffer has correct type
>> >
>> > 3) machine is on DMI whitelist
>> >
>> > Now after all those patches, only the last (3) check need to pass to
>> > call that dell_wmi_events_set_enabled() function which issue SMM call.
>> >
>> > Do not know how big this issue is, I just want to point to this
>> > hypothetical problem that SMM call could be issued also in more cases.
>>
>>
>> Thanks for raising the point. In my opinion, it seems reasonable to expect that
>> #1 and #2 are implicit in #3 being coded up. We don't like to rely on hard coded
>> assumptions of course. What is the failure mode if #1 or #2 aren't satisfied?
>
> What would happen if we call dell_wmi_events_set_enabled() on
> unsupported platform? E.g. on some Dell Latitudes it cause resetting
> keyboard backlight settings to default values, e.g. turn keyboard
> backlight on for every key press and turn keyboard backlight off after
> 5s of inactivity.
>
> I do not know more.
>
> But yes, #3 should imply that #1 and #2 are truth too...
Hmm. I could do a followup patch to move it into ->probe and refcount it.
--Andy
