Re: [PATCH v3 1/3] platform/x86: dell-privacy: Add support for Dell hardware privacy

On 1/12/21 11:17 AM, Perry Yuan wrote:

From: Perry Yuan perry_yuan@dell.com

add support for dell privacy driver for the dell units equipped hardware privacy design, which protect users privacy of audio and camera from hardware level. once the audio or camera privacy mode enabled, any applications will not get any audio or video stream when user pressed ctrl+F4 hotkey, audio privacy mode will be enabled,Micmute led will be also changed accordingly The micmute led is fully controlled by hardware & EC(embedded controller) and camera mute hotkey is ctrl+f9. currently design only emmit SW_CAMERA_LENS_COVER event while the camera lens shutter will be changed by EC & hw(hadware) control

It wouldn't hurt to use capital letters and punctuation, it helps with readility..

[...]

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c index 70edc5bb3a14..2fea1f34fcf9 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;

why is this variable needed? Was the intent to have a kernel parameter here?

module_param(force_rfkill, bool, 0444); MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted models"); @@ -2205,11 +2207,18 @@ static int __init dell_init(void) 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;
  

• 	dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) {
  

+#if IS_ENABLED(CONFIG_DELL_PRIVACY)

• ret = dell_privacy_valid();
  

• if (!ret)
  

• 	privacy_valid = true;
  

+#endif

• if (!privacy_valid) {
  

if it was intended to be used as a kernel parameter it's not done the right way: the value set by the user would be ignored...

• 	micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
  

• 	ret = led_classdev_register(&platform_device->dev, &micmute_led_cdev);
  

• 	if (ret < 0)
  

• 		goto fail_led;
  

• }
  

  }

  if (acpi_video_get_backlight_type() != acpi_backlight_vendor)

@@ -2257,7 +2266,8 @@ static int __init dell_init(void) fail_get_brightness: backlight_device_unregister(dell_backlight_device); fail_backlight:

• led_classdev_unregister(&micmute_led_cdev);

• if (!privacy_valid)

• led_classdev_unregister(&micmute_led_cdev);
  

  fail_led: dell_cleanup_rfkill(); fail_rfkill:

@@ -2278,7 +2288,8 @@ static void __exit dell_exit(void) touchpad_led_exit(); kbd_led_exit(); backlight_device_unregister(dell_backlight_device);

• led_classdev_unregister(&micmute_led_cdev);

