LinuxLists.cc - [PATCH] platform/x86: dell-privacy: Add support for new privacy driver

2020-11-03 12:58:04

Subject: [PATCH] platform/x86: dell-privacy: Add support for new privacy driver

From: perry_yuan <[email protected]>

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 enabled, any applications will not get any audio or
video stream.
when user pressed ctrl+F4 hotkey, audio privacy mode will be enabled
and camera mute hotkey is ctrl+F9.

Signed-off-by: Perry Yuan <[email protected]>
Signed-off-by: Limonciello Mario <[email protected]>
---
drivers/platform/x86/Kconfig | 12 ++
drivers/platform/x86/Makefile | 4 +-
drivers/platform/x86/dell-laptop.c | 41 ++--
drivers/platform/x86/dell-privacy-acpi.c | 139 ++++++++++++
drivers/platform/x86/dell-privacy-wmi.c | 259 +++++++++++++++++++++++
drivers/platform/x86/dell-privacy-wmi.h | 23 ++
drivers/platform/x86/dell-wmi.c | 90 ++++----
7 files changed, 513 insertions(+), 55 deletions(-)
create mode 100644 drivers/platform/x86/dell-privacy-acpi.c
create mode 100644 drivers/platform/x86/dell-privacy-wmi.c
create mode 100644 drivers/platform/x86/dell-privacy-wmi.h

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 40219bba6801..0cb6bf5a9565 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -454,6 +454,18 @@ config DELL_WMI_LED
This adds support for the Latitude 2100 and similar
notebooks that have an external LED.

+config DELL_PRIVACY
+ tristate "Dell Hardware Privacy Support"
+ depends on ACPI
+ depends on ACPI_WMI
+ depends on INPUT
+ depends on DELL_LAPTOP
+ select DELL_WMI
+ help
+ This driver provides a driver to support messaging related to the
+ privacy button presses on applicable Dell laptops from 2021 and
+ newer.
+
config AMILO_RFKILL
tristate "Fujitsu-Siemens Amilo rfkill support"
depends on RFKILL
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 5f823f7eff45..111f7215db2f 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -47,7 +47,9 @@ obj-$(CONFIG_DELL_WMI) += dell-wmi.o
obj-$(CONFIG_DELL_WMI_DESCRIPTOR) += dell-wmi-descriptor.o
obj-$(CONFIG_DELL_WMI_AIO) += dell-wmi-aio.o
obj-$(CONFIG_DELL_WMI_LED) += dell-wmi-led.o
-
+obj-$(CONFIG_DELL_PRIVACY) += dell-privacy.o
+dell-privacy-objs := dell-privacy-wmi.o \
+ dell-privacy-acpi.o
# Fujitsu
obj-$(CONFIG_AMILO_RFKILL) += amilo-rfkill.o
obj-$(CONFIG_FUJITSU_LAPTOP) += fujitsu-laptop.o
diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 5e9c2296931c..12b91de09356 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -30,6 +30,7 @@
#include <acpi/video.h>
#include "dell-rbtn.h"
#include "dell-smbios.h"
+#include "dell-privacy-wmi.h"

struct quirk_entry {
bool touchpad_led;
@@ -90,6 +91,7 @@ static struct rfkill *wifi_rfkill;
static struct rfkill *bluetooth_rfkill;
static struct rfkill *wwan_rfkill;
static bool force_rfkill;
+static bool privacy_valid;

module_param(force_rfkill, bool, 0444);
MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted models");
@@ -2202,20 +2204,25 @@ static int __init dell_init(void)
debugfs_create_file("rfkill", 0444, dell_laptop_dir, NULL,
&dell_debugfs_fops);

- dell_laptop_register_notifier(&dell_laptop_notifier);
-
- 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 (acpi_video_get_backlight_type() != acpi_backlight_vendor)
- return 0;
-
- token = dell_smbios_find_token(BRIGHTNESS_TOKEN);
+ dell_laptop_register_notifier(&dell_laptop_notifier);
+
+ if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
+ dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) {
+#if IS_ENABLED(CONFIG_DELL_PRIVACY)
+ privacy_valid = dell_privacy_valid() == -ENODEV;
+#endif
+ if (!privacy_valid) {
+ 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)
+ return 0;
+
+ token = dell_smbios_find_token(BRIGHTNESS_TOKEN);
if (token) {
struct calling_interface_buffer buffer;

@@ -2257,7 +2264,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 (!privacy_valid)
+ led_classdev_unregister(&micmute_led_cdev);
fail_led:
dell_cleanup_rfkill();
fail_rfkill:
@@ -2278,7 +2286,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 (!privacy_valid)
+ led_classdev_unregister(&micmute_led_cdev);
dell_cleanup_rfkill();
if (platform_device) {
platform_device_unregister(platform_device);
diff --git a/drivers/platform/x86/dell-privacy-acpi.c b/drivers/platform/x86/dell-privacy-acpi.c
new file mode 100644
index 000000000000..516cd99167c3
--- /dev/null
+++ b/drivers/platform/x86/dell-privacy-acpi.c
@@ -0,0 +1,139 @@
+// 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/device.h>
+#include <linux/fs.h>
+#include <linux/kernel.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/string.h>
+#include <linux/sysfs.h>
+#include <linux/types.h>
+#include <linux/wmi.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include "dell-privacy-wmi.h"
+
+#define PRIVACY_PlATFORM_NAME "dell-privacy-acpi"
+#define ACPI_PRIVACY_DEVICE "\\_SB.PC00.LPCB.ECDV"
+#define ACPI_PRIVACY_EC_ACK ACPI_PRIVACY_DEVICE ".ECAK"
+
+static struct platform_device *privacy_acpi_pdev;
+
+struct privacy_acpi_priv {
+ struct device *dev;
+ struct acpi_device *acpi_dev;
+ struct input_dev *input_dev;
+ struct platform_device *platform_device;
+};
+
+static int micmute_led_set(struct led_classdev *led_cdev,
+ enum led_brightness brightness)
+{
+ acpi_status status;
+
+ status = acpi_evaluate_object(NULL, ACPI_PRIVACY_EC_ACK, NULL, NULL);
+ if (ACPI_FAILURE(status)) {
+ dev_err(led_cdev->dev, "Error setting privacy audio EC ack value: %d\n",status);
+ return -EIO;
+ }
+ return 0;
+}
+
+static struct led_classdev micmute_led_cdev = {
+ .name = "platform::micmute",
+ .max_brightness = 1,
+ .brightness_set_blocking = micmute_led_set,
+ .default_trigger = "audio-micmute",
+};
+
+static int privacy_acpi_remove(struct platform_device *pdev)
+{
+ dev_set_drvdata(&pdev->dev, NULL);
+ return 0;
+}
+
+static int privacy_acpi_probe(struct platform_device *pdev)
+{
+ struct privacy_acpi_priv *privacy;
+ acpi_handle handle;
+ acpi_status status;
+ int err;
+
+ privacy = kzalloc(sizeof(struct privacy_acpi_priv), GFP_KERNEL);
+ if (!privacy)
+ return -ENOMEM;
+
+ privacy->dev = &pdev->dev;
+ privacy->platform_device = pdev;
+ platform_set_drvdata(pdev, privacy);
+ /* Look for software micmute complete notification device */
+ status = acpi_get_handle(ACPI_ROOT_OBJECT,
+ ACPI_PRIVACY_DEVICE,
+ &handle);
+ if (ACPI_FAILURE(status)) {
+ dev_err(privacy->dev, "Unable to find Dell`s EC device %s: %d\n",
+ ACPI_PRIVACY_DEVICE, status);
+ return -ENXIO;
+ }
+
+ micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
+ err = led_classdev_register(&privacy_acpi_pdev->dev, &micmute_led_cdev);
+ if (err < 0)
+ return -EIO;
+
+ return 0;
+}
+
+static const struct acpi_device_id privacy_acpi_device_ids[] = {
+ {"PNP0C09", 0},
+ {"", 0},
+};
+MODULE_DEVICE_TABLE(acpi, privacy_acpi_device_ids);
+
+static struct platform_driver privacy_platform_driver = {
+ .driver = {
+ .name = PRIVACY_PlATFORM_NAME,
+ .acpi_match_table = ACPI_PTR(privacy_acpi_device_ids),
+ },
+ .probe = privacy_acpi_probe,
+ .remove = privacy_acpi_remove,
+};
+
+int privacy_acpi_init(void)
+{
+ int err;
+
+ err = platform_driver_register(&privacy_platform_driver);
+ if (err)
+ return err;
+
+ privacy_acpi_pdev = platform_device_register_simple(
+ PRIVACY_PlATFORM_NAME, -1, NULL, 0);
+ if (IS_ERR(privacy_acpi_pdev)) {
+ err = PTR_ERR(privacy_acpi_pdev);
+ goto err_platform;
+ }
+ return 0;
+
+err_platform:
+ platform_driver_unregister(&privacy_platform_driver);
+ return err;
+}
+
+void privacy_acpi_cleanup(void)
+{
+ platform_driver_unregister(&privacy_platform_driver);
+}
+
+MODULE_AUTHOR("Perry Yuan <[email protected]>");
+MODULE_DESCRIPTION("DELL Privacy ACPI Driver");
+MODULE_LICENSE("GPL");
+
diff --git a/drivers/platform/x86/dell-privacy-wmi.c b/drivers/platform/x86/dell-privacy-wmi.c
new file mode 100644
index 000000000000..6c36b7ec44c6
--- /dev/null
+++ b/drivers/platform/x86/dell-privacy-wmi.c
@@ -0,0 +1,259 @@
+// 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/input.h>
+#include <linux/input/sparse-keymap.h>
+#include <linux/list.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 PRIVACY_SCREEN_STATUS BIT(2)
+
+static int privacy_valid = -EPROBE_DEFER;
+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;
+ u32 features_present;
+ u32 last_status;
+};
+
+/*
+ * 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 } },
+};
+
+bool dell_privacy_valid(void)
+{
+ bool ret;
+
+ mutex_lock(&list_mutex);
+ ret = wmi_has_guid(DELL_PRIVACY_GUID);
+ if (!ret){
+ return -ENODEV;
+ }
+ ret = privacy_valid;
+ mutex_unlock(&list_mutex);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(dell_privacy_valid);
+
+void dell_privacy_process_event(int type, int code, int status)
+{
+ struct privacy_wmi_data *priv;
+ const struct key_entry *key;
+
+ mutex_lock(&list_mutex);
+ priv = list_first_entry_or_null(&wmi_list,
+ struct privacy_wmi_data,
+ list);
+ if (priv == NULL) {
+ pr_err("dell privacy priv is NULL\n");
+ goto error;
+ }
+
+ key = sparse_keymap_entry_from_scancode(priv->input_dev, (type << 16)|code);
+ if (!key) {
+ dev_dbg(&priv->wdev->dev, "Unknown key with type 0x%04x and code 0x%04x pressed\n",
+ type, code);
+ goto error;
+ }
+
+ switch (code) {
+ case DELL_PRIVACY_TYPE_AUDIO: /* Mic mute */
+ priv->last_status = status;
+ sparse_keymap_report_entry(priv->input_dev, key, 1, true);
+ break;
+ case DELL_PRIVACY_TYPE_CAMERA: /* Camera mute */
+ priv->last_status = status;
+ sparse_keymap_report_entry(priv->input_dev, key, 1, true);
+ break;
+ default:
+ dev_dbg(&priv->wdev->dev, "unknown event type %u /%u",
+ type, code);
+ }
+error:
+ mutex_unlock(&list_mutex);
+ return;
+}
+EXPORT_SYMBOL_GPL(dell_privacy_process_event);
+
+static int get_current_status(struct wmi_device *wdev)
+{
+ struct privacy_wmi_data *priv = dev_get_drvdata(&wdev->dev);
+ union acpi_object *obj_present;
+ union acpi_object *obj_current;
+ int ret = 0;
+
+ if (priv == NULL) {
+ pr_err("dell privacy priv is NULL\n");
+ return -EINVAL;
+ }
+ /* get devices supported */
+ obj_present = wmidev_block_query(wdev, 0);
+ if (obj_present->type != ACPI_TYPE_INTEGER) {
+ ret = -EIO;
+ goto present_free;
+ }
+ priv->features_present = obj_present->integer.value;
+
+ /* get current state */
+ obj_current = wmidev_block_query(wdev, 1);
+ if (obj_current->type != ACPI_TYPE_INTEGER) {
+ ret = -EIO;
+ goto current_free;
+ }
+ priv->last_status = obj_current->integer.value;
+current_free:
+ kfree(obj_current);
+present_free:
+ kfree(obj_present);
+ return ret;
+}
+
+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, pos = 0;
+
+ priv = devm_kzalloc(&wdev->dev, sizeof(struct privacy_wmi_data),
+ GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ /* create evdev passing interface */
+ priv->input_dev = devm_input_allocate_device(&wdev->dev);
+ if (!priv->input_dev)
+ return -ENOMEM;
+
+ __set_bit(EV_KEY, priv->input_dev->evbit);
+ __set_bit(KEY_MICMUTE, priv->input_dev->keybit);
+ __set_bit(EV_MSC, priv->input_dev->evbit);
+ __set_bit(MSC_SCAN, priv->input_dev->mscbit);
+ keymap = kcalloc(
+ ARRAY_SIZE(dell_wmi_keymap_type_0012) +
+ 1,
+ sizeof(struct key_entry), GFP_KERNEL);
+ if (!keymap) {
+ ret = -ENOMEM;
+ goto err_free_dev;
+ }
+ for (i = 0; i < ARRAY_SIZE(dell_wmi_keymap_type_0012); i++) {
+ keymap[pos] = dell_wmi_keymap_type_0012[i];
+ keymap[pos].code |= (0x0012 << 16);
+ pos++;
+ }
+ 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;
+
+ if (input_register_device(priv->input_dev)) {
+ pr_debug("input_register_device failed to register! \n");
+ return -ENODEV;
+ }
+
+ priv->wdev = wdev;
+ dev_set_drvdata(&wdev->dev, priv);
+ mutex_lock(&list_mutex);
+ list_add_tail(&priv->list, &wmi_list);
+ privacy_valid = true;
+ if (get_current_status(wdev)) {
+ goto err_free_dev;
+ }
+ mutex_unlock(&list_mutex);
+ kfree(keymap);
+ return 0;
+
+err_free_dev:
+ input_free_device(priv->input_dev);
+ kfree(keymap);
+ return ret;
+}
+
+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);
+ privacy_valid = -ENODEV;
+ 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,
+};
+
+static int __init init_dell_privacy(void)
+{
+ int ret, wmi, acpi;
+
+ wmi = wmi_driver_register(&dell_privacy_wmi_driver);
+ if (wmi) {
+ pr_debug("Failed to initialize privacy wmi driver: %d\n", wmi);
+ return wmi;
+ }
+
+ acpi = privacy_acpi_init();
+ if (acpi) {
+ pr_debug("failed to initialize privacy wmi acpi driver: %d\n", acpi);
+ return acpi;
+ }
+
+ return 0;
+}
+
+void exit_dell_privacy_wmi(void)
+{
+ wmi_driver_unregister(&dell_privacy_wmi_driver);
+}
+
+static void __exit exit_dell_privacy(void)
+{
+ privacy_acpi_cleanup();
+ exit_dell_privacy_wmi();
+}
+
+module_init(init_dell_privacy);
+module_exit(exit_dell_privacy);
+
+MODULE_DEVICE_TABLE(wmi, dell_wmi_privacy_wmi_id_table);
+MODULE_AUTHOR("Perry Yuan <[email protected]>");
+MODULE_DESCRIPTION("Dell Privacy WMI Driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/platform/x86/dell-privacy-wmi.h b/drivers/platform/x86/dell-privacy-wmi.h
new file mode 100644
index 000000000000..94af81d76e44
--- /dev/null
+++ b/drivers/platform/x86/dell-privacy-wmi.h
@@ -0,0 +1,23 @@
+/* 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_
+#include <linux/wmi.h>
+
+bool dell_privacy_valid(void);
+void dell_privacy_process_event(int type, int code, int status);
+int privacy_acpi_init(void);
+void privacy_acpi_cleanup(void);
+
+/* DELL Privacy Type */
+enum {
+ DELL_PRIVACY_TYPE_UNKNOWN = 0x0,
+ DELL_PRIVACY_TYPE_AUDIO,
+ DELL_PRIVACY_TYPE_CAMERA,
+};
+#endif
diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index bbdb3e860892..44bb74e4df86 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/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 <[email protected]>");
MODULE_AUTHOR("Pali Rohár <[email protected]>");
@@ -410,44 +411,57 @@ static void dell_wmi_notify(struct wmi_device *wdev,
if (buffer_end > buffer_entry + buffer_entry[0] + 1)
buffer_end = buffer_entry + buffer_entry[0] + 1;

- while (buffer_entry < buffer_end) {
-
- len = buffer_entry[0];
- if (len == 0)
- break;
-
- len++;
-
- if (buffer_entry + len > buffer_end) {
- pr_warn("Invalid length of WMI event\n");
- break;
- }
-
- pr_debug("Process buffer (%*ph)\n", len*2, buffer_entry);
-
- 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]);
- /* Extended data is currently ignored */
- break;
- case 0x0010: /* Sequence of keys pressed */
- case 0x0011: /* Sequence of events occurred */
- for (i = 2; i < len; ++i)
- dell_wmi_process_key(wdev, buffer_entry[1],
- buffer_entry[i]);
- break;
- default: /* Unknown event */
- pr_info("Unknown WMI event type 0x%x\n",
- (int)buffer_entry[1]);
- break;
- }
-
- buffer_entry += len;
-
- }
+ while (buffer_entry < buffer_end) {
+
+ len = buffer_entry[0];
+ if (len == 0)
+ break;
+
+ len++;
+
+ if (buffer_entry + len > buffer_end) {
+ pr_warn("Invalid length of WMI event\n");
+ break;
+ }
+
+ pr_debug("Process buffer (%*ph)\n", len*2, buffer_entry);
+
+ switch (buffer_entry[1]) {
+ case 0x0000: /* One key pressed or event occurred */
+ if (len > 2)
+ dell_wmi_process_key(wdev, buffer_entry[1],
+ buffer_entry[2]);
+ break;
+ case 0x0010: /* Sequence of keys pressed */
+ case 0x0011: /* Sequence of events occurred */
+ for (i = 2; i < len; ++i)
+ dell_wmi_process_key(wdev, buffer_entry[1],
+ buffer_entry[i]);
+ break;
+ case 0x0012:
+#if IS_ENABLED(CONFIG_DELL_PRIVACY)
+ if (dell_privacy_valid()) {
+ 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]);
+ }
+#else
+ /* Extended data is currently ignored */
+ if (len > 2)
+ dell_wmi_process_key(wdev, buffer_entry[1], buffer_entry[2]);
+#endif
+ break;
+ default: /* Unknown event */
+ pr_info("Unknown WMI event type 0x%x\n",
+ (int)buffer_entry[1]);
+ break;
+ }
+
+ buffer_entry += len;
+
+ }

}

