Message-ID: <DM6PR19MB26364970D0981212E811E1B0FA2E0@DM6PR19MB2636.namprd19.prod.outlook.com>
Date:   Tue, 1 Sep 2020 14:17:42 +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

> Going forward I will be helping Andy and Darren with maintaining the
> drivers/platform/x86/* drivers.
>
> So one of the first things which I'm doing with that hat on,
> is review this patch.

Congrats on the new responsibilities :)

<snip>

>
> So first of all some comments on the userspace (sysfs) API for that. Getting
> this
> part right is the most important part of this patch, as that will be set in
> stone
> once merged.
>
> My first reaction to the suggest API is that I find the sorting by type thing
> really weird,
> so if I were to do:
>
> ls /sys/devices/platform/dell-wmi-sysman/attributes/
>
> I would get the following as output:
>
> enumeration
> integer
> string
>
> And then to see the actual attributes I would need to do:
>
> ls /sys/devices/platform/dell-wmi-sysman/attributes/{enumeration,integer,string}
>
> This feels less then ideal both when interacting from a shell, but also when
> e.g. envisioning C-code enumerating attributes.
>
> IMHO it would be better to have:
>
> /sys/devices/platform/dell-wmi-sysman/attributes/<attr>/type
>
> Which can be one of "enumeration,integer,string"
>
> and then have the other sysfs files (default_Value, current_value, max..., etc.)
> as:
>
> /sys/devices/platform/dell-wmi-sysman/attributes/<attr>/default_value
> etc.
>
> Where which files exactly are present for a specific <attr> depends on the type.
>
> This will make e.g C-code enumerating all attributes be a single readdir,
> followed
> by reading the type for each dir entry; and if we add a new type the C-code can
> warn the user that it encountered an atribute with unknown type <new-type>,
> rather then not being aware that there is a fourth dir (for the new type) with
> attributes to check.

I agree this is the most important part to get correct.  This proposal seems pretty
good to me.

>
> Other then that the sysfs interface generally looks good to me, except for
> one other big thing (and one small thing, see below).
>
> This interface seems pretty generic (which is a good thing), but then having
> it live in the hardcoded /sys/devices/platform/dell-wmi-sysman/attributes
> name-space seems less then ideal. I also see in the code that you are creating
> a dummy platform device, just to have a place/parent to register the attributes
> dir with.
>
> Combining these 2 things I think that it would be better to make a new class
> for this, like how we e.g. have a backlight class under /sys/class/backlight
> we could have a /sys/class/firmware_attributes class and then we would get
> a dell_wmi entry under that (and drop the "attributes" dir), so we would get:
>
> /sys/class/firmware_attributes/dell_wmi/<attr>/type
>
> Etc.
>
> So instead of creating a dummy platform device, you would create a
> firmware_attributes
> class device.
>
> I think it is likely that other vendors may eventually also support modifying
> BIOS settings without going into the BIOS setup menu and I would like us to
> use one unified userspace API for this.  Note this changes little for the Dell
> code /
> this patch (although eventually some code may be moved into shared helpers), but
> it does allow userspace to discover if the firmware-attr sysfs API is supported
> in
> a vendor agnostic API by doing a readdir on /sys/class/firmware_attributes
>

This area I'm not sure I'm aligned.  Two reasons come to mind:

1) The interface that Dell offers isn't guaranteed to work the same as any other
Vendors. Do we want to force them to use the same interface as Dell?  For example what
if another vendor doesn't offer an interface from their firmware to enumerate possible
options for any attribute but you have to know them in advance?  Would those possible options
be hardcoded in their kernel driver?  Dell sets precedent here by having the first driver.

2) Dell has some extensions planned for other authentication mechanisms than password.
That is *definitely* going to be Dell specific, so should it be done in this vendor
agnostic directory?

> There could even be multiple instances implementing this interface, e.g. if
> their
> is an add-on card with its own option-ROM, see for iscsi booting then the iscsi
> boot
> options could be available under:
>
> /sys/class/firmware_attributes/iscsi_boot_nic/<attr>/*
>
> While the main system firmware settings would be available under:
>
> /sys/class/firmware_attributes/dell_wmi/<attr>/*
>
> Since you have already designed a nice generic API for this it seems
> sensible to me to make it possible to use this outside the Dell WMI case.
>
>
> So as mentioned I also have one smaller issue with the API, how is a
> UI supposed to represent all these attributes?  In the BIOS setup screen
> they are typically grouped together under e.g. CPU settings, power-management
> settings,
> etc.  I wonder if it would be possible to add a "group" sysfs file to each
> attribute
> which represent the typical grouping. E.g. for pm related settings the group
> file
> would contain "Power Management" then an userspace Ui can enumerate the groups
> and
> have e.g. 1 tab per group, or a tree with the groups as parents oof the
> attributes
> for each group. This is just an idea I don't know if such grouping info is
> available
> in the WMI interface for this.

This information isn't available in the ACPI-WMI interface unfortunately.

<snip>

>
> At first I was thinking that things like is_password_set would live directly
> under
> /sys/devices/platform/dell-wmi-sysman/attributes/password/ so we would have:
>
> /sys/devices/platform/dell-wmi-sysman/attributes/password/is_password_set
>
> But now I see that password really is just another type of attribute and we will
> have:
>
> /sys/devices/platform/dell-wmi-sysman/attributes/password/System/is_password_set
>
> and:
>
> /sys/devices/platform/dell-wmi-sysman/attributes/password/User/is_password_set
>
> That makes more sense, and will also work well with the changes I suggest above.
>

Correct.

>
> I would like to split the overall discussion of the API, versus doing a
> detailed review of the code, so I will review the code in a separate email.

Thanks, I think this is a productive way to review it.

<snip>

Powered by blists - more mailing lists