* [PATCH v6 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
@ 2021-04-04 8:31 Perry Yuan
2021-04-04 16:53 ` Barnabás Pőcze
0 siblings, 1 reply; 5+ messages in thread
From: Perry Yuan @ 2021-04-04 8:31 UTC (permalink / raw)
To: pobrn, pierre-louis.bossart, oder_chiou, perex, tiwai, hdegoede,
mgross, Mario.Limonciello
Cc: lgirdwood, broonie, alsa-devel, linux-kernel,
platform-driver-x86, Perry.Yuan, mario.limonciello,
Dell.Client.Kernel
From: Perry Yuan <perry_yuan@dell.com>
add support for Dell privacy driver for the Dell units equipped
hardware privacy design, which protect users privacy of audio and
camera from hardware level. Once the audio or camera privacy mode
activated, any applications will not get any audio or video stream
when user pressed ctrl+F4 hotkey, audio privacy mode will be
enabled, micmute led will be also changed accordingly
The micmute led is fully controlled by hardware & EC(embedded controller)
and camera mute hotkey is Ctrl+F9. Currently design only emits
SW_CAMERA_LENS_COVER event while the camera lens shutter will be
changed by EC & HW(hardware) control
*The flow is like this:
1) User presses key. HW does stuff with this key (timeout timer is started)
2) WMI event is emitted from BIOS to kernel
3) WMI event is received by dell-privacy
4) KEY_MICMUTE emitted from dell-privacy
5) Userland picks up key and modifies kcontrol for SW mute
6) Codec kernel driver catches and calls ledtrig_audio_set, like this:
ledtrig_audio_set(LED_AUDIO_MICMUTE, rt715->micmute_led ? LED_ON :LED_OFF);
7) If "LED" is set to on dell-privacy notifies EC, and timeout is cancelled,
HW mic mute activated. If EC not notified, HW mic mute will also be
activated when timeout used up, it is just later than active ack
Signed-off-by: Perry Yuan <perry_yuan@dell.com>
---
v5 -> v6:
* addressed feedback from Hans
* addressed feedback from Pierre
* optimize some debug format with dev_dbg()
* remove platform driver,combined privacy acpi driver into single wmi
driver file
* optimize sysfs interface with string added to be more clearly reading
v4 -> v5:
* addressed feedback from Randy Dunlap
* addressed feedback from Pierre-Louis Bossart
* rebase to latest 5.12 rc4 upstream kernel
* fix some space alignment problem
v3 -> v4:
* fix format for Kconfig
* add sysfs document
* add flow comments to the privacy wmi/acpi driver
* addressed feedback from Barnabás Pőcze[Thanks very much]
* export privacy_valid to make the global state simpler to query
* fix one issue which will block the dell-laptop driver to load when
privacy driver invalid
* addressed feedback from Pierre-Louis Bossart,remove the EC ID match
v2 -> v3:
* add sysfs attributes doc
v1 -> v2:
* query EC handle from EC driver directly.
* fix some code style.
* add KEY_END to keymap array.
* clean platform device when cleanup called
* use hexadecimal format for log print in dev_dbg
* remove __set_bit for the report keys from probe.
* fix keymap leak
* add err_free_keymap in dell_privacy_wmi_probe
* wmi driver will be unregistered if privacy_acpi_init() fails
* add sysfs attribute files for user space query.
* add leds micmute driver to privacy acpi
* add more design info the commit info
---
---
.../testing/sysfs-platform-dell-privacy-wmi | 55 +++
drivers/platform/x86/dell/Kconfig | 14 +
drivers/platform/x86/dell/Makefile | 1 +
drivers/platform/x86/dell/dell-laptop.c | 23 +-
drivers/platform/x86/dell/dell-privacy-wmi.c | 441 ++++++++++++++++++
drivers/platform/x86/dell/dell-privacy-wmi.h | 32 ++
drivers/platform/x86/dell/dell-wmi.c | 13 +-
7 files changed, 572 insertions(+), 7 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
create mode 100644 drivers/platform/x86/dell/dell-privacy-wmi.c
create mode 100644 drivers/platform/x86/dell/dell-privacy-wmi.h
diff --git a/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi b/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
new file mode 100644
index 000000000000..a57ddc6a221e
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
@@ -0,0 +1,55 @@
+What: /sys/bus/wmi/devices/6932965F-1671-4CEB-B988-D3AB0A901919/dell_privacy_supported_type
+Date: Apr 2021
+KernelVersion: 5.13
+Contact: "perry.yuan@dell.com>"
+Description:
+ Display which dell hardware level privacy devices are supported
+ “Dell Privacy” is a set of HW, FW, and SW features to enhance
+ Dell’s commitment to platform privacy for MIC, Camera, and
+ ePrivacy screens.
+ The supported hardware privacy devices are:
+Attributes:
+ Microphone Mute:
+ Identifies the local microphone can be muted by hardware, no applications
+ is available to capture system mic sound
+
+ Camera Shutter:
+ Identifies camera shutter controlled by hardware, which is a micromechanical
+ shutter assembly that is built onto the camera module to block capturing images
+ from outside the laptop
+
+ supported:
+ The privacy device is supported by this system
+
+ unsupported:
+ The privacy device is not supported on this system
+
+ For example to check which privacy devices are supported:
+
+ # cat /sys/bus/wmi/drivers/dell-privacy/6932965F-1671-4CEB-B988-D3AB0A901919/dell_privacy_supported_type
+ [Microphone Mute] [supported]
+ [Camera Shutter] [supported]
+ [ePrivacy Screen] [unsupport]
+
+What: /sys/bus/wmi/devices/6932965F-1671-4CEB-B988-D3AB0A901919/dell_privacy_current_state
+Date: Apr 2021
+KernelVersion: 5.13
+Contact: "perry.yuan@dell.com>"
+Description:
+ Allow user space to check current dell privacy device state.
+ Describes the Device State class exposed by BIOS which can be
+ consumed by various applications interested in knowing the Privacy
+ feature capabilities
+Attributes:
+ muted:
+ Identifies the privacy device is turned off and cannot send stream to OS applications
+
+ unmuted:
+ Identifies the privacy device is turned on ,audio or camera driver can get
+ stream from mic and camera module to OS applications
+
+ For example to check all supported current privacy device states:
+
+ # cat /sys/bus/wmi/drivers/dell-privacy/6932965F-1671-4CEB-B988-D3AB0A901919/dell_privacy_current_state
+ [Microphone] [unmuted]
+ [Camera Shutter] [unmuted]
diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
index e0a55337f51a..05d124442b25 100644
--- a/drivers/platform/x86/dell/Kconfig
+++ b/drivers/platform/x86/dell/Kconfig
@@ -204,4 +204,18 @@ config DELL_WMI_SYSMAN
To compile this driver as a module, choose M here: the module will
be called dell-wmi-sysman.
+config DELL_PRIVACY
+ tristate "Dell Hardware Privacy Support"
+ depends on ACPI
+ depends on ACPI_WMI
+ depends on INPUT
+ depends on DELL_LAPTOP
+ depends on LEDS_TRIGGER_AUDIO
+ select DELL_WMI
+ help
+ This driver provides integration with the "Dell Hardware Privacy" feature
+ of Dell laptops.
+
+ To compile this driver as a module, choose M here: the module will
+ be called dell_privacy.
endif # X86_PLATFORM_DRIVERS_DELL
diff --git a/drivers/platform/x86/dell/Makefile b/drivers/platform/x86/dell/Makefile
index d720a3e42ae3..7da0c33dfcca 100644
--- a/drivers/platform/x86/dell/Makefile
+++ b/drivers/platform/x86/dell/Makefile
@@ -19,3 +19,4 @@ obj-$(CONFIG_DELL_WMI_AIO) += dell-wmi-aio.o
obj-$(CONFIG_DELL_WMI_DESCRIPTOR) += dell-wmi-descriptor.o
obj-$(CONFIG_DELL_WMI_LED) += dell-wmi-led.o
obj-$(CONFIG_DELL_WMI_SYSMAN) += dell-wmi-sysman/
+obj-$(CONFIG_DELL_PRIVACY) += dell-privacy-wmi.o
diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
index 70edc5bb3a14..a9b4cf13913c 100644
--- a/drivers/platform/x86/dell/dell-laptop.c
+++ b/drivers/platform/x86/dell/dell-laptop.c
@@ -31,6 +31,8 @@
#include "dell-rbtn.h"
#include "dell-smbios.h"
+#include "dell-privacy-wmi.h"
+
struct quirk_entry {
bool touchpad_led;
bool kbd_led_not_present;
@@ -90,6 +92,7 @@ static struct rfkill *wifi_rfkill;
static struct rfkill *bluetooth_rfkill;
static struct rfkill *wwan_rfkill;
static bool force_rfkill;
+static bool has_privacy;
module_param(force_rfkill, bool, 0444);
MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted models");
@@ -2206,10 +2209,16 @@ static int __init dell_init(void)
if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) {
- micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
- ret = led_classdev_register(&platform_device->dev, &micmute_led_cdev);
- if (ret < 0)
- goto fail_led;
+ if (dell_privacy_has_micmute())
+ has_privacy = true;
+ else
+ has_privacy = false;
+ if (!has_privacy) {
+ micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
+ ret = led_classdev_register(&platform_device->dev, &micmute_led_cdev);
+ if (ret < 0)
+ goto fail_led;
+ }
}
if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
@@ -2257,7 +2266,8 @@ static int __init dell_init(void)
fail_get_brightness:
backlight_device_unregister(dell_backlight_device);
fail_backlight:
- led_classdev_unregister(&micmute_led_cdev);
+ if (!has_privacy)
+ led_classdev_unregister(&micmute_led_cdev);
fail_led:
dell_cleanup_rfkill();
fail_rfkill:
@@ -2278,7 +2288,8 @@ static void __exit dell_exit(void)
touchpad_led_exit();
kbd_led_exit();
backlight_device_unregister(dell_backlight_device);
- led_classdev_unregister(&micmute_led_cdev);
+ if (!has_privacy)
+ led_classdev_unregister(&micmute_led_cdev);
dell_cleanup_rfkill();
if (platform_device) {
platform_device_unregister(platform_device);
diff --git a/drivers/platform/x86/dell/dell-privacy-wmi.c b/drivers/platform/x86/dell/dell-privacy-wmi.c
new file mode 100644
index 000000000000..3fae8dd077f1
--- /dev/null
+++ b/drivers/platform/x86/dell/dell-privacy-wmi.c
@@ -0,0 +1,441 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Dell privacy notification driver
+ *
+ * Copyright (C) 2021 Dell Inc. All Rights Reserved.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/acpi.h>
+#include <linux/bitops.h>
+#include <linux/input.h>
+#include <linux/input/sparse-keymap.h>
+#include <linux/list.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/wmi.h>
+
+#include "dell-privacy-wmi.h"
+
+#define DELL_PRIVACY_GUID "6932965F-1671-4CEB-B988-D3AB0A901919"
+#define MICROPHONE_STATUS BIT(0)
+#define CAMERA_STATUS BIT(1)
+#define DELL_PRIVACY_AUDIO_EVENT 0x1
+#define DELL_PRIVACY_CAMERA_EVENT 0x2
+#define cdev_to_led(c) container_of(c, struct privacy_wmi_data, cdev)
+
+/*
+ * The wmi_list is used to store the privacy_priv struct with mutex protecting
+ */
+static LIST_HEAD(wmi_list);
+static DEFINE_MUTEX(list_mutex);
+
+struct privacy_wmi_data {
+ struct input_dev *input_dev;
+ struct wmi_device *wdev;
+ struct list_head list;
+ struct led_classdev cdev;
+ u32 features_present;
+ u32 last_status;
+};
+
+/* DELL Privacy Type */
+enum dell_hardware_privacy_type {
+ DELL_PRIVACY_TYPE_AUDIO = 0,
+ DELL_PRIVACY_TYPE_CAMERA,
+ DELL_PRIVACY_TYPE_SCREEN,
+ DELL_PRIVACY_TYPE_MAX,
+};
+
+static const char * const privacy_types[DELL_PRIVACY_TYPE_MAX] = {
+ [DELL_PRIVACY_TYPE_AUDIO] = "Microphone",
+ [DELL_PRIVACY_TYPE_CAMERA] = "Camera Shutter",
+ [DELL_PRIVACY_TYPE_SCREEN] = "ePrivacy Screen",
+};
+
+/*
+ * Keymap for WMI privacy events of type 0x0012
+ */
+static const struct key_entry dell_wmi_keymap_type_0012[] = {
+ /* privacy mic mute */
+ { KE_KEY, 0x0001, { KEY_MICMUTE } },
+ /* privacy camera mute */
+ { KE_SW, 0x0002, { SW_CAMERA_LENS_COVER } },
+ { KE_END, 0},
+};
+
+/*
+ * global privacy state for other modules to query if the privacy driver loaded successfully
+ * or no privacy WMI device was detected at initial probe phase
+ */
+bool dell_privacy_present(void)
+{
+ struct privacy_wmi_data *priv;
+
+ mutex_lock(&list_mutex);
+ priv = list_first_entry_or_null(&wmi_list,
+ struct privacy_wmi_data,
+ list);
+ mutex_unlock(&list_mutex);
+
+ return priv;
+}
+EXPORT_SYMBOL_GPL(dell_privacy_present);
+
+/*
+ * Allow other modules to check current dell privacy mic mute state
+ */
+int dell_privacy_has_micmute(void)
+{
+ struct privacy_wmi_data *priv;
+ int ret;
+
+ mutex_lock(&list_mutex);
+ priv = list_first_entry_or_null(&wmi_list,
+ struct privacy_wmi_data,
+ list);
+ /*
+ * There is no privacy device node yet, so we need to wait
+ * until the privacy driver has created software node for the micmute device.
+ * This function will return -EPROBE_DEFER until the privacy device is registered.
+ */
+ if (!priv) {
+ ret = -EPROBE_DEFER;
+ goto out;
+ }
+ ret = priv->features_present & MICROPHONE_STATUS;
+ if (!ret) {
+ dev_err(&priv->wdev->dev, "privacy mic mute feature is not supported: %d\n", ret);
+ ret = -ENODEV;
+ goto out;
+ }
+ dev_err(&priv->wdev->dev, "privacy mic mute feature is supported: %d\n", ret);
+
+out:
+ mutex_unlock(&list_mutex);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(dell_privacy_has_micmute);
+
+/*
+ * The flow of privacy event:
+ * 1) User presses key. HW does stuff with this key (timeout is started)
+ * 2) WMI event is emitted from BIOS
+ * 3) WMI event is received by dell-privacy
+ * 4) KEY_MICMUTE emitted from dell-privacy
+ * 5) Userland picks up key and modifies kcontrol for SW mute
+ * 6) Codec kernel driver catches and calls ledtrig_audio_set defined by
+ * dell-privacy-acpi driver. Codec driver will call like this to switch micmute led state.
+ * ledtrig_audio_set(LED_AUDIO_MICMUTE, micmute_led ? LED_ON :LED_OFF);
+ * 7) If "LED" is set to on dell-privacy notifies EC,and timeout is cancelled,
+ * HW mic mute activated.
+ */
+bool dell_privacy_process_event(int type, int code, int status)
+{
+ struct privacy_wmi_data *priv;
+ const struct key_entry *key;
+ bool ret = false;
+
+ mutex_lock(&list_mutex);
+ priv = list_first_entry_or_null(&wmi_list,
+ struct privacy_wmi_data,
+ list);
+ if (!priv) {
+ dev_err(&priv->wdev->dev, "priv data is NULL\n");
+ goto error;
+ }
+
+ key = sparse_keymap_entry_from_scancode(priv->input_dev, (type << 16) | code);
+ if (!key) {
+ dev_warn(&priv->wdev->dev, "Unknown key with type 0x%04x and code 0x%04x pressed\n",
+ type, code);
+ goto error;
+ }
+ dev_dbg(&priv->wdev->dev, "Key with type 0x%04x and code 0x%04x pressed\n", type, code);
+
+ switch (code) {
+ case DELL_PRIVACY_AUDIO_EVENT: /* Mic mute */
+ case DELL_PRIVACY_CAMERA_EVENT: /* Camera mute */
+ priv->last_status = status;
+ sparse_keymap_report_entry(priv->input_dev, key, 1, true);
+ ret = true;
+ break;
+ default:
+ dev_dbg(&priv->wdev->dev, "unknown event type 0x%04x 0x%04x", type, code);
+ }
+
+error:
+ mutex_unlock(&list_mutex);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(dell_privacy_process_event);
+
+static ssize_t dell_privacy_supported_type_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ enum dell_hardware_privacy_type type;
+ struct privacy_wmi_data *priv = dev_get_drvdata(dev);
+ char *s = buf;
+ u32 privacy_list;
+
+ privacy_list = priv->features_present;
+ for (type = DELL_PRIVACY_TYPE_AUDIO; type < DELL_PRIVACY_TYPE_MAX; type++) {
+ if (privacy_types[type]) {
+ if (privacy_list & BIT(type))
+ s += sprintf(s, "[%s] [supported]\n", privacy_types[type]);
+ else
+ s += sprintf(s, "[%s] [unsupport]\n", privacy_types[type]);
+ }
+ }
+
+ if (s != buf)
+ /* convert the last space to a newline */
+ *(s-1) = '\n';
+ return (s - buf);
+}
+
+static ssize_t dell_privacy_current_state_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct privacy_wmi_data *priv = dev_get_drvdata(dev);
+ enum dell_hardware_privacy_type type;
+ u32 privacy_state = priv->last_status;
+ u32 privacy_supported = priv->features_present;
+ char *s = buf;
+
+ for (type = DELL_PRIVACY_TYPE_AUDIO; type < DELL_PRIVACY_TYPE_MAX; type++) {
+ if (privacy_supported & BIT(type)) {
+ if (privacy_state & BIT(type))
+ s += sprintf(s, "[%s] [unmuted]\n", privacy_types[type]);
+ else
+ s += sprintf(s, "[%s] [muted]\n", privacy_types[type]);
+ }
+ }
+
+ if (s != buf)
+ /* convert the last space to a newline */
+ *(s-1) = '\n';
+ return (s - buf);
+}
+
+static DEVICE_ATTR_RO(dell_privacy_supported_type);
+static DEVICE_ATTR_RO(dell_privacy_current_state);
+
+static struct attribute *privacy_attributes[] = {
+ &dev_attr_dell_privacy_supported_type.attr,
+ &dev_attr_dell_privacy_current_state.attr,
+ NULL,
+};
+
+static const struct attribute_group privacy_attribute_group = {
+ .attrs = privacy_attributes
+};
+
+/*
+ * Describes the Device State class exposed by BIOS which can be consumed by
+ * various applications interested in knowing the Privacy feature capabilities.
+ * class DeviceState
+ * {
+ * [key, read] string InstanceName;
+ * [read] boolean ReadOnly;
+ * [WmiDataId(1), read] uint32 DevicesSupported;
+ * 0 – None, 0x1 – Microphone, 0x2 – Camera, 0x4 -ePrivacy Screen
+ * [WmiDataId(2), read] uint32 CurrentState;
+ * 0:Off; 1:On. Bit0 – Microphone, Bit1 – Camera, Bit2 - ePrivacyScreen
+ * };
+ */
+static int get_current_status(struct wmi_device *wdev)
+{
+ struct privacy_wmi_data *priv = dev_get_drvdata(&wdev->dev);
+ union acpi_object *obj_present;
+ u32 *buffer;
+ int ret = 0;
+
+ if (!priv) {
+ pr_err("dell privacy priv is NULL\n");
+ return -EINVAL;
+ }
+ /* check privacy support features and device states */
+ obj_present = wmidev_block_query(wdev, 0);
+ if (!obj_present) {
+ dev_err(&wdev->dev, "failed to read Binary MOF\n");
+ ret = -EIO;
+ return ret;
+ }
+
+ if (obj_present->type != ACPI_TYPE_BUFFER) {
+ dev_err(&wdev->dev, "Binary MOF is not a buffer!\n");
+ ret = -EIO;
+ goto obj_free;
+ }
+ /* Although it's not technically a failure, this would lead to
+ * unexpected behavior
+ */
+ if (obj_present->buffer.length != 8) {
+ dev_err(&wdev->dev, "Dell privacy buffer has unexpected length (%d)!\n",
+ obj_present->buffer.length);
+ ret = -EINVAL;
+ goto obj_free;
+ }
+ buffer = (u32 *)obj_present->buffer.pointer;
+ priv->features_present = buffer[0];
+ priv->last_status = buffer[1];
+
+obj_free:
+ kfree(obj_present);
+ return ret;
+}
+
+static int dell_privacy_micmute_led_set(struct led_classdev *led_cdev,
+ enum led_brightness brightness)
+{
+ struct privacy_wmi_data *priv = cdev_to_led(led_cdev);
+ static char *acpi_method = (char *)"ECAK";
+ acpi_status status;
+ acpi_handle handle;
+
+ handle = ec_get_handle();
+ if (!handle)
+ return -EIO;
+
+ if (!acpi_has_method(handle, acpi_method))
+ return -EIO;
+
+ status = acpi_evaluate_object(handle, acpi_method, NULL, NULL);
+ if (ACPI_FAILURE(status)) {
+ dev_err(&priv->wdev->dev, "Error setting privacy EC ack value: %s\n",
+ acpi_format_exception(status));
+ return -EIO;
+ }
+
+ return 0;
+}
+
+/*
+ * Pressing the mute key activates a time delayed circuit to physically cut
+ * off the mute. The LED is in the same circuit, so it reflects the true
+ * state of the HW mute. The reason for the EC "ack" is so that software
+ * can first invoke a SW mute before the HW circuit is cut off. Without SW
+ * cutting this off first does not affect the time delayed muting or status
+ * of the LED but there is a possibility of a "popping" noise.
+ *
+ * If the EC receives the SW ack, the circuit will be activated before the
+ * delay completed.
+ *
+ * Exposing as an LED device allows the codec drivers notification path to
+ * EC ACK to work
+ */
+static int dell_privacy_leds_setup(struct device *dev)
+{
+ struct privacy_wmi_data *priv = dev_get_drvdata(dev);
+ int ret;
+
+ priv->cdev.name = "dell-privacy::micmute";
+ priv->cdev.max_brightness = 1;
+ priv->cdev.brightness_set_blocking = dell_privacy_micmute_led_set;
+ priv->cdev.default_trigger = "audio-micmute";
+ priv->cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
+ ret = devm_led_classdev_register(dev, &priv->cdev);
+ if (ret)
+ return ret;
+ return 0;
+}
+
+static int dell_privacy_wmi_probe(struct wmi_device *wdev, const void *context)
+{
+ struct privacy_wmi_data *priv;
+ struct key_entry *keymap;
+ int ret, i;
+
+ ret = wmi_has_guid(DELL_PRIVACY_GUID);
+ if (!ret)
+ pr_debug("Unable to detect available Dell privacy devices: %d\n", ret);
+
+ priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ dev_set_drvdata(&wdev->dev, priv);
+ priv->wdev = wdev;
+ /* create evdev passing interface */
+ priv->input_dev = devm_input_allocate_device(&wdev->dev);
+ if (!priv->input_dev)
+ return -ENOMEM;
+
+ /* remap the wmi keymap event to new keymap */
+ keymap = kcalloc(ARRAY_SIZE(dell_wmi_keymap_type_0012),
+ sizeof(struct key_entry), GFP_KERNEL);
+ if (!keymap)
+ return -ENOMEM;
+
+ /* remap the keymap code with Dell privacy key type 0x12 as prefix
+ * KEY_MICMUTE scancode will be reported as 0x120001
+ */
+ for (i = 0; i < ARRAY_SIZE(dell_wmi_keymap_type_0012); i++) {
+ keymap[i] = dell_wmi_keymap_type_0012[i];
+ keymap[i].code |= (0x0012 << 16);
+ }
+ ret = sparse_keymap_setup(priv->input_dev, keymap, NULL);
+ if (ret)
+ return ret;
+
+ priv->input_dev->dev.parent = &wdev->dev;
+ priv->input_dev->name = "Dell Privacy Driver";
+ priv->input_dev->id.bustype = BUS_HOST;
+
+ ret = input_register_device(priv->input_dev);
+ if (ret)
+ return ret;
+
+ ret = get_current_status(priv->wdev);
+ if (ret)
+ return ret;
+
+ ret = devm_device_add_group(&wdev->dev, &privacy_attribute_group);
+ if (ret)
+ return ret;
+
+ ret = dell_privacy_leds_setup(&priv->wdev->dev);
+ if (ret)
+ return ret;
+
+ mutex_lock(&list_mutex);
+ list_add_tail(&priv->list, &wmi_list);
+ mutex_unlock(&list_mutex);
+ kfree(keymap);
+ return 0;
+}
+
+static int dell_privacy_wmi_remove(struct wmi_device *wdev)
+{
+ struct privacy_wmi_data *priv = dev_get_drvdata(&wdev->dev);
+
+ mutex_lock(&list_mutex);
+ list_del(&priv->list);
+ mutex_unlock(&list_mutex);
+ return 0;
+}
+
+static const struct wmi_device_id dell_wmi_privacy_wmi_id_table[] = {
+ { .guid_string = DELL_PRIVACY_GUID },
+ { },
+};
+
+static struct wmi_driver dell_privacy_wmi_driver = {
+ .driver = {
+ .name = "dell-privacy",
+ },
+ .probe = dell_privacy_wmi_probe,
+ .remove = dell_privacy_wmi_remove,
+ .id_table = dell_wmi_privacy_wmi_id_table,
+};
+
+module_wmi_driver(dell_privacy_wmi_driver);
+
+MODULE_DEVICE_TABLE(wmi, dell_wmi_privacy_wmi_id_table);
+MODULE_AUTHOR("Perry Yuan <perry_yuan@dell.com>");
+MODULE_DESCRIPTION("Dell Privacy WMI Driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/platform/x86/dell/dell-privacy-wmi.h b/drivers/platform/x86/dell/dell-privacy-wmi.h
new file mode 100644
index 000000000000..a24893754286
--- /dev/null
+++ b/drivers/platform/x86/dell/dell-privacy-wmi.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Dell privacy notification driver
+ *
+ * Copyright (C) 2021 Dell Inc. All Rights Reserved.
+ */
+
+#ifndef _DELL_PRIVACY_WMI_H_
+#define _DELL_PRIVACY_WMI_H_
+
+#if IS_ENABLED(CONFIG_DELL_PRIVACY)
+int dell_privacy_has_micmute(void);
+bool dell_privacy_present(void);
+bool dell_privacy_process_event(int type, int code, int status);
+#else /* CONFIG_DELL_PRIVACY */
+static inline int dell_privacy_has_micmute(void)
+{
+ return -ENODEV;
+}
+
+static inline int dell_privacy_present(void)
+{
+ return -ENODEV;
+}
+
+static inline void dell_privacy_process_event(int type, int code, int status)
+{}
+#endif /* CONFIG_DELL_PRIVACY */
+
+int dell_privacy_acpi_init(void);
+void dell_privacy_acpi_exit(void);
+#endif
diff --git a/drivers/platform/x86/dell/dell-wmi.c b/drivers/platform/x86/dell/dell-wmi.c
index bbdb3e860892..8ef9e22a538f 100644
--- a/drivers/platform/x86/dell/dell-wmi.c
+++ b/drivers/platform/x86/dell/dell-wmi.c
@@ -27,6 +27,7 @@
#include <acpi/video.h>
#include "dell-smbios.h"
#include "dell-wmi-descriptor.h"
+#include "dell-privacy-wmi.h"
MODULE_AUTHOR("Matthew Garrett <mjg@redhat.com>");
MODULE_AUTHOR("Pali Rohár <pali@kernel.org>");
@@ -381,6 +382,7 @@ static void dell_wmi_notify(struct wmi_device *wdev,
u16 *buffer_entry, *buffer_end;
acpi_size buffer_size;
int len, i;
+ int ret;
if (obj->type != ACPI_TYPE_BUFFER) {
pr_warn("bad response type %x\n", obj->type);
@@ -427,7 +429,6 @@ static void dell_wmi_notify(struct wmi_device *wdev,
switch (buffer_entry[1]) {
case 0x0000: /* One key pressed or event occurred */
- case 0x0012: /* Event with extended data occurred */
if (len > 2)
dell_wmi_process_key(wdev, buffer_entry[1],
buffer_entry[2]);
@@ -439,6 +440,16 @@ static void dell_wmi_notify(struct wmi_device *wdev,
dell_wmi_process_key(wdev, buffer_entry[1],
buffer_entry[i]);
break;
+ case 0x0012:
+ ret = dell_privacy_present();
+ if ((ret) && (len > 2)) {
+ dell_privacy_process_event(buffer_entry[1],
+ buffer_entry[3], buffer_entry[4]);
+ } else if (len > 2) {
+ dell_wmi_process_key(wdev, buffer_entry[1],
+ buffer_entry[2]);
+ }
+ break;
default: /* Unknown event */
pr_info("Unknown WMI event type 0x%x\n",
(int)buffer_entry[1]);
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v6 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
2021-04-04 8:31 [PATCH v6 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy Perry Yuan
@ 2021-04-04 16:53 ` Barnabás Pőcze
2021-04-05 15:17 ` Limonciello, Mario
2021-04-07 15:15 ` Hans de Goede
0 siblings, 2 replies; 5+ messages in thread
From: Barnabás Pőcze @ 2021-04-04 16:53 UTC (permalink / raw)
To: Perry Yuan
Cc: pierre-louis.bossart, oder_chiou, perex, tiwai, hdegoede, mgross,
Mario.Limonciello, lgirdwood, broonie, alsa-devel, linux-kernel,
platform-driver-x86, mario.limonciello, Dell.Client.Kernel
Hi
2021. április 4., vasárnap 10:31 keltezéssel, Perry Yuan írta:
> From: Perry Yuan <perry_yuan@dell.com>
>
> add support for Dell privacy driver for the Dell units equipped
> hardware privacy design, which protect users privacy of audio and
> camera from hardware level. Once the audio or camera privacy mode
> activated, any applications will not get any audio or video stream
> when user pressed ctrl+F4 hotkey, audio privacy mode will be
> enabled, micmute led will be also changed accordingly
> The micmute led is fully controlled by hardware & EC(embedded controller)
> and camera mute hotkey is Ctrl+F9. Currently design only emits
> SW_CAMERA_LENS_COVER event while the camera lens shutter will be
> changed by EC & HW(hardware) control
>
> *The flow is like this:
> 1) User presses key. HW does stuff with this key (timeout timer is started)
> 2) WMI event is emitted from BIOS to kernel
> 3) WMI event is received by dell-privacy
> 4) KEY_MICMUTE emitted from dell-privacy
> 5) Userland picks up key and modifies kcontrol for SW mute
> 6) Codec kernel driver catches and calls ledtrig_audio_set, like this:
> ledtrig_audio_set(LED_AUDIO_MICMUTE, rt715->micmute_led ? LED_ON :LED_OFF);
> 7) If "LED" is set to on dell-privacy notifies EC, and timeout is cancelled,
> HW mic mute activated. If EC not notified, HW mic mute will also be
> activated when timeout used up, it is just later than active ack
>
> Signed-off-by: Perry Yuan <perry_yuan@dell.com>
> ---
> v5 -> v6:
> * addressed feedback from Hans
> * addressed feedback from Pierre
> * optimize some debug format with dev_dbg()
> * remove platform driver,combined privacy acpi driver into single wmi
> driver file
> * optimize sysfs interface with string added to be more clearly reading
> v4 -> v5:
> * addressed feedback from Randy Dunlap
> * addressed feedback from Pierre-Louis Bossart
> * rebase to latest 5.12 rc4 upstream kernel
> * fix some space alignment problem
> v3 -> v4:
> * fix format for Kconfig
> * add sysfs document
> * add flow comments to the privacy wmi/acpi driver
> * addressed feedback from Barnabás Pőcze[Thanks very much]
> * export privacy_valid to make the global state simpler to query
> * fix one issue which will block the dell-laptop driver to load when
> privacy driver invalid
> * addressed feedback from Pierre-Louis Bossart,remove the EC ID match
> v2 -> v3:
> * add sysfs attributes doc
> v1 -> v2:
> * query EC handle from EC driver directly.
> * fix some code style.
> * add KEY_END to keymap array.
> * clean platform device when cleanup called
> * use hexadecimal format for log print in dev_dbg
> * remove __set_bit for the report keys from probe.
> * fix keymap leak
> * add err_free_keymap in dell_privacy_wmi_probe
> * wmi driver will be unregistered if privacy_acpi_init() fails
> * add sysfs attribute files for user space query.
> * add leds micmute driver to privacy acpi
> * add more design info the commit info
> ---
> ---
> .../testing/sysfs-platform-dell-privacy-wmi | 55 +++
> drivers/platform/x86/dell/Kconfig | 14 +
> drivers/platform/x86/dell/Makefile | 1 +
> drivers/platform/x86/dell/dell-laptop.c | 23 +-
> drivers/platform/x86/dell/dell-privacy-wmi.c | 441 ++++++++++++++++++
> drivers/platform/x86/dell/dell-privacy-wmi.h | 32 ++
> drivers/platform/x86/dell/dell-wmi.c | 13 +-
> 7 files changed, 572 insertions(+), 7 deletions(-)
> create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
> create mode 100644 drivers/platform/x86/dell/dell-privacy-wmi.c
> create mode 100644 drivers/platform/x86/dell/dell-privacy-wmi.h
>
> diff --git a/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi b/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
> new file mode 100644
> index 000000000000..a57ddc6a221e
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
> @@ -0,0 +1,55 @@
> +What: /sys/bus/wmi/devices/6932965F-1671-4CEB-B988-D3AB0A901919/dell_privacy_supported_type
> +Date: Apr 2021
> +KernelVersion: 5.13
> +Contact: "perry.yuan@dell.com>"
> +Description:
> + Display which dell hardware level privacy devices are supported
> + “Dell Privacy” is a set of HW, FW, and SW features to enhance
> + Dell’s commitment to platform privacy for MIC, Camera, and
> + ePrivacy screens.
> + The supported hardware privacy devices are:
> +Attributes:
> + Microphone Mute:
> + Identifies the local microphone can be muted by hardware, no applications
> + is available to capture system mic sound
> +
> + Camera Shutter:
^^^^^^^^^^^^^^^^
Spaces used instead of tabs.
> + Identifies camera shutter controlled by hardware, which is a micromechanical
> + shutter assembly that is built onto the camera module to block capturing images
> + from outside the laptop
> +
> + supported:
> + The privacy device is supported by this system
> +
> + unsupported:
> + The privacy device is not supported on this system
> +
> + For example to check which privacy devices are supported:
> +
> + # cat /sys/bus/wmi/drivers/dell-privacy/6932965F-1671-4CEB-B988-D3AB0A901919/dell_privacy_supported_type
> + [Microphone Mute] [supported]
> + [Camera Shutter] [supported]
> + [ePrivacy Screen] [unsupport]
> +
> +What: /sys/bus/wmi/devices/6932965F-1671-4CEB-B988-D3AB0A901919/dell_privacy_current_state
> +Date: Apr 2021
> +KernelVersion: 5.13
> +Contact: "perry.yuan@dell.com>"
> +Description:
> + Allow user space to check current dell privacy device state.
> + Describes the Device State class exposed by BIOS which can be
> + consumed by various applications interested in knowing the Privacy
> + feature capabilities
> +Attributes:
> + muted:
> + Identifies the privacy device is turned off and cannot send stream to OS applications
> +
> + unmuted:
> + Identifies the privacy device is turned on ,audio or camera driver can get
> + stream from mic and camera module to OS applications
> +
> + For example to check all supported current privacy device states:
> +
> + # cat /sys/bus/wmi/drivers/dell-privacy/6932965F-1671-4CEB-B988-D3AB0A901919/dell_privacy_current_state
> + [Microphone] [unmuted]
> + [Camera Shutter] [unmuted]
> diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
> index e0a55337f51a..05d124442b25 100644
> --- a/drivers/platform/x86/dell/Kconfig
> +++ b/drivers/platform/x86/dell/Kconfig
> @@ -204,4 +204,18 @@ config DELL_WMI_SYSMAN
> To compile this driver as a module, choose M here: the module will
> be called dell-wmi-sysman.
>
> +config DELL_PRIVACY
> + tristate "Dell Hardware Privacy Support"
> + depends on ACPI
> + depends on ACPI_WMI
> + depends on INPUT
> + depends on DELL_LAPTOP
> + depends on LEDS_TRIGGER_AUDIO
> + select DELL_WMI
> + help
> + This driver provides integration with the "Dell Hardware Privacy" feature
> + of Dell laptops.
> +
> + To compile this driver as a module, choose M here: the module will
> + be called dell_privacy.
> endif # X86_PLATFORM_DRIVERS_DELL
> diff --git a/drivers/platform/x86/dell/Makefile b/drivers/platform/x86/dell/Makefile
> index d720a3e42ae3..7da0c33dfcca 100644
> --- a/drivers/platform/x86/dell/Makefile
> +++ b/drivers/platform/x86/dell/Makefile
> @@ -19,3 +19,4 @@ obj-$(CONFIG_DELL_WMI_AIO) += dell-wmi-aio.o
> obj-$(CONFIG_DELL_WMI_DESCRIPTOR) += dell-wmi-descriptor.o
> obj-$(CONFIG_DELL_WMI_LED) += dell-wmi-led.o
> obj-$(CONFIG_DELL_WMI_SYSMAN) += dell-wmi-sysman/
> +obj-$(CONFIG_DELL_PRIVACY) += dell-privacy-wmi.o
> diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
> index 70edc5bb3a14..a9b4cf13913c 100644
> --- a/drivers/platform/x86/dell/dell-laptop.c
> +++ b/drivers/platform/x86/dell/dell-laptop.c
> @@ -31,6 +31,8 @@
> #include "dell-rbtn.h"
> #include "dell-smbios.h"
>
> +#include "dell-privacy-wmi.h"
> +
> struct quirk_entry {
> bool touchpad_led;
> bool kbd_led_not_present;
> @@ -90,6 +92,7 @@ static struct rfkill *wifi_rfkill;
> static struct rfkill *bluetooth_rfkill;
> static struct rfkill *wwan_rfkill;
> static bool force_rfkill;
> +static bool has_privacy;
>
> module_param(force_rfkill, bool, 0444);
> MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted models");
> @@ -2206,10 +2209,16 @@ static int __init dell_init(void)
>
> if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
> dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) {
> - micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
> - ret = led_classdev_register(&platform_device->dev, &micmute_led_cdev);
> - if (ret < 0)
> - goto fail_led;
> + if (dell_privacy_has_micmute())
> + has_privacy = true;
> + else
> + has_privacy = false;
> + if (!has_privacy) {
> + micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
> + ret = led_classdev_register(&platform_device->dev, &micmute_led_cdev);
> + if (ret < 0)
> + goto fail_led;
> + }
> }
>
> if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
> @@ -2257,7 +2266,8 @@ static int __init dell_init(void)
> fail_get_brightness:
> backlight_device_unregister(dell_backlight_device);
> fail_backlight:
> - led_classdev_unregister(&micmute_led_cdev);
> + if (!has_privacy)
> + led_classdev_unregister(&micmute_led_cdev);
> fail_led:
> dell_cleanup_rfkill();
> fail_rfkill:
> @@ -2278,7 +2288,8 @@ static void __exit dell_exit(void)
> touchpad_led_exit();
> kbd_led_exit();
> backlight_device_unregister(dell_backlight_device);
> - led_classdev_unregister(&micmute_led_cdev);
> + if (!has_privacy)
> + led_classdev_unregister(&micmute_led_cdev);
> dell_cleanup_rfkill();
> if (platform_device) {
> platform_device_unregister(platform_device);
> diff --git a/drivers/platform/x86/dell/dell-privacy-wmi.c b/drivers/platform/x86/dell/dell-privacy-wmi.c
> new file mode 100644
> index 000000000000..3fae8dd077f1
> --- /dev/null
> +++ b/drivers/platform/x86/dell/dell-privacy-wmi.c
> @@ -0,0 +1,441 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Dell privacy notification driver
> + *
> + * Copyright (C) 2021 Dell Inc. All Rights Reserved.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/acpi.h>
> +#include <linux/bitops.h>
> +#include <linux/input.h>
> +#include <linux/input/sparse-keymap.h>
> +#include <linux/list.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/wmi.h>
> +
> +#include "dell-privacy-wmi.h"
> +
> +#define DELL_PRIVACY_GUID "6932965F-1671-4CEB-B988-D3AB0A901919"
> +#define MICROPHONE_STATUS BIT(0)
> +#define CAMERA_STATUS BIT(1)
> +#define DELL_PRIVACY_AUDIO_EVENT 0x1
> +#define DELL_PRIVACY_CAMERA_EVENT 0x2
> +#define cdev_to_led(c) container_of(c, struct privacy_wmi_data, cdev)
This name is confusing in my opinion. Something like `led_to_priv` could be
clearer.
> +
> +/*
> + * The wmi_list is used to store the privacy_priv struct with mutex protecting
> + */
> +static LIST_HEAD(wmi_list);
> +static DEFINE_MUTEX(list_mutex);
> +
> +struct privacy_wmi_data {
> + struct input_dev *input_dev;
> + struct wmi_device *wdev;
> + struct list_head list;
> + struct led_classdev cdev;
> + u32 features_present;
> + u32 last_status;
> +};
> +
> +/* DELL Privacy Type */
> +enum dell_hardware_privacy_type {
> + DELL_PRIVACY_TYPE_AUDIO = 0,
> + DELL_PRIVACY_TYPE_CAMERA,
> + DELL_PRIVACY_TYPE_SCREEN,
> + DELL_PRIVACY_TYPE_MAX,
> +};
> +
> +static const char * const privacy_types[DELL_PRIVACY_TYPE_MAX] = {
> + [DELL_PRIVACY_TYPE_AUDIO] = "Microphone",
> + [DELL_PRIVACY_TYPE_CAMERA] = "Camera Shutter",
> + [DELL_PRIVACY_TYPE_SCREEN] = "ePrivacy Screen",
> +};
> +
> +/*
> + * Keymap for WMI privacy events of type 0x0012
> + */
> +static const struct key_entry dell_wmi_keymap_type_0012[] = {
> + /* privacy mic mute */
> + { KE_KEY, 0x0001, { KEY_MICMUTE } },
> + /* privacy camera mute */
> + { KE_SW, 0x0002, { SW_CAMERA_LENS_COVER } },
> + { KE_END, 0},
> +};
> +
> +/*
> + * global privacy state for other modules to query if the privacy driver loaded successfully
> + * or no privacy WMI device was detected at initial probe phase
> + */
> +bool dell_privacy_present(void)
> +{
> + struct privacy_wmi_data *priv;
> +
> + mutex_lock(&list_mutex);
> + priv = list_first_entry_or_null(&wmi_list,
> + struct privacy_wmi_data,
> + list);
> + mutex_unlock(&list_mutex);
> +
> + return priv;
> +}
> +EXPORT_SYMBOL_GPL(dell_privacy_present);
> +
> +/*
> + * Allow other modules to check current dell privacy mic mute state
> + */
> +int dell_privacy_has_micmute(void)
> +{
> + struct privacy_wmi_data *priv;
> + int ret;
> +
> + mutex_lock(&list_mutex);
> + priv = list_first_entry_or_null(&wmi_list,
> + struct privacy_wmi_data,
> + list);
> + /*
> + * There is no privacy device node yet, so we need to wait
> + * until the privacy driver has created software node for the micmute device.
> + * This function will return -EPROBE_DEFER until the privacy device is registered.
> + */
> + if (!priv) {
> + ret = -EPROBE_DEFER;
> + goto out;
> + }
> + ret = priv->features_present & MICROPHONE_STATUS;
> + if (!ret) {
> + dev_err(&priv->wdev->dev, "privacy mic mute feature is not supported: %d\n", ret);
> + ret = -ENODEV;
> + goto out;
> + }
> + dev_err(&priv->wdev->dev, "privacy mic mute feature is supported: %d\n", ret);
I think this could be `dev_info()`, but definitely not `dev_err()`. Although I'd
personally move the logging from here to the probe function if you want to log
which features are available. `ret` is necessarily 1 here, so I don't think printing it
provides additional information.
> +
> +out:
> + mutex_unlock(&list_mutex);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(dell_privacy_has_micmute);
> +
> +/*
> + * The flow of privacy event:
> + * 1) User presses key. HW does stuff with this key (timeout is started)
> + * 2) WMI event is emitted from BIOS
> + * 3) WMI event is received by dell-privacy
> + * 4) KEY_MICMUTE emitted from dell-privacy
> + * 5) Userland picks up key and modifies kcontrol for SW mute
> + * 6) Codec kernel driver catches and calls ledtrig_audio_set defined by
> + * dell-privacy-acpi driver. Codec driver will call like this to switch micmute led state.
> + * ledtrig_audio_set(LED_AUDIO_MICMUTE, micmute_led ? LED_ON :LED_OFF);
> + * 7) If "LED" is set to on dell-privacy notifies EC,and timeout is cancelled,
> + * HW mic mute activated.
> + */
> +bool dell_privacy_process_event(int type, int code, int status)
> +{
> + struct privacy_wmi_data *priv;
> + const struct key_entry *key;
> + bool ret = false;
> +
> + mutex_lock(&list_mutex);
> + priv = list_first_entry_or_null(&wmi_list,
> + struct privacy_wmi_data,
> + list);
> + if (!priv) {
> + dev_err(&priv->wdev->dev, "priv data is NULL\n");
> + goto error;
> + }
> +
I think the rest of the function could be replaced with:
ret = sparse_keymap_report_event(priv->input_dev, DELL_SCAN_CODE(type, code), 1, true)
if (ret)
priv->last_status = status;
error:
[...]
(see later a comment for the definition of DELL_SCAN_CODE())
> + key = sparse_keymap_entry_from_scancode(priv->input_dev, (type << 16) | code);
> + if (!key) {
> + dev_warn(&priv->wdev->dev, "Unknown key with type 0x%04x and code 0x%04x pressed\n",
> + type, code);
> + goto error;
> + }
> + dev_dbg(&priv->wdev->dev, "Key with type 0x%04x and code 0x%04x pressed\n", type, code);
> +
> + switch (code) {
> + case DELL_PRIVACY_AUDIO_EVENT: /* Mic mute */
> + case DELL_PRIVACY_CAMERA_EVENT: /* Camera mute */
> + priv->last_status = status;
> + sparse_keymap_report_entry(priv->input_dev, key, 1, true);
> + ret = true;
> + break;
> + default:
> + dev_dbg(&priv->wdev->dev, "unknown event type 0x%04x 0x%04x", type, code);
The capitalization is inconsistent. Please either make all messages lowercase or
make them all start with an uppercase letter. (And a newline character is missing.)
> + }
> +
> +error:
> + mutex_unlock(&list_mutex);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(dell_privacy_process_event);
> +
> +static ssize_t dell_privacy_supported_type_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + enum dell_hardware_privacy_type type;
> + struct privacy_wmi_data *priv = dev_get_drvdata(dev);
> + char *s = buf;
> + u32 privacy_list;
> +
> + privacy_list = priv->features_present;
> + for (type = DELL_PRIVACY_TYPE_AUDIO; type < DELL_PRIVACY_TYPE_MAX; type++) {
> + if (privacy_types[type]) {
Is this check necessary?
> + if (privacy_list & BIT(type))
> + s += sprintf(s, "[%s] [supported]\n", privacy_types[type]);
> + else
> + s += sprintf(s, "[%s] [unsupport]\n", privacy_types[type]);
You can use `sysfs_emit_at()` here.
> + }
> + }
> +
> + if (s != buf)
> + /* convert the last space to a newline */
> + *(s-1) = '\n';
I believe this is not needed?
> + return (s - buf);
> +}
> +
> +static ssize_t dell_privacy_current_state_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct privacy_wmi_data *priv = dev_get_drvdata(dev);
> + enum dell_hardware_privacy_type type;
> + u32 privacy_state = priv->last_status;
> + u32 privacy_supported = priv->features_present;
> + char *s = buf;
> +
> + for (type = DELL_PRIVACY_TYPE_AUDIO; type < DELL_PRIVACY_TYPE_MAX; type++) {
> + if (privacy_supported & BIT(type)) {
> + if (privacy_state & BIT(type))
> + s += sprintf(s, "[%s] [unmuted]\n", privacy_types[type]);
> + else
> + s += sprintf(s, "[%s] [muted]\n", privacy_types[type]);
sysfs_emit_at
> + }
> + }
> +
> + if (s != buf)
> + /* convert the last space to a newline */
> + *(s-1) = '\n';
not needed?
> + return (s - buf);
> +}
> +
> +static DEVICE_ATTR_RO(dell_privacy_supported_type);
> +static DEVICE_ATTR_RO(dell_privacy_current_state);
> +
> +static struct attribute *privacy_attributes[] = {
> + &dev_attr_dell_privacy_supported_type.attr,
> + &dev_attr_dell_privacy_current_state.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group privacy_attribute_group = {
> + .attrs = privacy_attributes
> +};
> +
> +/*
> + * Describes the Device State class exposed by BIOS which can be consumed by
> + * various applications interested in knowing the Privacy feature capabilities.
> + * class DeviceState
> + * {
> + * [key, read] string InstanceName;
> + * [read] boolean ReadOnly;
> + * [WmiDataId(1), read] uint32 DevicesSupported;
> + * 0 – None, 0x1 – Microphone, 0x2 – Camera, 0x4 -ePrivacy Screen
^ ^ ^ ^ ^^
Please use a single type of hyphen/dash consistently. One space is enough.
> + * [WmiDataId(2), read] uint32 CurrentState;
> + * 0:Off; 1:On. Bit0 – Microphone, Bit1 – Camera, Bit2 - ePrivacyScreen
^ ^ ^
Same here.
> + * };
> + */
> +static int get_current_status(struct wmi_device *wdev)
> +{
> + struct privacy_wmi_data *priv = dev_get_drvdata(&wdev->dev);
> + union acpi_object *obj_present;
> + u32 *buffer;
> + int ret = 0;
> +
> + if (!priv) {
> + pr_err("dell privacy priv is NULL\n");
dev_err?
> + return -EINVAL;
> + }
> + /* check privacy support features and device states */
> + obj_present = wmidev_block_query(wdev, 0);
> + if (!obj_present) {
> + dev_err(&wdev->dev, "failed to read Binary MOF\n");
> + ret = -EIO;
> + return ret;
return -EIO ?
> + }
> +
> + if (obj_present->type != ACPI_TYPE_BUFFER) {
> + dev_err(&wdev->dev, "Binary MOF is not a buffer!\n");
> + ret = -EIO;
> + goto obj_free;
> + }
> + /* Although it's not technically a failure, this would lead to
> + * unexpected behavior
> + */
> + if (obj_present->buffer.length != 8) {
> + dev_err(&wdev->dev, "Dell privacy buffer has unexpected length (%d)!\n",
> + obj_present->buffer.length);
> + ret = -EINVAL;
> + goto obj_free;
> + }
> + buffer = (u32 *)obj_present->buffer.pointer;
> + priv->features_present = buffer[0];
> + priv->last_status = buffer[1];
It's a minor thing, but I still think it'd more explicit and somewhat better to
use `get_unaligned_le32()` (or `get_unaligned_cpu32()`).
> +
> +obj_free:
> + kfree(obj_present);
> + return ret;
> +}
> +
> +static int dell_privacy_micmute_led_set(struct led_classdev *led_cdev,
> + enum led_brightness brightness)
> +{
> + struct privacy_wmi_data *priv = cdev_to_led(led_cdev);
> + static char *acpi_method = (char *)"ECAK";
> + acpi_status status;
> + acpi_handle handle;
> +
> + handle = ec_get_handle();
> + if (!handle)
> + return -EIO;
> +
> + if (!acpi_has_method(handle, acpi_method))
> + return -EIO;
> +
> + status = acpi_evaluate_object(handle, acpi_method, NULL, NULL);
> + if (ACPI_FAILURE(status)) {
> + dev_err(&priv->wdev->dev, "Error setting privacy EC ack value: %s\n",
> + acpi_format_exception(status));
> + return -EIO;
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * Pressing the mute key activates a time delayed circuit to physically cut
> + * off the mute. The LED is in the same circuit, so it reflects the true
> + * state of the HW mute. The reason for the EC "ack" is so that software
> + * can first invoke a SW mute before the HW circuit is cut off. Without SW
> + * cutting this off first does not affect the time delayed muting or status
> + * of the LED but there is a possibility of a "popping" noise.
> + *
> + * If the EC receives the SW ack, the circuit will be activated before the
> + * delay completed.
> + *
> + * Exposing as an LED device allows the codec drivers notification path to
> + * EC ACK to work
> + */
> +static int dell_privacy_leds_setup(struct device *dev)
> +{
> + struct privacy_wmi_data *priv = dev_get_drvdata(dev);
> + int ret;
> +
> + priv->cdev.name = "dell-privacy::micmute";
> + priv->cdev.max_brightness = 1;
> + priv->cdev.brightness_set_blocking = dell_privacy_micmute_led_set;
> + priv->cdev.default_trigger = "audio-micmute";
> + priv->cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
> + ret = devm_led_classdev_register(dev, &priv->cdev);
> + if (ret)
> + return ret;
> + return 0;
You can replace the last four lines with:
return devm_led_classdev_register(...);
> +}
> +
> +static int dell_privacy_wmi_probe(struct wmi_device *wdev, const void *context)
> +{
> + struct privacy_wmi_data *priv;
> + struct key_entry *keymap;
> + int ret, i;
> +
> + ret = wmi_has_guid(DELL_PRIVACY_GUID);
> + if (!ret)
> + pr_debug("Unable to detect available Dell privacy devices: %d\n", ret);
When this branch is taken, `ret` is necessarily zero, so I don't think printing it
provides useful information.
And I believe this `wmi_has_guid()` check is unnecessary since the probe method
would not be called if the device didn't have such WMI guid if I'm not mistaken.
> +
> + priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + dev_set_drvdata(&wdev->dev, priv);
> + priv->wdev = wdev;
> + /* create evdev passing interface */
> + priv->input_dev = devm_input_allocate_device(&wdev->dev);
> + if (!priv->input_dev)
> + return -ENOMEM;
> +
> + /* remap the wmi keymap event to new keymap */
> + keymap = kcalloc(ARRAY_SIZE(dell_wmi_keymap_type_0012),
> + sizeof(struct key_entry), GFP_KERNEL);
> + if (!keymap)
> + return -ENOMEM;
> +
> + /* remap the keymap code with Dell privacy key type 0x12 as prefix
> + * KEY_MICMUTE scancode will be reported as 0x120001
> + */
> + for (i = 0; i < ARRAY_SIZE(dell_wmi_keymap_type_0012); i++) {
> + keymap[i] = dell_wmi_keymap_type_0012[i];
> + keymap[i].code |= (0x0012 << 16);
> + }
I still don't see the need for allocating and copying the keymap. Wouldn't the
following be sufficient?
#define DELL_SCAN_CODE(type, code) ((type) << 16 | (code))
static const struct key_entry dell_wmi_keymap_type_0012[] = {
{ KE_KEY, DELL_SCAN_CODE(0x0012, 0x0001), { KEY_MICMUTE } },
{ KE_SW, DELL_SCAN_CODE(0x0012, 0x0002), { SW_CAMERA_LENS_COVER } },
{ KE_END, 0},
};
Other Dell drivers potentially merge multiple keymaps, so dynamically allocating
the key_entry array is justified. Here I see no such need. Can you explain why
this copying is done?
> + ret = sparse_keymap_setup(priv->input_dev, keymap, NULL);
> + if (ret)
> + return ret;
`keymap` is leaked if this or any of the later early-returns are taken.
> +
> + priv->input_dev->dev.parent = &wdev->dev;
> + priv->input_dev->name = "Dell Privacy Driver";
> + priv->input_dev->id.bustype = BUS_HOST;
> +
> + ret = input_register_device(priv->input_dev);
> + if (ret)
> + return ret;
> +
> + ret = get_current_status(priv->wdev);
> + if (ret)
> + return ret;
> +
> + ret = devm_device_add_group(&wdev->dev, &privacy_attribute_group);
> + if (ret)
> + return ret;
> +
> + ret = dell_privacy_leds_setup(&priv->wdev->dev);
> + if (ret)
> + return ret;
> +
> + mutex_lock(&list_mutex);
> + list_add_tail(&priv->list, &wmi_list);
> + mutex_unlock(&list_mutex);
> + kfree(keymap);
> + return 0;
> +}
> +
> +static int dell_privacy_wmi_remove(struct wmi_device *wdev)
> +{
> + struct privacy_wmi_data *priv = dev_get_drvdata(&wdev->dev);
> +
> + mutex_lock(&list_mutex);
> + list_del(&priv->list);
> + mutex_unlock(&list_mutex);
> + return 0;
> +}
> +
> +static const struct wmi_device_id dell_wmi_privacy_wmi_id_table[] = {
> + { .guid_string = DELL_PRIVACY_GUID },
> + { },
> +};
> +
> +static struct wmi_driver dell_privacy_wmi_driver = {
> + .driver = {
> + .name = "dell-privacy",
> + },
> + .probe = dell_privacy_wmi_probe,
> + .remove = dell_privacy_wmi_remove,
> + .id_table = dell_wmi_privacy_wmi_id_table,
> +};
> +
> +module_wmi_driver(dell_privacy_wmi_driver);
> +
> +MODULE_DEVICE_TABLE(wmi, dell_wmi_privacy_wmi_id_table);
> +MODULE_AUTHOR("Perry Yuan <perry_yuan@dell.com>");
> +MODULE_DESCRIPTION("Dell Privacy WMI Driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/platform/x86/dell/dell-privacy-wmi.h b/drivers/platform/x86/dell/dell-privacy-wmi.h
> new file mode 100644
> index 000000000000..a24893754286
> --- /dev/null
> +++ b/drivers/platform/x86/dell/dell-privacy-wmi.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Dell privacy notification driver
> + *
> + * Copyright (C) 2021 Dell Inc. All Rights Reserved.
> + */
> +
> +#ifndef _DELL_PRIVACY_WMI_H_
> +#define _DELL_PRIVACY_WMI_H_
> +
> +#if IS_ENABLED(CONFIG_DELL_PRIVACY)
> +int dell_privacy_has_micmute(void);
> +bool dell_privacy_present(void);
> +bool dell_privacy_process_event(int type, int code, int status);
> +#else /* CONFIG_DELL_PRIVACY */
> +static inline int dell_privacy_has_micmute(void)
> +{
> + return -ENODEV;
> +}
> +
> +static inline int dell_privacy_present(void)
^^^
It's declared with `bool` return type a couple lines above.
> +{
> + return -ENODEV;
> +}
> +
> +static inline void dell_privacy_process_event(int type, int code, int status)
^^^^
It's declared with `bool` return type a couple lines above.
> +{}
> +#endif /* CONFIG_DELL_PRIVACY */
> +
> +int dell_privacy_acpi_init(void);
> +void dell_privacy_acpi_exit(void);
These two don't seem to be referenced anywhere?
> +#endif
> diff --git a/drivers/platform/x86/dell/dell-wmi.c b/drivers/platform/x86/dell/dell-wmi.c
> index bbdb3e860892..8ef9e22a538f 100644
> --- a/drivers/platform/x86/dell/dell-wmi.c
> +++ b/drivers/platform/x86/dell/dell-wmi.c
> @@ -27,6 +27,7 @@
> #include <acpi/video.h>
> #include "dell-smbios.h"
> #include "dell-wmi-descriptor.h"
> +#include "dell-privacy-wmi.h"
>
> MODULE_AUTHOR("Matthew Garrett <mjg@redhat.com>");
> MODULE_AUTHOR("Pali Rohár <pali@kernel.org>");
> @@ -381,6 +382,7 @@ static void dell_wmi_notify(struct wmi_device *wdev,
> u16 *buffer_entry, *buffer_end;
> acpi_size buffer_size;
> int len, i;
> + int ret;
>
> if (obj->type != ACPI_TYPE_BUFFER) {
> pr_warn("bad response type %x\n", obj->type);
> @@ -427,7 +429,6 @@ static void dell_wmi_notify(struct wmi_device *wdev,
>
> switch (buffer_entry[1]) {
> case 0x0000: /* One key pressed or event occurred */
> - case 0x0012: /* Event with extended data occurred */
> if (len > 2)
> dell_wmi_process_key(wdev, buffer_entry[1],
> buffer_entry[2]);
> @@ -439,6 +440,16 @@ static void dell_wmi_notify(struct wmi_device *wdev,
> dell_wmi_process_key(wdev, buffer_entry[1],
> buffer_entry[i]);
> break;
> + case 0x0012:
> + ret = dell_privacy_present();
> + if ((ret) && (len > 2)) {
Is the `len > 2` check correct?
Moreover, I personally don't see any reason to use a new variable here (`ret`).
If you incorporate the `dell_privacy_present()` check into
`dell_privacy_process_event()`, then even
if (len > ?? && dell_privacy_process_event(...))
/* nothing */ ;
else if (len > 2)
dell_wmi_process_key(...);
would work as Hans has already pointed it out. And there'd be no need for
`dell_privacy_present()` anymore.
> + dell_privacy_process_event(buffer_entry[1],
> + buffer_entry[3], buffer_entry[4]);
> + } else if (len > 2) {
> + dell_wmi_process_key(wdev, buffer_entry[1],
> + buffer_entry[2]);
> + }
> + break;
> default: /* Unknown event */
> pr_info("Unknown WMI event type 0x%x\n",
> (int)buffer_entry[1]);
> --
> 2.25.1
>
Regards,
Barnabás Pőcze
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH v6 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
2021-04-04 16:53 ` Barnabás Pőcze
@ 2021-04-05 15:17 ` Limonciello, Mario
2021-04-07 15:15 ` Hans de Goede
1 sibling, 0 replies; 5+ messages in thread
From: Limonciello, Mario @ 2021-04-05 15:17 UTC (permalink / raw)
To: Barnabás Pőcze, Yuan, Perry
Cc: pierre-louis.bossart, oder_chiou, perex, tiwai, hdegoede, mgross,
lgirdwood, broonie, alsa-devel, linux-kernel,
platform-driver-x86, mario.limonciello, Dell Client Kernel
>
> I think this could be `dev_info()`, but definitely not `dev_err()`. Although
> I'd
> personally move the logging from here to the probe function if you want to log
> which features are available. `ret` is necessarily 1 here, so I don't think
> printing it
> provides additional information.
To the layman I would say dev_info is too noisy actually. I think debugging
(dev_dbg) would be just fine. If there is a function not working, adding dynamic
debugging on kernel command line of modprobe line will be plenty sufficient to get
this information.
>
>
> > +
> > +out:
> > + mutex_unlock(&list_mutex);
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(dell_privacy_has_micmute);
> > +
> > +/*
> > + * The flow of privacy event:
> > + * 1) User presses key. HW does stuff with this key (timeout is started)
> > + * 2) WMI event is emitted from BIOS
> > + * 3) WMI event is received by dell-privacy
> > + * 4) KEY_MICMUTE emitted from dell-privacy
> > + * 5) Userland picks up key and modifies kcontrol for SW mute
> > + * 6) Codec kernel driver catches and calls ledtrig_audio_set defined by
> > + * dell-privacy-acpi driver. Codec driver will call like this to switch
> micmute led state.
> > + * ledtrig_audio_set(LED_AUDIO_MICMUTE, micmute_led ? LED_ON :LED_OFF);
> > + * 7) If "LED" is set to on dell-privacy notifies EC,and timeout is
> cancelled,
> > + * HW mic mute activated.
> > + */
> > +bool dell_privacy_process_event(int type, int code, int status)
> > +{
> > + struct privacy_wmi_data *priv;
> > + const struct key_entry *key;
> > + bool ret = false;
> > +
> > + mutex_lock(&list_mutex);
> > + priv = list_first_entry_or_null(&wmi_list,
> > + struct privacy_wmi_data,
> > + list);
> > + if (!priv) {
> > + dev_err(&priv->wdev->dev, "priv data is NULL\n");
> > + goto error;
> > + }
> > +
>
> I think the rest of the function could be replaced with:
>
> ret = sparse_keymap_report_event(priv->input_dev, DELL_SCAN_CODE(type,
> code), 1, true)
>
> if (ret)
> priv->last_status = status;
>
> error:
> [...]
>
> (see later a comment for the definition of DELL_SCAN_CODE())
>
>
> > + key = sparse_keymap_entry_from_scancode(priv->input_dev, (type << 16) |
> code);
> > + if (!key) {
> > + dev_warn(&priv->wdev->dev, "Unknown key with type 0x%04x and code
> 0x%04x pressed\n",
> > + type, code);
> > + goto error;
> > + }
> > + dev_dbg(&priv->wdev->dev, "Key with type 0x%04x and code 0x%04x
> pressed\n", type, code);
> > +
> > + switch (code) {
> > + case DELL_PRIVACY_AUDIO_EVENT: /* Mic mute */
> > + case DELL_PRIVACY_CAMERA_EVENT: /* Camera mute */
> > + priv->last_status = status;
> > + sparse_keymap_report_entry(priv->input_dev, key, 1, true);
> > + ret = true;
> > + break;
> > + default:
> > + dev_dbg(&priv->wdev->dev, "unknown event type 0x%04x 0x%04x", type,
> code);
>
> The capitalization is inconsistent. Please either make all messages lowercase
> or
> make them all start with an uppercase letter. (And a newline character is
> missing.)
>
>
> > + }
> > +
> > +error:
> > + mutex_unlock(&list_mutex);
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(dell_privacy_process_event);
> > +
> > +static ssize_t dell_privacy_supported_type_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + enum dell_hardware_privacy_type type;
> > + struct privacy_wmi_data *priv = dev_get_drvdata(dev);
> > + char *s = buf;
> > + u32 privacy_list;
> > +
> > + privacy_list = priv->features_present;
> > + for (type = DELL_PRIVACY_TYPE_AUDIO; type < DELL_PRIVACY_TYPE_MAX;
> type++) {
> > + if (privacy_types[type]) {
>
> Is this check necessary?
>
>
> > + if (privacy_list & BIT(type))
> > + s += sprintf(s, "[%s] [supported]\n",
> privacy_types[type]);
> > + else
> > + s += sprintf(s, "[%s] [unsupport]\n",
> privacy_types[type]);
>
> You can use `sysfs_emit_at()` here.
>
>
> > + }
> > + }
> > +
> > + if (s != buf)
> > + /* convert the last space to a newline */
> > + *(s-1) = '\n';
>
> I believe this is not needed?
>
>
> > + return (s - buf);
> > +}
> > +
> > +static ssize_t dell_privacy_current_state_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct privacy_wmi_data *priv = dev_get_drvdata(dev);
> > + enum dell_hardware_privacy_type type;
> > + u32 privacy_state = priv->last_status;
> > + u32 privacy_supported = priv->features_present;
> > + char *s = buf;
> > +
> > + for (type = DELL_PRIVACY_TYPE_AUDIO; type < DELL_PRIVACY_TYPE_MAX;
> type++) {
> > + if (privacy_supported & BIT(type)) {
> > + if (privacy_state & BIT(type))
> > + s += sprintf(s, "[%s] [unmuted]\n",
> privacy_types[type]);
> > + else
> > + s += sprintf(s, "[%s] [muted]\n", privacy_types[type]);
>
> sysfs_emit_at
>
>
> > + }
> > + }
> > +
> > + if (s != buf)
> > + /* convert the last space to a newline */
> > + *(s-1) = '\n';
>
> not needed?
>
>
> > + return (s - buf);
> > +}
> > +
> > +static DEVICE_ATTR_RO(dell_privacy_supported_type);
> > +static DEVICE_ATTR_RO(dell_privacy_current_state);
> > +
> > +static struct attribute *privacy_attributes[] = {
> > + &dev_attr_dell_privacy_supported_type.attr,
> > + &dev_attr_dell_privacy_current_state.attr,
> > + NULL,
> > +};
> > +
> > +static const struct attribute_group privacy_attribute_group = {
> > + .attrs = privacy_attributes
> > +};
> > +
> > +/*
> > + * Describes the Device State class exposed by BIOS which can be consumed
> by
> > + * various applications interested in knowing the Privacy feature
> capabilities.
> > + * class DeviceState
> > + * {
> > + * [key, read] string InstanceName;
> > + * [read] boolean ReadOnly;
> > + * [WmiDataId(1), read] uint32 DevicesSupported;
> > + * 0 – None, 0x1 – Microphone, 0x2 – Camera, 0x4 -ePrivacy Screen
> ^ ^ ^ ^ ^^
> Please use a single type of hyphen/dash consistently. One space is enough.
>
>
> > + * [WmiDataId(2), read] uint32 CurrentState;
> > + * 0:Off; 1:On. Bit0 – Microphone, Bit1 – Camera, Bit2 - ePrivacyScreen
> ^ ^ ^
> Same here.
>
>
> > + * };
> > + */
> > +static int get_current_status(struct wmi_device *wdev)
> > +{
> > + struct privacy_wmi_data *priv = dev_get_drvdata(&wdev->dev);
> > + union acpi_object *obj_present;
> > + u32 *buffer;
> > + int ret = 0;
> > +
> > + if (!priv) {
> > + pr_err("dell privacy priv is NULL\n");
>
> dev_err?
>
>
> > + return -EINVAL;
> > + }
> > + /* check privacy support features and device states */
> > + obj_present = wmidev_block_query(wdev, 0);
> > + if (!obj_present) {
> > + dev_err(&wdev->dev, "failed to read Binary MOF\n");
> > + ret = -EIO;
> > + return ret;
>
> return -EIO ?
>
>
> > + }
> > +
> > + if (obj_present->type != ACPI_TYPE_BUFFER) {
> > + dev_err(&wdev->dev, "Binary MOF is not a buffer!\n");
> > + ret = -EIO;
> > + goto obj_free;
> > + }
> > + /* Although it's not technically a failure, this would lead to
> > + * unexpected behavior
> > + */
> > + if (obj_present->buffer.length != 8) {
> > + dev_err(&wdev->dev, "Dell privacy buffer has unexpected length
> (%d)!\n",
> > + obj_present->buffer.length);
> > + ret = -EINVAL;
> > + goto obj_free;
> > + }
> > + buffer = (u32 *)obj_present->buffer.pointer;
> > + priv->features_present = buffer[0];
> > + priv->last_status = buffer[1];
>
> It's a minor thing, but I still think it'd more explicit and somewhat better
> to
> use `get_unaligned_le32()` (or `get_unaligned_cpu32()`).
>
>
> > +
> > +obj_free:
> > + kfree(obj_present);
> > + return ret;
> > +}
> > +
> > +static int dell_privacy_micmute_led_set(struct led_classdev *led_cdev,
> > + enum led_brightness brightness)
> > +{
> > + struct privacy_wmi_data *priv = cdev_to_led(led_cdev);
> > + static char *acpi_method = (char *)"ECAK";
> > + acpi_status status;
> > + acpi_handle handle;
> > +
> > + handle = ec_get_handle();
> > + if (!handle)
> > + return -EIO;
> > +
> > + if (!acpi_has_method(handle, acpi_method))
> > + return -EIO;
> > +
> > + status = acpi_evaluate_object(handle, acpi_method, NULL, NULL);
> > + if (ACPI_FAILURE(status)) {
> > + dev_err(&priv->wdev->dev, "Error setting privacy EC ack value:
> %s\n",
> > + acpi_format_exception(status));
> > + return -EIO;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * Pressing the mute key activates a time delayed circuit to physically cut
> > + * off the mute. The LED is in the same circuit, so it reflects the true
> > + * state of the HW mute. The reason for the EC "ack" is so that software
> > + * can first invoke a SW mute before the HW circuit is cut off. Without SW
> > + * cutting this off first does not affect the time delayed muting or status
> > + * of the LED but there is a possibility of a "popping" noise.
> > + *
> > + * If the EC receives the SW ack, the circuit will be activated before the
> > + * delay completed.
> > + *
> > + * Exposing as an LED device allows the codec drivers notification path to
> > + * EC ACK to work
> > + */
> > +static int dell_privacy_leds_setup(struct device *dev)
> > +{
> > + struct privacy_wmi_data *priv = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + priv->cdev.name = "dell-privacy::micmute";
> > + priv->cdev.max_brightness = 1;
> > + priv->cdev.brightness_set_blocking = dell_privacy_micmute_led_set;
> > + priv->cdev.default_trigger = "audio-micmute";
> > + priv->cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
> > + ret = devm_led_classdev_register(dev, &priv->cdev);
> > + if (ret)
> > + return ret;
> > + return 0;
>
> You can replace the last four lines with:
>
> return devm_led_classdev_register(...);
>
>
> > +}
> > +
> > +static int dell_privacy_wmi_probe(struct wmi_device *wdev, const void
> *context)
> > +{
> > + struct privacy_wmi_data *priv;
> > + struct key_entry *keymap;
> > + int ret, i;
> > +
> > + ret = wmi_has_guid(DELL_PRIVACY_GUID);
> > + if (!ret)
> > + pr_debug("Unable to detect available Dell privacy devices: %d\n",
> ret);
>
> When this branch is taken, `ret` is necessarily zero, so I don't think
> printing it
> provides useful information.
>
> And I believe this `wmi_has_guid()` check is unnecessary since the probe
> method
> would not be called if the device didn't have such WMI guid if I'm not
> mistaken.
>
>
> > +
> > + priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + dev_set_drvdata(&wdev->dev, priv);
> > + priv->wdev = wdev;
> > + /* create evdev passing interface */
> > + priv->input_dev = devm_input_allocate_device(&wdev->dev);
> > + if (!priv->input_dev)
> > + return -ENOMEM;
> > +
> > + /* remap the wmi keymap event to new keymap */
> > + keymap = kcalloc(ARRAY_SIZE(dell_wmi_keymap_type_0012),
> > + sizeof(struct key_entry), GFP_KERNEL);
> > + if (!keymap)
> > + return -ENOMEM;
> > +
> > + /* remap the keymap code with Dell privacy key type 0x12 as prefix
> > + * KEY_MICMUTE scancode will be reported as 0x120001
> > + */
> > + for (i = 0; i < ARRAY_SIZE(dell_wmi_keymap_type_0012); i++) {
> > + keymap[i] = dell_wmi_keymap_type_0012[i];
> > + keymap[i].code |= (0x0012 << 16);
> > + }
>
> I still don't see the need for allocating and copying the keymap. Wouldn't the
> following be sufficient?
>
> #define DELL_SCAN_CODE(type, code) ((type) << 16 | (code))
> static const struct key_entry dell_wmi_keymap_type_0012[] = {
> { KE_KEY, DELL_SCAN_CODE(0x0012, 0x0001), { KEY_MICMUTE } },
> { KE_SW, DELL_SCAN_CODE(0x0012, 0x0002), { SW_CAMERA_LENS_COVER } },
> { KE_END, 0},
> };
>
> Other Dell drivers potentially merge multiple keymaps, so dynamically
> allocating
> the key_entry array is justified. Here I see no such need. Can you explain why
> this copying is done?
>
>
> > + ret = sparse_keymap_setup(priv->input_dev, keymap, NULL);
> > + if (ret)
> > + return ret;
>
> `keymap` is leaked if this or any of the later early-returns are taken.
>
>
> > +
> > + priv->input_dev->dev.parent = &wdev->dev;
> > + priv->input_dev->name = "Dell Privacy Driver";
> > + priv->input_dev->id.bustype = BUS_HOST;
> > +
> > + ret = input_register_device(priv->input_dev);
> > + if (ret)
> > + return ret;
> > +
> > + ret = get_current_status(priv->wdev);
> > + if (ret)
> > + return ret;
> > +
> > + ret = devm_device_add_group(&wdev->dev, &privacy_attribute_group);
> > + if (ret)
> > + return ret;
> > +
> > + ret = dell_privacy_leds_setup(&priv->wdev->dev);
> > + if (ret)
> > + return ret;
> > +
> > + mutex_lock(&list_mutex);
> > + list_add_tail(&priv->list, &wmi_list);
> > + mutex_unlock(&list_mutex);
> > + kfree(keymap);
> > + return 0;
> > +}
> > +
> > +static int dell_privacy_wmi_remove(struct wmi_device *wdev)
> > +{
> > + struct privacy_wmi_data *priv = dev_get_drvdata(&wdev->dev);
> > +
> > + mutex_lock(&list_mutex);
> > + list_del(&priv->list);
> > + mutex_unlock(&list_mutex);
> > + return 0;
> > +}
> > +
> > +static const struct wmi_device_id dell_wmi_privacy_wmi_id_table[] = {
> > + { .guid_string = DELL_PRIVACY_GUID },
> > + { },
> > +};
> > +
> > +static struct wmi_driver dell_privacy_wmi_driver = {
> > + .driver = {
> > + .name = "dell-privacy",
> > + },
> > + .probe = dell_privacy_wmi_probe,
> > + .remove = dell_privacy_wmi_remove,
> > + .id_table = dell_wmi_privacy_wmi_id_table,
> > +};
> > +
> > +module_wmi_driver(dell_privacy_wmi_driver);
> > +
> > +MODULE_DEVICE_TABLE(wmi, dell_wmi_privacy_wmi_id_table);
> > +MODULE_AUTHOR("Perry Yuan <perry_yuan@dell.com>");
> > +MODULE_DESCRIPTION("Dell Privacy WMI Driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/platform/x86/dell/dell-privacy-wmi.h
> b/drivers/platform/x86/dell/dell-privacy-wmi.h
> > new file mode 100644
> > index 000000000000..a24893754286
> > --- /dev/null
> > +++ b/drivers/platform/x86/dell/dell-privacy-wmi.h
> > @@ -0,0 +1,32 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Dell privacy notification driver
> > + *
> > + * Copyright (C) 2021 Dell Inc. All Rights Reserved.
> > + */
> > +
> > +#ifndef _DELL_PRIVACY_WMI_H_
> > +#define _DELL_PRIVACY_WMI_H_
> > +
> > +#if IS_ENABLED(CONFIG_DELL_PRIVACY)
> > +int dell_privacy_has_micmute(void);
> > +bool dell_privacy_present(void);
> > +bool dell_privacy_process_event(int type, int code, int status);
> > +#else /* CONFIG_DELL_PRIVACY */
> > +static inline int dell_privacy_has_micmute(void)
> > +{
> > + return -ENODEV;
> > +}
> > +
> > +static inline int dell_privacy_present(void)
> ^^^
> It's declared with `bool` return type a couple lines above.
>
>
> > +{
> > + return -ENODEV;
> > +}
> > +
> > +static inline void dell_privacy_process_event(int type, int code, int
> status)
> ^^^^
> It's declared with `bool` return type a couple lines above.
>
>
> > +{}
> > +#endif /* CONFIG_DELL_PRIVACY */
> > +
> > +int dell_privacy_acpi_init(void);
> > +void dell_privacy_acpi_exit(void);
>
> These two don't seem to be referenced anywhere?
>
>
> > +#endif
> > diff --git a/drivers/platform/x86/dell/dell-wmi.c
> b/drivers/platform/x86/dell/dell-wmi.c
> > index bbdb3e860892..8ef9e22a538f 100644
> > --- a/drivers/platform/x86/dell/dell-wmi.c
> > +++ b/drivers/platform/x86/dell/dell-wmi.c
> > @@ -27,6 +27,7 @@
> > #include <acpi/video.h>
> > #include "dell-smbios.h"
> > #include "dell-wmi-descriptor.h"
> > +#include "dell-privacy-wmi.h"
> >
> > MODULE_AUTHOR("Matthew Garrett <mjg@redhat.com>");
> > MODULE_AUTHOR("Pali Rohár <pali@kernel.org>");
> > @@ -381,6 +382,7 @@ static void dell_wmi_notify(struct wmi_device *wdev,
> > u16 *buffer_entry, *buffer_end;
> > acpi_size buffer_size;
> > int len, i;
> > + int ret;
> >
> > if (obj->type != ACPI_TYPE_BUFFER) {
> > pr_warn("bad response type %x\n", obj->type);
> > @@ -427,7 +429,6 @@ static void dell_wmi_notify(struct wmi_device *wdev,
> >
> > switch (buffer_entry[1]) {
> > case 0x0000: /* One key pressed or event occurred */
> > - case 0x0012: /* Event with extended data occurred */
> > if (len > 2)
> > dell_wmi_process_key(wdev, buffer_entry[1],
> > buffer_entry[2]);
> > @@ -439,6 +440,16 @@ static void dell_wmi_notify(struct wmi_device *wdev,
> > dell_wmi_process_key(wdev, buffer_entry[1],
> > buffer_entry[i]);
> > break;
> > + case 0x0012:
> > + ret = dell_privacy_present();
> > + if ((ret) && (len > 2)) {
>
> Is the `len > 2` check correct?
>
> Moreover, I personally don't see any reason to use a new variable here
> (`ret`).
>
> If you incorporate the `dell_privacy_present()` check into
> `dell_privacy_process_event()`, then even
>
> if (len > ?? && dell_privacy_process_event(...))
> /* nothing */ ;
> else if (len > 2)
> dell_wmi_process_key(...);
>
> would work as Hans has already pointed it out. And there'd be no need for
> `dell_privacy_present()` anymore.
>
>
> > + dell_privacy_process_event(buffer_entry[1],
> > + buffer_entry[3], buffer_entry[4]);
> > + } else if (len > 2) {
> > + dell_wmi_process_key(wdev, buffer_entry[1],
> > + buffer_entry[2]);
> > + }
> > + break;
> > default: /* Unknown event */
> > pr_info("Unknown WMI event type 0x%x\n",
> > (int)buffer_entry[1]);
> > --
> > 2.25.1
> >
>
>
> Regards,
> Barnabás Pőcze
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v6 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
2021-04-04 16:53 ` Barnabás Pőcze
2021-04-05 15:17 ` Limonciello, Mario
@ 2021-04-07 15:15 ` Hans de Goede
2021-04-12 4:11 ` Yuan, Perry
1 sibling, 1 reply; 5+ messages in thread
From: Hans de Goede @ 2021-04-07 15:15 UTC (permalink / raw)
To: Barnabás Pőcze, Perry Yuan
Cc: pierre-louis.bossart, oder_chiou, perex, tiwai, mgross,
Mario.Limonciello, lgirdwood, broonie, alsa-devel, linux-kernel,
platform-driver-x86, mario.limonciello, Dell.Client.Kernel
Hi Perry, Barnabás,
Barnabás thank you for your review. I agree with all your remarks.
Perry, Thank you for the new version, this version is looking good.
This is almost ready for merging.
Please fix the review remarks from Barnabás. I also have a couple of small
review remarks myself below.
On 4/4/21 6:53 PM, Barnabás Pőcze wrote:
> Hi
>
>
> 2021. április 4., vasárnap 10:31 keltezéssel, Perry Yuan írta:
>
>> From: Perry Yuan <perry_yuan@dell.com>
>>
>> add support for Dell privacy driver for the Dell units equipped
>> hardware privacy design, which protect users privacy of audio and
>> camera from hardware level. Once the audio or camera privacy mode
>> activated, any applications will not get any audio or video stream
>> when user pressed ctrl+F4 hotkey, audio privacy mode will be
>> enabled, micmute led will be also changed accordingly
>> The micmute led is fully controlled by hardware & EC(embedded controller)
>> and camera mute hotkey is Ctrl+F9. Currently design only emits
>> SW_CAMERA_LENS_COVER event while the camera lens shutter will be
>> changed by EC & HW(hardware) control
>>
>> *The flow is like this:
>> 1) User presses key. HW does stuff with this key (timeout timer is started)
>> 2) WMI event is emitted from BIOS to kernel
>> 3) WMI event is received by dell-privacy
>> 4) KEY_MICMUTE emitted from dell-privacy
>> 5) Userland picks up key and modifies kcontrol for SW mute
>> 6) Codec kernel driver catches and calls ledtrig_audio_set, like this:
>> ledtrig_audio_set(LED_AUDIO_MICMUTE, rt715->micmute_led ? LED_ON :LED_OFF);
>> 7) If "LED" is set to on dell-privacy notifies EC, and timeout is cancelled,
>> HW mic mute activated. If EC not notified, HW mic mute will also be
>> activated when timeout used up, it is just later than active ack
>>
>> Signed-off-by: Perry Yuan <perry_yuan@dell.com>
>> ---
>> v5 -> v6:
>> * addressed feedback from Hans
>> * addressed feedback from Pierre
>> * optimize some debug format with dev_dbg()
>> * remove platform driver,combined privacy acpi driver into single wmi
>> driver file
>> * optimize sysfs interface with string added to be more clearly reading
>> v4 -> v5:
>> * addressed feedback from Randy Dunlap
>> * addressed feedback from Pierre-Louis Bossart
>> * rebase to latest 5.12 rc4 upstream kernel
>> * fix some space alignment problem
>> v3 -> v4:
>> * fix format for Kconfig
>> * add sysfs document
>> * add flow comments to the privacy wmi/acpi driver
>> * addressed feedback from Barnabás Pőcze[Thanks very much]
>> * export privacy_valid to make the global state simpler to query
>> * fix one issue which will block the dell-laptop driver to load when
>> privacy driver invalid
>> * addressed feedback from Pierre-Louis Bossart,remove the EC ID match
>> v2 -> v3:
>> * add sysfs attributes doc
>> v1 -> v2:
>> * query EC handle from EC driver directly.
>> * fix some code style.
>> * add KEY_END to keymap array.
>> * clean platform device when cleanup called
>> * use hexadecimal format for log print in dev_dbg
>> * remove __set_bit for the report keys from probe.
>> * fix keymap leak
>> * add err_free_keymap in dell_privacy_wmi_probe
>> * wmi driver will be unregistered if privacy_acpi_init() fails
>> * add sysfs attribute files for user space query.
>> * add leds micmute driver to privacy acpi
>> * add more design info the commit info
>> ---
>> ---
>> .../testing/sysfs-platform-dell-privacy-wmi | 55 +++
>> drivers/platform/x86/dell/Kconfig | 14 +
>> drivers/platform/x86/dell/Makefile | 1 +
>> drivers/platform/x86/dell/dell-laptop.c | 23 +-
>> drivers/platform/x86/dell/dell-privacy-wmi.c | 441 ++++++++++++++++++
>> drivers/platform/x86/dell/dell-privacy-wmi.h | 32 ++
>> drivers/platform/x86/dell/dell-wmi.c | 13 +-
>> 7 files changed, 572 insertions(+), 7 deletions(-)
>> create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
>> create mode 100644 drivers/platform/x86/dell/dell-privacy-wmi.c
>> create mode 100644 drivers/platform/x86/dell/dell-privacy-wmi.h
>>
>> diff --git a/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi b/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
>> new file mode 100644
>> index 000000000000..a57ddc6a221e
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
>> @@ -0,0 +1,55 @@
>> +What: /sys/bus/wmi/devices/6932965F-1671-4CEB-B988-D3AB0A901919/dell_privacy_supported_type
>> +Date: Apr 2021
>> +KernelVersion: 5.13
>> +Contact: "perry.yuan@dell.com>"
>> +Description:
>> + Display which dell hardware level privacy devices are supported
>> + “Dell Privacy” is a set of HW, FW, and SW features to enhance
>> + Dell’s commitment to platform privacy for MIC, Camera, and
>> + ePrivacy screens.
>> + The supported hardware privacy devices are:
>> +Attributes:
>> + Microphone Mute:
>> + Identifies the local microphone can be muted by hardware, no applications
>> + is available to capture system mic sound
>> +
>> + Camera Shutter:
> ^^^^^^^^^^^^^^^^
>
> Spaces used instead of tabs.
>
>
>> + Identifies camera shutter controlled by hardware, which is a micromechanical
>> + shutter assembly that is built onto the camera module to block capturing images
>> + from outside the laptop
>> +
>> + supported:
>> + The privacy device is supported by this system
>> +
>> + unsupported:
>> + The privacy device is not supported on this system
>> +
>> + For example to check which privacy devices are supported:
>> +
>> + # cat /sys/bus/wmi/drivers/dell-privacy/6932965F-1671-4CEB-B988-D3AB0A901919/dell_privacy_supported_type
>> + [Microphone Mute] [supported]
>> + [Camera Shutter] [supported]
>> + [ePrivacy Screen] [unsupport]
[unsupport] should be [unsupported] here.
>> +
>> +What: /sys/bus/wmi/devices/6932965F-1671-4CEB-B988-D3AB0A901919/dell_privacy_current_state
>> +Date: Apr 2021
>> +KernelVersion: 5.13
>> +Contact: "perry.yuan@dell.com>"
>> +Description:
>> + Allow user space to check current dell privacy device state.
>> + Describes the Device State class exposed by BIOS which can be
>> + consumed by various applications interested in knowing the Privacy
>> + feature capabilities
>> +Attributes:
>> + muted:
>> + Identifies the privacy device is turned off and cannot send stream to OS applications
>> +
>> + unmuted:
>> + Identifies the privacy device is turned on ,audio or camera driver can get
>> + stream from mic and camera module to OS applications
>> +
>> + For example to check all supported current privacy device states:
>> +
>> + # cat /sys/bus/wmi/drivers/dell-privacy/6932965F-1671-4CEB-B988-D3AB0A901919/dell_privacy_current_state
>> + [Microphone] [unmuted]
>> + [Camera Shutter] [unmuted]
>> diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
>> index e0a55337f51a..05d124442b25 100644
>> --- a/drivers/platform/x86/dell/Kconfig
>> +++ b/drivers/platform/x86/dell/Kconfig
>> @@ -204,4 +204,18 @@ config DELL_WMI_SYSMAN
>> To compile this driver as a module, choose M here: the module will
>> be called dell-wmi-sysman.
>>
>> +config DELL_PRIVACY
>> + tristate "Dell Hardware Privacy Support"
>> + depends on ACPI
>> + depends on ACPI_WMI
>> + depends on INPUT
>> + depends on DELL_LAPTOP
>> + depends on LEDS_TRIGGER_AUDIO
>> + select DELL_WMI
>> + help
>> + This driver provides integration with the "Dell Hardware Privacy" feature
>> + of Dell laptops.
>> +
>> + To compile this driver as a module, choose M here: the module will
>> + be called dell_privacy.
>> endif # X86_PLATFORM_DRIVERS_DELL
>> diff --git a/drivers/platform/x86/dell/Makefile b/drivers/platform/x86/dell/Makefile
>> index d720a3e42ae3..7da0c33dfcca 100644
>> --- a/drivers/platform/x86/dell/Makefile
>> +++ b/drivers/platform/x86/dell/Makefile
>> @@ -19,3 +19,4 @@ obj-$(CONFIG_DELL_WMI_AIO) += dell-wmi-aio.o
>> obj-$(CONFIG_DELL_WMI_DESCRIPTOR) += dell-wmi-descriptor.o
>> obj-$(CONFIG_DELL_WMI_LED) += dell-wmi-led.o
>> obj-$(CONFIG_DELL_WMI_SYSMAN) += dell-wmi-sysman/
>> +obj-$(CONFIG_DELL_PRIVACY) += dell-privacy-wmi.o
>> diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
>> index 70edc5bb3a14..a9b4cf13913c 100644
>> --- a/drivers/platform/x86/dell/dell-laptop.c
>> +++ b/drivers/platform/x86/dell/dell-laptop.c
>> @@ -31,6 +31,8 @@
>> #include "dell-rbtn.h"
>> #include "dell-smbios.h"
>>
>> +#include "dell-privacy-wmi.h"
>> +
>> struct quirk_entry {
>> bool touchpad_led;
>> bool kbd_led_not_present;
>> @@ -90,6 +92,7 @@ static struct rfkill *wifi_rfkill;
>> static struct rfkill *bluetooth_rfkill;
>> static struct rfkill *wwan_rfkill;
>> static bool force_rfkill;
>> +static bool has_privacy;
>>
>> module_param(force_rfkill, bool, 0444);
>> MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted models");
>> @@ -2206,10 +2209,16 @@ static int __init dell_init(void)
>>
>> if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
>> dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) {
>> - micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
>> - ret = led_classdev_register(&platform_device->dev, &micmute_led_cdev);
>> - if (ret < 0)
>> - goto fail_led;
>> + if (dell_privacy_has_micmute())
>> + has_privacy = true;
>> + else
>> + has_privacy = false;
>> + if (!has_privacy) {
>> + micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
>> + ret = led_classdev_register(&platform_device->dev, &micmute_led_cdev);
>> + if (ret < 0)
>> + goto fail_led;
>> + }
>> }
>>
>> if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
>> @@ -2257,7 +2266,8 @@ static int __init dell_init(void)
>> fail_get_brightness:
>> backlight_device_unregister(dell_backlight_device);
>> fail_backlight:
>> - led_classdev_unregister(&micmute_led_cdev);
>> + if (!has_privacy)
>> + led_classdev_unregister(&micmute_led_cdev);
>> fail_led:
>> dell_cleanup_rfkill();
>> fail_rfkill:
>> @@ -2278,7 +2288,8 @@ static void __exit dell_exit(void)
>> touchpad_led_exit();
>> kbd_led_exit();
>> backlight_device_unregister(dell_backlight_device);
>> - led_classdev_unregister(&micmute_led_cdev);
>> + if (!has_privacy)
>> + led_classdev_unregister(&micmute_led_cdev);
>> dell_cleanup_rfkill();
>> if (platform_device) {
>> platform_device_unregister(platform_device);
>> diff --git a/drivers/platform/x86/dell/dell-privacy-wmi.c b/drivers/platform/x86/dell/dell-privacy-wmi.c
>> new file mode 100644
>> index 000000000000..3fae8dd077f1
>> --- /dev/null
>> +++ b/drivers/platform/x86/dell/dell-privacy-wmi.c
>> @@ -0,0 +1,441 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Dell privacy notification driver
>> + *
>> + * Copyright (C) 2021 Dell Inc. All Rights Reserved.
>> + */
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/bitops.h>
>> +#include <linux/input.h>
>> +#include <linux/input/sparse-keymap.h>
>> +#include <linux/list.h>
>> +#include <linux/leds.h>
>> +#include <linux/module.h>
>> +#include <linux/wmi.h>
>> +
>> +#include "dell-privacy-wmi.h"
>> +
>> +#define DELL_PRIVACY_GUID "6932965F-1671-4CEB-B988-D3AB0A901919"
>> +#define MICROPHONE_STATUS BIT(0)
>> +#define CAMERA_STATUS BIT(1)
>> +#define DELL_PRIVACY_AUDIO_EVENT 0x1
>> +#define DELL_PRIVACY_CAMERA_EVENT 0x2
>> +#define cdev_to_led(c) container_of(c, struct privacy_wmi_data, cdev)
>
> This name is confusing in my opinion. Something like `led_to_priv` could be
> clearer.
>
>
>> +
>> +/*
>> + * The wmi_list is used to store the privacy_priv struct with mutex protecting
>> + */
>> +static LIST_HEAD(wmi_list);
>> +static DEFINE_MUTEX(list_mutex);
>> +
>> +struct privacy_wmi_data {
>> + struct input_dev *input_dev;
>> + struct wmi_device *wdev;
>> + struct list_head list;
>> + struct led_classdev cdev;
>> + u32 features_present;
>> + u32 last_status;
>> +};
>> +
>> +/* DELL Privacy Type */
>> +enum dell_hardware_privacy_type {
>> + DELL_PRIVACY_TYPE_AUDIO = 0,
>> + DELL_PRIVACY_TYPE_CAMERA,
>> + DELL_PRIVACY_TYPE_SCREEN,
>> + DELL_PRIVACY_TYPE_MAX,
>> +};
>> +
>> +static const char * const privacy_types[DELL_PRIVACY_TYPE_MAX] = {
>> + [DELL_PRIVACY_TYPE_AUDIO] = "Microphone",
>> + [DELL_PRIVACY_TYPE_CAMERA] = "Camera Shutter",
>> + [DELL_PRIVACY_TYPE_SCREEN] = "ePrivacy Screen",
>> +};
>> +
>> +/*
>> + * Keymap for WMI privacy events of type 0x0012
>> + */
>> +static const struct key_entry dell_wmi_keymap_type_0012[] = {
>> + /* privacy mic mute */
>> + { KE_KEY, 0x0001, { KEY_MICMUTE } },
>> + /* privacy camera mute */
>> + { KE_SW, 0x0002, { SW_CAMERA_LENS_COVER } },
>> + { KE_END, 0},
>> +};
>> +
>> +/*
>> + * global privacy state for other modules to query if the privacy driver loaded successfully
>> + * or no privacy WMI device was detected at initial probe phase
>> + */
>> +bool dell_privacy_present(void)
>> +{
>> + struct privacy_wmi_data *priv;
>> +
>> + mutex_lock(&list_mutex);
>> + priv = list_first_entry_or_null(&wmi_list,
>> + struct privacy_wmi_data,
>> + list);
>> + mutex_unlock(&list_mutex);
>> +
>> + return priv;
>> +}
>> +EXPORT_SYMBOL_GPL(dell_privacy_present);
As Barnabás also says below this function is not necessary, please remove it.
>> +
>> +/*
>> + * Allow other modules to check current dell privacy mic mute state
>> + */
>> +int dell_privacy_has_micmute(void)
>> +{
>> + struct privacy_wmi_data *priv;
>> + int ret;
>> +
>> + mutex_lock(&list_mutex);
>> + priv = list_first_entry_or_null(&wmi_list,
>> + struct privacy_wmi_data,
>> + list);
>> + /*
>> + * There is no privacy device node yet, so we need to wait
>> + * until the privacy driver has created software node for the micmute device.
>> + * This function will return -EPROBE_DEFER until the privacy device is registered.
>> + */
>> + if (!priv) {
>> + ret = -EPROBE_DEFER;
>> + goto out;
>> + }
>> + ret = priv->features_present & MICROPHONE_STATUS;
>> + if (!ret) {
>> + dev_err(&priv->wdev->dev, "privacy mic mute feature is not supported: %d\n", ret);
>> + ret = -ENODEV;
>> + goto out;
>> + }
>> + dev_err(&priv->wdev->dev, "privacy mic mute feature is supported: %d\n", ret);
>
> I think this could be `dev_info()`, but definitely not `dev_err()`. Although I'd
> personally move the logging from here to the probe function if you want to log
> which features are available. `ret` is necessarily 1 here, so I don't think printing it
> provides additional information.
>
>
>> +
>> +out:
>> + mutex_unlock(&list_mutex);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(dell_privacy_has_micmute);
>> +
>> +/*
>> + * The flow of privacy event:
>> + * 1) User presses key. HW does stuff with this key (timeout is started)
>> + * 2) WMI event is emitted from BIOS
>> + * 3) WMI event is received by dell-privacy
>> + * 4) KEY_MICMUTE emitted from dell-privacy
>> + * 5) Userland picks up key and modifies kcontrol for SW mute
>> + * 6) Codec kernel driver catches and calls ledtrig_audio_set defined by
>> + * dell-privacy-acpi driver. Codec driver will call like this to switch micmute led state.
>> + * ledtrig_audio_set(LED_AUDIO_MICMUTE, micmute_led ? LED_ON :LED_OFF);
>> + * 7) If "LED" is set to on dell-privacy notifies EC,and timeout is cancelled,
>> + * HW mic mute activated.
>> + */
>> +bool dell_privacy_process_event(int type, int code, int status)
>> +{
>> + struct privacy_wmi_data *priv;
>> + const struct key_entry *key;
>> + bool ret = false;
>> +
>> + mutex_lock(&list_mutex);
>> + priv = list_first_entry_or_null(&wmi_list,
>> + struct privacy_wmi_data,
>> + list);
>> + if (!priv) {
>> + dev_err(&priv->wdev->dev, "priv data is NULL\n");
Please drop the dev_err here, this is expected to happen on devices without privacy features,
since the intention is to use this function as dell_privacy_present() + process_event()
functionality in one function. Doing things this way is simpler and avoids a race since this
function holds the mutex the entire time.
>> + goto error;
>> + }
>> +
>
> I think the rest of the function could be replaced with:
>
> ret = sparse_keymap_report_event(priv->input_dev, DELL_SCAN_CODE(type, code), 1, true)
>
> if (ret)
> priv->last_status = status;
>
> error:
> [...]
>
> (see later a comment for the definition of DELL_SCAN_CODE())
>
>
>> + key = sparse_keymap_entry_from_scancode(priv->input_dev, (type << 16) | code);
>> + if (!key) {
>> + dev_warn(&priv->wdev->dev, "Unknown key with type 0x%04x and code 0x%04x pressed\n",
>> + type, code);
>> + goto error;
>> + }
>> + dev_dbg(&priv->wdev->dev, "Key with type 0x%04x and code 0x%04x pressed\n", type, code);
>> +
>> + switch (code) {
>> + case DELL_PRIVACY_AUDIO_EVENT: /* Mic mute */
>> + case DELL_PRIVACY_CAMERA_EVENT: /* Camera mute */
>> + priv->last_status = status;
>> + sparse_keymap_report_entry(priv->input_dev, key, 1, true);
>> + ret = true;
>> + break;
>> + default:
>> + dev_dbg(&priv->wdev->dev, "unknown event type 0x%04x 0x%04x", type, code);
>
> The capitalization is inconsistent. Please either make all messages lowercase or
> make them all start with an uppercase letter. (And a newline character is missing.)
>
>
>> + }
>> +
>> +error:
>> + mutex_unlock(&list_mutex);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(dell_privacy_process_event);
>> +
>> +static ssize_t dell_privacy_supported_type_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + enum dell_hardware_privacy_type type;
>> + struct privacy_wmi_data *priv = dev_get_drvdata(dev);
>> + char *s = buf;
>> + u32 privacy_list;
>> +
>> + privacy_list = priv->features_present;
>> + for (type = DELL_PRIVACY_TYPE_AUDIO; type < DELL_PRIVACY_TYPE_MAX; type++) {
>> + if (privacy_types[type]) {
>
> Is this check necessary?
No I don't think it is, this check can be removed, good catch.
>
>
>> + if (privacy_list & BIT(type))
>> + s += sprintf(s, "[%s] [supported]\n", privacy_types[type]);
>> + else
>> + s += sprintf(s, "[%s] [unsupport]\n", privacy_types[type]);
[unsupport] should be [unsupported] here.
>
> You can use `sysfs_emit_at()` here.
>
>
>> + }
>> + }
>> +
>> + if (s != buf)
>> + /* convert the last space to a newline */
>> + *(s-1) = '\n';
>
> I believe this is not needed?
Right this is no longer necessary now that we have 1 line per feature.
>
>
>> + return (s - buf);
>> +}
>> +
>> +static ssize_t dell_privacy_current_state_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct privacy_wmi_data *priv = dev_get_drvdata(dev);
>> + enum dell_hardware_privacy_type type;
>> + u32 privacy_state = priv->last_status;
>> + u32 privacy_supported = priv->features_present;
>> + char *s = buf;
>> +
>> + for (type = DELL_PRIVACY_TYPE_AUDIO; type < DELL_PRIVACY_TYPE_MAX; type++) {
>> + if (privacy_supported & BIT(type)) {
>> + if (privacy_state & BIT(type))
>> + s += sprintf(s, "[%s] [unmuted]\n", privacy_types[type]);
>> + else
>> + s += sprintf(s, "[%s] [muted]\n", privacy_types[type]);
>
> sysfs_emit_at
>
>
>> + }
>> + }
>> +
>> + if (s != buf)
>> + /* convert the last space to a newline */
>> + *(s-1) = '\n';
>
> not needed?
Ack.
>> + return (s - buf);
>> +}
>> +
>> +static DEVICE_ATTR_RO(dell_privacy_supported_type);
>> +static DEVICE_ATTR_RO(dell_privacy_current_state);
>> +
>> +static struct attribute *privacy_attributes[] = {
>> + &dev_attr_dell_privacy_supported_type.attr,
>> + &dev_attr_dell_privacy_current_state.attr,
>> + NULL,
>> +};
>> +
>> +static const struct attribute_group privacy_attribute_group = {
>> + .attrs = privacy_attributes
>> +};
>> +
>> +/*
>> + * Describes the Device State class exposed by BIOS which can be consumed by
>> + * various applications interested in knowing the Privacy feature capabilities.
>> + * class DeviceState
>> + * {
>> + * [key, read] string InstanceName;
>> + * [read] boolean ReadOnly;
>> + * [WmiDataId(1), read] uint32 DevicesSupported;
>> + * 0 – None, 0x1 – Microphone, 0x2 – Camera, 0x4 -ePrivacy Screen
> ^ ^ ^ ^ ^^
> Please use a single type of hyphen/dash consistently. One space is enough.
>
>
>> + * [WmiDataId(2), read] uint32 CurrentState;
>> + * 0:Off; 1:On. Bit0 – Microphone, Bit1 – Camera, Bit2 - ePrivacyScreen
> ^ ^ ^
> Same here.
>
>
>> + * };
>> + */
>> +static int get_current_status(struct wmi_device *wdev)
>> +{
>> + struct privacy_wmi_data *priv = dev_get_drvdata(&wdev->dev);
>> + union acpi_object *obj_present;
>> + u32 *buffer;
>> + int ret = 0;
>> +
>> + if (!priv) {
>> + pr_err("dell privacy priv is NULL\n");
>
> dev_err?
Ack.
>> + return -EINVAL;
>> + }
>> + /* check privacy support features and device states */
>> + obj_present = wmidev_block_query(wdev, 0);
>> + if (!obj_present) {
>> + dev_err(&wdev->dev, "failed to read Binary MOF\n");
>> + ret = -EIO;
>> + return ret;
>
> return -EIO ?
>
>
>> + }
>> +
>> + if (obj_present->type != ACPI_TYPE_BUFFER) {
>> + dev_err(&wdev->dev, "Binary MOF is not a buffer!\n");
>> + ret = -EIO;
>> + goto obj_free;
>> + }
>> + /* Although it's not technically a failure, this would lead to
>> + * unexpected behavior
>> + */
>> + if (obj_present->buffer.length != 8) {
>> + dev_err(&wdev->dev, "Dell privacy buffer has unexpected length (%d)!\n",
>> + obj_present->buffer.length);
>> + ret = -EINVAL;
>> + goto obj_free;
>> + }
>> + buffer = (u32 *)obj_present->buffer.pointer;
>> + priv->features_present = buffer[0];
>> + priv->last_status = buffer[1];
>
> It's a minor thing, but I still think it'd more explicit and somewhat better to
> use `get_unaligned_le32()` (or `get_unaligned_cpu32()`).
>
>
>> +
>> +obj_free:
>> + kfree(obj_present);
>> + return ret;
>> +}
>> +
>> +static int dell_privacy_micmute_led_set(struct led_classdev *led_cdev,
>> + enum led_brightness brightness)
>> +{
>> + struct privacy_wmi_data *priv = cdev_to_led(led_cdev);
>> + static char *acpi_method = (char *)"ECAK";
>> + acpi_status status;
>> + acpi_handle handle;
>> +
>> + handle = ec_get_handle();
>> + if (!handle)
>> + return -EIO;
>> +
>> + if (!acpi_has_method(handle, acpi_method))
>> + return -EIO;
>> +
>> + status = acpi_evaluate_object(handle, acpi_method, NULL, NULL);
>> + if (ACPI_FAILURE(status)) {
>> + dev_err(&priv->wdev->dev, "Error setting privacy EC ack value: %s\n",
>> + acpi_format_exception(status));
>> + return -EIO;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/*
>> + * Pressing the mute key activates a time delayed circuit to physically cut
>> + * off the mute. The LED is in the same circuit, so it reflects the true
>> + * state of the HW mute. The reason for the EC "ack" is so that software
>> + * can first invoke a SW mute before the HW circuit is cut off. Without SW
>> + * cutting this off first does not affect the time delayed muting or status
>> + * of the LED but there is a possibility of a "popping" noise.
>> + *
>> + * If the EC receives the SW ack, the circuit will be activated before the
>> + * delay completed.
>> + *
>> + * Exposing as an LED device allows the codec drivers notification path to
>> + * EC ACK to work
>> + */
>> +static int dell_privacy_leds_setup(struct device *dev)
>> +{
>> + struct privacy_wmi_data *priv = dev_get_drvdata(dev);
>> + int ret;
>> +
>> + priv->cdev.name = "dell-privacy::micmute";
>> + priv->cdev.max_brightness = 1;
>> + priv->cdev.brightness_set_blocking = dell_privacy_micmute_led_set;
>> + priv->cdev.default_trigger = "audio-micmute";
>> + priv->cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
>> + ret = devm_led_classdev_register(dev, &priv->cdev);
>> + if (ret)
>> + return ret;
>> + return 0;
>
> You can replace the last four lines with:
>
> return devm_led_classdev_register(...);
>
>
>> +}
>> +
>> +static int dell_privacy_wmi_probe(struct wmi_device *wdev, const void *context)
>> +{
>> + struct privacy_wmi_data *priv;
>> + struct key_entry *keymap;
>> + int ret, i;
>> +
>> + ret = wmi_has_guid(DELL_PRIVACY_GUID);
>> + if (!ret)
>> + pr_debug("Unable to detect available Dell privacy devices: %d\n", ret);
>
> When this branch is taken, `ret` is necessarily zero, so I don't think printing it
> provides useful information.
>
> And I believe this `wmi_has_guid()` check is unnecessary since the probe method
> would not be called if the device didn't have such WMI guid if I'm not mistaken.
Ack, Perry please drop this check.
>> +
>> + priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>> +
>> + dev_set_drvdata(&wdev->dev, priv);
>> + priv->wdev = wdev;
>> + /* create evdev passing interface */
>> + priv->input_dev = devm_input_allocate_device(&wdev->dev);
>> + if (!priv->input_dev)
>> + return -ENOMEM;
>> +
>> + /* remap the wmi keymap event to new keymap */
>> + keymap = kcalloc(ARRAY_SIZE(dell_wmi_keymap_type_0012),
>> + sizeof(struct key_entry), GFP_KERNEL);
>> + if (!keymap)
>> + return -ENOMEM;
>> +
>> + /* remap the keymap code with Dell privacy key type 0x12 as prefix
>> + * KEY_MICMUTE scancode will be reported as 0x120001
>> + */
>> + for (i = 0; i < ARRAY_SIZE(dell_wmi_keymap_type_0012); i++) {
>> + keymap[i] = dell_wmi_keymap_type_0012[i];
>> + keymap[i].code |= (0x0012 << 16);
>> + }
>
> I still don't see the need for allocating and copying the keymap. Wouldn't the
> following be sufficient?
>
> #define DELL_SCAN_CODE(type, code) ((type) << 16 | (code))
> static const struct key_entry dell_wmi_keymap_type_0012[] = {
> { KE_KEY, DELL_SCAN_CODE(0x0012, 0x0001), { KEY_MICMUTE } },
> { KE_SW, DELL_SCAN_CODE(0x0012, 0x0002), { SW_CAMERA_LENS_COVER } },
> { KE_END, 0},
> };
>
> Other Dell drivers potentially merge multiple keymaps, so dynamically allocating
> the key_entry array is justified. Here I see no such need. Can you explain why
> this copying is done?
I was wondering the same thing when I reviewed v5, but this is consistent with
the handling in dell-wmi.c so I'm fine with keeping it as is for consistency.
But if you (perry) want to change this to the simpler code which Barnabás is
suggesting that is fine too.
>
>
>> + ret = sparse_keymap_setup(priv->input_dev, keymap, NULL);
>> + if (ret)
>> + return ret;
>
> `keymap` is leaked if this or any of the later early-returns are taken.
Right as I mentioned in my review of v5, the "kfree(keymap);" needs to be moved
before the "if (ret)" here, so the end results looks like this:
ret = sparse_keymap_setup(priv->input_dev, keymap, NULL);
kfree(keymap);
if (ret)
return ret;
And the kfree(keymap) below goes away.
>
>
>> +
>> + priv->input_dev->dev.parent = &wdev->dev;
>> + priv->input_dev->name = "Dell Privacy Driver";
>> + priv->input_dev->id.bustype = BUS_HOST;
>> +
>> + ret = input_register_device(priv->input_dev);
>> + if (ret)
>> + return ret;
>> +
>> + ret = get_current_status(priv->wdev);
>> + if (ret)
>> + return ret;
>> +
>> + ret = devm_device_add_group(&wdev->dev, &privacy_attribute_group);
>> + if (ret)
>> + return ret;
>> +
>> + ret = dell_privacy_leds_setup(&priv->wdev->dev);
>> + if (ret)
>> + return ret;
>> +
>> + mutex_lock(&list_mutex);
>> + list_add_tail(&priv->list, &wmi_list);
>> + mutex_unlock(&list_mutex);
>> + kfree(keymap);
This kfree(keymap) needs to be removed. Note that switching to the suggested
non copied keymap would get rid of all of this.
>> + return 0;
>> +}
>> +
>> +static int dell_privacy_wmi_remove(struct wmi_device *wdev)
>> +{
>> + struct privacy_wmi_data *priv = dev_get_drvdata(&wdev->dev);
>> +
>> + mutex_lock(&list_mutex);
>> + list_del(&priv->list);
>> + mutex_unlock(&list_mutex);
>> + return 0;
>> +}
>> +
>> +static const struct wmi_device_id dell_wmi_privacy_wmi_id_table[] = {
>> + { .guid_string = DELL_PRIVACY_GUID },
>> + { },
>> +};
>> +
>> +static struct wmi_driver dell_privacy_wmi_driver = {
>> + .driver = {
>> + .name = "dell-privacy",
>> + },
>> + .probe = dell_privacy_wmi_probe,
>> + .remove = dell_privacy_wmi_remove,
>> + .id_table = dell_wmi_privacy_wmi_id_table,
>> +};
>> +
>> +module_wmi_driver(dell_privacy_wmi_driver);
>> +
>> +MODULE_DEVICE_TABLE(wmi, dell_wmi_privacy_wmi_id_table);
>> +MODULE_AUTHOR("Perry Yuan <perry_yuan@dell.com>");
>> +MODULE_DESCRIPTION("Dell Privacy WMI Driver");
>> +MODULE_LICENSE("GPL");
>> diff --git a/drivers/platform/x86/dell/dell-privacy-wmi.h b/drivers/platform/x86/dell/dell-privacy-wmi.h
>> new file mode 100644
>> index 000000000000..a24893754286
>> --- /dev/null
>> +++ b/drivers/platform/x86/dell/dell-privacy-wmi.h
>> @@ -0,0 +1,32 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Dell privacy notification driver
>> + *
>> + * Copyright (C) 2021 Dell Inc. All Rights Reserved.
>> + */
>> +
>> +#ifndef _DELL_PRIVACY_WMI_H_
>> +#define _DELL_PRIVACY_WMI_H_
>> +
>> +#if IS_ENABLED(CONFIG_DELL_PRIVACY)
>> +int dell_privacy_has_micmute(void);
>> +bool dell_privacy_present(void);
>> +bool dell_privacy_process_event(int type, int code, int status);
>> +#else /* CONFIG_DELL_PRIVACY */
>> +static inline int dell_privacy_has_micmute(void)
>> +{
>> + return -ENODEV;
>> +}
>> +
>> +static inline int dell_privacy_present(void)
> ^^^
> It's declared with `bool` return type a couple lines above.
>
>
>> +{
>> + return -ENODEV;
>> +}
>> +
>> +static inline void dell_privacy_process_event(int type, int code, int status)
> ^^^^
> It's declared with `bool` return type a couple lines above.
>
>
>> +{}
>> +#endif /* CONFIG_DELL_PRIVACY */
>> +
>> +int dell_privacy_acpi_init(void);
>> +void dell_privacy_acpi_exit(void);
>
> These two don't seem to be referenced anywhere?
These no longer exist and should be removed from the header.
>
>
>> +#endif
>> diff --git a/drivers/platform/x86/dell/dell-wmi.c b/drivers/platform/x86/dell/dell-wmi.c
>> index bbdb3e860892..8ef9e22a538f 100644
>> --- a/drivers/platform/x86/dell/dell-wmi.c
>> +++ b/drivers/platform/x86/dell/dell-wmi.c
>> @@ -27,6 +27,7 @@
>> #include <acpi/video.h>
>> #include "dell-smbios.h"
>> #include "dell-wmi-descriptor.h"
>> +#include "dell-privacy-wmi.h"
>>
>> MODULE_AUTHOR("Matthew Garrett <mjg@redhat.com>");
>> MODULE_AUTHOR("Pali Rohár <pali@kernel.org>");
>> @@ -381,6 +382,7 @@ static void dell_wmi_notify(struct wmi_device *wdev,
>> u16 *buffer_entry, *buffer_end;
>> acpi_size buffer_size;
>> int len, i;
>> + int ret;
>>
>> if (obj->type != ACPI_TYPE_BUFFER) {
>> pr_warn("bad response type %x\n", obj->type);
>> @@ -427,7 +429,6 @@ static void dell_wmi_notify(struct wmi_device *wdev,
>>
>> switch (buffer_entry[1]) {
>> case 0x0000: /* One key pressed or event occurred */
>> - case 0x0012: /* Event with extended data occurred */
>> if (len > 2)
>> dell_wmi_process_key(wdev, buffer_entry[1],
>> buffer_entry[2]);
>> @@ -439,6 +440,16 @@ static void dell_wmi_notify(struct wmi_device *wdev,
>> dell_wmi_process_key(wdev, buffer_entry[1],
>> buffer_entry[i]);
>> break;
>> + case 0x0012:
>> + ret = dell_privacy_present();
>> + if ((ret) && (len > 2)) {
>
> Is the `len > 2` check correct?
>
> Moreover, I personally don't see any reason to use a new variable here (`ret`).
>
> If you incorporate the `dell_privacy_present()` check into
> `dell_privacy_process_event()`,
`dell_privacy_process_event()` already incorporates a presency check; and
by only relying on that check we also avoid a race between the check and
processing the event.
> then even
>
> if (len > ?? && dell_privacy_process_event(...))
> /* nothing */ ;
> else if (len > 2)
> dell_wmi_process_key(...);
>
> would work as Hans has already pointed it out. And there'd be no need for
> `dell_privacy_present()` anymore.
Ack, please change this and remove the entire dell_privacy_present() function.
So you should end up with this here:
if (len > 4 && dell_privacy_process_event(buffer_entry[1], buffer_entry[3], buffer_entry[4]))
/* nothing */ ;
else if (len > 2)
dell_wmi_process_key(wdev, buffer_entry[1], dell_wmi_process_key(wdev, buffer_entry[1],
break;
Regards,
Hans
>
>
>> + dell_privacy_process_event(buffer_entry[1],
>> + buffer_entry[3], buffer_entry[4]);
>> + } else if (len > 2) {
>> + dell_wmi_process_key(wdev, buffer_entry[1],
>> + buffer_entry[2]);
>> + }
>> + break;
>> default: /* Unknown event */
>> pr_info("Unknown WMI event type 0x%x\n",
>> (int)buffer_entry[1]);
>> --
>> 2.25.1
>>
>
>
> Regards,
> Barnabás Pőcze
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH v6 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
2021-04-07 15:15 ` Hans de Goede
@ 2021-04-12 4:11 ` Yuan, Perry
0 siblings, 0 replies; 5+ messages in thread
From: Yuan, Perry @ 2021-04-12 4:11 UTC (permalink / raw)
To: Hans de Goede, Barnabás Pocze
Cc: pierre-louis.bossart, oder_chiou, perex, tiwai, mgross,
Limonciello, Mario, lgirdwood, broonie, alsa-devel, linux-kernel,
platform-driver-x86, mario.limonciello, Dell Client Kernel
Hi Hans, Barnabás.
Thanks for your review very much!
I have no concern to change the most of the comments.
Just one sysfs_emit replacement will cause problem , adding comments at below.
> -----Original Message-----
> From: Hans de Goede <hdegoede@redhat.com>
> Sent: Wednesday, April 7, 2021 11:16 PM
> To: Barnabás Pőcze; Yuan, Perry
> Cc: pierre-louis.bossart@linux.intel.com; oder_chiou@realtek.com;
> perex@perex.cz; tiwai@suse.com; mgross@linux.intel.com; Limonciello,
> Mario; lgirdwood@gmail.com; broonie@kernel.org; alsa-devel@alsa-
> project.org; linux-kernel@vger.kernel.org; platform-driver-
> x86@vger.kernel.org; mario.limonciello@outlook.com; Dell Client Kernel
> Subject: Re: [PATCH v6 1/2] platform/x86: dell-privacy: Add support for Dell
> hardware privacy
>
>
> [EXTERNAL EMAIL]
>
> Hi Perry, Barnabás,
>
> Barnabás thank you for your review. I agree with all your remarks.
>
> Perry, Thank you for the new version, this version is looking good.
> This is almost ready for merging.
>
> Please fix the review remarks from Barnabás. I also have a couple of small
> review remarks myself below.
>
> On 4/4/21 6:53 PM, Barnabás Pőcze wrote:
> > Hi
> >
> >
> > 2021. április 4., vasárnap 10:31 keltezéssel, Perry Yuan írta:
> >
> >> From: Perry Yuan <perry_yuan@dell.com>
> >>
> >> add support for Dell privacy driver for the Dell units equipped
> >> hardware privacy design, which protect users privacy of audio and
> >> camera from hardware level. Once the audio or camera privacy mode
> >> activated, any applications will not get any audio or video stream
> >> when user pressed ctrl+F4 hotkey, audio privacy mode will be enabled,
> >> micmute led will be also changed accordingly The micmute led is fully
> >> controlled by hardware & EC(embedded controller) and camera mute
> >> hotkey is Ctrl+F9. Currently design only emits SW_CAMERA_LENS_COVER
> >> event while the camera lens shutter will be changed by EC &
> >> HW(hardware) control
> >>
> >> *The flow is like this:
> >> 1) User presses key. HW does stuff with this key (timeout timer is
> >> started)
> >> 2) WMI event is emitted from BIOS to kernel
> >> 3) WMI event is received by dell-privacy
> >> 4) KEY_MICMUTE emitted from dell-privacy
> >> 5) Userland picks up key and modifies kcontrol for SW mute
> >> 6) Codec kernel driver catches and calls ledtrig_audio_set, like this:
> >> ledtrig_audio_set(LED_AUDIO_MICMUTE, rt715->micmute_led ?
> LED_ON
> >> :LED_OFF);
> >> 7) If "LED" is set to on dell-privacy notifies EC, and timeout is cancelled,
> >> HW mic mute activated. If EC not notified, HW mic mute will also be
> >> activated when timeout used up, it is just later than active ack
> >>
> >> Signed-off-by: Perry Yuan <perry_yuan@dell.com>
> >> ---
> >> v5 -> v6:
> >> * addressed feedback from Hans
> >> * addressed feedback from Pierre
> >> * optimize some debug format with dev_dbg()
> >> * remove platform driver,combined privacy acpi driver into single wmi
> >> driver file
> >> * optimize sysfs interface with string added to be more clearly
> >> reading
> >> v4 -> v5:
> >> * addressed feedback from Randy Dunlap
> >> * addressed feedback from Pierre-Louis Bossart
> >> * rebase to latest 5.12 rc4 upstream kernel
> >> * fix some space alignment problem
> >> v3 -> v4:
> >> * fix format for Kconfig
> >> * add sysfs document
> >> * add flow comments to the privacy wmi/acpi driver
> >> * addressed feedback from Barnabás Pőcze[Thanks very much]
> >> * export privacy_valid to make the global state simpler to query
> >> * fix one issue which will block the dell-laptop driver to load when
> >> privacy driver invalid
> >> * addressed feedback from Pierre-Louis Bossart,remove the EC ID match
> >> v2 -> v3:
> >> * add sysfs attributes doc
> >> v1 -> v2:
> >> * query EC handle from EC driver directly.
> >> * fix some code style.
> >> * add KEY_END to keymap array.
> >> * clean platform device when cleanup called
> >> * use hexadecimal format for log print in dev_dbg
> >> * remove __set_bit for the report keys from probe.
> >> * fix keymap leak
> >> * add err_free_keymap in dell_privacy_wmi_probe
> >> * wmi driver will be unregistered if privacy_acpi_init() fails
> >> * add sysfs attribute files for user space query.
> >> * add leds micmute driver to privacy acpi
> >> * add more design info the commit info
> >> ---
> >> ---
> >> .../testing/sysfs-platform-dell-privacy-wmi | 55 +++
> >> drivers/platform/x86/dell/Kconfig | 14 +
> >> drivers/platform/x86/dell/Makefile | 1 +
> >> drivers/platform/x86/dell/dell-laptop.c | 23 +-
> >> drivers/platform/x86/dell/dell-privacy-wmi.c | 441
> >> ++++++++++++++++++ drivers/platform/x86/dell/dell-privacy-wmi.h | 32
> ++
> >> drivers/platform/x86/dell/dell-wmi.c | 13 +-
> >> 7 files changed, 572 insertions(+), 7 deletions(-) create mode
> >> 100644 Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
> >> create mode 100644 drivers/platform/x86/dell/dell-privacy-wmi.c
> >> create mode 100644 drivers/platform/x86/dell/dell-privacy-wmi.h
> >>
> >> diff --git
> >> a/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
> >> b/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
> >> new file mode 100644
> >> index 000000000000..a57ddc6a221e
> >> --- /dev/null
> >> +++ b/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
> >> @@ -0,0 +1,55 @@
> >> +What: /sys/bus/wmi/devices/6932965F-1671-4CEB-B988-
> D3AB0A901919/dell_privacy_supported_type
> >> +Date: Apr 2021
> >> +KernelVersion: 5.13
> >> +Contact: "perry.yuan@dell.com>"
> >> +Description:
> >> + Display which dell hardware level privacy devices are
> supported
> >> + “Dell Privacy” is a set of HW, FW, and SW features to
> enhance
> >> + Dell’s commitment to platform privacy for MIC, Camera, and
> >> + ePrivacy screens.
> >> + The supported hardware privacy devices are:
> >> +Attributes:
> >> + Microphone Mute:
> >> + Identifies the local microphone can be muted
> by hardware, no applications
> >> + is available to capture system mic sound
> >> +
> >> + Camera Shutter:
> > ^^^^^^^^^^^^^^^^
> >
> > Spaces used instead of tabs.
> >
> >
> >> + Identifies camera shutter controlled by
> hardware, which is a micromechanical
> >> + shutter assembly that is built onto the
> camera module to block capturing images
> >> + from outside the laptop
> >> +
> >> + supported:
> >> + The privacy device is supported by this
> system
> >> +
> >> + unsupported:
> >> + The privacy device is not supported on this
> system
> >> +
> >> + For example to check which privacy devices are supported:
> >> +
> >> + # cat /sys/bus/wmi/drivers/dell-privacy/6932965F-1671-
> 4CEB-B988-D3AB0A901919/dell_privacy_supported_type
> >> + [Microphone Mute] [supported]
> >> + [Camera Shutter] [supported]
> >> + [ePrivacy Screen] [unsupport]
>
> [unsupport] should be [unsupported] here.
>
> >> +
> >> +What: /sys/bus/wmi/devices/6932965F-1671-4CEB-B988-
> D3AB0A901919/dell_privacy_current_state
> >> +Date: Apr 2021
> >> +KernelVersion: 5.13
> >> +Contact: "perry.yuan@dell.com>"
> >> +Description:
> >> + Allow user space to check current dell privacy device state.
> >> + Describes the Device State class exposed by BIOS which can
> be
> >> + consumed by various applications interested in knowing the
> Privacy
> >> + feature capabilities
> >> +Attributes:
> >> + muted:
> >> + Identifies the privacy device is turned off and cannot
> send
> >> +stream to OS applications
> >> +
> >> + unmuted:
> >> + Identifies the privacy device is turned on ,audio or
> camera driver can get
> >> + stream from mic and camera module to OS
> applications
> >> +
> >> + For example to check all supported current privacy device
> states:
> >> +
> >> + # cat /sys/bus/wmi/drivers/dell-privacy/6932965F-1671-
> 4CEB-B988-D3AB0A901919/dell_privacy_current_state
> >> + [Microphone] [unmuted]
> >> + [Camera Shutter] [unmuted]
> >> diff --git a/drivers/platform/x86/dell/Kconfig
> >> b/drivers/platform/x86/dell/Kconfig
> >> index e0a55337f51a..05d124442b25 100644
> >> --- a/drivers/platform/x86/dell/Kconfig
> >> +++ b/drivers/platform/x86/dell/Kconfig
> >> @@ -204,4 +204,18 @@ config DELL_WMI_SYSMAN
> >> To compile this driver as a module, choose M here: the module will
> >> be called dell-wmi-sysman.
> >>
> >> +config DELL_PRIVACY
> >> + tristate "Dell Hardware Privacy Support"
> >> + depends on ACPI
> >> + depends on ACPI_WMI
> >> + depends on INPUT
> >> + depends on DELL_LAPTOP
> >> + depends on LEDS_TRIGGER_AUDIO
> >> + select DELL_WMI
> >> + help
> >> + This driver provides integration with the "Dell Hardware Privacy"
> feature
> >> + of Dell laptops.
> >> +
> >> + To compile this driver as a module, choose M here: the module will
> >> + be called dell_privacy.
> >> endif # X86_PLATFORM_DRIVERS_DELL
> >> diff --git a/drivers/platform/x86/dell/Makefile
> >> b/drivers/platform/x86/dell/Makefile
> >> index d720a3e42ae3..7da0c33dfcca 100644
> >> --- a/drivers/platform/x86/dell/Makefile
> >> +++ b/drivers/platform/x86/dell/Makefile
> >> @@ -19,3 +19,4 @@ obj-$(CONFIG_DELL_WMI_AIO) +=
> dell-wmi-aio.o
> >> obj-$(CONFIG_DELL_WMI_DESCRIPTOR) += dell-wmi-descriptor.o
> >> obj-$(CONFIG_DELL_WMI_LED) += dell-wmi-led.o
> >> obj-$(CONFIG_DELL_WMI_SYSMAN) += dell-wmi-sysman/
> >> +obj-$(CONFIG_DELL_PRIVACY) += dell-privacy-wmi.o
> >> diff --git a/drivers/platform/x86/dell/dell-laptop.c
> >> b/drivers/platform/x86/dell/dell-laptop.c
> >> index 70edc5bb3a14..a9b4cf13913c 100644
> >> --- a/drivers/platform/x86/dell/dell-laptop.c
> >> +++ b/drivers/platform/x86/dell/dell-laptop.c
> >> @@ -31,6 +31,8 @@
> >> #include "dell-rbtn.h"
> >> #include "dell-smbios.h"
> >>
> >> +#include "dell-privacy-wmi.h"
> >> +
> >> struct quirk_entry {
> >> bool touchpad_led;
> >> bool kbd_led_not_present;
> >> @@ -90,6 +92,7 @@ static struct rfkill *wifi_rfkill; static struct
> >> rfkill *bluetooth_rfkill; static struct rfkill *wwan_rfkill; static
> >> bool force_rfkill;
> >> +static bool has_privacy;
> >>
> >> module_param(force_rfkill, bool, 0444);
> >> MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted
> >> models"); @@ -2206,10 +2209,16 @@ static int __init dell_init(void)
> >>
> >> if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
> >> dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) {
> >> - micmute_led_cdev.brightness =
> ledtrig_audio_get(LED_AUDIO_MICMUTE);
> >> - ret = led_classdev_register(&platform_device->dev,
> &micmute_led_cdev);
> >> - if (ret < 0)
> >> - goto fail_led;
> >> + if (dell_privacy_has_micmute())
> >> + has_privacy = true;
> >> + else
> >> + has_privacy = false;
> >> + if (!has_privacy) {
> >> + micmute_led_cdev.brightness =
> ledtrig_audio_get(LED_AUDIO_MICMUTE);
> >> + ret = led_classdev_register(&platform_device->dev,
> &micmute_led_cdev);
> >> + if (ret < 0)
> >> + goto fail_led;
> >> + }
> >> }
> >>
> >> if (acpi_video_get_backlight_type() != acpi_backlight_vendor) @@
> >> -2257,7 +2266,8 @@ static int __init dell_init(void)
> >> fail_get_brightness:
> >> backlight_device_unregister(dell_backlight_device);
> >> fail_backlight:
> >> - led_classdev_unregister(&micmute_led_cdev);
> >> + if (!has_privacy)
> >> + led_classdev_unregister(&micmute_led_cdev);
> >> fail_led:
> >> dell_cleanup_rfkill();
> >> fail_rfkill:
> >> @@ -2278,7 +2288,8 @@ static void __exit dell_exit(void)
> >> touchpad_led_exit();
> >> kbd_led_exit();
> >> backlight_device_unregister(dell_backlight_device);
> >> - led_classdev_unregister(&micmute_led_cdev);
> >> + if (!has_privacy)
> >> + led_classdev_unregister(&micmute_led_cdev);
> >> dell_cleanup_rfkill();
> >> if (platform_device) {
> >> platform_device_unregister(platform_device);
> >> diff --git a/drivers/platform/x86/dell/dell-privacy-wmi.c
> >> b/drivers/platform/x86/dell/dell-privacy-wmi.c
> >> new file mode 100644
> >> index 000000000000..3fae8dd077f1
> >> --- /dev/null
> >> +++ b/drivers/platform/x86/dell/dell-privacy-wmi.c
> >> @@ -0,0 +1,441 @@
> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +/*
> >> + * Dell privacy notification driver
> >> + *
> >> + * Copyright (C) 2021 Dell Inc. All Rights Reserved.
> >> + */
> >> +
> >> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >> +
> >> +#include <linux/acpi.h>
> >> +#include <linux/bitops.h>
> >> +#include <linux/input.h>
> >> +#include <linux/input/sparse-keymap.h> #include <linux/list.h>
> >> +#include <linux/leds.h> #include <linux/module.h> #include
> >> +<linux/wmi.h>
> >> +
> >> +#include "dell-privacy-wmi.h"
> >> +
> >> +#define DELL_PRIVACY_GUID "6932965F-1671-4CEB-B988-
> D3AB0A901919"
> >> +#define MICROPHONE_STATUS BIT(0)
> >> +#define CAMERA_STATUS BIT(1)
> >> +#define DELL_PRIVACY_AUDIO_EVENT 0x1 #define
> >> +DELL_PRIVACY_CAMERA_EVENT 0x2
> >> +#define cdev_to_led(c) container_of(c, struct privacy_wmi_data, cdev)
> >
> > This name is confusing in my opinion. Something like `led_to_priv`
> > could be clearer.
> >
> >
> >> +
> >> +/*
> >> + * The wmi_list is used to store the privacy_priv struct with mutex
> >> +protecting */ static LIST_HEAD(wmi_list); static
> >> +DEFINE_MUTEX(list_mutex);
> >> +
> >> +struct privacy_wmi_data {
> >> + struct input_dev *input_dev;
> >> + struct wmi_device *wdev;
> >> + struct list_head list;
> >> + struct led_classdev cdev;
> >> + u32 features_present;
> >> + u32 last_status;
> >> +};
> >> +
> >> +/* DELL Privacy Type */
> >> +enum dell_hardware_privacy_type {
> >> + DELL_PRIVACY_TYPE_AUDIO = 0,
> >> + DELL_PRIVACY_TYPE_CAMERA,
> >> + DELL_PRIVACY_TYPE_SCREEN,
> >> + DELL_PRIVACY_TYPE_MAX,
> >> +};
> >> +
> >> +static const char * const privacy_types[DELL_PRIVACY_TYPE_MAX] = {
> >> + [DELL_PRIVACY_TYPE_AUDIO] = "Microphone",
> >> + [DELL_PRIVACY_TYPE_CAMERA] = "Camera Shutter",
> >> + [DELL_PRIVACY_TYPE_SCREEN] = "ePrivacy Screen", };
> >> +
> >> +/*
> >> + * Keymap for WMI privacy events of type 0x0012 */ static const
> >> +struct key_entry dell_wmi_keymap_type_0012[] = {
> >> + /* privacy mic mute */
> >> + { KE_KEY, 0x0001, { KEY_MICMUTE } },
> >> + /* privacy camera mute */
> >> + { KE_SW, 0x0002, { SW_CAMERA_LENS_COVER } },
> >> + { KE_END, 0},
> >> +};
> >> +
> >> +/*
> >> + * global privacy state for other modules to query if the privacy
> >> +driver loaded successfully
> >> + * or no privacy WMI device was detected at initial probe phase */
> >> +bool dell_privacy_present(void) {
> >> + struct privacy_wmi_data *priv;
> >> +
> >> + mutex_lock(&list_mutex);
> >> + priv = list_first_entry_or_null(&wmi_list,
> >> + struct privacy_wmi_data,
> >> + list);
> >> + mutex_unlock(&list_mutex);
> >> +
> >> + return priv;
> >> +}
> >> +EXPORT_SYMBOL_GPL(dell_privacy_present);
>
> As Barnabás also says below this function is not necessary, please remove it.
>
> >> +
> >> +/*
> >> + * Allow other modules to check current dell privacy mic mute state
> >> +*/ int dell_privacy_has_micmute(void) {
> >> + struct privacy_wmi_data *priv;
> >> + int ret;
> >> +
> >> + mutex_lock(&list_mutex);
> >> + priv = list_first_entry_or_null(&wmi_list,
> >> + struct privacy_wmi_data,
> >> + list);
> >> + /*
> >> + * There is no privacy device node yet, so we need to wait
> >> + * until the privacy driver has created software node for the micmute
> device.
> >> + * This function will return -EPROBE_DEFER until the privacy device is
> registered.
> >> + */
> >> + if (!priv) {
> >> + ret = -EPROBE_DEFER;
> >> + goto out;
> >> + }
> >> + ret = priv->features_present & MICROPHONE_STATUS;
> >> + if (!ret) {
> >> + dev_err(&priv->wdev->dev, "privacy mic mute feature is not
> supported: %d\n", ret);
> >> + ret = -ENODEV;
> >> + goto out;
> >> + }
> >> + dev_err(&priv->wdev->dev, "privacy mic mute feature is supported:
> >> +%d\n", ret);
> >
> > I think this could be `dev_info()`, but definitely not `dev_err()`.
> > Although I'd personally move the logging from here to the probe
> > function if you want to log which features are available. `ret` is
> > necessarily 1 here, so I don't think printing it provides additional
> information.
> >
> >
> >> +
> >> +out:
> >> + mutex_unlock(&list_mutex);
> >> + return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(dell_privacy_has_micmute);
> >> +
> >> +/*
> >> + * The flow of privacy event:
> >> + * 1) User presses key. HW does stuff with this key (timeout is
> >> +started)
> >> + * 2) WMI event is emitted from BIOS
> >> + * 3) WMI event is received by dell-privacy
> >> + * 4) KEY_MICMUTE emitted from dell-privacy
> >> + * 5) Userland picks up key and modifies kcontrol for SW mute
> >> + * 6) Codec kernel driver catches and calls ledtrig_audio_set defined by
> >> + * dell-privacy-acpi driver. Codec driver will call like this to switch
> micmute led state.
> >> + * ledtrig_audio_set(LED_AUDIO_MICMUTE, micmute_led ?
> LED_ON :LED_OFF);
> >> + * 7) If "LED" is set to on dell-privacy notifies EC,and timeout is cancelled,
> >> + * HW mic mute activated.
> >> + */
> >> +bool dell_privacy_process_event(int type, int code, int status) {
> >> + struct privacy_wmi_data *priv;
> >> + const struct key_entry *key;
> >> + bool ret = false;
> >> +
> >> + mutex_lock(&list_mutex);
> >> + priv = list_first_entry_or_null(&wmi_list,
> >> + struct privacy_wmi_data,
> >> + list);
> >> + if (!priv) {
> >> + dev_err(&priv->wdev->dev, "priv data is NULL\n");
>
> Please drop the dev_err here, this is expected to happen on devices without
> privacy features, since the intention is to use this function as
> dell_privacy_present() + process_event() functionality in one function. Doing
> things this way is simpler and avoids a race since this function holds the
> mutex the entire time.
>
> >> + goto error;
> >> + }
> >> +
> >
> > I think the rest of the function could be replaced with:
> >
> > ret = sparse_keymap_report_event(priv->input_dev,
> > DELL_SCAN_CODE(type, code), 1, true)
> >
> > if (ret)
> > priv->last_status = status;
> >
> > error:
> > [...]
> >
> > (see later a comment for the definition of DELL_SCAN_CODE())
> >
> >
> >> + key = sparse_keymap_entry_from_scancode(priv->input_dev, (type
> << 16) | code);
> >> + if (!key) {
> >> + dev_warn(&priv->wdev->dev, "Unknown key with type
> 0x%04x and code 0x%04x pressed\n",
> >> + type, code);
> >> + goto error;
> >> + }
> >> + dev_dbg(&priv->wdev->dev, "Key with type 0x%04x and code 0x%04x
> >> +pressed\n", type, code);
> >> +
> >> + switch (code) {
> >> + case DELL_PRIVACY_AUDIO_EVENT: /* Mic mute */
> >> + case DELL_PRIVACY_CAMERA_EVENT: /* Camera mute */
> >> + priv->last_status = status;
> >> + sparse_keymap_report_entry(priv->input_dev, key, 1, true);
> >> + ret = true;
> >> + break;
> >> + default:
> >> + dev_dbg(&priv->wdev->dev, "unknown event type 0x%04x
> 0x%04x",
> >> +type, code);
> >
> > The capitalization is inconsistent. Please either make all messages
> > lowercase or make them all start with an uppercase letter. (And a
> > newline character is missing.)
> >
> >
> >> + }
> >> +
> >> +error:
> >> + mutex_unlock(&list_mutex);
> >> + return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(dell_privacy_process_event);
> >> +
> >> +static ssize_t dell_privacy_supported_type_show(struct device *dev,
> >> + struct device_attribute *attr,
> >> + char *buf)
> >> +{
> >> + enum dell_hardware_privacy_type type;
> >> + struct privacy_wmi_data *priv = dev_get_drvdata(dev);
> >> + char *s = buf;
> >> + u32 privacy_list;
> >> +
> >> + privacy_list = priv->features_present;
> >> + for (type = DELL_PRIVACY_TYPE_AUDIO; type <
> DELL_PRIVACY_TYPE_MAX; type++) {
> >> + if (privacy_types[type]) {
> >
> > Is this check necessary?
>
> No I don't think it is, this check can be removed, good catch.
>
> >
> >
> >> + if (privacy_list & BIT(type))
> >> + s += sprintf(s, "[%s] [supported]\n",
> privacy_types[type]);
> >> + else
> >> + s += sprintf(s, "[%s] [unsupport]\n",
> privacy_types[type]);
>
> [unsupport] should be [unsupported] here.
>
> >
> > You can use `sysfs_emit_at()` here.
I found that sysfs_emit_at and sysfs_emit() , will cause the module built failed .
Because it is RO attributes,to save time , I will prefer to keep using the sprintf as before.
Perry
> >
> >
> >> + }
> >> + }
> >> +
> >> + if (s != buf)
> >> + /* convert the last space to a newline */
> >> + *(s-1) = '\n';
> >
> > I believe this is not needed?
>
> Right this is no longer necessary now that we have 1 line per feature.
>
> >
> >
> >> + return (s - buf);
> >> +}
> >> +
> >> +static ssize_t dell_privacy_current_state_show(struct device *dev,
> >> + struct device_attribute *attr,
> >> + char *buf)
> >> +{
> >> + struct privacy_wmi_data *priv = dev_get_drvdata(dev);
> >> + enum dell_hardware_privacy_type type;
> >> + u32 privacy_state = priv->last_status;
> >> + u32 privacy_supported = priv->features_present;
> >> + char *s = buf;
> >> +
> >> + for (type = DELL_PRIVACY_TYPE_AUDIO; type <
> DELL_PRIVACY_TYPE_MAX; type++) {
> >> + if (privacy_supported & BIT(type)) {
> >> + if (privacy_state & BIT(type))
> >> + s += sprintf(s, "[%s] [unmuted]\n",
> privacy_types[type]);
> >> + else
> >> + s += sprintf(s, "[%s] [muted]\n",
> privacy_types[type]);
> >
> > sysfs_emit_at
> >
> >
> >> + }
> >> + }
> >> +
> >> + if (s != buf)
> >> + /* convert the last space to a newline */
> >> + *(s-1) = '\n';
> >
> > not needed?
>
> Ack.
>
> >> + return (s - buf);
> >> +}
> >> +
> >> +static DEVICE_ATTR_RO(dell_privacy_supported_type);
> >> +static DEVICE_ATTR_RO(dell_privacy_current_state);
> >> +
> >> +static struct attribute *privacy_attributes[] = {
> >> + &dev_attr_dell_privacy_supported_type.attr,
> >> + &dev_attr_dell_privacy_current_state.attr,
> >> + NULL,
> >> +};
> >> +
> >> +static const struct attribute_group privacy_attribute_group = {
> >> + .attrs = privacy_attributes
> >> +};
> >> +
> >> +/*
> >> + * Describes the Device State class exposed by BIOS which can be
> >> +consumed by
> >> + * various applications interested in knowing the Privacy feature
> capabilities.
> >> + * class DeviceState
> >> + * {
> >> + * [key, read] string InstanceName;
> >> + * [read] boolean ReadOnly;
> >> + * [WmiDataId(1), read] uint32 DevicesSupported;
> >> + * 0 – None, 0x1 – Microphone, 0x2 – Camera, 0x4 -ePrivacy Screen
> > ^ ^ ^ ^ ^^
> > Please use a single type of hyphen/dash consistently. One space is
> enough.
> >
> >
> >> + * [WmiDataId(2), read] uint32 CurrentState;
> >> + * 0:Off; 1:On. Bit0 – Microphone, Bit1 – Camera, Bit2 - ePrivacyScreen
> > ^ ^ ^
> > Same here.
> >
> >
> >> + * };
> >> + */
> >> +static int get_current_status(struct wmi_device *wdev) {
> >> + struct privacy_wmi_data *priv = dev_get_drvdata(&wdev->dev);
> >> + union acpi_object *obj_present;
> >> + u32 *buffer;
> >> + int ret = 0;
> >> +
> >> + if (!priv) {
> >> + pr_err("dell privacy priv is NULL\n");
> >
> > dev_err?
>
> Ack.
>
> >> + return -EINVAL;
> >> + }
> >> + /* check privacy support features and device states */
> >> + obj_present = wmidev_block_query(wdev, 0);
> >> + if (!obj_present) {
> >> + dev_err(&wdev->dev, "failed to read Binary MOF\n");
> >> + ret = -EIO;
> >> + return ret;
> >
> > return -EIO ?
> >
> >
> >> + }
> >> +
> >> + if (obj_present->type != ACPI_TYPE_BUFFER) {
> >> + dev_err(&wdev->dev, "Binary MOF is not a buffer!\n");
> >> + ret = -EIO;
> >> + goto obj_free;
> >> + }
> >> + /* Although it's not technically a failure, this would lead to
> >> + * unexpected behavior
> >> + */
> >> + if (obj_present->buffer.length != 8) {
> >> + dev_err(&wdev->dev, "Dell privacy buffer has unexpected
> length (%d)!\n",
> >> + obj_present->buffer.length);
> >> + ret = -EINVAL;
> >> + goto obj_free;
> >> + }
> >> + buffer = (u32 *)obj_present->buffer.pointer;
> >> + priv->features_present = buffer[0];
> >> + priv->last_status = buffer[1];
> >
> > It's a minor thing, but I still think it'd more explicit and somewhat
> > better to use `get_unaligned_le32()` (or `get_unaligned_cpu32()`).
> >
> >
> >> +
> >> +obj_free:
> >> + kfree(obj_present);
> >> + return ret;
> >> +}
> >> +
> >> +static int dell_privacy_micmute_led_set(struct led_classdev *led_cdev,
> >> + enum led_brightness brightness) {
> >> + struct privacy_wmi_data *priv = cdev_to_led(led_cdev);
> >> + static char *acpi_method = (char *)"ECAK";
> >> + acpi_status status;
> >> + acpi_handle handle;
> >> +
> >> + handle = ec_get_handle();
> >> + if (!handle)
> >> + return -EIO;
> >> +
> >> + if (!acpi_has_method(handle, acpi_method))
> >> + return -EIO;
> >> +
> >> + status = acpi_evaluate_object(handle, acpi_method, NULL, NULL);
> >> + if (ACPI_FAILURE(status)) {
> >> + dev_err(&priv->wdev->dev, "Error setting privacy EC ack
> value: %s\n",
> >> + acpi_format_exception(status));
> >> + return -EIO;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +/*
> >> + * Pressing the mute key activates a time delayed circuit to
> >> +physically cut
> >> + * off the mute. The LED is in the same circuit, so it reflects the
> >> +true
> >> + * state of the HW mute. The reason for the EC "ack" is so that
> >> +software
> >> + * can first invoke a SW mute before the HW circuit is cut off.
> >> +Without SW
> >> + * cutting this off first does not affect the time delayed muting or
> >> +status
> >> + * of the LED but there is a possibility of a "popping" noise.
> >> + *
> >> + * If the EC receives the SW ack, the circuit will be activated
> >> +before the
> >> + * delay completed.
> >> + *
> >> + * Exposing as an LED device allows the codec drivers notification
> >> +path to
> >> + * EC ACK to work
> >> + */
> >> +static int dell_privacy_leds_setup(struct device *dev) {
> >> + struct privacy_wmi_data *priv = dev_get_drvdata(dev);
> >> + int ret;
> >> +
> >> + priv->cdev.name = "dell-privacy::micmute";
> >> + priv->cdev.max_brightness = 1;
> >> + priv->cdev.brightness_set_blocking = dell_privacy_micmute_led_set;
> >> + priv->cdev.default_trigger = "audio-micmute";
> >> + priv->cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
> >> + ret = devm_led_classdev_register(dev, &priv->cdev);
> >> + if (ret)
> >> + return ret;
> >> + return 0;
> >
> > You can replace the last four lines with:
> >
> > return devm_led_classdev_register(...);
> >
> >
> >> +}
> >> +
> >> +static int dell_privacy_wmi_probe(struct wmi_device *wdev, const
> >> +void *context) {
> >> + struct privacy_wmi_data *priv;
> >> + struct key_entry *keymap;
> >> + int ret, i;
> >> +
> >> + ret = wmi_has_guid(DELL_PRIVACY_GUID);
> >> + if (!ret)
> >> + pr_debug("Unable to detect available Dell privacy
> devices: %d\n",
> >> +ret);
> >
> > When this branch is taken, `ret` is necessarily zero, so I don't think
> > printing it provides useful information.
> >
> > And I believe this `wmi_has_guid()` check is unnecessary since the
> > probe method would not be called if the device didn't have such WMI guid
> if I'm not mistaken.
>
> Ack, Perry please drop this check.
>
> >> +
> >> + priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
> >> + if (!priv)
> >> + return -ENOMEM;
> >> +
> >> + dev_set_drvdata(&wdev->dev, priv);
> >> + priv->wdev = wdev;
> >> + /* create evdev passing interface */
> >> + priv->input_dev = devm_input_allocate_device(&wdev->dev);
> >> + if (!priv->input_dev)
> >> + return -ENOMEM;
> >> +
> >> + /* remap the wmi keymap event to new keymap */
> >> + keymap = kcalloc(ARRAY_SIZE(dell_wmi_keymap_type_0012),
> >> + sizeof(struct key_entry), GFP_KERNEL);
> >> + if (!keymap)
> >> + return -ENOMEM;
> >> +
> >> + /* remap the keymap code with Dell privacy key type 0x12 as prefix
> >> + * KEY_MICMUTE scancode will be reported as 0x120001
> >> + */
> >> + for (i = 0; i < ARRAY_SIZE(dell_wmi_keymap_type_0012); i++) {
> >> + keymap[i] = dell_wmi_keymap_type_0012[i];
> >> + keymap[i].code |= (0x0012 << 16);
> >> + }
> >
> > I still don't see the need for allocating and copying the keymap.
> > Wouldn't the following be sufficient?
> >
> > #define DELL_SCAN_CODE(type, code) ((type) << 16 | (code))
> > static const struct key_entry dell_wmi_keymap_type_0012[] = {
> > { KE_KEY, DELL_SCAN_CODE(0x0012, 0x0001), { KEY_MICMUTE } },
> > { KE_SW, DELL_SCAN_CODE(0x0012, 0x0002),
> { SW_CAMERA_LENS_COVER } },
> > { KE_END, 0},
> > };
> >
> > Other Dell drivers potentially merge multiple keymaps, so dynamically
> > allocating the key_entry array is justified. Here I see no such need.
> > Can you explain why this copying is done?
>
> I was wondering the same thing when I reviewed v5, but this is consistent
> with the handling in dell-wmi.c so I'm fine with keeping it as is for consistency.
> But if you (perry) want to change this to the simpler code which Barnabás is
> suggesting that is fine too.
>
> >
> >
> >> + ret = sparse_keymap_setup(priv->input_dev, keymap, NULL);
> >> + if (ret)
> >> + return ret;
> >
> > `keymap` is leaked if this or any of the later early-returns are taken.
>
> Right as I mentioned in my review of v5, the "kfree(keymap);" needs to be
> moved before the "if (ret)" here, so the end results looks like this:
>
> ret = sparse_keymap_setup(priv->input_dev, keymap, NULL);
> kfree(keymap);
> if (ret)
> return ret;
>
> And the kfree(keymap) below goes away.
>
> >
> >
> >> +
> >> + priv->input_dev->dev.parent = &wdev->dev;
> >> + priv->input_dev->name = "Dell Privacy Driver";
> >> + priv->input_dev->id.bustype = BUS_HOST;
> >> +
> >> + ret = input_register_device(priv->input_dev);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + ret = get_current_status(priv->wdev);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + ret = devm_device_add_group(&wdev->dev,
> &privacy_attribute_group);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + ret = dell_privacy_leds_setup(&priv->wdev->dev);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + mutex_lock(&list_mutex);
> >> + list_add_tail(&priv->list, &wmi_list);
> >> + mutex_unlock(&list_mutex);
> >> + kfree(keymap);
>
> This kfree(keymap) needs to be removed. Note that switching to the
> suggested non copied keymap would get rid of all of this.
>
> >> + return 0;
> >> +}
> >> +
> >> +static int dell_privacy_wmi_remove(struct wmi_device *wdev) {
> >> + struct privacy_wmi_data *priv = dev_get_drvdata(&wdev->dev);
> >> +
> >> + mutex_lock(&list_mutex);
> >> + list_del(&priv->list);
> >> + mutex_unlock(&list_mutex);
> >> + return 0;
> >> +}
> >> +
> >> +static const struct wmi_device_id dell_wmi_privacy_wmi_id_table[] = {
> >> + { .guid_string = DELL_PRIVACY_GUID },
> >> + { },
> >> +};
> >> +
> >> +static struct wmi_driver dell_privacy_wmi_driver = {
> >> + .driver = {
> >> + .name = "dell-privacy",
> >> + },
> >> + .probe = dell_privacy_wmi_probe,
> >> + .remove = dell_privacy_wmi_remove,
> >> + .id_table = dell_wmi_privacy_wmi_id_table, };
> >> +
> >> +module_wmi_driver(dell_privacy_wmi_driver);
> >> +
> >> +MODULE_DEVICE_TABLE(wmi, dell_wmi_privacy_wmi_id_table);
> >> +MODULE_AUTHOR("Perry Yuan <perry_yuan@dell.com>");
> >> +MODULE_DESCRIPTION("Dell Privacy WMI Driver");
> >> +MODULE_LICENSE("GPL");
> >> diff --git a/drivers/platform/x86/dell/dell-privacy-wmi.h
> >> b/drivers/platform/x86/dell/dell-privacy-wmi.h
> >> new file mode 100644
> >> index 000000000000..a24893754286
> >> --- /dev/null
> >> +++ b/drivers/platform/x86/dell/dell-privacy-wmi.h
> >> @@ -0,0 +1,32 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-only */
> >> +/*
> >> + * Dell privacy notification driver
> >> + *
> >> + * Copyright (C) 2021 Dell Inc. All Rights Reserved.
> >> + */
> >> +
> >> +#ifndef _DELL_PRIVACY_WMI_H_
> >> +#define _DELL_PRIVACY_WMI_H_
> >> +
> >> +#if IS_ENABLED(CONFIG_DELL_PRIVACY)
> >> +int dell_privacy_has_micmute(void);
> >> +bool dell_privacy_present(void);
> >> +bool dell_privacy_process_event(int type, int code, int status);
> >> +#else /* CONFIG_DELL_PRIVACY */ static inline int
> >> +dell_privacy_has_micmute(void) {
> >> + return -ENODEV;
> >> +}
> >> +
> >> +static inline int dell_privacy_present(void)
> > ^^^
> > It's declared with `bool` return type a couple lines above.
> >
> >
> >> +{
> >> + return -ENODEV;
> >> +}
> >> +
> >> +static inline void dell_privacy_process_event(int type, int code,
> >> +int status)
> > ^^^^
> > It's declared with `bool` return type a couple lines above.
> >
> >
> >> +{}
> >> +#endif /* CONFIG_DELL_PRIVACY */
> >> +
> >> +int dell_privacy_acpi_init(void);
> >> +void dell_privacy_acpi_exit(void);
> >
> > These two don't seem to be referenced anywhere?
>
> These no longer exist and should be removed from the header.
>
> >
> >
> >> +#endif
> >> diff --git a/drivers/platform/x86/dell/dell-wmi.c
> >> b/drivers/platform/x86/dell/dell-wmi.c
> >> index bbdb3e860892..8ef9e22a538f 100644
> >> --- a/drivers/platform/x86/dell/dell-wmi.c
> >> +++ b/drivers/platform/x86/dell/dell-wmi.c
> >> @@ -27,6 +27,7 @@
> >> #include <acpi/video.h>
> >> #include "dell-smbios.h"
> >> #include "dell-wmi-descriptor.h"
> >> +#include "dell-privacy-wmi.h"
> >>
> >> MODULE_AUTHOR("Matthew Garrett <mjg@redhat.com>");
> >> MODULE_AUTHOR("Pali Rohár <pali@kernel.org>"); @@ -381,6 +382,7
> @@
> >> static void dell_wmi_notify(struct wmi_device *wdev,
> >> u16 *buffer_entry, *buffer_end;
> >> acpi_size buffer_size;
> >> int len, i;
> >> + int ret;
> >>
> >> if (obj->type != ACPI_TYPE_BUFFER) {
> >> pr_warn("bad response type %x\n", obj->type); @@ -427,7
> +429,6 @@
> >> static void dell_wmi_notify(struct wmi_device *wdev,
> >>
> >> switch (buffer_entry[1]) {
> >> case 0x0000: /* One key pressed or event occurred */
> >> - case 0x0012: /* Event with extended data occurred */
> >> if (len > 2)
> >> dell_wmi_process_key(wdev, buffer_entry[1],
> >> buffer_entry[2]);
> >> @@ -439,6 +440,16 @@ static void dell_wmi_notify(struct wmi_device
> *wdev,
> >> dell_wmi_process_key(wdev, buffer_entry[1],
> >> buffer_entry[i]);
> >> break;
> >> + case 0x0012:
> >> + ret = dell_privacy_present();
> >> + if ((ret) && (len > 2)) {
> >
> > Is the `len > 2` check correct?
> >
> > Moreover, I personally don't see any reason to use a new variable here
> (`ret`).
> >
> > If you incorporate the `dell_privacy_present()` check into
> > `dell_privacy_process_event()`,
>
> `dell_privacy_process_event()` already incorporates a presency check; and by
> only relying on that check we also avoid a race between the check and
> processing the event.
>
> > then even
> >
> > if (len > ?? && dell_privacy_process_event(...))
> > /* nothing */ ;
> > else if (len > 2)
> > dell_wmi_process_key(...);
> >
> > would work as Hans has already pointed it out. And there'd be no need
> > for `dell_privacy_present()` anymore.
>
> Ack, please change this and remove the entire dell_privacy_present()
> function.
>
> So you should end up with this here:
>
> if (len > 4 &&
> dell_privacy_process_event(buffer_entry[1], buffer_entry[3], buffer_entry[4]))
> /* nothing */ ;
> else if (len > 2)
> dell_wmi_process_key(wdev, buffer_entry[1],
> dell_wmi_process_key(wdev, buffer_entry[1],
> break;
>
> Regards,
>
> Hans
>
>
>
> >
> >
> >> + dell_privacy_process_event(buffer_entry[1],
> >> + buffer_entry[3],
> buffer_entry[4]);
> >> + } else if (len > 2) {
> >> + dell_wmi_process_key(wdev, buffer_entry[1],
> >> + buffer_entry[2]);
> >> + }
> >> + break;
> >> default: /* Unknown event */
> >> pr_info("Unknown WMI event type 0x%x\n",
> >> (int)buffer_entry[1]);
> >> --
> >> 2.25.1
> >>
> >
> >
> > Regards,
> > Barnabás Pőcze
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-04-12 4:12 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-04 8:31 [PATCH v6 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy Perry Yuan
2021-04-04 16:53 ` Barnabás Pőcze
2021-04-05 15:17 ` Limonciello, Mario
2021-04-07 15:15 ` Hans de Goede
2021-04-12 4:11 ` Yuan, Perry
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).