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?
>
> --
> Pali Rohár
> pali.rohar@xxxxxxxxx
>
--
Darren Hart
VMware Open Source Technology Center
