Hi,
> -----Original Message-----
> From: Hans de Goede <hdegoede@xxxxxxxxxx>
> Sent: Wednesday, April 7, 2021 5:02 PM
> To: Ksr, Prasanth; Mark Gross; Andy Shevchenko
> Cc: Limonciello, Mario; Bharathi, Divya; Alexander Naumann; platform-driver-
> x86@xxxxxxxxxxxxxxx; Yuan, Perry
> Subject: Re: RE: [PATCH v2] platform/x86: dell-wmi-sysman: Make
> init_bios_attributes() ACPI object parsing more robust
>
>
> [EXTERNAL EMAIL]
>
> Hi,
>
> On 4/1/21 6:45 PM, Ksr, Prasanth wrote:
> > Hi,
> >
> >> -----Original Message-----
> >> From: Hans de Goede <hdegoede@xxxxxxxxxx>
> >> Sent: Tuesday, March 30, 2021 1:24 PM
> >> To: Ksr, Prasanth; Mark Gross; Andy Shevchenko
> >> Cc: Limonciello, Mario; Bharathi, Divya; Alexander Naumann;
> >> platform-driver- x86@xxxxxxxxxxxxxxx; Yuan, Perry
> >> Subject: Re: [PATCH v2] platform/x86: dell-wmi-sysman: Make
> >> init_bios_attributes() ACPI object parsing more robust
> >>
> >>
> >> [EXTERNAL EMAIL]
> >>
> >> Hi,
> >>
> >> On 3/29/21 6:00 PM, Ksr, Prasanth wrote:
> >>> Hi,
> >>>
> >>>> -----Original Message-----
> >>>> From: Hans de Goede <hdegoede@xxxxxxxxxx>
> >>>> Sent: Friday, March 26, 2021 10:14 PM
> >>>> To: Ksr, Prasanth; Mark Gross; Andy Shevchenko
> >>>> Cc: Limonciello, Mario; Bharathi, Divya; Alexander Naumann;
> >>>> platform-driver- x86@xxxxxxxxxxxxxxx; Yuan, Perry
> >>>> Subject: Re: [PATCH v2] platform/x86: dell-wmi-sysman: Make
> >>>> init_bios_attributes() ACPI object parsing more robust
> >>>>
> >>>>
> >>>> [EXTERNAL EMAIL]
> >>>>
> >>>> Hi,
> >>>>
> >>>> On 3/26/21 4:40 PM, Ksr, Prasanth wrote:
> >>>>>
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>> [EXTERNAL EMAIL]
> >>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>> On 3/21/21 1:16 PM, Hans de Goede wrote:
> >>>>>>> Make init_bios_attributes() ACPI object parsing more robust:
> >>>>>>> 1. Always check that the type of the return ACPI object is
> >>>>>>> package,
> >>>> rather
> >>>>>>> then only checking this for instance_id == 0 2. Check that
> >>>>>>> the package has the minimum amount of elements which will
> >>>>>>> be consumed by the populate_foo_data() for the attr_type
> >>>>>>>
> >>>>>>> Note/TODO: The populate_foo_data() functions should also be
> made
> >>>>>>> more robust. The should check the type of each of the elements
> >>>>>>> matches the type which they expect and in case of
> >>>>>>> populate_enum_data()
> >>>>>>> obj->package.count should be passed to it as an argument and it
> >>>>>>> obj->should
> >>>>>>> re-check this itself since it consume a variable number of elements.
> >>>>>>>
> >>>>>>> Fixes: e8a60aa7404b ("platform/x86: Introduce support for
> >>>>>>> Systems Management Driver over WMI for Dell Systems")
> >>>>>>> Cc: Divya Bharathi <Divya_Bharathi@xxxxxxxx>
> >>>>>>> Cc: Mario Limonciello <mario.limonciello@xxxxxxxx>
> >>>>>>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> >>>>>>> ---
> >>>>>>> Changes in v2:
> >>>>>>> - Restore behavior of returning -ENODEV when the
> >>>> get_wmiobj_pointer() call
> >>>>>>> for instance_id == 0 returns NULL. Otherwise
> >>>>>>> /sys/class/firmware-attributes/dell-wmi-sysman will get
> >>>>>>> created with
> >>>> an
> >>>>>>> empty attributes dir (empty except for pending_reboot and
> >>>>>>> reset_bios)
> >>>>>
> >>>>>> So the reason for this change in v2 is that testing on a Dell
> >>>>>> Latitude
> >> E5570:
> >>>>>>
> https://urldefense.com/v3/__https://bugzilla.redhat.com/show_bug.
> >>>>>>
> cgi?id=1936171__;!!LpKI!1eYVxhE0Cyt6JtzgTBadfIvZLQQ7LImJYSQns0D_A
> >>>>>> Yl6M_p00yMgV6NTFBoL4zMU$ [bugzilla[.]redhat[.]com]
> >>>>>>
> >>>>>> With v1 of this patch results in an empty attributes dir (empty
> >>>>>> except for
> >>>> pending_reboot and reset_bios), interestingly enough there are both
> >>>> System and Admin dirs created under Authentication, so the BIOS of
> >>>> this device seems to have the GUIDs for both the attributes and the
> >>>> authentication interfaces; and it >has the 2 standard
> >>>> authentication objects, theoretically allowing changing the BIOS
> >>>> passwords from within Linux, but it does not advertise any
> >>>> attributes which can be
> >> changed.
> >>>>>>
> >>>>>> With the new v2 patch, the driver will now simply refuse to load
> >>>>>> (and it
> >>>> should no longer crash during cleanup because of the other patches).
> >>>>>>
> >>>>>> But I wonder what is actually the right thing to do here ?
> >>>>>> Arguably being
> >>>> able to change the BIOS passwords has some value on its own ?
> >>>>>>
> >>>>>> Mario, what is your take on this?
> >>>>>
> >>>>> Ideally we should not be hitting this code path at all. I am
> >>>>> working with
> >>>> perry to see the results after applying the patches on the system.
> >>>>> If this is behavior we consistently see on older system BIOS then
> >>>>> we will
> >>>> patch the code to avoid code path and bail out early not loading
> >>>> the
> >> driver.
> >>>>
> >>>> With v2 of my patches (which have been merged) we do bail out as
> >>>> soon as it is clear that there is no enum-type (*) attribute with
> >>>> instance_id 0. That is pretty-much the earliest that we can bail
> >>>> and that
> >> works fine.
> >>>>
> >>>> What I was wondering is if that is the right thing to do though. On
> >>>> the E5570 there seem to be no enum/int/str attributes at all but
> >>>> what if there are enum
> >>>> + int attributes + no str attributes for example ?
> >>>>
> >>> It would be right thing to do because this scenario happens because
> >>> of
> >> some BIOS issue.
> >>
> >> Right, this is the right thing to do on machines such as the Latitude E5570.
> >>
> >> My question is more, what if, in the future some machine does not
> >> have say string bios-attributes, but it does have enum and int attributes ?
> >>
> >> The current code will then still refuse to bind / load, which seems wrong.
> >>
> >> Note that on the E5570 there are no enum and no string and no int
> >> attributes, so we could also delay the return -ENODEV until after we
> >> have enumerated all 3 types and if all 3 have a
> >> wmi_priv.foo_instances_count of 0 then return -ENODEV that seems like
> the logical thing to do here to me.
> >
> > It is not only with Latitude E5570 and this device is an example of
> > one such case which we are mentioning We expect all 3 categories to be
> present always for supported platforms. Even a system with minimal BIOS
> attributes will have at least few mandatory attributes in each of the three
> categories.
> > If BIOS don't have any one category GUID exposed then it will be faulty
> BIOS and our driver must refuse to bind/load. Hence, we believe that current
> code has right logic.
>
> Ok, so that means that this v2 is doing the right thing, as this version returns -
> ENODEV if any of the types are missing, just as before.
>
> Since this adds a couple of useful robustness checks I'm going to merge this
> now.
>
> As mentioned before, can you (Prasanth and/or Divya) please take care of
> the TODO from the commit message:
>
> TODO: The populate_foo_data() functions should also be made more robust.
> The should check the type of each of the elements matches the type which
> they expect and in case of populate_enum_data()
> obj->package.count should be passed to it as an argument and it should
> re-check this itself since it consume a variable number of elements.
Ack. Will work on this TODO and send the patches soon.