• if (!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..df6a86e1345c --- /dev/null +++ b/drivers/platform/x86/dell-privacy-acpi.c @@ -0,0 +1,167 @@ +// 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/bits.h> +#include <linux/device.h> +#include <linux/fs.h> +#include <linux/kernel.h> +#include <linux/leds.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/platform_device.h> +#include <linux/string.h> +#include <linux/sysfs.h> +#include <linux/slab.h> +#include <linux/types.h> +#include <linux/wmi.h>

+#include "dell-privacy-wmi.h"

+#define PRIVACY_PLATFORM_NAME "dell-privacy-acpi" +#define DELL_PRIVACY_GUID "6932965F-1671-4CEB-B988-D3AB0A901919"

+struct privacy_acpi_priv {

• struct device *dev;
• struct platform_device *platform_device;
• struct led_classdev cdev;

+}; +static struct privacy_acpi_priv *privacy_acpi;

+static int dell_privacy_micmute_led_set(struct led_classdev *led_cdev,

• enum led_brightness brightness)
  

+{

• struct privacy_acpi_priv *priv = privacy_acpi;
• acpi_status status;
• acpi_handle handle;
• char *acpi_method;
• handle = ec_get_handle();
• if (!handle)

• return -EIO;
  

• acpi_method = "ECAK";
• status = acpi_evaluate_object(handle, acpi_method, NULL, NULL);
• if (ACPI_FAILURE(status)) {

• dev_err(priv->dev, "Error setting privacy EC ack value: %s\n",
  

• 		acpi_format_exception(status));
  

• return -EIO;
  

• }
• return 0;

+}

+static int dell_privacy_acpi_remove(struct platform_device *pdev) +{

• struct privacy_acpi_priv *priv = dev_get_drvdata(privacy_acpi->dev);
• led_classdev_unregister(&priv->cdev);
• return 0;

+} +/*

• • Pressing the mute key activates a time delayed circuit to physically cut
• • off the mute. The LED is in the same circuit, so it reflects the true
• • state of the HW mute. The reason for the EC "ack" is so that software
• • can first invoke a SW mute before the HW circuit is cut off. Without SW
• • cutting this off first does not affect the time delayed muting or status
• • of the LED but there is a possibility of a "popping" noise.
• • If the EC receives the SW ack, the circuit will be activated before the
• • delay completed.
• • Exposing as an LED device allows the codec drivers notification path to
• • EC ACK to work
• */

+static int dell_privacy_leds_setup(struct device *dev) +{

• struct privacy_acpi_priv *priv = dev_get_drvdata(dev);
• int ret = 0;
• priv->cdev.name = "dell-privacy::micmute";
• priv->cdev.max_brightness = 1;
• priv->cdev.brightness_set_blocking = dell_privacy_micmute_led_set;
• priv->cdev.default_trigger = "audio-micmute";
• priv->cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
• ret = devm_led_classdev_register(dev, &priv->cdev);
• if (ret < 0)

• return ret;
  

• return 0;

+}

+static int dell_privacy_acpi_probe(struct platform_device *pdev) +{

• platform_set_drvdata(pdev, privacy_acpi);
• privacy_acpi->dev = &pdev->dev;
• privacy_acpi->platform_device = pdev;
• return 0;

+}

+static const struct acpi_device_id privacy_acpi_device_ids[] = {

• {"PNP0C09", 0},
• { },

+}; +MODULE_DEVICE_TABLE(acpi, privacy_acpi_device_ids);

+static struct platform_driver dell_privacy_platform_drv = {

• .driver = {

• .name = PRIVACY_PLATFORM_NAME,
  

• .acpi_match_table = ACPI_PTR(privacy_acpi_device_ids),
  

• },

no .probe?

• .remove = dell_privacy_acpi_remove,

+};

+int __init dell_privacy_acpi_init(void) +{

• int err;
• struct platform_device *pdev;
• int privacy_capable = wmi_has_guid(DELL_PRIVACY_GUID);
• if (!wmi_has_guid(DELL_PRIVACY_GUID))

• return -ENODEV;
  

• privacy_acpi = kzalloc(sizeof(*privacy_acpi), GFP_KERNEL);
• if (!privacy_acpi)

• return -ENOMEM;
  

• pdev = platform_device_register_simple(

• 	PRIVACY_PLATFORM_NAME, PLATFORM_DEVID_NONE, NULL, 0);
  

• if (IS_ERR(pdev)) {

• err = PTR_ERR(pdev);
  

• goto pdev_err;
  

• }
• err = platform_driver_probe(&dell_privacy_platform_drv,

• 	dell_privacy_acpi_probe);
  

• if (err)

• goto pdrv_err;
  

why is the probe done here? Put differently, what prevents you from using a 'normal' platform driver, and do the leds_setup in the .probe()?

• err = dell_privacy_leds_setup(&pdev->dev);
• if (err < 0)

• goto pdrv_err;
  

• return 0;

+pdrv_err:

• platform_device_unregister(pdev);

+pdev_err:

• kfree(privacy_acpi);
• return err;

+}

+void __exit dell_privacy_acpi_exit(void) +{

• struct platform_device *pdev = to_platform_device(privacy_acpi->dev);
• platform_device_unregister(pdev);
• platform_driver_unregister(&dell_privacy_platform_drv);
• kfree(privacy_acpi);

+}

+MODULE_AUTHOR("Perry Yuan perry_yuan@dell.com"); +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..432a3f4ed226 --- /dev/null +++ b/drivers/platform/x86/dell-privacy-wmi.c @@ -0,0 +1,320 @@ +// 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;

this is set to -ENODEV on remove. that looks odd to me.

+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 } },
• { KE_END, 0},

+};

+int dell_privacy_valid(void) +{

• int ret;
• ret = wmi_has_guid(DELL_PRIVACY_GUID);
• if (!ret)

• return -ENODEV;
  

• ret = privacy_valid;
• return ret;

return privacy_valid?

+} +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) {

• pr_err("dell privacy priv is NULL\n");
  

• goto error;
  

• }
• key = sparse_keymap_entry_from_scancode(priv->input_dev, (type << 16)|code);

missing spaces, does this even pass with checkpatch.pl?

• 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 */

Never seen anyone 'mute' their camera? 'switch off' or 'disable'?

• priv->last_status = status;
  

• sparse_keymap_report_entry(priv->input_dev, key, 1, true);
  

• break;
  

• default:

• 	dev_dbg(&priv->wdev->dev, "unknown event type 0x%04x 0x%04x",
  

• 			type, code);
  

• }

+error:

• mutex_unlock(&list_mutex);

+} +EXPORT_SYMBOL_GPL(dell_privacy_process_event);

[...]