--
2.25.1

2020-11-03 16:25:26

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH] platform/x86: dell-privacy: Add support for new privacy driver

Hi Perry,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.10-rc2]
[cannot apply to next-20201103]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/0day-ci/linux/commits/Perry-Yuan/platform-x86-dell-privacy-Add-support-for-new-privacy-driver/20201103-205721
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git b7cbaf59f62f8ab8f157698f9e31642bff525bd0
config: i386-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/cee9f60d7ca58d8f0c6b113c5f7af2dec7e2e27d
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Perry-Yuan/platform-x86-dell-privacy-Add-support-for-new-privacy-driver/20201103-205721
git checkout cee9f60d7ca58d8f0c6b113c5f7af2dec7e2e27d
# save the attached .config to linux build tree
make W=1 ARCH=i386

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

drivers/platform/x86/dell-laptop.c: In function 'dell_init':
>> drivers/platform/x86/dell-laptop.c:2212:46: warning: comparison of constant '-19' with boolean expression is always false [-Wbool-compare]
2212 | privacy_valid = dell_privacy_valid() == -ENODEV;
| ^~
drivers/platform/x86/dell-laptop.c: In function 'dell_exit':
drivers/platform/x86/dell-laptop.c:2289:5: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
2289 | if (!privacy_valid)
| ^~
drivers/platform/x86/dell-laptop.c:2291:2: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if'
2291 | dell_cleanup_rfkill();
| ^~~~~~~~~~~~~~~~~~~
--
drivers/platform/x86/dell-privacy-wmi.c: In function 'init_dell_privacy':
drivers/platform/x86/dell-privacy-wmi.c:225:9: warning: unused variable 'ret' [-Wunused-variable]
225 | int ret, wmi, acpi;
| ^~~
drivers/platform/x86/dell-privacy-wmi.c: At top level:
>> drivers/platform/x86/dell-privacy-wmi.c:242:6: warning: no previous prototype for 'exit_dell_privacy_wmi' [-Wmissing-prototypes]
242 | void exit_dell_privacy_wmi(void)
| ^~~~~~~~~~~~~~~~~~~~~

vim +2212 drivers/platform/x86/dell-laptop.c

2165
2166 static int __init dell_init(void)
2167 {
2168 struct calling_interface_token *token;
2169 int max_intensity = 0;
2170 int ret;
2171
2172 if (!dmi_check_system(dell_device_table))
2173 return -ENODEV;
2174
2175 quirks = NULL;
2176 /* find if this machine support other functions */
2177 dmi_check_system(dell_quirks);
2178
2179 ret = platform_driver_register(&platform_driver);
2180 if (ret)
2181 goto fail_platform_driver;
2182 platform_device = platform_device_alloc("dell-laptop", -1);
2183 if (!platform_device) {
2184 ret = -ENOMEM;
2185 goto fail_platform_device1;
2186 }
2187 ret = platform_device_add(platform_device);
2188 if (ret)
2189 goto fail_platform_device2;
2190
2191 ret = dell_setup_rfkill();
2192
2193 if (ret) {
2194 pr_warn("Unable to setup rfkill\n");
2195 goto fail_rfkill;
2196 }
2197
2198 if (quirks && quirks->touchpad_led)
2199 touchpad_led_init(&platform_device->dev);
2200
2201 kbd_led_init(&platform_device->dev);
2202
2203 dell_laptop_dir = debugfs_create_dir("dell_laptop", NULL);
2204 debugfs_create_file("rfkill", 0444, dell_laptop_dir, NULL,
2205 &dell_debugfs_fops);
2206
2207 dell_laptop_register_notifier(&dell_laptop_notifier);
2208
2209 if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
2210 dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) {
2211 #if IS_ENABLED(CONFIG_DELL_PRIVACY)
> 2212 privacy_valid = dell_privacy_valid() == -ENODEV;
2213 #endif
2214 if (!privacy_valid) {
2215 micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
2216 ret = led_classdev_register(&platform_device->dev, &micmute_led_cdev);
2217 if (ret < 0)
2218 goto fail_led;
2219 }
2220 }
2221
2222 if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
2223 return 0;
2224
2225 token = dell_smbios_find_token(BRIGHTNESS_TOKEN);
2226 if (token) {
2227 struct calling_interface_buffer buffer;
2228
2229 dell_fill_request(&buffer, token->location, 0, 0, 0);
2230 ret = dell_send_request(&buffer,
2231 CLASS_TOKEN_READ, SELECT_TOKEN_AC);
2232 if (ret == 0)
2233 max_intensity = buffer.output[3];
2234 }
2235
2236 if (max_intensity) {
2237 struct backlight_properties props;
2238 memset(&props, 0, sizeof(struct backlight_properties));
2239 props.type = BACKLIGHT_PLATFORM;
2240 props.max_brightness = max_intensity;
2241 dell_backlight_device = backlight_device_register("dell_backlight",
2242 &platform_device->dev,
2243 NULL,
2244 &dell_ops,
2245 &props);
2246
2247 if (IS_ERR(dell_backlight_device)) {
2248 ret = PTR_ERR(dell_backlight_device);
2249 dell_backlight_device = NULL;
2250 goto fail_backlight;
2251 }
2252
2253 dell_backlight_device->props.brightness =
2254 dell_get_intensity(dell_backlight_device);
2255 if (dell_backlight_device->props.brightness < 0) {
2256 ret = dell_backlight_device->props.brightness;
2257 goto fail_get_brightness;
2258 }
2259 backlight_update_status(dell_backlight_device);
2260 }
2261
2262 return 0;
2263
2264 fail_get_brightness:
2265 backlight_device_unregister(dell_backlight_device);
2266 fail_backlight:
2267 if (!privacy_valid)
2268 led_classdev_unregister(&micmute_led_cdev);
2269 fail_led:
2270 dell_cleanup_rfkill();
2271 fail_rfkill:
2272 platform_device_del(platform_device);
2273 fail_platform_device2:
2274 platform_device_put(platform_device);
2275 fail_platform_device1:
2276 platform_driver_unregister(&platform_driver);
2277 fail_platform_driver:
2278 return ret;
2279 }
2280

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]

Attachments:

(No filename) (6.70 kB)
.config.gz (74.09 kB)
Download all attachments

2020-11-03 16:55:33

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH] platform/x86: dell-privacy: Add support for new privacy driver

Attachments:

(No filename) (2.57 kB)
.config.gz (31.95 kB)
Download all attachments

2020-11-03 19:18:30

by Barnabás Pőcze

[permalink] [raw]

Subject: Re: [PATCH] platform/x86: dell-privacy: Add support for new privacy driver

Hi

(I really hope Hans and Mark won't get mad at me for writing some thoughts about
this patch.)

First of all, indentation should be tabs (= 8 spaces), not spaces. If I see it
correctly, the two are mixed here.

And please make the printed messages consistent (capitalization, etc.),
I believe punctuation at the end is not necessary, and don't leave whitespaces
between the text and newline character. Please always run `checkpatch` on the patch
to see what can/needs to be improved.

There are also parts in the code (variables not actually used, etc.) that make me
feel like it's somewhat unfinished, or rather, incomplete.

Both `dell-privacy-acpi` and `dell-privacy-wmi` have the same comment:
"Dell privacy notification driver", but surely they are not the same thing?

I have also added a couple comments inline.

