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!). 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/*/ ? 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 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? 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 :) > > The allow-recording setting toggle is new to me. So this is changeable > at runtime, interesting. > > Joshua above you write this toggle both the microphone mute and > disables the camera? > > It would be good to report the camera state to userspace using > a separate input/evdev device which reports SW_CAMERA_LENS_COVER > as discussed here: > > https://lore.kernel.org/linux-media/CANiDSCtjpPG3XzaEOEeczZWO5gL-V_sj_Fv5=w82D6zKC9hnpw@xxxxxxxxxxxxxx/ > > the plan is to make that the canonical API to reported "muted" > cameras. > > What happens with the camera when recording is disallowed, > dus it drop of the USB bus or does it only produce black > frames ? > > It is a bit unexpected that this one button controls both > microphone and camera mute. But given that unique behavior > I guess that handling this in the kernel is probably best. > > The alsamixer should send some events for the mic mute/unmute > I hope and we can use SW_CAMERA_LENS_COVER to report the camera > state. > Yes this is kind of an interesting one, also... In the user manuals for these Samsung devices, they actually call this feature "Block Recording." See the second link here with the same title: https://www.samsung.com/ca/support/computing/samsung-laptop-disable-the-webcam/ There is this software control in their "Samsung Settings" and/or "Samsung Security" application plus the hotkey, but they both function exactly the same (executing this ACPI method with the right payload). The reason I called it "allow recording" is because I was trying to take a simple approach in the beginning, and let the device value and userspace value have a 1:1 mapping (you send 0x1 if you want the webcam and mic to NOT block recording, i.e. be "allowed" and 0x0 if you them to be blocked). I thought that echo 1 > block_recording to turn OFF "blocking" felt backwards so I just reversed the name instead ;) But in theory it could as easily be called "block_recording" and the kernel driver could handle the flip (0x1 from userspace becomes 0x0 to/from the device and vice-versa). When you press the hotkey in Windows then there is an OSD popup from their own background software, but nothing actually happens to the devices themselves. Even in Linux via this driver or if you just directly execute the ACPI method with the right values in the buffer, what happens is that the image feed from the camera just becomes solid black and the mic input is just completely silent. The USB camera device is not removed or seemingly touched in anyway, and there does not seem to be any kind of sound device event at all from what I can tell (I tried to check using "amixer events" and a few other methods but never saw anything, and the mixer control in alsa is always un-muted like normal when toggling the feature on and off even though it stops the sound from being able to be recorded). It is as this switch name SW_CAMERA_LENS_COVER indicates, almost like a physical (virtual?) cover has been drawn over the camera and the microphone but they are still seen as operational and completely unchanged from a device perspective. What I have started with for now based on this thread, is that as it seems like KE_SW key entries should have a "sw" struct with code and value, that I am assuming I should send .code = SW_CAMERA_LENS_COVER, .value = 0 for "cover is removed/off/not blocking", and .code = SW_CAMERA_LENS_COVER, .value = 1 for "cover is placed/on/blocking", also it looks like I should send this event as part of init for the current state at init time (the input device seems to have a "state" associated to this switch) -- is this correct? And then is there any events or functions that should be called to notify of a mic mute switch (not actually to send the event to toggle mic mute to userspace, but to just report to userspace somehow that the mic has been muted by hardware, similar to this SW_CAMERA_LENS_COVER or led_classdev_notify_brightness_hw_changed() etc ? At the same time I am still not convinced what exactly the name of this attribute should be ("allow_recording" vs "block_recording" vs "camera_mic_privacy" etc ??) Other notifications that I am wondering what the "right" way to handle / using the right interface: - Are there better events to use for these which these devices are reporting for "ACPI_NOTIFY_DEVICE_ON_TABLE" and "ACPI_NOTIFY_DEVICE_OFF_TABLE" , i.e. some kind of standard "switch"-like notification that the motion sensor in the device has detected that it has been placed or lifted from a flat surface? - 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). > <snip> > > Regards, > > Hans > Thanks again! Best regards, Joshua