+/*

• • Describes the Device State class exposed by BIOS which can be consumed by
• • various applications interested in knowing the Privacy feature capabilities.
• • class DeviceState
• • {
• • [key, read] string InstanceName;
• • [read] boolean ReadOnly;
• • [WmiDataId(1), read] uint32 DevicesSupported;
• • 0 – None, 0x1 – Microphone, 0x2 – Camera, 0x4 -ePrivacy Screen
• • [WmiDataId(2), read] uint32 CurrentState;
• • 0:Off; 1:On. Bit0 – Microphone, Bit1 – Camera, Bit2 - ePrivacyScreen
• • };
• */

+static int get_current_status(struct wmi_device *wdev) +{

• struct privacy_wmi_data *priv = dev_get_drvdata(&wdev->dev);
• union acpi_object *obj_present;
• u32 *buffer;
• int ret = 0;
• if (!priv) {

• pr_err("dell privacy priv is NULL\n");
  

• return -EINVAL;
  

• }

can this happen? when would this pointer be NULL?

• /* check privacy support features and device states */
• obj_present = wmidev_block_query(wdev, 0);
• if (!obj_present) {

• dev_err(&wdev->dev, "failed to read Binary MOF\n");
  

• ret = -EIO;
  

• privacy_valid = ret;
  

• return ret;
  

• }
• if (obj_present->type != ACPI_TYPE_BUFFER) {

• dev_err(&wdev->dev, "Binary MOF is not a buffer!\n");
  

• ret = -EIO;
  

• privacy_valid = ret;
  

• goto obj_free;
  

• }
• /* Although it's not technically a failure, this would lead to

• *  unexpected behavior
  

• */
  

• if (obj_present->buffer.length != 8) {

• dev_err(&wdev->dev, "Dell privacy buffer has unexpected length (%d)!\n",
  

• 		obj_present->buffer.length);
  

• ret = -EINVAL;
  

• privacy_valid = ret;
  

• goto obj_free;
  

• }
• buffer = (u32 *)obj_present->buffer.pointer;
• priv->features_present = buffer[0];
• priv->last_status = buffer[1];
• privacy_valid = 0;

+obj_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(*priv), GFP_KERNEL);
• if (!priv)

• return -ENOMEM;
  

• dev_set_drvdata(&wdev->dev, priv);
• priv->wdev = wdev;
• /* create evdev passing interface */
• priv->input_dev = devm_input_allocate_device(&wdev->dev);
• if (!priv->input_dev)

• return -ENOMEM;
  

• /* remap the wmi keymap event to new keymap */
• keymap = kcalloc(ARRAY_SIZE(dell_wmi_keymap_type_0012) +

• 	1,
  

same line?

• 	sizeof(struct key_entry), GFP_KERNEL);
  

• if (!keymap) {

• ret = -ENOMEM;
  

• goto err_free_dev;
  

• }
• /* remap the keymap code with Dell privacy key type 0x12 as prefix

• * KEY_MICMUTE scancode will be reported as 0x120001
  

• */
  

• for (i = 0; i < ARRAY_SIZE(dell_wmi_keymap_type_0012); i++) {

• keymap[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");
  

• goto err_free_keymap;
  

• }
• mutex_lock(&list_mutex);
• list_add_tail(&priv->list, &wmi_list);
• mutex_unlock(&list_mutex);
• if (get_current_status(priv->wdev))

• goto err_free_input;
  

• ret = devm_device_add_group(&wdev->dev, &privacy_attribute_group);
• if (ret)

• goto err_free_input;
  

• kfree(keymap);
• return 0;

having a set of newlines wouldn't hurt, thsi is not very easy to read and split in logical sections...

+err_free_input:

• input_unregister_device(priv->input_dev);

+err_free_keymap:

• privacy_valid = -ENODEV;
• kfree(keymap);

+err_free_dev:

• input_free_device(priv->input_dev);
• 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);
• mutex_unlock(&list_mutex);
• privacy_valid = -ENODEV;
• input_unregister_device(priv->input_dev);
• return 0;

+}

[...]

• case 0x0012:
  

+#if IS_ENABLED(CONFIG_DELL_PRIVACY)

• 	err = dell_privacy_valid();
  

• 	if (err == 0) {
  

• 		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]);
  

• 		/* Extended data is currently ignored */
  

• 	}
  

+#else

• 	if (len > 2)
  

• 		dell_wmi_process_key(wdev, buffer_entry[1],
  

• 				buffer_entry[2]);
  

• 	/* Extended data is currently ignored */
  

+#endif

this doesn't look very nice, can we avoid the duplication?

• 	break;
  

  default: /* Unknown event */ pr_info("Unknown WMI event type 0x%x\n", (int)buffer_entry[1]);