> From: perry_yuan <[email protected]>
>
> 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 enabled, any applications will not get any audio or
> video stream.
> when user pressed ctrl+F4 hotkey, audio privacy mode will be enabled
> and camera mute hotkey is ctrl+F9.
>
> Signed-off-by: Perry Yuan <[email protected]>
> Signed-off-by: Limonciello Mario <[email protected]>
> ---
> drivers/platform/x86/Kconfig | 12 ++
> drivers/platform/x86/Makefile | 4 +-
> drivers/platform/x86/dell-laptop.c | 41 ++--
> drivers/platform/x86/dell-privacy-acpi.c | 139 ++++++++++++
> drivers/platform/x86/dell-privacy-wmi.c | 259 +++++++++++++++++++++++
> drivers/platform/x86/dell-privacy-wmi.h | 23 ++
> drivers/platform/x86/dell-wmi.c | 90 ++++----
> 7 files changed, 513 insertions(+), 55 deletions(-)
> create mode 100644 drivers/platform/x86/dell-privacy-acpi.c
> create mode 100644 drivers/platform/x86/dell-privacy-wmi.c
> create mode 100644 drivers/platform/x86/dell-privacy-wmi.h
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 40219bba6801..0cb6bf5a9565 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -454,6 +454,18 @@ config DELL_WMI_LED
> This adds support for the Latitude 2100 and similar
> notebooks that have an external LED.
>
> +config DELL_PRIVACY
> + tristate "Dell Hardware Privacy Support"
> + depends on ACPI
> + depends on ACPI_WMI
> + depends on INPUT
> + depends on DELL_LAPTOP
> + select DELL_WMI
> + help
> + This driver provides a driver to support messaging related to the

I'm not a native English speaker, but "messaging" seems a strange choice of
words to me here.

> + privacy button presses on applicable Dell laptops from 2021 and
> + newer.

I have the same feeling about "from 2021 and newer".

> +
> config AMILO_RFKILL
> tristate "Fujitsu-Siemens Amilo rfkill support"
> depends on RFKILL
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 5f823f7eff45..111f7215db2f 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -47,7 +47,9 @@ obj-$(CONFIG_DELL_WMI) += dell-wmi.o
> obj-$(CONFIG_DELL_WMI_DESCRIPTOR) += dell-wmi-descriptor.o
> obj-$(CONFIG_DELL_WMI_AIO) += dell-wmi-aio.o
> obj-$(CONFIG_DELL_WMI_LED) += dell-wmi-led.o
> -
> +obj-$(CONFIG_DELL_PRIVACY) += dell-privacy.o
> +dell-privacy-objs := dell-privacy-wmi.o \
> + dell-privacy-acpi.o
> # Fujitsu
> obj-$(CONFIG_AMILO_RFKILL) += amilo-rfkill.o
> obj-$(CONFIG_FUJITSU_LAPTOP) += fujitsu-laptop.o
> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> index 5e9c2296931c..12b91de09356 100644
> -- a/drivers/platform/x86/dell-laptop.c
> ++ b/drivers/platform/x86/dell-laptop.c
> @@ -30,6 +30,7 @@
> #include <acpi/video.h>
> #include "dell-rbtn.h"
> #include "dell-smbios.h"
> #include "dell-privacy-wmi.h"
>
> struct quirk_entry {
> bool touchpad_led;
> @@ -90,6 +91,7 @@ static struct rfkill *wifi_rfkill;
> static struct rfkill *bluetooth_rfkill;
> static struct rfkill *wwan_rfkill;
> static bool force_rfkill;
> static bool privacy_valid;
>
> module_param(force_rfkill, bool, 0444);
> MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted models");
> @@ -2202,20 +2204,25 @@ static int __init dell_init(void)
> debugfs_create_file("rfkill", 0444, dell_laptop_dir, NULL,
> &dell_debugfs_fops);
>
> dell_laptop_register_notifier(&dell_laptop_notifier);
>
> 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 (acpi_video_get_backlight_type() != acpi_backlight_vendor)
> return 0;
>
> token = dell_smbios_find_token(BRIGHTNESS_TOKEN);
> dell_laptop_register_notifier(&dell_laptop_notifier);
>
> if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
> dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) {
> #if IS_ENABLED(CONFIG_DELL_PRIVACY)
> privacy_valid = dell_privacy_valid() == -ENODEV;

`dell_privacy_valid()` returns `bool`.

> #endif
> if (!privacy_valid) {
> 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)
> return 0;
>
> token = dell_smbios_find_token(BRIGHTNESS_TOKEN);
> if (token) {
> struct calling_interface_buffer buffer;
>
> @@ -2257,7 +2264,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 (!privacy_valid)
> led_classdev_unregister(&micmute_led_cdev);
> fail_led:
> dell_cleanup_rfkill();
> fail_rfkill:
> @@ -2278,7 +2286,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 (!privacy_valid)
> led_classdev_unregister(&micmute_led_cdev);
> dell_cleanup_rfkill();
> if (platform_device) {
> platform_device_unregister(platform_device);
> diff --git a/drivers/platform/x86/dell-privacy-acpi.c b/drivers/platform/x86/dell-privacy-acpi.c
> new file mode 100644
> index 000000000000..516cd99167c3
> --- /dev/null
> +++ b/drivers/platform/x86/dell-privacy-acpi.c
> @@ -0,0 +1,139 @@
> +// 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/device.h>
> +#include <linux/fs.h>
> +#include <linux/kernel.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/string.h>
> +#include <linux/sysfs.h>
> +#include <linux/types.h>
> +#include <linux/wmi.h>
> +#include <linux/slab.h>
> +#include <linux/platform_device.h>
> +#include "dell-privacy-wmi.h"
> +
> +#define PRIVACY_PlATFORM_NAME "dell-privacy-acpi"
^
should be upper case

> +#define ACPI_PRIVACY_DEVICE "\\_SB.PC00.LPCB.ECDV"
> +#define ACPI_PRIVACY_EC_ACK ACPI_PRIVACY_DEVICE ".ECAK"
> +
> +static struct platform_device *privacy_acpi_pdev;
> +
> +struct privacy_acpi_priv {
> + struct device *dev;
> + struct acpi_device *acpi_dev;
> + struct input_dev *input_dev;
> + struct platform_device *platform_device;
> +};
> +
> +static int micmute_led_set(struct led_classdev *led_cdev,
> + enum led_brightness brightness)
> +{
> + acpi_status status;
> +
> + status = acpi_evaluate_object(NULL, ACPI_PRIVACY_EC_ACK, NULL, NULL);

The handle of "ACPI_PRIVACY_DEVICE" is queried in `privacy_acpi_probe()`. Why
is that not used here?

> + if (ACPI_FAILURE(status)) {
> + dev_err(led_cdev->dev, "Error setting privacy audio EC ack value: %d\n",status);
^
missing space -/

I think `acpi_format_exception()` could be used here.

I don't quite see why brightness is completely ignored? Does this just toggle
the LED state? Even in that case I think something should be done to avoid the
sysfs attribute showing brightness=1 while the LED is actually off.

Does the `ACPI_PRIVACY_EC_ACK` method (?) acknowledge something? If so, what? And
why is it called in the brightness setting method of a LED class device?

> + return -EIO;
> + }
> + return 0;
> +}
> +
> +static struct led_classdev micmute_led_cdev = {
> + .name = "platform::micmute",
> + .max_brightness = 1,
> + .brightness_set_blocking = micmute_led_set,
> + .default_trigger = "audio-micmute",
> +};

There is also the exact same `micmute_led_cdev` is in dell-laptop.c. Both are
valid? What's the difference? Why can't the LED be handled in just a single place?

> [...]
> +static const struct acpi_device_id privacy_acpi_device_ids[] = {
> + {"PNP0C09", 0},
> + {"", 0},
> +};
> +MODULE_DEVICE_TABLE(acpi, privacy_acpi_device_ids);
> +
> +static struct platform_driver privacy_platform_driver = {
> + .driver = {
> + .name = PRIVACY_PlATFORM_NAME,
> + .acpi_match_table = ACPI_PTR(privacy_acpi_device_ids),
> + },
> + .probe = privacy_acpi_probe,
> + .remove = privacy_acpi_remove,
> +};
> +
> +int privacy_acpi_init(void)
> +{
> + int err;
> +
> + err = platform_driver_register(&privacy_platform_driver);
> + if (err)
> + return err;
> +
> + privacy_acpi_pdev = platform_device_register_simple(
> + PRIVACY_PlATFORM_NAME, -1, NULL, 0);
> + if (IS_ERR(privacy_acpi_pdev)) {
> + err = PTR_ERR(privacy_acpi_pdev);
> + goto err_platform;
> + }
> + return 0;
> +
> +err_platform:
> + platform_driver_unregister(&privacy_platform_driver);
> + return err;
> +}

Maybe I'm missing something obvious, but I do believe this is overly complicated.
I don't see why you cannot check the ACPI path, if it exists, register
a platform device, and then register the led to that device? The whole platform driver
part could've been avoided as far as I see.

I'm also wondering if the ACPI path is enough to decide undoubtedly that this
is indeed a compatible device.

> +
> +void privacy_acpi_cleanup(void)
> +{
> + platform_driver_unregister(&privacy_platform_driver);
> +}

The platform device is not cleaned up.

> +
> +MODULE_AUTHOR("Perry Yuan <[email protected]>");
> +MODULE_DESCRIPTION("DELL Privacy ACPI Driver");
> +MODULE_LICENSE("GPL");
> +
> diff --git a/drivers/platform/x86/dell-privacy-wmi.c b/drivers/platform/x86/dell-privacy-wmi.c
> new file mode 100644
> index 000000000000..6c36b7ec44c6
> --- /dev/null
> +++ b/drivers/platform/x86/dell-privacy-wmi.c
> @@ -0,0 +1,259 @@
> +// 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/input.h>
> +#include <linux/input/sparse-keymap.h>
> +#include <linux/list.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 PRIVACY_SCREEN_STATUS BIT(2)

`#include <linux/bits.h>`?

> +
> +static int privacy_valid = -EPROBE_DEFER;
> +static LIST_HEAD(wmi_list);
> +static DEFINE_MUTEX(list_mutex);

What is the purpose of this list? At the moment I can't really see it.

> +
> +struct privacy_wmi_data {
> + struct input_dev *input_dev;
> + struct wmi_device *wdev;
> + struct list_head list;
> + u32 features_present;
> + u32 last_status;

`last_status` and `features_present` are there for no actual benefit.

> +};
> +
> +/*
> + * 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 } },

I see the calloc trick later to avoid writing KE_END, but I still think it'd be
better if there was an explicit KE_END entry.

> +};
> +
> +bool dell_privacy_valid(void)
> +{
> + bool ret;
> +
> + mutex_lock(&list_mutex);
> + ret = wmi_has_guid(DELL_PRIVACY_GUID);
> + if (!ret){
> + return -ENODEV;

The functions returns `bool`.

> + }
> + ret = privacy_valid;

I'm not sure if it's a good idea to just plainly assign an `int` to a `bool`.

> + mutex_unlock(&list_mutex);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(dell_privacy_valid);

Instead of always querying for the presence of the WMI GUID, wouldn't a single
atomic_t or similar be sufficient?

> +
> +void dell_privacy_process_event(int type, int code, int status)
> +{
> + struct privacy_wmi_data *priv;
> + const struct key_entry *key;
> +
> + mutex_lock(&list_mutex);
> + priv = list_first_entry_or_null(&wmi_list,
> + struct privacy_wmi_data,
> + list);
> + if (priv == NULL) {

`if (!priv)`

> + pr_err("dell privacy priv is NULL\n");
> + goto error;
> + }
> +
> + key = sparse_keymap_entry_from_scancode(priv->input_dev, (type << 16)|code);
> + if (!key) {
> + dev_dbg(&priv->wdev->dev, "Unknown key with type 0x%04x and code 0x%04x pressed\n",
> + type, code);
> + goto error;
> + }
> +
> + switch (code) {
> + case DELL_PRIVACY_TYPE_AUDIO: /* Mic mute */
> + priv->last_status = status;
> + sparse_keymap_report_entry(priv->input_dev, key, 1, true);
> + break;
> + case DELL_PRIVACY_TYPE_CAMERA: /* Camera mute */
> + priv->last_status = status;
> + sparse_keymap_report_entry(priv->input_dev, key, 1, true);
> + break;
> + default:
> + dev_dbg(&priv->wdev->dev, "unknown event type %u /%u",

A couple lines above hexadecimal format and capitalization is used.

> + type, code);
> + }
> +error:
> + mutex_unlock(&list_mutex);
> + return;
> +}
> +EXPORT_SYMBOL_GPL(dell_privacy_process_event);
> [...]
> +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, pos = 0;
> +
> + priv = devm_kzalloc(&wdev->dev, sizeof(struct privacy_wmi_data),
> + GFP_KERNEL);

`sizeof(*priv)`

> + if (!priv)
> + return -ENOMEM;
> +
> + /* create evdev passing interface */
> + priv->input_dev = devm_input_allocate_device(&wdev->dev);
> + if (!priv->input_dev)
> + return -ENOMEM;
> +
> + __set_bit(EV_KEY, priv->input_dev->evbit);
> + __set_bit(KEY_MICMUTE, priv->input_dev->keybit);
> + __set_bit(EV_MSC, priv->input_dev->evbit);
> + __set_bit(MSC_SCAN, priv->input_dev->mscbit);

`sparse_keymap_setup()` takes care of this.

> + keymap = kcalloc(
> + ARRAY_SIZE(dell_wmi_keymap_type_0012) +
> + 1,
> + sizeof(struct key_entry), GFP_KERNEL);
> + if (!keymap) {
> + ret = -ENOMEM;
> + goto err_free_dev;
> + }
> + for (i = 0; i < ARRAY_SIZE(dell_wmi_keymap_type_0012); i++) {
> + keymap[pos] = dell_wmi_keymap_type_0012[i];
> + keymap[pos].code |= (0x0012 << 16);
> + pos++;
> + }

I can't quite see why you need a copy of the entries. If the key codes are initialized
to the "correct" values, this can be avoided altogether.

> + 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;
> +
> + if (input_register_device(priv->input_dev)) {
> + pr_debug("input_register_device failed to register! \n");
> + return -ENODEV;

`keymap` is leaked here.

> + }
> +
> + priv->wdev = wdev;
> + dev_set_drvdata(&wdev->dev, priv);
> + mutex_lock(&list_mutex);
> + list_add_tail(&priv->list, &wmi_list);
> + privacy_valid = true;
> + if (get_current_status(wdev)) {
> + goto err_free_dev;

Mutex is not unlocked. And some steps are not undone.

> + }
> + mutex_unlock(&list_mutex);
> + kfree(keymap);
> + return 0;
> +
> +err_free_dev:
> + input_free_device(priv->input_dev);
> + kfree(keymap);
> + return ret;
> +}
> +
> +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);
> + privacy_valid = -ENODEV;
> + 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,
> +};
> +
> +static int __init init_dell_privacy(void)
> +{
> + int ret, wmi, acpi;

`int ret;` would've been enough. The preferred and prevalent style is:

```
int ret;

ret = step_1();
if (ret) {
pr_err(...);
goto undo_step_1;
}

ret = step_2();
if (ret) {
pr_err(...);
goto undo_step_2;
}

...

return 0;

undo_step_2:
...
undo_step_1:
....

return ret;
```

> +
> + wmi = wmi_driver_register(&dell_privacy_wmi_driver);
> + if (wmi) {
> + pr_debug("Failed to initialize privacy wmi driver: %d\n", wmi);
> + return wmi;
> + }
> +
> + acpi = privacy_acpi_init();
> + if (acpi) {
> + pr_debug("failed to initialize privacy wmi acpi driver: %d\n", acpi);
> + return acpi;
> + }
> +
> + return 0;
> +}

