Am 22.12.24 um 13:09 schrieb Thomas Weißschuh:
Hi Joshua,
On 2024-12-19 18:31:22+0100, Joshua Grisham wrote:
Thank you both Thomas and Hans for your review and comments! I am
working on a v4 of the patch but had a few questions which I wanted to
clarify (they can also come after in a v5 etc in case I managed to get
this ready to go before anyone has the time to confirm and/or clarify
some things!).
Keep them coming :-)
Den tis 17 dec. 2024 kl 15:23 skrev Hans de Goede <hdegoede@xxxxxxxxxx>:
On 16-Dec-24 5:46 PM, Thomas Weißschuh wrote:
+Various hardware settings can be controlled by the following sysfs attributes:
+
+- ``allow_recording`` (allows or blocks usage of built-in camera and microphone)
+- ``start_on_lid_open`` (power on automatically when opening the lid)
+- ``usb_charge`` (allows USB ports to provide power even when device is off)
Non-standard sysfs attributes should be avoided where possible.
Userspace will need bespoke code to handle them.
This looks like it could be handled by the standard firmware_attributes
interface.
This would standardize discovery and usage.
Ack this really feels like firmware-attributes. I would not be surprised
if there are matching BIOS settings and if changing those also changes
the sysfs files and likewise if the sysfs settings persist over reboot.
Yes 2 of these (not this "allow_recording" I think) are available via
BIOS and all 3 of them persist over restarts.
Just so I am 100% clear what you mean here -- these type of attributes
should be created using the utilities available in
drivers/platform/x86/firmware_attributes_class.h so that they are
created under the path /sys/class/firmware-attributes/*/attributes/*/
?
Yes.
What exactly should they be named (any preference?) and should I also
add some documentation for them in
Documentation/ABI/testing/sysfs-class-firmware-attributes ?
I think they are meant to be named consistently with what the native
UEFI setup interface calls them.
And yes, they should be documented.
I am fairly sure I understand the concept and can agree that it kind
of makes a lot of sense to be able to standardize the userspace
interface, especially for attributes which do the exact same thing
across different vendors/devices (unless it just as easily possible to
go based on some pattern matching e.g. like is done in udev and upower
with "*kbd_backlight*" etc) but as of now it looks like the only
examples implemented are for thinklmi, dell-wmi, and hp-bioscfg that I
can see so far?
The firmware-attributes don't really have a standardized semantic.
Here the standardization is more about the discovery and interaction.
Somebody can build a generic UI to change these settings, without the UI
knowing anything about what the setting actually does.
If the setting maps to a another, more specific interface, that should
be used.
Before, I had tried to look through all of the various platform/x86
drivers and harmonize which names I picked for these sysfs attributes
(that is how I landed on "usb_charge" and "start_on_lid_open" as I
recall correctly) but I am not aware of any existing userspace tools
which are looking for anything like these (apart for
driver/vendor-specific utilities). Any recommendation from the very
wise people here would certainly be appreciated for these :)
[..] Snip, I don't feel qualify to comment on the input bits.
Harmonization with other platform driver regarding the firmware attribute names will
likely not result in any benefits. I am not aware of any utility profiting from such
a thing.
Other notifications that I am wondering what the "right" way to handle
/ using the right interface:
[..]
- When the battery charge control end threshold is reached, there is
an ACPI notification on this device as well that is the one I have
marked "ACPI_NOTIFY_BATTERY_STATE_CHANGED" -- the Samsung background
apps pop up a custom OSD that basically says something to the effect
that their "Battery saver is protecting the battery by stopping
charging" (can't remember the exact verbiage) and they change the
battery icon, but without doing anything else in my driver currently
the battery still reports state of "charging" even though it just sits
constantly at the percentage (and has the charging icon in GNOME etc).
I have seen the event come and go occasionally when I did not expect
it, but my working theory is that maybe it is if/when the battery
starts charging again if it dips too far below the target "end
threshold" and then notifies again when the threshold has been
reached. Armin also mentioned this before in a different mail; I guess
I would hope/expect there is an event or a function I could call to
have the state reflected correctly but I would not want that it
negatively impacts the normal behavior of charging the battery itself
(just that the state/icon would change would be ideal! as it functions
perfectly, it is just that the state and icon are not accurate).
Optimally the ACPI event would integrate with the ACPI battery driver.
See the handling of POWER_SUPPLY_STATUS_NOT_CHARGING in
drivers/acpi/battery.c.
Does the battery report the current rate as 0 when limiting?
Then something like acpi_battery_handle_discharging() could be used.
Thomas
Can you send me the acpidump of your machine (with the fan bug)? I can check how the
ACPI battery handles this case internally.
Thanks,
Armin Wolf
