Date:   Mon, 21 Sep 2020 15:26:44 +0000
From:   "Limonciello, Mario" <Mario.Limonciello@...l.com>
To:     Hans de Goede <hdegoede@...hat.com>,
        Divya Bharathi <divya27392@...il.com>,
        "dvhart@...radead.org" <dvhart@...radead.org>
CC:     LKML <linux-kernel@...r.kernel.org>,
        "platform-driver-x86@...r.kernel.org" 
        <platform-driver-x86@...r.kernel.org>,
        "Bharathi, Divya" <Divya.Bharathi@...l.com>,
        "Ksr, Prasanth" <Prasanth.Ksr@...l.com>,
        Richard Hughes <rhughes@...hat.com>,
        Jared Dominguez <jaredz@...hat.com>
Subject: RE: [PATCH] Introduce support for Systems Management Driver over WMI
 for Dell Systems

> 
> Well if different schemes are supported and each scheme has its own type,
> then I would expect there to be say / e.g.:
> 
> /sys/class/firmware-attributes/dell/authentication/admin-password
> (with a type of "password") and:
> /sys/class/firmware-attributes/dell/authentication/admin-hotp
> (with a type of "hotp")
> 
> And then the user / userspace can choose which one to use,
> I guess if the kernel knows that only hotp has been setup and
> there is no standard password set, then it could hide the
> /sys/class/firmware-attributes/dell/authentication/admin-password
> password.

So you're proposing the flow to userspace that would look like this:

Authentication is off
----
# cat /sys/class/firmware-attributes/dell-wmi-sysman/attributes/Touchscreen/is_authentication_needed
0
# echo "enabled" | sud tee /sys/class/firmware-attributes/dell-wmi-sysman/attributes/Touchscreen/current_value



Turning on and things that happen using authentication (error examples too):
----
# cat /sys/class/firmware-attributes/dell-wmi-sysman/attributes/Touchscreen/is_authentication_needed
0
# echo "foobar123" | sudo tee /sys/class/firmware-attributes/dell-wmi-sysman/authentication/Admin/new_password
# cat /sys/class/firmware-attributes/dell-wmi-sysman/attributes/Touchscreen/is_authentication_needed
1
# echo "enabled" | sud tee /sys/class/firmware-attributes/dell-wmi-sysman/attributes/Touchscreen/current_value
-EOPNOTSUPP
# echo "foobar456" | sudo tee /sys/class/firmware-attributes/dell-wmi-sysman/authentication/Admin/current_password
# echo "enabled" | sud tee /sys/class/firmware-attributes/dell-wmi-sysman/attributes/Touchscreen/current_value
-EACCES
# echo "foobar123" | sudo tee /sys/class/firmware-attributes/dell-wmi-sysman/authentication/Admin/current_password
# echo "enabled" | sud tee /sys/class/firmware-attributes/dell-wmi-sysman/attributes/Touchscreen/current_value
# echo "" | sudo tee /sys/class/firmware-attributes/dell-wmi-sysman/authentication/Admin/current_password
# echo "enabled" | sud tee /sys/class/firmware-attributes/dell-wmi-sysman/attributes/Touchscreen/current_value
-EOPNOTSUPP


> 
> TBH I think all these things are (mostly) easily solvable if/when we
> encounter them. I mean it is definitely good to keep these kind of things
> in mind. But at some point we might get lost in all the what-ifs we
> can come up with.

In trying to come up with a generic interface that scales to everyone's needs
the what-ifs are critical.  Making assumptions on how authentication works means
future authentication mechanisms will be painful.

> 
> If a vendor comes along where authentication is not necessary
> for *all* attributes, then we could add the "is_authentication_required"
> as an optional sysfs-attribute for the firmware-attributes and state
> in the documentation that if that file is lacking that means that
> authentication is always required. That way the Dell code would not
> even have to have the "is_authentication_required" sysfs-attribute.

But it's not true on Dell's systems even right now.  If you don't have
an Admin password configured then you don't need it set for any attribute.
If you do have one set you need them for all.  And if you need to know to look for
/sys/class/firmware-attributes/dell-wmi-sysman/authentication/Admin/is_password_set
then userspace needs to know to do this differently for Dell and someone else.

So you either need to have a top level is_authentication_required
IE /sys/class/firmware-attributes/dell-wmi-sysman/is_authentication_required

Or a per attribute one
IE /sys/class/firmware-attributes/dell-wmi-sysman/attributes/Touchscreen/is_authentication_required

And this decision can't be put off because it has an implication that another
vendor may choose to do their authentication differently than Dell.

> 
> Since we also seem to have some trouble to get these 2 properly documented
> (I have not looked at v3 yet), I'm fine with making them dell specific by
> prefixing them
> with dell-. I guess that that probably even makes sense.

They're documented in v3.  The moment that you have a "Dell specific" attribute
what's the point of a common class?  You're going to end up with Dell expresses
dependencies this way, Lenovo expresses them that way, and HP expresses them some
other way and userspace is going to have to sort out the differences.