Even ignoring stylistic questions, the WMI driver is not unregistered if
`privacy_acpi_init()` fails, which is a bigger problem.

Even ignoring that, I'm not sure it's a good idea that a module that exports
symbols for others to use can fail to load.

> +
> +void exit_dell_privacy_wmi(void)
> +{
> + wmi_driver_unregister(&dell_privacy_wmi_driver);
> +}

At the moment I can't quite see the purpose of this function.

> +
> +static void __exit exit_dell_privacy(void)
> +{
> + privacy_acpi_cleanup();
> + exit_dell_privacy_wmi();
> +}
> +
> +module_init(init_dell_privacy);
> +module_exit(exit_dell_privacy);
> +
> +MODULE_DEVICE_TABLE(wmi, dell_wmi_privacy_wmi_id_table);

A couple lines above the `MODULE_DEVICE_TABLE` macro was invoked right after
the device table.

> +MODULE_AUTHOR("Perry Yuan <[email protected]>");
> +MODULE_DESCRIPTION("Dell Privacy WMI Driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/platform/x86/dell-privacy-wmi.h b/drivers/platform/x86/dell-privacy-wmi.h
> new file mode 100644
> index 000000000000..94af81d76e44
> --- /dev/null
> +++ b/drivers/platform/x86/dell-privacy-wmi.h
> @@ -0,0 +1,23 @@
> +/* 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_
> +#include <linux/wmi.h>

This include is not needed.

> +
> +bool dell_privacy_valid(void);
> +void dell_privacy_process_event(int type, int code, int status);
> +int privacy_acpi_init(void);
> +void privacy_acpi_cleanup(void);

These aren't prefixed by `dell_`?

> +
> +/* DELL Privacy Type */
> +enum {
> + DELL_PRIVACY_TYPE_UNKNOWN = 0x0,
> + DELL_PRIVACY_TYPE_AUDIO,
> + DELL_PRIVACY_TYPE_CAMERA,
> +};
> +#endif
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index bbdb3e860892..44bb74e4df86 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/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 <[email protected]>");
> MODULE_AUTHOR("Pali Rohár <[email protected]>");
> @@ -410,44 +411,57 @@ static void dell_wmi_notify(struct wmi_device *wdev,
> if (buffer_end > buffer_entry + buffer_entry[0] + 1)
> buffer_end = buffer_entry + buffer_entry[0] + 1;
>
> - while (buffer_entry < buffer_end) {
> -
> - len = buffer_entry[0];
> - if (len == 0)
> - break;
> -
> - len++;
> -
> - if (buffer_entry + len > buffer_end) {
> - pr_warn("Invalid length of WMI event\n");
> - break;
> - }
> -
> - pr_debug("Process buffer (%*ph)\n", len*2, buffer_entry);
> -
> - 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]);
> - /* Extended data is currently ignored */
> - break;
> - case 0x0010: /* Sequence of keys pressed */
> - case 0x0011: /* Sequence of events occurred */
> - for (i = 2; i < len; ++i)
> - dell_wmi_process_key(wdev, buffer_entry[1],
> - buffer_entry[i]);
> - break;
> - default: /* Unknown event */
> - pr_info("Unknown WMI event type 0x%x\n",
> - (int)buffer_entry[1]);
> - break;
> - }
> -
> - buffer_entry += len;
> -
> - }
> + while (buffer_entry < buffer_end) {
> +
> + len = buffer_entry[0];
> + if (len == 0)
> + break;
> +
> + len++;
> +
> + if (buffer_entry + len > buffer_end) {
> + pr_warn("Invalid length of WMI event\n");
> + break;
> + }
> +
> + pr_debug("Process buffer (%*ph)\n", len*2, buffer_entry);
> +
> + switch (buffer_entry[1]) {
> + case 0x0000: /* One key pressed or event occurred */
> + if (len > 2)
> + dell_wmi_process_key(wdev, buffer_entry[1],
> + buffer_entry[2]);
> + break;
> + case 0x0010: /* Sequence of keys pressed */
> + case 0x0011: /* Sequence of events occurred */
> + for (i = 2; i < len; ++i)
> + dell_wmi_process_key(wdev, buffer_entry[1],
> + buffer_entry[i]);
> + break;
> + case 0x0012:
> +#if IS_ENABLED(CONFIG_DELL_PRIVACY)
> + if (dell_privacy_valid()) {
> + 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]);
> + }
> +#else
> + /* Extended data is currently ignored */
> + if (len > 2)
> + dell_wmi_process_key(wdev, buffer_entry[1], buffer_entry[2]);
> +#endif

Wouldn't it be better if the header file provided a static inline definitions
for `dell_privacy_valid()` and `dell_privacy_process_event()` - if CONFIG_DELL_PRIVACY
is not enabled - that return false and do nothing, respectively? The same way
it's done in dell-smbios.h.

> + break;
> + default: /* Unknown event */
> + pr_info("Unknown WMI event type 0x%x\n",
> + (int)buffer_entry[1]);
> + break;
> + }
> +
> + buffer_entry += len;
> +
> + }
>
> }
> [...]

Regards,
Barnabás Pőcze

2020-11-04 02:09:52

by Matthew Garrett

[permalink] [raw]

Subject: Re: [PATCH] platform/x86: dell-privacy: Add support for new privacy driver

On Tue, Nov 03, 2020 at 04:55:42AM -0800, Perry Yuan wrote:

> +#define PRIVACY_PlATFORM_NAME "dell-privacy-acpi"
> +#define ACPI_PRIVACY_DEVICE "\\_SB.PC00.LPCB.ECDV"

This looks like the EC rather than a privacy device? If so, you probably
want to collaborate with the EC driver to obtain the handle rather than
depending on the path, unless it's guaranteed that this path will never
change.

> +static int micmute_led_set(struct led_classdev *led_cdev,
> + enum led_brightness brightness)
> +{
> + acpi_status status;
> +
> + status = acpi_evaluate_object(NULL, ACPI_PRIVACY_EC_ACK, NULL, NULL);
> + if (ACPI_FAILURE(status)) {
> + dev_err(led_cdev->dev, "Error setting privacy audio EC ack value: %d\n",status);
> + return -EIO;
> + }
> + return 0;
> +}

What's actually being set here? You don't seem to be passing any
arguments.

> +static const struct acpi_device_id privacy_acpi_device_ids[] = {
> + {"PNP0C09", 0},

Oooh no please don't do this - you'll trigger autoloading on everything
that exposes a PNP0C09 device.

--
Matthew Garrett | [email protected]

2020-11-04 06:46:18

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH] platform/x86: dell-privacy: Add support for new privacy driver

Hi Perry,

