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. > 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