So in userspace you end up with logic that is something like this:
1) (Generic) Check if authentication is set
2) (Dell) Check if you're running on Dell's driver, interpret this dependency or show a message
3) (Lenovo) Check if you're running on Lenovo's driver, interpret this dependency or show a message
4) (HP) Check if you're running on HP's driver, interpret this dependency or show a message
5) (Generic) Check what authentication schemes are supported
6) (Dell) Apply Dell's admin password authentication scheme
7) (Lenovo Example) Apply Lenovo's admin password authentication scheme or their TOTP authentication scheme
8) (Generic) write value into current_value
9) (Generic) Disable authentication

So if userspace is going to have to be different anyway for evaluating dependencies and authentication, why
go through the trouble to fit everyone into the same class?

> 
> > Lastly I want to caution that individual firmware items with the same name
> might have
> > a different meaning across vendors.  Here is my hypothetical example:
> >
> > Dell has an attribute called "Camera"  With V3 it populates under:
> > /sys/devices/platform/dell-wmi-sysman/attributes/Camera
> >
> > The description sysfs for it reads as "Enable Camera" and it's possible
> values are
> > "Disabled;Enabled;".  For Dell this is pretty obviously it turns on and off
> the camera
> > functionality.
> >
> > For another vendor they might actually not offer to enable/disable the
> camera but instead
> > To enable the control of an electromagnetic camera shutter from such an
> attribute.
> > Their attribute could still be called "Camera" but the description might
> read as
> > "Enable camera shutter control".  For them it would still read as
> "Disabled;Enabled;"
> > for possible values but have a completely different meaning!
> >
> > There is no standard for this, and again userspace will need to basically
> look at
> > the directory and structure to figure out what the meaning actually is.
> 
> I can envision similar issues popping up between different generations /
> models
> of Dell hardware even. 

Dell has an internal committee that oversees the attribute registry.  Not all platforms
will expose the same attributes.  Across generations different sets of attributes will
be exposed based upon what they do.

So userspace would be able to look at /sys/devices/platform/dell-wmi-sysman/attributes/Camera
and know it's turning on/off camera on a Dell system.

>Specifying what changing the attributes actually does
> falls
> (way) outside of the scope of the sysfs ABI IMHO. That will always be the case
> of please consult your Laptop's / Workstation's / Server's manual.
> That is actually not much different from the current builtin
> firmware setup utility experience where the help text is often,
> well, not helpful.
> 
> For all I care there is an enum called "HWvirt" with a description of
> "Hardware virtualization support" and values of "Enabled" and "Disabled"
> which controls something somewhat or even totally different from what the
> name and description suggest. That would be less then ideal, but not a problem
> from the pov of the sysfs ABI for firmware-attributes. It would be a simple
> case of the garbage in garbage out principle.
> 
> So this is one problem which I'm happy to punt to userspace and I guess Dell
> might do a Dell specific utility, which only works one certain model Dell's,
> which is a lot fancier then the basic sysfs functionality and e.g. consumes
> the dell-value_modifier and dell-modifier sysfs-attribures.

The goal here is that all of the functionality that would otherwise be expressed
in a proprietary utility could also be expressed in sysfs.  Having to de-feature
the sysfs interface for the purpose of fitting into what's generic across vendors
defeats that goal and is why I think it should be a Dell interface in the first
place.

> 
> The purpose behind having a unified userspace ABI is to e.g. allow configuring
> firmware settings for a fleet of machines from:
> 
> https://wiki.gnome.org/Projects/FleetCommander
> 
> Using a generic plugin which works across different vendors.
> 

Conceptually this makes great sense until you dig into details.  In addition to the
things I've outlined above here's a few more places that might break:

* Attribute naming won't be the same across vendors.  To my point with Camera above
  what if Lenovo DOES support a camera control and calls their value EnableCamera?
  You couldn't have a single setting across vendors in your fleet and need to carry
  some logic in your plugin anyway to say if Dell: Camera if Lenovo: EnableCamera.

* you could run into situations that Dell's firmware accepts certain requirements
  for password length or complexity that aren't present in another firmware so you
  can't use the same password for your whole fleet

> And maybe have a simple vendor-agnostic pygtk3 UI which allows users to
> poke at things, even if they have to figure out in which order they need
> to change things in some cases (which again is actually not that
> different from the current builtin firmware setup utility experience
> for a lot of vendors).
> 
> I guess a could way to look at the generic sysfs firmware attributes
> class I'm proposing is looking at it as a lowest common denominator
> solution. With the addition of vendor specific extensions so that
> vendors (e.g. Dell) are not limited to only offering functionality
> offered by the generic, shared ABI. Does that make sense ?
> 
> Regards,
> 

I really think that trying to fit all the vendors into the same interface is going
to stifle areas for innovation in the firmware and kernel space in the name of
"simplicity" which really only goes as far as the kernel side.  Userspace has
to carry delta between vendors no matter what, so why introduce a LCD then?

Just as easily we could have:
/sys/devices/platform/dell-wmi-sysman/attributes/

Which works 90% the same as:
/sys/devices/platform/lenovo-wmi-sysman/attributes/

Powered by blists - more mailing lists