Thank you for the
[auto build test [also build test [cannot apply to next-20201103]
[If your patch is And when submitting WARNING on linus/master]
WARNING on v5.10-rc2]
applied to the wrong git tree, kindly drop us a note.
patch, we suggest to use '--base' as documented in
com/docs/git-format-patch]">https://git-scm.com/docs/git-format-patch]
github.com/0day-ci/linux/commits/Perry-Yuan/platform-x86-dell-privacy-Add-support-for-new-privacy-driver/20201103-205721">https://github.com/0day-ci/linux/commits/Perry-Yuan/platform-x86-dell-privacy-Add-support-for-new-privacy-driver/20201103-205721
git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git">https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git b7cbaf59f62f8ab8f157698f9e31642bff525bd0
(attached as .config)
9.3.0-15) 9.3.0
kindly add following tag as appropriate
test robot <[email protected]>
dell_wmi_notify() warn: inconsistent indenting
dell_init() warn: inconsistent indenting
dell_exit() warn: inconsistent indenting
dell_exit() warn: curly braces intended?
x86/dell-wmi.c
2014-11-11 377
Lutomirski 2015-11-25 378 static void dell_wmi_notify(struct wmi_device *wdev,
Lutomirski 2015-11-25 379 union acpi_object *obj)
Garrett 2009-01-09 380 {
Lutomirski 2017-08-01 381 struct dell_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
2014-11-11 382 u16 *buffer_entry, *buffer_end;
Lutomirski 2015-11-25 383 acpi_size buffer_size;
2014-11-11 384 int len, i;
Garrett 2009-01-09 385
2014-11-11 386 if (obj->type != ACPI_TYPE_BUFFER) {
2014-11-11 387 pr_warn("bad response type %x\n", obj->type);
2009-11-02 388 return;
2009-11-02 389 }
2009-11-02 390
2014-11-11 391 pr_debug("Received WMI event (%*ph)\n",
2014-11-11 392 obj->buffer.length, obj->buffer.pointer);
2014-11-11 393
2014-11-11 394 buffer_entry = (u16 *)obj->buffer.pointer;
2014-11-11 395 buffer_size = obj->buffer.length/2;
2014-11-11 396 buffer_end = buffer_entry + buffer_size;
2014-11-11 397
2016-01-04 398 /*
2016-01-04 399 * BIOS/ACPI on devices with WMI interface version 0 does not clear
2016-01-04 400 * buffer before filling it. So next time when BIOS/ACPI send WMI event
2016-01-04 401 * which is smaller as previous then it contains garbage in buffer from
2016-01-04 402 * previous event.
2016-01-04 403 *
2016-01-04 404 * BIOS/ACPI on devices with WMI interface version 1 clears buffer and
2016-01-04 405 * sometimes send more events in buffer at one call.
2016-01-04 406 *
2016-01-04 407 * So to prevent reading garbage from buffer we will process only first
2016-01-04 408 * one event on devices with WMI interface version 0.
2016-01-04 409 */
Lutomirski 2017-08-01 410 if (priv->interface_version == 0 && buffer_entry < buffer_end)
2016-01-04 411 if (buffer_end > buffer_entry + buffer_entry[0] + 1)
2016-01-04 412 buffer_end = buffer_entry + buffer_entry[0] + 1;
2016-01-04 413
2014-11-11 @414 while (buffer_entry < buffer_end) {
2014-11-11 415
2014-11-11 416 len = buffer_entry[0];
2014-11-11 417 if (len == 0)
2014-11-11 418 break;
2014-11-11 419
2014-11-11 420 len++;
2014-11-11 421
2014-11-11 422 if (buffer_entry + len > buffer_end) {
2014-11-11 423 pr_warn("Invalid length of WMI event\n");
2014-11-11 424 break;
Garrett 2009-01-09 425 }
2014-11-11 426
2014-11-11 427 pr_debug("Process buffer (%*ph)\n", len*2, buffer_entry);
2014-11-11 428
2014-11-11 429 switch (buffer_entry[1]) {
2016-06-15 430 case 0x0000: /* One key pressed or event occurred */
2016-06-15 431 if (len > 2)
2020-06-10 432 dell_wmi_process_key(wdev, buffer_entry[1],
Lutomirski 2015-11-25 433 buffer_entry[2]);
2014-11-11 434 break;
2016-06-15 435 case 0x0010: /* Sequence of keys pressed */
2016-06-15 436 case 0x0011: /* Sequence of events occurred */
2014-11-11 437 for (i = 2; i < len; ++i)
Lutomirski 2015-11-25 438 dell_wmi_process_key(wdev, buffer_entry[1],
2016-06-15 439 buffer_entry[i]);
2014-11-11 440 break;
2020-11-03 441 case 0x0012:
2020-11-03 442 #if IS_ENABLED(CONFIG_DELL_PRIVACY)
2020-11-03 443 if (dell_privacy_valid()) {
2020-11-03 444 dell_privacy_process_event(buffer_entry[1], buffer_entry[3],
2020-11-03 445 buffer_entry[4]);
2020-11-03 446 } else {
2020-11-03 447 if (len > 2)
2020-11-03 448 dell_wmi_process_key(wdev, buffer_entry[1], buffer_entry[2]);
2020-11-03 449 }
2020-11-03 450 #else
2020-11-03 451 /* Extended data is currently ignored */
2020-11-03 452 if (len > 2)
2020-11-03 453 dell_wmi_process_key(wdev, buffer_entry[1], buffer_entry[2]);
2020-11-03 454 #endif
2020-11-03 455 break;
2016-06-15 456 default: /* Unknown event */
2014-11-11 457 pr_info("Unknown WMI event type 0x%x\n",
2014-11-11 458 (int)buffer_entry[1]);
2014-11-11 459 break;
Garrett 2009-01-09 460 }
2014-11-11 461
2014-11-11 462 buffer_entry += len;
2014-11-11 463
2014-11-11 464 }
2014-11-11 465
Service, Intel Corporation
.org/hyperkitty/list/kbuild-all@lists.01.org">https://lists.01.org/hyperkitty/list/[email protected]

Attachments:

(No filename) (8.51 kB)
.config.gz (29.99 kB)
Download all attachments

2020-11-09 11:18:33

by Hans de Goede

[permalink] [raw]

Subject: Re: [PATCH] platform/x86: dell-privacy: Add support for new privacy driver

Hi,

On 11/3/20 1:55 PM, Perry Yuan wrote:
> From: perry_yuan <[email protected]>
>
> 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 enabled, any applications will not get any audio or
> video stream.
> when user pressed ctrl+F4 hotkey, audio privacy mode will be enabled
> and camera mute hotkey is ctrl+F9.
>
> Signed-off-by: Perry Yuan <[email protected]>
> Signed-off-by: Limonciello Mario <[email protected]>

Perry, you have had multiple comments and kernel-test-robot
reports about this patch. Please prepare a new version addressing
these.

Once you have send out a new version I will try to make some time
to do a full review soon(ish).

Regards,

Hans

> ---
> drivers/platform/x86/Kconfig | 12 ++
> drivers/platform/x86/Makefile | 4 +-
> drivers/platform/x86/dell-laptop.c | 41 ++--
> drivers/platform/x86/dell-privacy-acpi.c | 139 ++++++++++++
> drivers/platform/x86/dell-privacy-wmi.c | 259 +++++++++++++++++++++++
> drivers/platform/x86/dell-privacy-wmi.h | 23 ++
> drivers/platform/x86/dell-wmi.c | 90 ++++----
> 7 files changed, 513 insertions(+), 55 deletions(-)
> create mode 100644 drivers/platform/x86/dell-privacy-acpi.c
> create mode 100644 drivers/platform/x86/dell-privacy-wmi.c
> create mode 100644 drivers/platform/x86/dell-privacy-wmi.h
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 40219bba6801..0cb6bf5a9565 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -454,6 +454,18 @@ config DELL_WMI_LED
> This adds support for the Latitude 2100 and similar
> notebooks that have an external LED.
>
> +config DELL_PRIVACY
> + tristate "Dell Hardware Privacy Support"
> + depends on ACPI
> + depends on ACPI_WMI
> + depends on INPUT
> + depends on DELL_LAPTOP
> + select DELL_WMI
> + help
> + This driver provides a driver to support messaging related to the
> + privacy button presses on applicable Dell laptops from 2021 and
> + newer.
> +
> config AMILO_RFKILL
> tristate "Fujitsu-Siemens Amilo rfkill support"
> depends on RFKILL
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 5f823f7eff45..111f7215db2f 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -47,7 +47,9 @@ obj-$(CONFIG_DELL_WMI) += dell-wmi.o
> obj-$(CONFIG_DELL_WMI_DESCRIPTOR) += dell-wmi-descriptor.o
> obj-$(CONFIG_DELL_WMI_AIO) += dell-wmi-aio.o
> obj-$(CONFIG_DELL_WMI_LED) += dell-wmi-led.o
> -
> +obj-$(CONFIG_DELL_PRIVACY) += dell-privacy.o
> +dell-privacy-objs := dell-privacy-wmi.o \
> + dell-privacy-acpi.o
> # Fujitsu
> obj-$(CONFIG_AMILO_RFKILL) += amilo-rfkill.o
> obj-$(CONFIG_FUJITSU_LAPTOP) += fujitsu-laptop.o
> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> index 5e9c2296931c..12b91de09356 100644
> --- a/drivers/platform/x86/dell-laptop.c
> +++ b/drivers/platform/x86/dell-laptop.c
> @@ -30,6 +30,7 @@
> #include <acpi/video.h>
> #include "dell-rbtn.h"
> #include "dell-smbios.h"
> +#include "dell-privacy-wmi.h"
>
> struct quirk_entry {
> bool touchpad_led;
> @@ -90,6 +91,7 @@ static struct rfkill *wifi_rfkill;
> static struct rfkill *bluetooth_rfkill;
> static struct rfkill *wwan_rfkill;
> static bool force_rfkill;
> +static bool privacy_valid;
>
> module_param(force_rfkill, bool, 0444);
> MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted models");
> @@ -2202,20 +2204,25 @@ static int __init dell_init(void)
> debugfs_create_file("rfkill", 0444, dell_laptop_dir, NULL,
> &dell_debugfs_fops);
>
> - dell_laptop_register_notifier(&dell_laptop_notifier);
> -
> - 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 (acpi_video_get_backlight_type() != acpi_backlight_vendor)
> - return 0;
> -
> - token = dell_smbios_find_token(BRIGHTNESS_TOKEN);
> + dell_laptop_register_notifier(&dell_laptop_notifier);
> +
> + if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
> + dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) {
> +#if IS_ENABLED(CONFIG_DELL_PRIVACY)
> + privacy_valid = dell_privacy_valid() == -ENODEV;
> +#endif
> + if (!privacy_valid) {
> + 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)
> + return 0;
> +
> + token = dell_smbios_find_token(BRIGHTNESS_TOKEN);
> if (token) {
> struct calling_interface_buffer buffer;
>
> @@ -2257,7 +2264,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 (!privacy_valid)
> + led_classdev_unregister(&micmute_led_cdev);
> fail_led:
> dell_cleanup_rfkill();
> fail_rfkill:
> @@ -2278,7 +2286,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 (!privacy_valid)
> + led_classdev_unregister(&micmute_led_cdev);
> dell_cleanup_rfkill();
> if (platform_device) {
> platform_device_unregister(platform_device);
> diff --git a/drivers/platform/x86/dell-privacy-acpi.c b/drivers/platform/x86/dell-privacy-acpi.c
> new file mode 100644
> index 000000000000..516cd99167c3
> --- /dev/null
> +++ b/drivers/platform/x86/dell-privacy-acpi.c
> @@ -0,0 +1,139 @@
> +// 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/device.h>
> +#include <linux/fs.h>
> +#include <linux/kernel.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/string.h>
> +#include <linux/sysfs.h>
> +#include <linux/types.h>
> +#include <linux/wmi.h>
> +#include <linux/slab.h>
> +#include <linux/platform_device.h>
> +#include "dell-privacy-wmi.h"
> +
> +#define PRIVACY_PlATFORM_NAME "dell-privacy-acpi"
> +#define ACPI_PRIVACY_DEVICE "\\_SB.PC00.LPCB.ECDV"
> +#define ACPI_PRIVACY_EC_ACK ACPI_PRIVACY_DEVICE ".ECAK"
> +
> +static struct platform_device *privacy_acpi_pdev;
> +
> +struct privacy_acpi_priv {
> + struct device *dev;
> + struct acpi_device *acpi_dev;
> + struct input_dev *input_dev;
> + struct platform_device *platform_device;
> +};
> +
> +static int micmute_led_set(struct led_classdev *led_cdev,
> + enum led_brightness brightness)
> +{
> + acpi_status status;
> +
> + status = acpi_evaluate_object(NULL, ACPI_PRIVACY_EC_ACK, NULL, NULL);
> + if (ACPI_FAILURE(status)) {
> + dev_err(led_cdev->dev, "Error setting privacy audio EC ack value: %d\n",status);
> + return -EIO;
> + }
> + return 0;
> +}
> +
> +static struct led_classdev micmute_led_cdev = {
> + .name = "platform::micmute",
> + .max_brightness = 1,
> + .brightness_set_blocking = micmute_led_set,
> + .default_trigger = "audio-micmute",
> +};
> +
> +static int privacy_acpi_remove(struct platform_device *pdev)
> +{
> + dev_set_drvdata(&pdev->dev, NULL);
> + return 0;
> +}
> +
> +static int privacy_acpi_probe(struct platform_device *pdev)
> +{
> + struct privacy_acpi_priv *privacy;
> + acpi_handle handle;
> + acpi_status status;
> + int err;
> +
> + privacy = kzalloc(sizeof(struct privacy_acpi_priv), GFP_KERNEL);
> + if (!privacy)
> + return -ENOMEM;
> +
> + privacy->dev = &pdev->dev;
> + privacy->platform_device = pdev;
> + platform_set_drvdata(pdev, privacy);
> + /* Look for software micmute complete notification device */
> + status = acpi_get_handle(ACPI_ROOT_OBJECT,
> + ACPI_PRIVACY_DEVICE,
> + &handle);
> + if (ACPI_FAILURE(status)) {
> + dev_err(privacy->dev, "Unable to find Dell`s EC device %s: %d\n",
> + ACPI_PRIVACY_DEVICE, status);
> + return -ENXIO;
> + }
> +
> + micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
> + err = led_classdev_register(&privacy_acpi_pdev->dev, &micmute_led_cdev);
> + if (err < 0)
> + return -EIO;
> +
> + return 0;
> +}
> +
> +static const struct acpi_device_id privacy_acpi_device_ids[] = {
> + {"PNP0C09", 0},
> + {"", 0},
> +};
> +MODULE_DEVICE_TABLE(acpi, privacy_acpi_device_ids);
> +
> +static struct platform_driver privacy_platform_driver = {
> + .driver = {
> + .name = PRIVACY_PlATFORM_NAME,
> + .acpi_match_table = ACPI_PTR(privacy_acpi_device_ids),
> + },
> + .probe = privacy_acpi_probe,
> + .remove = privacy_acpi_remove,
> +};
> +
> +int privacy_acpi_init(void)
> +{
> + int err;
> +
> + err = platform_driver_register(&privacy_platform_driver);
> + if (err)
> + return err;
> +
> + privacy_acpi_pdev = platform_device_register_simple(
> + PRIVACY_PlATFORM_NAME, -1, NULL, 0);
> + if (IS_ERR(privacy_acpi_pdev)) {
> + err = PTR_ERR(privacy_acpi_pdev);
> + goto err_platform;
> + }
> + return 0;
> +
> +err_platform:
> + platform_driver_unregister(&privacy_platform_driver);
> + return err;
> +}
> +
> +void privacy_acpi_cleanup(void)
> +{
> + platform_driver_unregister(&privacy_platform_driver);
> +}
> +
> +MODULE_AUTHOR("Perry Yuan <[email protected]>");
> +MODULE_DESCRIPTION("DELL Privacy ACPI Driver");
> +MODULE_LICENSE("GPL");
> +
> diff --git a/drivers/platform/x86/dell-privacy-wmi.c b/drivers/platform/x86/dell-privacy-wmi.c
> new file mode 100644
> index 000000000000..6c36b7ec44c6
> --- /dev/null
> +++ b/drivers/platform/x86/dell-privacy-wmi.c
> @@ -0,0 +1,259 @@
> +// 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/input.h>
> +#include <linux/input/sparse-keymap.h>
> +#include <linux/list.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 PRIVACY_SCREEN_STATUS BIT(2)
> +
> +static int privacy_valid = -EPROBE_DEFER;
> +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;
> + u32 features_present;
> + u32 last_status;
> +};
> +
> +/*
> + * 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 } },
> +};
> +
> +bool dell_privacy_valid(void)
> +{
> + bool ret;
> +
> + mutex_lock(&list_mutex);
> + ret = wmi_has_guid(DELL_PRIVACY_GUID);
> + if (!ret){
> + return -ENODEV;
> + }
> + ret = privacy_valid;
> + mutex_unlock(&list_mutex);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(dell_privacy_valid);
> +
> +void dell_privacy_process_event(int type, int code, int status)
> +{
> + struct privacy_wmi_data *priv;
> + const struct key_entry *key;
> +
> + mutex_lock(&list_mutex);
> + priv = list_first_entry_or_null(&wmi_list,
> + struct privacy_wmi_data,
> + list);
> + if (priv == NULL) {
> + pr_err("dell privacy priv is NULL\n");
> + goto error;
> + }
> +
> + key = sparse_keymap_entry_from_scancode(priv->input_dev, (type << 16)|code);
> + if (!key) {
> + dev_dbg(&priv->wdev->dev, "Unknown key with type 0x%04x and code 0x%04x pressed\n",
> + type, code);
> + goto error;
> + }
> +
> + switch (code) {
> + case DELL_PRIVACY_TYPE_AUDIO: /* Mic mute */
> + priv->last_status = status;
> + sparse_keymap_report_entry(priv->input_dev, key, 1, true);
> + break;
> + case DELL_PRIVACY_TYPE_CAMERA: /* Camera mute */
> + priv->last_status = status;
> + sparse_keymap_report_entry(priv->input_dev, key, 1, true);
> + break;
> + default:
> + dev_dbg(&priv->wdev->dev, "unknown event type %u /%u",
> + type, code);
> + }
> +error:
> + mutex_unlock(&list_mutex);
> + return;
> +}
> +EXPORT_SYMBOL_GPL(dell_privacy_process_event);
> +
> +static int get_current_status(struct wmi_device *wdev)
> +{
> + struct privacy_wmi_data *priv = dev_get_drvdata(&wdev->dev);
> + union acpi_object *obj_present;
> + union acpi_object *obj_current;
> + int ret = 0;
> +
> + if (priv == NULL) {
> + pr_err("dell privacy priv is NULL\n");
> + return -EINVAL;
> + }
> + /* get devices supported */
> + obj_present = wmidev_block_query(wdev, 0);
> + if (obj_present->type != ACPI_TYPE_INTEGER) {
> + ret = -EIO;
> + goto present_free;
> + }
> + priv->features_present = obj_present->integer.value;
> +
> + /* get current state */
> + obj_current = wmidev_block_query(wdev, 1);
> + if (obj_current->type != ACPI_TYPE_INTEGER) {
> + ret = -EIO;
> + goto current_free;
> + }
> + priv->last_status = obj_current->integer.value;
> +current_free:
> + kfree(obj_current);
> +present_free:
> + kfree(obj_present);
> + return ret;
> +}
> +
> +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, pos = 0;
> +
> + priv = devm_kzalloc(&wdev->dev, sizeof(struct privacy_wmi_data),
> + GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + /* create evdev passing interface */
> + priv->input_dev = devm_input_allocate_device(&wdev->dev);
> + if (!priv->input_dev)
> + return -ENOMEM;
> +
> + __set_bit(EV_KEY, priv->input_dev->evbit);
> + __set_bit(KEY_MICMUTE, priv->input_dev->keybit);
> + __set_bit(EV_MSC, priv->input_dev->evbit);
> + __set_bit(MSC_SCAN, priv->input_dev->mscbit);
> + keymap = kcalloc(
> + ARRAY_SIZE(dell_wmi_keymap_type_0012) +
> + 1,
> + sizeof(struct key_entry), GFP_KERNEL);
> + if (!keymap) {
> + ret = -ENOMEM;
> + goto err_free_dev;
> + }
> + for (i = 0; i < ARRAY_SIZE(dell_wmi_keymap_type_0012); i++) {
> + keymap[pos] = dell_wmi_keymap_type_0012[i];
> + keymap[pos].code |= (0x0012 << 16);
> + pos++;
> + }
> + 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;
> +
> + if (input_register_device(priv->input_dev)) {
> + pr_debug("input_register_device failed to register! \n");
> + return -ENODEV;
> + }
> +
> + priv->wdev = wdev;
> + dev_set_drvdata(&wdev->dev, priv);
> + mutex_lock(&list_mutex);
> + list_add_tail(&priv->list, &wmi_list);
> + privacy_valid = true;
> + if (get_current_status(wdev)) {
> + goto err_free_dev;
> + }
> + mutex_unlock(&list_mutex);
> + kfree(keymap);
> + return 0;
> +
> +err_free_dev:
> + input_free_device(priv->input_dev);
> + kfree(keymap);
> + return ret;
> +}
> +
> +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);
> + privacy_valid = -ENODEV;
> + 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,
> +};
> +
> +static int __init init_dell_privacy(void)
> +{
> + int ret, wmi, acpi;
> +
> + wmi = wmi_driver_register(&dell_privacy_wmi_driver);
> + if (wmi) {
> + pr_debug("Failed to initialize privacy wmi driver: %d\n", wmi);
> + return wmi;
> + }
> +
> + acpi = privacy_acpi_init();
> + if (acpi) {
> + pr_debug("failed to initialize privacy wmi acpi driver: %d\n", acpi);
> + return acpi;
> + }
> +
> + return 0;
> +}
> +
> +void exit_dell_privacy_wmi(void)
> +{
> + wmi_driver_unregister(&dell_privacy_wmi_driver);
> +}
> +
> +static void __exit exit_dell_privacy(void)
> +{
> + privacy_acpi_cleanup();
> + exit_dell_privacy_wmi();
> +}
> +
> +module_init(init_dell_privacy);
> +module_exit(exit_dell_privacy);
> +
> +MODULE_DEVICE_TABLE(wmi, dell_wmi_privacy_wmi_id_table);
> +MODULE_AUTHOR("Perry Yuan <[email protected]>");
> +MODULE_DESCRIPTION("Dell Privacy WMI Driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/platform/x86/dell-privacy-wmi.h b/drivers/platform/x86/dell-privacy-wmi.h
> new file mode 100644
> index 000000000000..94af81d76e44
> --- /dev/null
> +++ b/drivers/platform/x86/dell-privacy-wmi.h
> @@ -0,0 +1,23 @@
> +/* 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_
> +#include <linux/wmi.h>
> +
> +bool dell_privacy_valid(void);
> +void dell_privacy_process_event(int type, int code, int status);
> +int privacy_acpi_init(void);
> +void privacy_acpi_cleanup(void);
> +
> +/* DELL Privacy Type */
> +enum {
> + DELL_PRIVACY_TYPE_UNKNOWN = 0x0,
> + DELL_PRIVACY_TYPE_AUDIO,
> + DELL_PRIVACY_TYPE_CAMERA,
> +};
> +#endif
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index bbdb3e860892..44bb74e4df86 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/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 <[email protected]>");
> MODULE_AUTHOR("Pali Rohár <[email protected]>");
> @@ -410,44 +411,57 @@ static void dell_wmi_notify(struct wmi_device *wdev,
> if (buffer_end > buffer_entry + buffer_entry[0] + 1)
> buffer_end = buffer_entry + buffer_entry[0] + 1;
>
> - while (buffer_entry < buffer_end) {
> -
> - len = buffer_entry[0];
> - if (len == 0)
> - break;
> -
> - len++;
> -
> - if (buffer_entry + len > buffer_end) {
> - pr_warn("Invalid length of WMI event\n");
> - break;
> - }
> -
> - pr_debug("Process buffer (%*ph)\n", len*2, buffer_entry);
> -
> - 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]);
> - /* Extended data is currently ignored */
> - break;
> - case 0x0010: /* Sequence of keys pressed */
> - case 0x0011: /* Sequence of events occurred */
> - for (i = 2; i < len; ++i)
> - dell_wmi_process_key(wdev, buffer_entry[1],
> - buffer_entry[i]);
> - break;
> - default: /* Unknown event */
> - pr_info("Unknown WMI event type 0x%x\n",
> - (int)buffer_entry[1]);
> - break;
> - }
> -
> - buffer_entry += len;
> -
> - }
> + while (buffer_entry < buffer_end) {
> +
> + len = buffer_entry[0];
> + if (len == 0)
> + break;
> +
> + len++;
> +
> + if (buffer_entry + len > buffer_end) {
> + pr_warn("Invalid length of WMI event\n");
> + break;
> + }
> +
> + pr_debug("Process buffer (%*ph)\n", len*2, buffer_entry);
> +
> + switch (buffer_entry[1]) {
> + case 0x0000: /* One key pressed or event occurred */
> + if (len > 2)
> + dell_wmi_process_key(wdev, buffer_entry[1],
> + buffer_entry[2]);
> + break;
> + case 0x0010: /* Sequence of keys pressed */
> + case 0x0011: /* Sequence of events occurred */
> + for (i = 2; i < len; ++i)
> + dell_wmi_process_key(wdev, buffer_entry[1],
> + buffer_entry[i]);
> + break;
> + case 0x0012:
> +#if IS_ENABLED(CONFIG_DELL_PRIVACY)
> + if (dell_privacy_valid()) {
> + 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]);
> + }
> +#else
> + /* Extended data is currently ignored */
> + if (len > 2)
> + dell_wmi_process_key(wdev, buffer_entry[1], buffer_entry[2]);
> +#endif
> + break;
> + default: /* Unknown event */
> + pr_info("Unknown WMI event type 0x%x\n",
> + (int)buffer_entry[1]);
> + break;
> + }
> +
> + buffer_entry += len;
> +
> + }
>
> }
>
>

2020-11-11 07:24:01

by Yuan, Perry

[permalink] [raw]

Subject: RE: [PATCH] platform/x86: dell-privacy: Add support for new privacy driver

> -----Original Message-----
> From: Matthew Garrett <[email protected]>
> Sent: Wednesday, November 4, 2020 9:49 AM
> To: Yuan, Perry
> Cc: [email protected]; [email protected]; [email protected]; linux-
> [email protected]; [email protected]; Limonciello,
> Mario
> Subject: Re: [PATCH] platform/x86: dell-privacy: Add support for new privacy
> driver
>
>
> [EXTERNAL EMAIL]
>
> On Tue, Nov 03, 2020 at 04:55:42AM -0800, Perry Yuan wrote:
>
> > +#define PRIVACY_PlATFORM_NAME "dell-privacy-acpi"
> > +#define ACPI_PRIVACY_DEVICE "\\_SB.PC00.LPCB.ECDV"
>
> This looks like the EC rather than a privacy device? If so, you probably want
> to collaborate with the EC driver to obtain the handle rather than depending
> on the path, unless it's guaranteed that this path will never change.

Thanks Matthew
I will change the path to handle as you suggested.

>
> > +static int micmute_led_set(struct led_classdev *led_cdev,
> > + enum led_brightness brightness) {
> > + acpi_status status;
> > +
> > + status = acpi_evaluate_object(NULL, ACPI_PRIVACY_EC_ACK, NULL,
> NULL);
> > + if (ACPI_FAILURE(status)) {
> > + dev_err(led_cdev->dev, "Error setting privacy audio EC ack
> value: %d\n",status);
> > + return -EIO;
> > + }
> > + return 0;
> > +}
>
> What's actually being set here? You don't seem to be passing any arguments.

Yes, it is a EC ack notification without any arguments needed.

>
> > +static const struct acpi_device_id privacy_acpi_device_ids[] = {
> > + {"PNP0C09", 0},
>
> Oooh no please don't do this - you'll trigger autoloading on everything that
> exposes a PNP0C09 device.

Got it , I need to adjust the driver register logic.
In drivers/platform/x86/dell-privacy-wmi.c .
The privacy acpi driver will be loaded by privacy wmi driver.
The privacy wmi driver need to check if the privacy device is present.
It can avoid loading driver on non-dell-privacy system.

+static const struct wmi_device_id dell_wmi_privacy_wmi_id_table[] = {
+ { .guid_string = DELL_PRIVACY_GUID },
+ { },

>
> --
> Matthew Garrett | [email protected]

2020-11-11 07:27:22

by Matthew Garrett

[permalink] [raw]

Subject: Re: [PATCH] platform/x86: dell-privacy: Add support for new privacy driver

On Wed, Nov 11, 2020 at 07:21:07AM +0000, Yuan, Perry wrote:
> > > + status = acpi_evaluate_object(NULL, ACPI_PRIVACY_EC_ACK, NULL,
> > NULL);
> > > + if (ACPI_FAILURE(status)) {
> > > + dev_err(led_cdev->dev, "Error setting privacy audio EC ack
> > value: %d\n",status);
> > > + return -EIO;
> > > + }
> > > + return 0;
> > > +}
> >
> > What's actually being set here? You don't seem to be passing any arguments.
>
> Yes, it is a EC ack notification without any arguments needed.

I'm confused why it's being exposed as an LED device in that case -
there's an expectation that this is something that actually controls a
real LED, which means responding to state. Are you able to share the
acpidump of a machine with this device?

--
Matthew Garrett | [email protected]

2020-11-11 07:28:29

by Yuan, Perry

[permalink] [raw]

Subject: RE: [PATCH] platform/x86: dell-privacy: Add support for new privacy driver

> -----Original Message-----
> From: Hans de Goede <[email protected]>
> Sent: Monday, November 9, 2020 7:16 PM
> To: Yuan, Perry; [email protected]; [email protected];
> [email protected]
> Cc: [email protected]; [email protected];
> Limonciello, Mario
> Subject: Re: [PATCH] platform/x86: dell-privacy: Add support for new privacy
> driver
>
>
> [EXTERNAL EMAIL]
>
> Hi,
>
> On 11/3/20 1:55 PM, Perry Yuan wrote:
> > From: perry_yuan <[email protected]>
> >
> > 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
> > enabled, any applications will not get any audio or video stream.
> > when user pressed ctrl+F4 hotkey, audio privacy mode will be enabled
> > and camera mute hotkey is ctrl+F9.
> >
> > Signed-off-by: Perry Yuan <[email protected]>
> > Signed-off-by: Limonciello Mario <[email protected]>
>
> Perry, you have had multiple comments and kernel-test-robot reports about
> this patch. Please prepare a new version addressing these.
>
> Once you have send out a new version I will try to make some time to do a
> full review soon(ish).
>
> Regards,
>
> Hans

Hi Hans:
I got some review feedback from Barnabás and Matthew,Mario.
I am working on another new version and will submit v2 patch soon for your review.
Thank you in advance.

Perry

>
>
> > ---
> > drivers/platform/x86/Kconfig | 12 ++
> > drivers/platform/x86/Makefile | 4 +-
> > drivers/platform/x86/dell-laptop.c | 41 ++--
> > drivers/platform/x86/dell-privacy-acpi.c | 139 ++++++++++++
> > drivers/platform/x86/dell-privacy-wmi.c | 259 +++++++++++++++++++++++
> > drivers/platform/x86/dell-privacy-wmi.h | 23 ++
> > drivers/platform/x86/dell-wmi.c | 90 ++++----
> > 7 files changed, 513 insertions(+), 55 deletions(-) create mode
> > 100644 drivers/platform/x86/dell-privacy-acpi.c
> > create mode 100644 drivers/platform/x86/dell-privacy-wmi.c
> > create mode 100644 drivers/platform/x86/dell-privacy-wmi.h
> >
> > diff --git a/drivers/platform/x86/Kconfig
> > b/drivers/platform/x86/Kconfig index 40219bba6801..0cb6bf5a9565
> 100644
> > --- a/drivers/platform/x86/Kconfig
> > +++ b/drivers/platform/x86/Kconfig
> > @@ -454,6 +454,18 @@ config DELL_WMI_LED
> > This adds support for the Latitude 2100 and similar
> > notebooks that have an external LED.
> >
> > +config DELL_PRIVACY
> > + tristate "Dell Hardware Privacy Support"
> > + depends on ACPI
> > + depends on ACPI_WMI
> > + depends on INPUT
> > + depends on DELL_LAPTOP
> > + select DELL_WMI
> > + help
> > + This driver provides a driver to support messaging related to the
> > + privacy button presses on applicable Dell laptops from 2021 and
> > + newer.
> > +
> > config AMILO_RFKILL
> > tristate "Fujitsu-Siemens Amilo rfkill support"
> > depends on RFKILL
> > diff --git a/drivers/platform/x86/Makefile
> > b/drivers/platform/x86/Makefile index 5f823f7eff45..111f7215db2f
> > 100644
> > --- a/drivers/platform/x86/Makefile
> > +++ b/drivers/platform/x86/Makefile
> > @@ -47,7 +47,9 @@ obj-$(CONFIG_DELL_WMI) +=
> dell-wmi.o
> > obj-$(CONFIG_DELL_WMI_DESCRIPTOR) += dell-wmi-descriptor.o
> > obj-$(CONFIG_DELL_WMI_AIO) += dell-wmi-aio.o
> > obj-$(CONFIG_DELL_WMI_LED) += dell-wmi-led.o
> > -
> > +obj-$(CONFIG_DELL_PRIVACY) += dell-privacy.o
> > +dell-privacy-objs := dell-privacy-wmi.o \
> > + dell-privacy-acpi.o
> > # Fujitsu
> > obj-$(CONFIG_AMILO_RFKILL) += amilo-rfkill.o
> > obj-$(CONFIG_FUJITSU_LAPTOP) += fujitsu-laptop.o
> > diff --git a/drivers/platform/x86/dell-laptop.c
> > b/drivers/platform/x86/dell-laptop.c
> > index 5e9c2296931c..12b91de09356 100644
> > --- a/drivers/platform/x86/dell-laptop.c
> > +++ b/drivers/platform/x86/dell-laptop.c
> > @@ -30,6 +30,7 @@
> > #include <acpi/video.h>
> > #include "dell-rbtn.h"
> > #include "dell-smbios.h"
> > +#include "dell-privacy-wmi.h"
> >
> > struct quirk_entry {
> > bool touchpad_led;
> > @@ -90,6 +91,7 @@ static struct rfkill *wifi_rfkill; static struct
> > rfkill *bluetooth_rfkill; static struct rfkill *wwan_rfkill; static
> > bool force_rfkill;
> > +static bool privacy_valid;
> >
> > module_param(force_rfkill, bool, 0444);
> > MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted
> > models"); @@ -2202,20 +2204,25 @@ static int __init dell_init(void)
> > debugfs_create_file("rfkill", 0444, dell_laptop_dir, NULL,
> > &dell_debugfs_fops);
> >
> > - dell_laptop_register_notifier(&dell_laptop_notifier);
> > -
> > - 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 (acpi_video_get_backlight_type() != acpi_backlight_vendor)
> > - return 0;
> > -
> > - token = dell_smbios_find_token(BRIGHTNESS_TOKEN);
> > + dell_laptop_register_notifier(&dell_laptop_notifier);
> > +
> > + if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
> > + dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) { #if
> > +IS_ENABLED(CONFIG_DELL_PRIVACY)
> > + privacy_valid = dell_privacy_valid() == -ENODEV; #endif
> > + if (!privacy_valid) {
> > + 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)
> > + return 0;
> > +
> > + token = dell_smbios_find_token(BRIGHTNESS_TOKEN);
> > if (token) {
> > struct calling_interface_buffer buffer;
> >
> > @@ -2257,7 +2264,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 (!privacy_valid)
> > + led_classdev_unregister(&micmute_led_cdev);
> > fail_led:
> > dell_cleanup_rfkill();
> > fail_rfkill:
> > @@ -2278,7 +2286,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 (!privacy_valid)
> > + led_classdev_unregister(&micmute_led_cdev);
> > dell_cleanup_rfkill();
> > if (platform_device) {
> > platform_device_unregister(platform_device);
> > diff --git a/drivers/platform/x86/dell-privacy-acpi.c
> > b/drivers/platform/x86/dell-privacy-acpi.c
> > new file mode 100644
> > index 000000000000..516cd99167c3
> > --- /dev/null
> > +++ b/drivers/platform/x86/dell-privacy-acpi.c
> > @@ -0,0 +1,139 @@
> > +// 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/device.h>
> > +#include <linux/fs.h>
> > +#include <linux/kernel.h>
> > +#include <linux/leds.h>
> > +#include <linux/module.h>
> > +#include <linux/string.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/types.h>
> > +#include <linux/wmi.h>
> > +#include <linux/slab.h>
> > +#include <linux/platform_device.h>
> > +#include "dell-privacy-wmi.h"
> > +
> > +#define PRIVACY_PlATFORM_NAME "dell-privacy-acpi"
> > +#define ACPI_PRIVACY_DEVICE "\\_SB.PC00.LPCB.ECDV"
> > +#define ACPI_PRIVACY_EC_ACK ACPI_PRIVACY_DEVICE ".ECAK"
> > +
> > +static struct platform_device *privacy_acpi_pdev;
> > +
> > +struct privacy_acpi_priv {
> > + struct device *dev;
> > + struct acpi_device *acpi_dev;
> > + struct input_dev *input_dev;
> > + struct platform_device *platform_device; };
> > +
> > +static int micmute_led_set(struct led_classdev *led_cdev,
> > + enum led_brightness brightness) {
> > + acpi_status status;
> > +
> > + status = acpi_evaluate_object(NULL, ACPI_PRIVACY_EC_ACK, NULL,
> NULL);
> > + if (ACPI_FAILURE(status)) {
> > + dev_err(led_cdev->dev, "Error setting privacy audio EC ack
> value: %d\n",status);
> > + return -EIO;
> > + }
> > + return 0;
> > +}
> > +
> > +static struct led_classdev micmute_led_cdev = {
> > + .name = "platform::micmute",
> > + .max_brightness = 1,
> > + .brightness_set_blocking = micmute_led_set,
> > + .default_trigger = "audio-micmute", };
> > +
> > +static int privacy_acpi_remove(struct platform_device *pdev) {
> > + dev_set_drvdata(&pdev->dev, NULL);
> > + return 0;
> > +}
> > +
> > +static int privacy_acpi_probe(struct platform_device *pdev) {
> > + struct privacy_acpi_priv *privacy;
> > + acpi_handle handle;
> > + acpi_status status;
> > + int err;
> > +
> > + privacy = kzalloc(sizeof(struct privacy_acpi_priv), GFP_KERNEL);
> > + if (!privacy)
> > + return -ENOMEM;
> > +
> > + privacy->dev = &pdev->dev;
> > + privacy->platform_device = pdev;
> > + platform_set_drvdata(pdev, privacy);
> > + /* Look for software micmute complete notification device */
> > + status = acpi_get_handle(ACPI_ROOT_OBJECT,
> > + ACPI_PRIVACY_DEVICE,
> > + &handle);
> > + if (ACPI_FAILURE(status)) {
> > + dev_err(privacy->dev, "Unable to find Dell`s EC device %s: %d\n",
> > + ACPI_PRIVACY_DEVICE, status);
> > + return -ENXIO;
> > + }
> > +
> > + micmute_led_cdev.brightness =
> ledtrig_audio_get(LED_AUDIO_MICMUTE);
> > + err = led_classdev_register(&privacy_acpi_pdev->dev,
> &micmute_led_cdev);
> > + if (err < 0)
> > + return -EIO;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct acpi_device_id privacy_acpi_device_ids[] = {
> > + {"PNP0C09", 0},
> > + {"", 0},
> > +};
> > +MODULE_DEVICE_TABLE(acpi, privacy_acpi_device_ids);
> > +
> > +static struct platform_driver privacy_platform_driver = {
> > + .driver = {
> > + .name = PRIVACY_PlATFORM_NAME,
> > + .acpi_match_table = ACPI_PTR(privacy_acpi_device_ids),
> > + },
> > + .probe = privacy_acpi_probe,
> > + .remove = privacy_acpi_remove,
> > +};
> > +
> > +int privacy_acpi_init(void)
> > +{
> > + int err;
> > +
> > + err = platform_driver_register(&privacy_platform_driver);
> > + if (err)
> > + return err;
> > +
> > + privacy_acpi_pdev = platform_device_register_simple(
> > + PRIVACY_PlATFORM_NAME, -1, NULL, 0);
> > + if (IS_ERR(privacy_acpi_pdev)) {
> > + err = PTR_ERR(privacy_acpi_pdev);
> > + goto err_platform;
> > + }
> > + return 0;
> > +
> > +err_platform:
> > + platform_driver_unregister(&privacy_platform_driver);
> > + return err;
> > +}
> > +
> > +void privacy_acpi_cleanup(void)
> > +{
> > + platform_driver_unregister(&privacy_platform_driver);
> > +}
> > +
> > +MODULE_AUTHOR("Perry Yuan <[email protected]>");
> > +MODULE_DESCRIPTION("DELL Privacy ACPI Driver");
> > +MODULE_LICENSE("GPL");
> > +
> > diff --git a/drivers/platform/x86/dell-privacy-wmi.c
> > b/drivers/platform/x86/dell-privacy-wmi.c
> > new file mode 100644
> > index 000000000000..6c36b7ec44c6
> > --- /dev/null
> > +++ b/drivers/platform/x86/dell-privacy-wmi.c
> > @@ -0,0 +1,259 @@
> > +// 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/input.h>
> > +#include <linux/input/sparse-keymap.h> #include <linux/list.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 PRIVACY_SCREEN_STATUS BIT(2)
> > +
> > +static int privacy_valid = -EPROBE_DEFER; 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;
> > + u32 features_present;
> > + u32 last_status;
> > +};
> > +
> > +/*
> > + * 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 } }, };
> > +
> > +bool dell_privacy_valid(void)
> > +{
> > + bool ret;
> > +
> > + mutex_lock(&list_mutex);
> > + ret = wmi_has_guid(DELL_PRIVACY_GUID);
> > + if (!ret){
> > + return -ENODEV;
> > + }
> > + ret = privacy_valid;
> > + mutex_unlock(&list_mutex);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(dell_privacy_valid);
> > +
> > +void dell_privacy_process_event(int type, int code, int status) {
> > + struct privacy_wmi_data *priv;
> > + const struct key_entry *key;
> > +
> > + mutex_lock(&list_mutex);
> > + priv = list_first_entry_or_null(&wmi_list,
> > + struct privacy_wmi_data,
> > + list);
> > + if (priv == NULL) {
> > + pr_err("dell privacy priv is NULL\n");
> > + goto error;
> > + }
> > +
> > + key = sparse_keymap_entry_from_scancode(priv->input_dev, (type <<
> 16)|code);
> > + if (!key) {
> > + dev_dbg(&priv->wdev->dev, "Unknown key with type 0x%04x and
> code 0x%04x pressed\n",
> > + type, code);
> > + goto error;
> > + }
> > +
> > + switch (code) {
> > + case DELL_PRIVACY_TYPE_AUDIO: /* Mic mute */
> > + priv->last_status = status;
> > + sparse_keymap_report_entry(priv->input_dev, key, 1, true);
> > + break;
> > + case DELL_PRIVACY_TYPE_CAMERA: /* Camera mute */
> > + priv->last_status = status;
> > + sparse_keymap_report_entry(priv->input_dev, key, 1, true);
> > + break;
> > + default:
> > + dev_dbg(&priv->wdev->dev, "unknown event type %u /%u",
> > + type, code);
> > + }
> > +error:
> > + mutex_unlock(&list_mutex);
> > + return;
> > +}
> > +EXPORT_SYMBOL_GPL(dell_privacy_process_event);
> > +
> > +static int get_current_status(struct wmi_device *wdev) {
> > + struct privacy_wmi_data *priv = dev_get_drvdata(&wdev->dev);
> > + union acpi_object *obj_present;
> > + union acpi_object *obj_current;
> > + int ret = 0;
> > +
> > + if (priv == NULL) {
> > + pr_err("dell privacy priv is NULL\n");
> > + return -EINVAL;
> > + }
> > + /* get devices supported */
> > + obj_present = wmidev_block_query(wdev, 0);
> > + if (obj_present->type != ACPI_TYPE_INTEGER) {
> > + ret = -EIO;
> > + goto present_free;
> > + }
> > + priv->features_present = obj_present->integer.value;
> > +
> > + /* get current state */
> > + obj_current = wmidev_block_query(wdev, 1);
> > + if (obj_current->type != ACPI_TYPE_INTEGER) {
> > + ret = -EIO;
> > + goto current_free;
> > + }
> > + priv->last_status = obj_current->integer.value;
> > +current_free:
> > + kfree(obj_current);
> > +present_free:
> > + kfree(obj_present);
> > + return ret;
> > +}
> > +
> > +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, pos = 0;
> > +
> > + priv = devm_kzalloc(&wdev->dev, sizeof(struct privacy_wmi_data),
> > + GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + /* create evdev passing interface */
> > + priv->input_dev = devm_input_allocate_device(&wdev->dev);
> > + if (!priv->input_dev)
> > + return -ENOMEM;
> > +
> > + __set_bit(EV_KEY, priv->input_dev->evbit);
> > + __set_bit(KEY_MICMUTE, priv->input_dev->keybit);
> > + __set_bit(EV_MSC, priv->input_dev->evbit);
> > + __set_bit(MSC_SCAN, priv->input_dev->mscbit);
> > + keymap = kcalloc(
> > + ARRAY_SIZE(dell_wmi_keymap_type_0012) +
> > + 1,
> > + sizeof(struct key_entry), GFP_KERNEL);
> > + if (!keymap) {
> > + ret = -ENOMEM;
> > + goto err_free_dev;
> > + }
> > + for (i = 0; i < ARRAY_SIZE(dell_wmi_keymap_type_0012); i++) {
> > + keymap[pos] = dell_wmi_keymap_type_0012[i];
> > + keymap[pos].code |= (0x0012 << 16);
> > + pos++;
> > + }
> > + 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;
> > +
> > + if (input_register_device(priv->input_dev)) {
> > + pr_debug("input_register_device failed to register! \n");
> > + return -ENODEV;
> > + }
> > +
> > + priv->wdev = wdev;
> > + dev_set_drvdata(&wdev->dev, priv);
> > + mutex_lock(&list_mutex);
> > + list_add_tail(&priv->list, &wmi_list);
> > + privacy_valid = true;
> > + if (get_current_status(wdev)) {
> > + goto err_free_dev;
> > + }
> > + mutex_unlock(&list_mutex);
> > + kfree(keymap);
> > + return 0;
> > +
> > +err_free_dev:
> > + input_free_device(priv->input_dev);
> > + kfree(keymap);
> > + return ret;
> > +}
> > +
> > +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);
> > + privacy_valid = -ENODEV;
> > + 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, };
> > +
> > +static int __init init_dell_privacy(void) {
> > + int ret, wmi, acpi;
> > +
> > + wmi = wmi_driver_register(&dell_privacy_wmi_driver);
> > + if (wmi) {
> > + pr_debug("Failed to initialize privacy wmi driver: %d\n", wmi);
> > + return wmi;
> > + }
> > +
> > + acpi = privacy_acpi_init();
> > + if (acpi) {
> > + pr_debug("failed to initialize privacy wmi acpi driver: %d\n", acpi);
> > + return acpi;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +void exit_dell_privacy_wmi(void)
> > +{
> > + wmi_driver_unregister(&dell_privacy_wmi_driver);
> > +}
> > +
> > +static void __exit exit_dell_privacy(void) {
> > + privacy_acpi_cleanup();
> > + exit_dell_privacy_wmi();
> > +}
> > +
> > +module_init(init_dell_privacy);
> > +module_exit(exit_dell_privacy);
> > +
> > +MODULE_DEVICE_TABLE(wmi, dell_wmi_privacy_wmi_id_table);
> > +MODULE_AUTHOR("Perry Yuan <[email protected]>");
> > +MODULE_DESCRIPTION("Dell Privacy WMI Driver");
> MODULE_LICENSE("GPL");
> > diff --git a/drivers/platform/x86/dell-privacy-wmi.h
> > b/drivers/platform/x86/dell-privacy-wmi.h
> > new file mode 100644
> > index 000000000000..94af81d76e44
> > --- /dev/null
> > +++ b/drivers/platform/x86/dell-privacy-wmi.h
> > @@ -0,0 +1,23 @@
> > +/* 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_
> > +#include <linux/wmi.h>
> > +
> > +bool dell_privacy_valid(void);
> > +void dell_privacy_process_event(int type, int code, int status); int
> > +privacy_acpi_init(void); void privacy_acpi_cleanup(void);
> > +
> > +/* DELL Privacy Type */
> > +enum {
> > + DELL_PRIVACY_TYPE_UNKNOWN = 0x0,
> > + DELL_PRIVACY_TYPE_AUDIO,
> > + DELL_PRIVACY_TYPE_CAMERA,
> > +};
> > +#endif
> > diff --git a/drivers/platform/x86/dell-wmi.c
> > b/drivers/platform/x86/dell-wmi.c index bbdb3e860892..44bb74e4df86
> > 100644
> > --- a/drivers/platform/x86/dell-wmi.c
> > +++ b/drivers/platform/x86/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 <[email protected]>");
> > MODULE_AUTHOR("Pali Rohár <[email protected]>"); @@ -410,44 +411,57
> @@
> > static void dell_wmi_notify(struct wmi_device *wdev,
> > if (buffer_end > buffer_entry + buffer_entry[0] + 1)
> > buffer_end = buffer_entry + buffer_entry[0] + 1;
> >
> > - while (buffer_entry < buffer_end) {
> > -
> > - len = buffer_entry[0];
> > - if (len == 0)
> > - break;
> > -
> > - len++;
> > -
> > - if (buffer_entry + len > buffer_end) {
> > - pr_warn("Invalid length of WMI event\n");
> > - break;
> > - }
> > -
> > - pr_debug("Process buffer (%*ph)\n", len*2, buffer_entry);
> > -
> > - 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]);
> > - /* Extended data is currently ignored */
> > - break;
> > - case 0x0010: /* Sequence of keys pressed */
> > - case 0x0011: /* Sequence of events occurred */
> > - for (i = 2; i < len; ++i)
> > - dell_wmi_process_key(wdev, buffer_entry[1],
> > - buffer_entry[i]);
> > - break;
> > - default: /* Unknown event */
> > - pr_info("Unknown WMI event type 0x%x\n",
> > - (int)buffer_entry[1]);
> > - break;
> > - }
> > -
> > - buffer_entry += len;
> > -
> > - }
> > + while (buffer_entry < buffer_end) {
> > +
> > + len = buffer_entry[0];
> > + if (len == 0)
> > + break;
> > +
> > + len++;
> > +
> > + if (buffer_entry + len > buffer_end) {
> > + pr_warn("Invalid length of WMI event\n");
> > + break;
> > + }
> > +
> > + pr_debug("Process buffer (%*ph)\n", len*2, buffer_entry);
> > +
> > + switch (buffer_entry[1]) {
> > + case 0x0000: /* One key pressed or event occurred */
> > + if (len > 2)
> > + dell_wmi_process_key(wdev, buffer_entry[1],
> > + buffer_entry[2]);
> > + break;
> > + case 0x0010: /* Sequence of keys pressed */
> > + case 0x0011: /* Sequence of events occurred */
> > + for (i = 2; i < len; ++i)
> > + dell_wmi_process_key(wdev, buffer_entry[1],
> > + buffer_entry[i]);
> > + break;
> > + case 0x0012:
> > +#if IS_ENABLED(CONFIG_DELL_PRIVACY)
> > + if (dell_privacy_valid()) {
> > + 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]);
> > + }
> > +#else
> > + /* Extended data is currently ignored */
> > + if (len > 2)
> > + dell_wmi_process_key(wdev, buffer_entry[1],
> > +buffer_entry[2]); #endif
> > + break;
> > + default: /* Unknown event */
> > + pr_info("Unknown WMI event type 0x%x\n",
> > + (int)buffer_entry[1]);
> > + break;
> > + }
> > +
> > + buffer_entry += len;
> > +
> > + }
> >
> > }
> >
> >

2020-11-11 14:34:33

by Mario Limonciello

[permalink] [raw]

Subject: RE: [PATCH] platform/x86: dell-privacy: Add support for new privacy driver

>
> On Wed, Nov 11, 2020 at 07:21:07AM +0000, Yuan, Perry wrote:
> > > > + status = acpi_evaluate_object(NULL, ACPI_PRIVACY_EC_ACK, NULL,
> > > NULL);
> > > > + if (ACPI_FAILURE(status)) {
> > > > + dev_err(led_cdev->dev, "Error setting privacy audio EC ack
> > > value: %d\n",status);
> > > > + return -EIO;
> > > > + }
> > > > + return 0;
> > > > +}
> > >
> > > What's actually being set here? You don't seem to be passing any
> arguments.
> >
> > Yes, it is a EC ack notification without any arguments needed.
>
> I'm confused why it's being exposed as an LED device in that case -
> there's an expectation that this is something that actually controls a
> real LED, which means responding to state. Are you able to share the
> acpidump of a machine with this device?
>
> --

Matthew,

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 leading to a
poor user experience.

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. Later follow up work is already envisioned that if HW
mute is already enacted but SW mute is modified (IE LED notifier goes
through) that a message can come back out to userspace to tell the user
something along the lines of

"Your laptop mic is muted, press fn+f4 to unmute".

I don't believe that will be part of the first commits to land, but that's
why an LED is used, to know the state of SW.

Perry,

Some suggestions for v2:
* You should better explain this hardware design in the commit
message.
* I think the codec changes should be in same patch series as 1/2 and this
be 2/2 rather than them going separately. It won't make sense for one of them
to go in without the other. For example if codec change goes in and dell-laptop
driver tries to change legacy LED it won't do anything. And if this goes in
but codec driver doesn't, nothing will ever send EC ack.

2020-11-12 15:08:30

by Enrico Weigelt, metux IT consult

[permalink] [raw]

Subject: Re: [PATCH] platform/x86: dell-privacy: Add support for new privacy driver

On 11.11.20 15:30, Limonciello, Mario wrote:

Hi,

> 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 leading to a
> poor user experience.

how long is that timeout ?

> Exposing as an LED device allows the codec drivers notification path to
> EC ACK to work.

Which driver exactly ? Who's gonna access this LED ?

--mtx

--
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
[email protected] -- +49-151-27565287

2020-11-12 15:34:38

by Mario Limonciello

[permalink] [raw]

Subject: RE: [PATCH] platform/x86: dell-privacy: Add support for new privacy driver

> > 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 leading to a
> > poor user experience.
>
> how long is that timeout ?

The exact duration is controlled by component selection in the circuit.
Linux is typically able to respond faster than Windows in this case.

>
> > Exposing as an LED device allows the codec drivers notification path to
> > EC ACK to work.
>
> Which driver exactly ? Who's gonna access this LED ?

The flow is like this:

1) User presses key. HW does stuff with this key (timeout is started)
2) Event is emitted from FW
3) Event 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.

Again, if anything in this flow doesn't happen HW mic mute is still activated,
just will take longer (for duration of timeout) and have popping noise.

>
>
> --mtx
>
> --
> ---
> Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
> werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
> GPG/PGP-Schlüssel zu.
> ---
> Enrico Weigelt, metux IT consult
> Free software and Linux embedded engineering
> [email protected] -- +49-151-27565287

2020-11-12 15:57:12

by Hans de Goede

[permalink] [raw]

Subject: Re: [PATCH] platform/x86: dell-privacy: Add support for new privacy driver

Hi,

On 11/12/20 4:31 PM, Limonciello, Mario wrote:
>>> 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 leading to a
>>> poor user experience.
>>
>> how long is that timeout ?
>
> The exact duration is controlled by component selection in the circuit.
> Linux is typically able to respond faster than Windows in this case.
>
>>
>>> Exposing as an LED device allows the codec drivers notification path to
>>> EC ACK to work.
>>
>> Which driver exactly ? Who's gonna access this LED ?
>
> The flow is like this:
>
> 1) User presses key. HW does stuff with this key (timeout is started)
> 2) Event is emitted from FW
> 3) Event 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.
>
> Again, if anything in this flow doesn't happen HW mic mute is still activated,
> just will take longer (for duration of timeout) and have popping noise.

Thank you, can we put this in a comment in the driver please ?

I guess this also means that the led_class device is just there to
catch the ledtrig_audio_set() call so that dell-firmware can tell the
EC that the sw-mute is done and that it can move ahead with the hw-mute.

While the real, physical LED is fully under hardware control, right ?

That should probably also be in the same comment in the driver
(feel free to re-use part of my wording for that if that helps).

Regards,

Hans

>
>>
>>
>> --mtx
>>
>> --
>> ---
>> Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
>> werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
>> GPG/PGP-Schlüssel zu.
>> ---
>> Enrico Weigelt, metux IT consult
>> Free software and Linux embedded engineering
>> [email protected] -- +49-151-27565287

2020-11-12 16:00:20

by Mario Limonciello

[permalink] [raw]

Subject: RE: [PATCH] platform/x86: dell-privacy: Add support for new privacy driver

> >
> > Again, if anything in this flow doesn't happen HW mic mute is still
> activated,
> > just will take longer (for duration of timeout) and have popping noise.
>
> Thank you, can we put this in a comment in the driver please ?

Yes, I agree. I suggested to Perry that his next submission of this driver
needs a lot more context in commit message (and it sounds like probably
code comments too).

>
> I guess this also means that the led_class device is just there to
> catch the ledtrig_audio_set() call so that dell-firmware can tell the
> EC that the sw-mute is done and that it can move ahead with the hw-mute.
>
> While the real, physical LED is fully under hardware control, right ?
>
> That should probably also be in the same comment in the driver
> (feel free to re-use part of my wording for that if that helps).
>
> Regards,
>
> Hans

Yes - exactly.