[v2,1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy

Commit Message

Yuan, Perry Dec. 28, 2020, 1:28 p.m. UTC

From: Perry Yuan <[e-mail]>

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

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

Signed-off-by: Perry Yuan  <[e-mail]>
Signed-off-by: Limonciello Mario <[e-mail]>
---
v1 -> v2:
* query EC handle from EC driver directly.
* fix some code style.
* add KEY_END to keymap array.
* clean platform device when cleanup called
* use hexadecimal format for log print in dev_dbg
* remove __set_bit for the report keys from probe.
* fix keymap leak
* add err_free_keymap in dell_privacy_wmi_probe
* wmi driver will be unregistered if privacy_acpi_init() fails
* add sysfs attribute files for user space query.
* add leds micmute driver to privacy acpi
* add more design info the commit info
---
---
 drivers/platform/x86/Kconfig             |  17 ++
 drivers/platform/x86/Makefile            |   4 +-
 drivers/platform/x86/dell-laptop.c       |  29 ++-
 drivers/platform/x86/dell-privacy-acpi.c | 165 ++++++++++++
 drivers/platform/x86/dell-privacy-wmi.c  | 309 +++++++++++++++++++++++
 drivers/platform/x86/dell-privacy-wmi.h  |  33 +++
 drivers/platform/x86/dell-wmi.c          |  26 +-
 7 files changed, 567 insertions(+), 16 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

Comments

Barnabás Pőcze Dec. 28, 2020, 8:26 p.m. UTC | #1

Hi


Firstly, a minor note: the newly created sysfs attributes are not documented, so
I believe you should either move them to debugfs or add documentation. And I
believe you might have forgotten to send the second patch in the series.

I added a couple comments regarding the code, but unfortunately I believe there
are deeper, architectural problems. I cannot help but think that this is a bit
over-complicated with its 1 platform device, 1 platform driver, 1 WMI driver,
2 source files, not-immediately-clear relationship between the two "submodules",
and (a bit) forced integration with the dell-wmi module.

If it were up to me I would do it the simplest way: a single module,
exports dell_privacy_is_ok() and dell_privacy_event(); its probe checks the WMI
GUID, the EC handle, and the ECAK method; registers a single platform device
with PLATFORM_DEVID_NONE and with the necessary attributes (devices_supported,
current_state); registers the LED and input device under the platform device.

But, of course, you should wait for Hans or Mark to see what they'd prefer.


2020. december 28., hétfő 14:28 keltezéssel, Perry Yuan írta:

> From: Perry Yuan <[e-mail]>
>
> 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.

I believe at the first occurrence of "EC" it should be noted what it stands for.


> and camera mute hotkey is ctrl+F9.currently design only emmit
> SW_CAMERA_LENS_COVER event while the camera LENS shutter will be

Why is "LENS" capitalized?


> changed by EC & HW control.
>
> *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.
>

Please proofread the commit message again, and pay attention to capitalization
and spacing.


> Signed-off-by: Perry Yuan  <[e-mail]>
> Signed-off-by: Limonciello Mario <[e-mail]>
> ---
> v1 -> v2:
> * query EC handle from EC driver directly.
> * fix some code style.
> * add KEY_END to keymap array.
> * clean platform device when cleanup called
> * use hexadecimal format for log print in dev_dbg
> * remove __set_bit for the report keys from probe.
> * fix keymap leak
> * add err_free_keymap in dell_privacy_wmi_probe
> * wmi driver will be unregistered if privacy_acpi_init() fails
> * add sysfs attribute files for user space query.
> * add leds micmute driver to privacy acpi
> * add more design info the commit info
> ---
> ---
>  drivers/platform/x86/Kconfig             |  17 ++
>  drivers/platform/x86/Makefile            |   4 +-
>  drivers/platform/x86/dell-laptop.c       |  29 ++-
>  drivers/platform/x86/dell-privacy-acpi.c | 165 ++++++++++++
>  drivers/platform/x86/dell-privacy-wmi.c  | 309 +++++++++++++++++++++++
>  drivers/platform/x86/dell-privacy-wmi.h  |  33 +++
>  drivers/platform/x86/dell-wmi.c          |  26 +-
>  7 files changed, 567 insertions(+), 16 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 91e6176cdfbd..9d5cc2454b3e 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -491,6 +491,23 @@ 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
> +	depends on LEDS_TRIGGER_AUDIO
> +	select DELL_WMI
> +	help
> +	This driver provides support for the "Dell Hardware Privacy" feature
> +	of Dell laptops.
> +	Support for a micmute and camera mute privacy will be provided as
> +	hardware button Ctrl+F4 and Ctrl+F9 hotkey
> +
> +	To compile this driver as a module, choose M here: the module will
> +	be called dell_privacy.
> +
>  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 581475f59819..18c430456de7 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -51,7 +51,9 @@ 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_WMI_SYSMAN)		+= dell-wmi-sysman/
> -
> +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 70edc5bb3a14..ea0c8a8099ff 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");
> @@ -1587,10 +1589,10 @@ static ssize_t kbd_led_timeout_store(struct device *dev,
>  		switch (unit) {
>  		case KBD_TIMEOUT_DAYS:
>  			value *= 24;
> -			fallthrough;
> +			/* fall through */
>  		case KBD_TIMEOUT_HOURS:
>  			value *= 60;
> -			fallthrough;
> +			/* fall through */

What is the reason behind changing "fallthrough;" to a comment?


>  		case KBD_TIMEOUT_MINUTES:
>  			value *= 60;
>  			unit = KBD_TIMEOUT_SECONDS;
> @@ -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) {
> +			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..fef781555b67
> --- /dev/null
> +++ b/drivers/platform/x86/dell-privacy-acpi.c
> @@ -0,0 +1,165 @@
> +// 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/bits.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include "dell-privacy-wmi.h"

I believe it would be preferable to sort the list of includes lexicographically.
("dell-privacy-wmi.h" can remain separate)


> +
> +#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;

Any reason it needs to be dynamically allocated?


> +
> +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;
> +	if (acpi_has_method(handle, "ECAK"))
> +		acpi_method = "ECAK";
> +	else
> +		return -ENODEV;

I find this if-else a bit cumbersome. Any reason why

if (!acpi_has_method(handle, "ECAK"))
  return ...;

would not work? I believe you could also easily do away with the `acpi_method`
variable.


> +
> +	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);
> +	dev_set_drvdata(&pdev->dev, NULL);

This is not needed as the driver sets the driver data to NULL when a driver
unbinds from a device.


> +
> +	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 void dell_privacy_leds_setup(struct device *dev)
> +{
> +	struct privacy_acpi_priv *priv = dev_get_drvdata(dev);
> +
> +	priv->cdev.name = "privacy::micmute";

Maybe "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);
> +	priv->cdev.dev = dev;

There is no need for this assignment.


> +	devm_led_classdev_register(dev, &priv->cdev);

I believe it'd be preferable to return the return value of this call.


> +}
> +
> +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),
> +	},
> +	.remove = dell_privacy_acpi_remove,
> +};

I think using a platform driver here just complicates things for no reason.
Furthermore, I'm not sure if there's actually any need for the ACPI match table.


> +
> +int dell_privacy_acpi_init(void)

I believe this could be marked __init.


> +{
> +	int err;
> +	struct platform_device *pdev;
> +	int privacy_capable = wmi_has_guid(DELL_PRIVACY_GUID);
> +
> +	if (!privacy_capable)

It could just be `if (!wmi_has_guid(...))`.


> +		return -ENODEV;
> +
> +	privacy_acpi = kzalloc(sizeof(struct privacy_acpi_priv), GFP_KERNEL);

Please use `sizeof(*privacy_acpi)`.


> +	if (!privacy_acpi)
> +		return -ENOMEM;
> +
> +	pdev = platform_device_register_simple(
> +			PRIVACY_PLATFORM_NAME, -1, NULL, 0);

Please use `PLATFORM_DEVID_NONE` instead of -1.


> +	if (IS_ERR(pdev)) {
> +		err = PTR_ERR(pdev);
> +		goto pdev_err;
> +	}
> +	err = platform_driver_probe(&dell_privacy_platform_drv,
> +			dell_privacy_acpi_probe);

What is the reason for preferring this instead of specifying the probe callback
in the platform_driver struct and registering it?


> +	if (err)
> +		goto pdrv_err;
> +
> +	dell_privacy_leds_setup(&pdev->dev);

I think you should check if the call succeeds or not.


> +
> +	return 0;
> +
> +pdrv_err:
> +	platform_device_unregister(pdev);
> +pdev_err:
> +	kfree(privacy_acpi);
> +	return err;
> +}
> +
> +void dell_privacy_acpi_exit(void)

I believe this could be marked __exit.


> +{
> +	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 <[e-mail]>");
> +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..80637c7f617c
> --- /dev/null
> +++ b/drivers/platform/x86/dell-privacy-wmi.c
> @@ -0,0 +1,309 @@
> +// 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;

I think it'd be better `privacy_valid` was a `bool` (or maybe an enum):

```
enum dell_privacy_state {
	DELL_PRIVACY_STATE_OK,
	DELL_PRIVACY_STATE_NOK,
	DELL_PRIVACY_STATE_UNKNOWN,
};
```

or something similar.


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

Any reason for "MIC" being capitalized?


> +	{ 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;

I find this function really confusing, and too verbose for what it does.


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

Can you please explain why this list is needed if only the first entry will
ever be used?


> +	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 0x%04x 0x%04x",
> +					type, code);
> +	}

Is this switch needed at all?


> +error:
> +	mutex_unlock(&list_mutex);
> +}
> +EXPORT_SYMBOL_GPL(dell_privacy_process_event);
> +
> +static ssize_t devices_supported_show(struct device *dev,
> +		struct device_attribute *attr,
> +		char *buf)
> +{
> +	struct privacy_wmi_data *priv = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%d\n", priv->features_present);

Please use `sysfs_emit()`. And I believe printing with %x would be preferable.


> +}
> +
> +static ssize_t current_state_show(struct device *dev,
> +		struct device_attribute *attr,
> +		char *buf)
> +{
> +	struct privacy_wmi_data *priv = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%d\n", priv->last_status);

Same here.


> +}
> +
> +static DEVICE_ATTR_RO(devices_supported);
> +static DEVICE_ATTR_RO(current_state);
> +
> +static struct attribute *platform_attributes[] = {

Maybe "privacy_attributes" or something similar would be more expressive?


> +	&dev_attr_devices_supported.attr,
> +	&dev_attr_current_state.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group privacy_attribute_group = {
> +	.attrs = platform_attributes
> +};
> +
> +/*
> + * Describes the Device State class exposed by BIOS which can be consumed by
> + * various applications interested in knowing the Privacy feature capabilities.
> + * class DeviceState
> + * {
> + *  [key, read] string InstanceName;
> + *  [read] boolean ReadOnly;
> + *  [WmiDataId(1), read] uint32 DevicesSupported;
> + *   0 – None, 0x1 – Microphone, 0x2 – Camera, 0x4 -ePrivacy  Screen
> + *  [WmiDataId(2), read] uint32 CurrentState;
> + *   0:Off; 1:On. Bit0 – Microphone, Bit1 – Camera, Bit2 - ePrivacyScreen
> + * };
> + */
> +
> +static int get_current_status(struct wmi_device *wdev)
> +{
> +	struct privacy_wmi_data *priv = dev_get_drvdata(&wdev->dev);
> +	union acpi_object *obj_present = NULL;

As far as I see there is not need to initialize `obj_present`.


> +	u32 *buffer;
> +	int ret = 0;
> +
> +	if (priv == NULL) {

Maybe `if (WARN_ON(!priv))`? But `!priv` is preferred in any case.


> +		pr_err("dell privacy priv is NULL\n");
> +		return -EINVAL;
> +	}
> +	/* check privacy support features and device states */
> +	obj_present = wmidev_block_query(wdev, 0);

`wmidev_block_query()` may return `NULL`, so you should check for that as well.


> +	if (obj_present->type != ACPI_TYPE_BUFFER) {
> +		dev_err(&wdev->dev, "Dell privacy failed to get device status!\n");

I think a more specific error message ("unexpected type") would be preferable.


> +		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 = -ENODEV;

I personally don't think ENODEV is the most suitable error code here. EINVAL/EILSEQ
seem more appropriate to me.


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

I believe `get_unaligned_{le,be}32()` from `asm/unaligned.h` would be preferable
here.


> +	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;

There is actually no need for the `pos` variable.


> +
> +	priv = devm_kzalloc(&wdev->dev, sizeof(struct privacy_wmi_data),

Please use `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,

I don't think that `+ 1` is not needed since the KE_END entry is already in the array.


> +			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;

A copy of the keymap is created by `sparse_keymap_setup()`, so returning
here will leak `keymap`. You could just call `kfree(keymap)` directly after
the `sparse_keymap_setup()` call. But I find it completely unnecessary
to do this allocate-copy-modify thing. Is there any reason why the static array
(`dell_wmi_keymap_type_0012`) cannot already contain the correct values?


> +	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_keymap;

The input device is not unregistered in this branch.


> +	ret = sysfs_create_group(&wdev->dev.kobj, &privacy_attribute_group);

I suggest you replace `sysfs_{create,remove}_group()` with
`device_{add,remove}_group()` as it is more expressive in my opinion.


> +	if (ret)
> +		goto err_free_keymap;

Similarly, the input device is not unregistered in this branch.


> +	return 0;

`keymap` is again leaked by this return.


> +
> +err_free_keymap:
> +	privacy_valid = -ENODEV;
> +	kfree(keymap);
> +err_free_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;
> +	sysfs_remove_group(&wdev->dev.kobj, &privacy_attribute_group);
> +

The input device is not unregistered.

> +	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;
> +
> +	ret = wmi_has_guid(DELL_PRIVACY_GUID);
> +	if (!ret)
> +		return -ENODEV;

The init function of a module that exports symbols must not fail, otherwise
it'll prevent the loading of dependent modules.


> +
> +	ret = dell_privacy_acpi_init();
> +	if (ret) {
> +		pr_err("failed to initialize privacy acpi driver: %d\n", ret);
> +		goto err_init;
> +	}
> +
> +	ret = wmi_driver_register(&dell_privacy_wmi_driver);
> +	if (ret) {
> +		pr_err("failed to initialize privacy wmi driver: %d\n", ret);
> +		return ret;
> +	}
> +	return 0;
> +
> +err_init:
> +	wmi_driver_unregister(&dell_privacy_wmi_driver);

At this point the WMI driver is not registered.


> +	return ret;
> +}
> +
> +static void dell_privacy_wmi_exit(void)

I believe this function could be marked __exit as well.


> +{
> +	wmi_driver_unregister(&dell_privacy_wmi_driver);
> +}
> +
> +static void __exit exit_dell_privacy(void)
> +{
> +	dell_privacy_wmi_exit();
> +	dell_privacy_acpi_exit();
> +}
> +
> +module_init(init_dell_privacy);
> +module_exit(exit_dell_privacy);
> +
> +MODULE_DEVICE_TABLE(wmi, dell_wmi_privacy_wmi_id_table);
> +MODULE_AUTHOR("Perry Yuan <[e-mail]>");
> +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..9fa01d084f7d
> --- /dev/null
> +++ b/drivers/platform/x86/dell-privacy-wmi.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Dell privacy notification driver
> + *
> + * Copyright (C) 2021 Dell Inc. All Rights Reserved.
> + */
> +
> +#ifndef _DELL_PRIVACY_WMI_H_
> +#define _DELL_PRIVACY_WMI_H_
> +
> +#if IS_ENABLED(CONFIG_DELL_PRIVACY)
> +int  dell_privacy_valid(void);
> +void dell_privacy_process_event(int type, int code, int status);
> +#else /* CONFIG_DELL_PRIVACY */
> +static inline int dell_privacy_valid(void)
> +{
> +	return  -ENODEV;
> +}
> +
> +static inline void dell_privacy_process_event(int type, int code, int status)
> +{}
> +#endif /* CONFIG_DELL_PRIVACY */
> +
> +int  dell_privacy_acpi_init(void);
> +void dell_privacy_acpi_exit(void);
> +
> +/* 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..4b22bd21fc42 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 <[e-mail]>");
>  MODULE_AUTHOR("Pali Rohár <[e-mail]>");
> @@ -381,6 +382,7 @@ static void dell_wmi_notify(struct wmi_device *wdev,
>  	u16 *buffer_entry, *buffer_end;
>  	acpi_size buffer_size;
>  	int len, i;
> +	int err;
>
>  	if (obj->type != ACPI_TYPE_BUFFER) {
>  		pr_warn("bad response type %x\n", obj->type);
> @@ -427,18 +429,30 @@ static void dell_wmi_notify(struct wmi_device *wdev,
>
>  		switch (buffer_entry[1]) {
>  		case 0x0000: /* One key pressed or event occurred */
> -		case 0x0012: /* Event with extended data occurred */
> -			if (len > 2)
> -				dell_wmi_process_key(wdev, buffer_entry[1],
> -						     buffer_entry[2]);
> -			/* 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;
> +		case 0x0012:

The comment "Event with extended data occurred" has been deleted.


> +#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]);

What if `len < 5`?


> +			} 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]);
> --
> 2.25.1


Regards,
Barnabás Pőcze

Hans de Goede Jan. 6, 2021, 11:43 p.m. UTC | #2

Hi Perry,

On 12/28/20 2:28 PM, Perry Yuan wrote:
> From: Perry Yuan <[e-mail]>
> 
> 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.
> 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 control.
> 
> *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.
> 
> Signed-off-by: Perry Yuan  <[e-mail]>
> Signed-off-by: Limonciello Mario <[e-mail]>

Thank you for your patch, please send a new version addressing
Barnabás' review comment and including the second patch of the
series.

Regards,

Hans


> ---
> v1 -> v2:
> * query EC handle from EC driver directly.
> * fix some code style.
> * add KEY_END to keymap array.
> * clean platform device when cleanup called
> * use hexadecimal format for log print in dev_dbg
> * remove __set_bit for the report keys from probe.
> * fix keymap leak
> * add err_free_keymap in dell_privacy_wmi_probe
> * wmi driver will be unregistered if privacy_acpi_init() fails
> * add sysfs attribute files for user space query.
> * add leds micmute driver to privacy acpi
> * add more design info the commit info
> ---
> ---
>  drivers/platform/x86/Kconfig             |  17 ++
>  drivers/platform/x86/Makefile            |   4 +-
>  drivers/platform/x86/dell-laptop.c       |  29 ++-
>  drivers/platform/x86/dell-privacy-acpi.c | 165 ++++++++++++
>  drivers/platform/x86/dell-privacy-wmi.c  | 309 +++++++++++++++++++++++
>  drivers/platform/x86/dell-privacy-wmi.h  |  33 +++
>  drivers/platform/x86/dell-wmi.c          |  26 +-
>  7 files changed, 567 insertions(+), 16 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 91e6176cdfbd..9d5cc2454b3e 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -491,6 +491,23 @@ 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
> +	depends on LEDS_TRIGGER_AUDIO
> +	select DELL_WMI
> +	help
> +	This driver provides support for the "Dell Hardware Privacy" feature
> +	of Dell laptops.
> +	Support for a micmute and camera mute privacy will be provided as
> +	hardware button Ctrl+F4 and Ctrl+F9 hotkey
> +
> +	To compile this driver as a module, choose M here: the module will
> +	be called dell_privacy.
> +
>  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 581475f59819..18c430456de7 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -51,7 +51,9 @@ 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_WMI_SYSMAN)		+= dell-wmi-sysman/
> -
> +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 70edc5bb3a14..ea0c8a8099ff 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");
> @@ -1587,10 +1589,10 @@ static ssize_t kbd_led_timeout_store(struct device *dev,
>  		switch (unit) {
>  		case KBD_TIMEOUT_DAYS:
>  			value *= 24;
> -			fallthrough;
> +			/* fall through */
>  		case KBD_TIMEOUT_HOURS:
>  			value *= 60;
> -			fallthrough;
> +			/* fall through */
>  		case KBD_TIMEOUT_MINUTES:
>  			value *= 60;
>  			unit = KBD_TIMEOUT_SECONDS;
> @@ -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) {
> +			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..fef781555b67
> --- /dev/null
> +++ b/drivers/platform/x86/dell-privacy-acpi.c
> @@ -0,0 +1,165 @@
> +// 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/bits.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.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;
> +	if (acpi_has_method(handle, "ECAK"))
> +		acpi_method = "ECAK";
> +	else
> +		return -ENODEV;
> +
> +	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);
> +	dev_set_drvdata(&pdev->dev, NULL);
> +
> +	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 void dell_privacy_leds_setup(struct device *dev)
> +{
> +	struct privacy_acpi_priv *priv = dev_get_drvdata(dev);
> +
> +	priv->cdev.name = "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);
> +	priv->cdev.dev = dev;
> +	devm_led_classdev_register(dev, &priv->cdev);
> +}
> +
> +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),
> +	},
> +	.remove = dell_privacy_acpi_remove,
> +};
> +
> +int dell_privacy_acpi_init(void)
> +{
> +	int err;
> +	struct platform_device *pdev;
> +	int privacy_capable = wmi_has_guid(DELL_PRIVACY_GUID);
> +
> +	if (!privacy_capable)
> +		return -ENODEV;
> +
> +	privacy_acpi = kzalloc(sizeof(struct privacy_acpi_priv), GFP_KERNEL);
> +	if (!privacy_acpi)
> +		return -ENOMEM;
> +
> +	pdev = platform_device_register_simple(
> +			PRIVACY_PLATFORM_NAME, -1, 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;
> +
> +	dell_privacy_leds_setup(&pdev->dev);
> +
> +	return 0;
> +
> +pdrv_err:
> +	platform_device_unregister(pdev);
> +pdev_err:
> +	kfree(privacy_acpi);
> +	return err;
> +}
> +
> +void 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 <[e-mail]>");
> +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..80637c7f617c
> --- /dev/null
> +++ b/drivers/platform/x86/dell-privacy-wmi.c
> @@ -0,0 +1,309 @@
> +// 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 } },
> +	{ 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;
> +}
> +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);
> +	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 0x%04x 0x%04x",
> +					type, code);
> +	}
> +error:
> +	mutex_unlock(&list_mutex);
> +}
> +EXPORT_SYMBOL_GPL(dell_privacy_process_event);
> +
> +static ssize_t devices_supported_show(struct device *dev,
> +		struct device_attribute *attr,
> +		char *buf)
> +{
> +	struct privacy_wmi_data *priv = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%d\n", priv->features_present);
> +}
> +
> +static ssize_t current_state_show(struct device *dev,
> +		struct device_attribute *attr,
> +		char *buf)
> +{
> +	struct privacy_wmi_data *priv = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%d\n", priv->last_status);
> +}
> +
> +static DEVICE_ATTR_RO(devices_supported);
> +static DEVICE_ATTR_RO(current_state);
> +
> +static struct attribute *platform_attributes[] = {
> +	&dev_attr_devices_supported.attr,
> +	&dev_attr_current_state.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group privacy_attribute_group = {
> +	.attrs = platform_attributes
> +};
> +
> +/*
> + * Describes the Device State class exposed by BIOS which can be consumed by
> + * various applications interested in knowing the Privacy feature capabilities.
> + * class DeviceState
> + * {
> + *  [key, read] string InstanceName;
> + *  [read] boolean ReadOnly;
> + *  [WmiDataId(1), read] uint32 DevicesSupported;
> + *   0 – None, 0x1 – Microphone, 0x2 – Camera, 0x4 -ePrivacy  Screen
> + *  [WmiDataId(2), read] uint32 CurrentState;
> + *   0:Off; 1:On. Bit0 – Microphone, Bit1 – Camera, Bit2 - ePrivacyScreen
> + * };
> + */
> +
> +static int get_current_status(struct wmi_device *wdev)
> +{
> +	struct privacy_wmi_data *priv = dev_get_drvdata(&wdev->dev);
> +	union acpi_object *obj_present = NULL;
> +	u32 *buffer;
> +	int ret = 0;
> +
> +	if (priv == NULL) {
> +		pr_err("dell privacy priv is NULL\n");
> +		return -EINVAL;
> +	}
> +	/* check privacy support features and device states */
> +	obj_present = wmidev_block_query(wdev, 0);
> +	if (obj_present->type != ACPI_TYPE_BUFFER) {
> +		dev_err(&wdev->dev, "Dell privacy failed to get device status!\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 = -ENODEV;
> +		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(struct privacy_wmi_data),
> +			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,
> +			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_keymap;
> +	ret = sysfs_create_group(&wdev->dev.kobj, &privacy_attribute_group);
> +	if (ret)
> +		goto err_free_keymap;
> +	return 0;
> +
> +err_free_keymap:
> +	privacy_valid = -ENODEV;
> +	kfree(keymap);
> +err_free_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;
> +	sysfs_remove_group(&wdev->dev.kobj, &privacy_attribute_group);
> +
> +	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;
> +
> +	ret = wmi_has_guid(DELL_PRIVACY_GUID);
> +	if (!ret)
> +		return -ENODEV;
> +
> +	ret = dell_privacy_acpi_init();
> +	if (ret) {
> +		pr_err("failed to initialize privacy acpi driver: %d\n", ret);
> +		goto err_init;
> +	}
> +
> +	ret = wmi_driver_register(&dell_privacy_wmi_driver);
> +	if (ret) {
> +		pr_err("failed to initialize privacy wmi driver: %d\n", ret);
> +		return ret;
> +	}
> +	return 0;
> +
> +err_init:
> +	wmi_driver_unregister(&dell_privacy_wmi_driver);
> +	return ret;
> +}
> +
> +static void dell_privacy_wmi_exit(void)
> +{
> +	wmi_driver_unregister(&dell_privacy_wmi_driver);
> +}
> +
> +static void __exit exit_dell_privacy(void)
> +{
> +	dell_privacy_wmi_exit();
> +	dell_privacy_acpi_exit();
> +}
> +
> +module_init(init_dell_privacy);
> +module_exit(exit_dell_privacy);
> +
> +MODULE_DEVICE_TABLE(wmi, dell_wmi_privacy_wmi_id_table);
> +MODULE_AUTHOR("Perry Yuan <[e-mail]>");
> +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..9fa01d084f7d
> --- /dev/null
> +++ b/drivers/platform/x86/dell-privacy-wmi.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Dell privacy notification driver
> + *
> + * Copyright (C) 2021 Dell Inc. All Rights Reserved.
> + */
> +
> +#ifndef _DELL_PRIVACY_WMI_H_
> +#define _DELL_PRIVACY_WMI_H_
> +
> +#if IS_ENABLED(CONFIG_DELL_PRIVACY)
> +int  dell_privacy_valid(void);
> +void dell_privacy_process_event(int type, int code, int status);
> +#else /* CONFIG_DELL_PRIVACY */
> +static inline int dell_privacy_valid(void)
> +{
> +	return  -ENODEV;
> +}
> +
> +static inline void dell_privacy_process_event(int type, int code, int status)
> +{}
> +#endif /* CONFIG_DELL_PRIVACY */
> +
> +int  dell_privacy_acpi_init(void);
> +void dell_privacy_acpi_exit(void);
> +
> +/* 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..4b22bd21fc42 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 <[e-mail]>");
>  MODULE_AUTHOR("Pali Rohár <[e-mail]>");
> @@ -381,6 +382,7 @@ static void dell_wmi_notify(struct wmi_device *wdev,
>  	u16 *buffer_entry, *buffer_end;
>  	acpi_size buffer_size;
>  	int len, i;
> +	int err;
>  
>  	if (obj->type != ACPI_TYPE_BUFFER) {
>  		pr_warn("bad response type %x\n", obj->type);
> @@ -427,18 +429,30 @@ static void dell_wmi_notify(struct wmi_device *wdev,
>  
>  		switch (buffer_entry[1]) {
>  		case 0x0000: /* One key pressed or event occurred */
> -		case 0x0012: /* Event with extended data occurred */
> -			if (len > 2)
> -				dell_wmi_process_key(wdev, buffer_entry[1],
> -						     buffer_entry[2]);
> -			/* 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;
> +		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]);
> +			}
> +#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]);
>

Barnabás Pőcze Jan. 7, 2021, 4:03 p.m. UTC | #3

Hi


2021. január 7., csütörtök 0:43 keltezéssel, Hans de Goede írta:

> Hi Perry,
>
> On 12/28/20 2:28 PM, Perry Yuan wrote:
>
> > From: Perry Yuan [e-mail]
> > 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.
> > 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 control.
> > *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.
> >
> >
> > Signed-off-by: Perry Yuan [e-mail]
> > Signed-off-by: Limonciello Mario [e-mail]
>
> Thank you for your patch, please send a new version addressing
> Barnabás' review comment and including the second patch of the
> series.
> [...]

I think first something needs to be figured out regarding
the integration with the rest of the Dell modules. I feel
that list is not a desirable way to do it.


Regards,
Barnabás Pőcze

Yuan, Perry Jan. 8, 2021, 5:04 p.m. UTC | #4

Dell Customer Communication - Confidential

> -----Original Message-----
> From: Barnabás Pőcze <[e-mail]>
> Sent: 2021年1月8日 0:04
> To: Hans de Goede
> Cc: Yuan, Perry; [e-mail]; platform-driver-
> [e-mail]; [e-mail]; Limonciello, Mario
> Subject: Re: [PATCH v2 1/2] platform/x86: dell-privacy: Add support for Dell
> hardware privacy
> 
> 
> [EXTERNAL EMAIL]
> 
> Hi
> 
> 
> 2021. január 7., csütörtök 0:43 keltezéssel, Hans de Goede írta:
> 
> > Hi Perry,
> >
> > On 12/28/20 2:28 PM, Perry Yuan wrote:
> >
> > > From: Perry Yuan [e-mail] 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.
> > > 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 control.
> > > *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.
> > >
> > >
> > > Signed-off-by: Perry Yuan [e-mail]
> > > Signed-off-by: Limonciello Mario [e-mail]
> >
> > Thank you for your patch, please send a new version addressing
> > Barnabás' review comment and including the second patch of the series.
> > [...]
> 
> I think first something needs to be figured out regarding the integration with
> the rest of the Dell modules. I feel that list is not a desirable way to do it.
> 
> 
> Regards,
> Barnabás Pőcze

Hi Barnabás, Hans.
Thanks for your review and comments.
Before I send V3, I would explain some detail why we have two files for the dell privacy arch design.

Firstly, all these privacy feature are hardware level privacy solution.  
Dell is going to implement  some other dell privacy devices using interface acpi and  wmi differently.
such as electronic privacy display, privacy camera, and other privacy technology that is coming soon.
we need to have dell-privacy-acpi and dell-privacy-wmi files to handle the privacy device interaction with BIOS and EC(embedded controller)
  - the dell-privacy-wmi file handle the wmi event from BIOS and emit it to userspace , it is wmi interface related driver file.
  - the dell-privacy-acpi interact with ACPI interface ,dell privacy feature need to send ACK content to EC  through ACPI interface.
  - other privacy device function will be developed according to the interface type. the two driver files will be extended for new devices.

Meanwhile, actually we will consider to add one ACPI device to BIOS DSDT table in future , then dell-privacy-acpi driver can register to kernel matching that device with specific ACPI device ID.

If you have any other concerns for arch design , Mario could help to explain some more details to get this clear.

Limonciello, Mario Jan. 8, 2021, 5:06 p.m. UTC | #5

Dell Customer Communication - Confidential

Perry,

For discussion in public mailing lists you'll have to make sure that you get
Dell's mail service to turn off this clause and resend:

> 
> Dell Customer Communication - Confidential
>

Yuan, Perry Jan. 8, 2021, 5:34 p.m. UTC | #6

> -----Original Message-----
> From: Barnabás Pőcze <[e-mail]>
> Sent: 2021年1月8日 0:04
> To: Hans de Goede
> Cc: Yuan, Perry; [e-mail]; platform-driver-
> [e-mail]; [e-mail]; Limonciello, Mario
> Subject: Re: [PATCH v2 1/2] platform/x86: dell-privacy: Add support for Dell
> hardware privacy
> 
> 
> [EXTERNAL EMAIL]
> 
> Hi
> 
> 
> 2021. január 7., csütörtök 0:43 keltezéssel, Hans de Goede írta:
> 
> > Hi Perry,
> >
> > On 12/28/20 2:28 PM, Perry Yuan wrote:
> >
> > > From: Perry Yuan [e-mail] 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.
> > > 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 control.
> > > *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.
> > >
> > >
> > > Signed-off-by: Perry Yuan [e-mail]
> > > Signed-off-by: Limonciello Mario [e-mail]
> >
> > Thank you for your patch, please send a new version addressing
> > Barnabás' review comment and including the second patch of the series.
> > [...]
> 
> I think first something needs to be figured out regarding the integration with
> the rest of the Dell modules. I feel that list is not a desirable way to do it.
> 
> 
> Regards,
> Barnabás Pőcze

Hi Barnabás, Hans.
Thanks for your review and comments.
Before I send V3, I would explain some detail why we have two files for the dell privacy arch design.

Firstly, all these privacy feature are hardware level privacy solution.  
Dell is going to implement  some other dell privacy devices using interface acpi and  wmi differently.
such as electronic privacy display, privacy camera, and other privacy technology that is coming soon.
we need to have dell-privacy-acpi and dell-privacy-wmi files to handle the privacy device interaction with BIOS and EC(embedded controller)
  - the dell-privacy-wmi file handle the wmi event from BIOS and emit it to userspace , it is wmi interface related driver file.
  - the dell-privacy-acpi interact with ACPI interface ,dell privacy feature need to send ACK content to EC  through ACPI interface.
  - other privacy device function will be developed according to the interface type. the two driver files will be extended for new devices.

If you have any other concerns for arch design , Mario could help to explain some more details to get this clear.

Perry  Yuan
Dell | Client Software Group | CDC Linux OS

Barnabás Pőcze Jan. 11, 2021, 4:07 p.m. UTC | #7

Hi


2021. január 11., hétfő 14:42 keltezéssel, Perry Yuan írta:

> [...]
> > > +#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;
> >
> > Any reason it needs to be dynamically allocated?
>
> I need to set one static struct to let some function to get the priv pointer.
>
> It is more simple if i add some function and get the priv easily.
>
> If you have better suggestion, i would be glad to optimize it.
>
> for example.
>
> static int dell_privacy_micmute_led_set(struct led_classdev *led_cdev,
>                 enum led_brightness brightness)
> {
>         struct privacy_acpi_priv *priv = privacy_acpi;
>
>         ..........................
>
> }
>

My comment was referring to the fact that you could've used
```
static struct privacy_acpi_priv privacy_acpi;
```
to achieve the same result without the dynamic memory allocation.


>
> >
> >
> > > +
> > > +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;
> > > +	if (acpi_has_method(handle, "ECAK"))
> > > +		acpi_method = "ECAK";
> > > +	else
> > > +		return -ENODEV;
> >
> > I find this if-else a bit cumbersome. Any reason why
> >
> > if (!acpi_has_method(handle, "ECAK"))
> >   return ...;
> >
> > would not work? I believe you could also easily do away with the `acpi_method`
> > variable.
>
> Just want to make sure the BIOS has the correct  method to call.
>
> normally it will not be failed, just keep it safe to call BIOS, not to cause any panic.
>
> I changed it as below .
>
>         handle = ec_get_handle();
>         if (!handle)
>                 return -EIO;
>         acpi_method = "ECAK";
>

You could keep the method name in a static variable:
```
static char *acpi_method = (char *)"ECAK"; // this is inside the function

handle = ...;
if (!handle)
  return ...

if (!acpi_has_method(handle, acpi_method))
  return ...

... acpi_evaluate_object(handle, acpi_method, ...
```

Another thing is that I believe you could do these checks only once,
before registering the LED device.


>
> >
> >
> > > +
> > > +	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 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),
> > > +	},
> > > +	.remove = dell_privacy_acpi_remove,
> > > +};
> >
> > I think using a platform driver here just complicates things for no reason.
> > Furthermore, I'm not sure if there's actually any need for the ACPI match table.
>
> there will be some more privacy devices need to add some acpi interface function here.
>
> including  elctronic privacy screen and privacy camera.
>
> the platform driver can provide more flexible framework to extend.
>

I see. I'm wondering if the ACPI match table is needed at the moment. If I'm not
mistaken the platform driver already binds the platform device the module creates.
And there is no real need to bind to the ACPI EC devices.


> [...]
> > > diff --git a/drivers/platform/x86/dell-privacy-wmi.c b/drivers/platform/x86/dell-privacy-wmi.c
> > > new file mode 100644
> > > index 000000000000..80637c7f617c
> > > --- /dev/null
> > > +++ b/drivers/platform/x86/dell-privacy-wmi.c
> [...]
> > > +int dell_privacy_valid(void)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = wmi_has_guid(DELL_PRIVACY_GUID);
> > > +	if (!ret)
> > > +		return -ENODEV;
> > > +	ret = privacy_valid;
> > > +	return ret;
> >
> > I find this function really confusing, and too verbose for what it does.
> >
>
> 1. when the DELL_PRIVACY_GUID not found, it will return  ENODEV showing no privacy devices.
>
> 2. when DELL_PRIVACY_GUID found,and wmi privacy driver is registered, it will return "0"
>
> 3.when DELL_PRIVACY_GUID found,and wmi privacy driver is NOT registered yet, it will return "EPROBE_DEFE"
>
>   the  EPROBE_DEFER is defined in "include/linux/errno.h"
>
> #define EPROBE_DEFER    517     /* Driver requests probe retry */
>
> when caller get this returned , it will defer the caller  1s ~ 7s to probe again.
>
> This will make sure dell-laptop can get the correct privacy status to register its micmute led trigger driver or not.
>
> ----dell-laptoo.c-------------------
>
> #if IS_ENABLED(CONFIG_DELL_PRIVACY)
>                 ret = dell_privacy_valid();
>                 if (!ret)
>                         privacy_valid = true;
> #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;
>                 }
>         }
>

My problem is that those three states are never used.
The only thing that's ever checked is if the return value of dell_privacy_valid()
is zero or not. No part of the code distinguishes between -ENODEV and -EPROBE_DEFER.
(As far as I see, I may be missing something.)

My initial point was that - if I'm not missing anything significant - the whole
function could just be:
```
int dell_privacy_valid(void)
{
  return privacy_valid;
}
```
given that you manipulate the value of `privacy_valid` appropriately. And assuming
the current form is needed, this would be enough:
```
int dell_privacy_valid(void)
{
  if (!wmi_has_guid(...))
    return -ENODEV;

  return privacy_valid;
}
```

But I'd personally prefer the first one.


>
> >
> > > +}
> > > +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);
> >
> > Can you please explain why this list is needed if only the first entry will
> > ever be used?
>
>         mutex_lock(&list_mutex);
>         list_add_tail(&priv->list, &wmi_list);
>         mutex_unlock(&list_mutex);
>
> only one priv struct is added to the list with list mutex protecting.
>
> So it will not have two more entry data added to the list .
>

I'm not sure I fully get it.


>
> >
> >
> > > +	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 0x%04x 0x%04x",
> > > +					type, code);
> > > +	}
> >
> > Is this switch needed at all?
>
> It is needed here, camra mute and privacy screen will add more codes here.
>
> It want to keep the switch for further extention.
>

My point here is that the two cases of the switch do the exact same thing. The
whole switch could be replaced with:
```
sparse_keymap_report_entry(priv->input_dev, key, 1, true);
```

I would go as far as use this:
```
if (!sparse_keymap_report_event(priv->input_dev, (type << 16) | code, 1, true))
  dev_dbg(&priv->wdev->dev, "unknown event type=0x%04x code=0x%04x\n", type, code)
```

This would elliminate the need for the `key` variable, the call to
`sparse_keymap_entry_from_scancode()`, and the whole switch.


> [...]
> > There is actually no need for the `pos` variable.
>
> It is used in this keymap codes.
>
>        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++;
>         }
>

You can use `i` as the index, so no need for `pos`.


>
> >
> >
> > > +
> > > +	priv = devm_kzalloc(&wdev->dev, sizeof(struct privacy_wmi_data),
> >
> > Please use `sizeof(*priv)`.
>
> fixed.
>
>
> >
> >
> > > +			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,
> >
> > I don't think that `+ 1` is not needed since the KE_END entry is already in the array.
> >
> >
> > > +			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;
> >
> > A copy of the keymap is created by `sparse_keymap_setup()`, so returning
> > here will leak `keymap`. You could just call `kfree(keymap)` directly after
> > the `sparse_keymap_setup()` call. But I find it completely unnecessary
> > to do this allocate-copy-modify thing. Is there any reason why the static array
> > (`dell_wmi_keymap_type_0012`) cannot already contain the correct values?
>
> I don`t quite undestand what you meant
>
> what "correct values" should be contained by the dell_wmi_keymap_type_0012?
>

My point is that if you had:
```
static const struct key_entry dell_wmi_keymap_type_0012[] = {
	{ KE_KEY, 0x00120001, { KEY_MICMUTE } },
	{ KE_SW,  0x00120002, { SW_CAMERA_LENS_COVER } },
	{ KE_END, 0},
};
```
then there would be no need to create a copy and the code would be simpler.


> [...]
> > > +static int __init init_dell_privacy(void)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = wmi_has_guid(DELL_PRIVACY_GUID);
> > > +	if (!ret)
> > > +		return -ENODEV;
> >
> > The init function of a module that exports symbols must not fail, otherwise
> > it'll prevent the loading of dependent modules.
>
> Yes, the wmi driver is the entry for whole privacy driver.
>
> if wmi guid query failed, the privacy driver will not be registered.
>
> dell-laptop driver will get "-ENOVDE" from dell_privacy_valid().
>
> it will register its micmute led trigger driver from dell laptop driver.
>

My point is that if dell-privacy is a dependency of dell-laptop, and dell-privacy
fails to load, then dell-laptop cannot be loaded. Effectively, the lack of
the DELL_PRIVACY_GUID WMI interface will cause the dell-laptop module not be
able to be loaded.


> [...]
> > > +#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]);
> >
> > What if `len < 5`?
>
> when CONFIG_DELL_PRIVACY is enabled, and dell_privacy_valid return zero which means privacy driver loaded as expected.
>
> for example ,the micmute report wmi event data len is "5", it will not be less than 5 words.

I'm wondering if there are such guarantees, why is the length checked just a
couple lines below?


>
> Process buffer (04 00 12 00 0e 00 01 00 03 00)
>
>
> #if IS_ENABLED(CONFIG_DELL_PRIVACY)

One thing I might have forgotten to point out initially, is that there is no need
for this #if as dell-privacy-wmi.h provides stub definitions for
`dell_privacy_valid()` and `dell_privacy_process_event()`.


>                         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 */
>                         }
> [...]

Another thing I may have forgotten to say: the name `dell_privacy_valid()` is
misleading in my opinion, as it gives the impression of being a predicate, even
though it is not. `dell_privacy_state()` or something like that would be better,
I think.


Regards,
Barnabás Pőcze


p.s. please send text emails.

Perry Yuan Jan. 12, 2021, 5:26 p.m. UTC | #8

Hi Barnabás,

On 2021/1/12 0:07, Barnabás Pőcze wrote:
> Hi
>
>
> 2021. január 11., hétfő 14:42 keltezéssel, Perry Yuan írta:
>
>> [...]
>>>> +#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;
>>> Any reason it needs to be dynamically allocated?
>> I need to set one static struct to let some function to get the priv pointer.
>>
>> It is more simple if i add some function and get the priv easily.
>>
>> If you have better suggestion, i would be glad to optimize it.
>>
>> for example.
>>
>> static int dell_privacy_micmute_led_set(struct led_classdev *led_cdev,
>>                  enum led_brightness brightness)
>> {
>>          struct privacy_acpi_priv *priv = privacy_acpi;
>>
>>          ..........................
>>
>> }
>>
> My comment was referring to the fact that you could've used
> ```
> static struct privacy_acpi_priv privacy_acpi;
> ```
> to achieve the same result without the dynamic memory allocation.
>
>
>>>
>>>> +
>>>> +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;
>>>> +	if (acpi_has_method(handle, "ECAK"))
>>>> +		acpi_method = "ECAK";
>>>> +	else
>>>> +		return -ENODEV;
>>> I find this if-else a bit cumbersome. Any reason why
>>>
>>> if (!acpi_has_method(handle, "ECAK"))
>>>    return ...;
>>>
>>> would not work? I believe you could also easily do away with the `acpi_method`
>>> variable.
>> Just want to make sure the BIOS has the correct  method to call.
>>
>> normally it will not be failed, just keep it safe to call BIOS, not to cause any panic.
>>
>> I changed it as below .
>>
>>          handle = ec_get_handle();
>>          if (!handle)
>>                  return -EIO;
>>          acpi_method = "ECAK";
>>
> You could keep the method name in a static variable:
> ```
> static char *acpi_method = (char *)"ECAK"; // this is inside the function
>
> handle = ...;
> if (!handle)
>    return ...
>
> if (!acpi_has_method(handle, acpi_method))
>    return ...
>
> ... acpi_evaluate_object(handle, acpi_method, ...
> ```
>
> Another thing is that I believe you could do these checks only once,
> before registering the LED device.
>
>
>>>
>>>> +
>>>> +	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 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),
>>>> +	},
>>>> +	.remove = dell_privacy_acpi_remove,
>>>> +};
>>> I think using a platform driver here just complicates things for no reason.
>>> Furthermore, I'm not sure if there's actually any need for the ACPI match table.
>> there will be some more privacy devices need to add some acpi interface function here.
>>
>> including  elctronic privacy screen and privacy camera.
>>
>> the platform driver can provide more flexible framework to extend.
>>
> I see. I'm wondering if the ACPI match table is needed at the moment. If I'm not
> mistaken the platform driver already binds the platform device the module creates.
> And there is no real need to bind to the ACPI EC devices.
>
>
>> [...]
>>>> diff --git a/drivers/platform/x86/dell-privacy-wmi.c b/drivers/platform/x86/dell-privacy-wmi.c
>>>> new file mode 100644
>>>> index 000000000000..80637c7f617c
>>>> --- /dev/null
>>>> +++ b/drivers/platform/x86/dell-privacy-wmi.c
>> [...]
>>>> +int dell_privacy_valid(void)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	ret = wmi_has_guid(DELL_PRIVACY_GUID);
>>>> +	if (!ret)
>>>> +		return -ENODEV;
>>>> +	ret = privacy_valid;
>>>> +	return ret;
>>> I find this function really confusing, and too verbose for what it does.
>>>
>> 1. when the DELL_PRIVACY_GUID not found, it will return  ENODEV showing no privacy devices.
>>
>> 2. when DELL_PRIVACY_GUID found,and wmi privacy driver is registered, it will return "0"
>>
>> 3.when DELL_PRIVACY_GUID found,and wmi privacy driver is NOT registered yet, it will return "EPROBE_DEFE"
>>
>>    the  EPROBE_DEFER is defined in "include/linux/errno.h"
>>
>> #define EPROBE_DEFER    517     /* Driver requests probe retry */
>>
>> when caller get this returned , it will defer the caller  1s ~ 7s to probe again.
>>
>> This will make sure dell-laptop can get the correct privacy status to register its micmute led trigger driver or not.
>>
>> ----dell-laptoo.c-------------------
>>
>> #if IS_ENABLED(CONFIG_DELL_PRIVACY)
>>                  ret = dell_privacy_valid();
>>                  if (!ret)
>>                          privacy_valid = true;
>> #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;
>>                  }
>>          }
>>
> My problem is that those three states are never used.
> The only thing that's ever checked is if the return value of dell_privacy_valid()
> is zero or not. No part of the code distinguishes between -ENODEV and -EPROBE_DEFER.
> (As far as I see, I may be missing something.)
>
> My initial point was that - if I'm not missing anything significant - the whole
> function could just be:
> ```
> int dell_privacy_valid(void)
> {
>    return privacy_valid;
> }
> ```
> given that you manipulate the value of `privacy_valid` appropriately. And assuming
> the current form is needed, this would be enough:
> ```
> int dell_privacy_valid(void)
> {
>    if (!wmi_has_guid(...))
>      return -ENODEV;
>
>    return privacy_valid;
> }
> ```
>
> But I'd personally prefer the first one.
>
>
>>>> +}
>>>> +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);
>>> Can you please explain why this list is needed if only the first entry will
>>> ever be used?
>>          mutex_lock(&list_mutex);
>>          list_add_tail(&priv->list, &wmi_list);
>>          mutex_unlock(&list_mutex);
>>
>> only one priv struct is added to the list with list mutex protecting.
>>
>> So it will not have two more entry data added to the list .
>>
> I'm not sure I fully get it.
>
>
>>>
>>>> +	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 0x%04x 0x%04x",
>>>> +					type, code);
>>>> +	}
>>> Is this switch needed at all?
>> It is needed here, camra mute and privacy screen will add more codes here.
>>
>> It want to keep the switch for further extention.
>>
> My point here is that the two cases of the switch do the exact same thing. The
> whole switch could be replaced with:
> ```
> sparse_keymap_report_entry(priv->input_dev, key, 1, true);
> ```
>
> I would go as far as use this:
> ```
> if (!sparse_keymap_report_event(priv->input_dev, (type << 16) | code, 1, true))
>    dev_dbg(&priv->wdev->dev, "unknown event type=0x%04x code=0x%04x\n", type, code)
> ```
>
> This would elliminate the need for the `key` variable, the call to
> `sparse_keymap_entry_from_scancode()`, and the whole switch.
>
>
>> [...]
>>> There is actually no need for the `pos` variable.
>> It is used in this keymap codes.
>>
>>         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++;
>>          }
>>
> You can use `i` as the index, so no need for `pos`.
>
>
>>>
>>>> +
>>>> +	priv = devm_kzalloc(&wdev->dev, sizeof(struct privacy_wmi_data),
>>> Please use `sizeof(*priv)`.
>> fixed.
>>
>>
>>>
>>>> +			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,
>>> I don't think that `+ 1` is not needed since the KE_END entry is already in the array.
>>>
>>>
>>>> +			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;
>>> A copy of the keymap is created by `sparse_keymap_setup()`, so returning
>>> here will leak `keymap`. You could just call `kfree(keymap)` directly after
>>> the `sparse_keymap_setup()` call. But I find it completely unnecessary
>>> to do this allocate-copy-modify thing. Is there any reason why the static array
>>> (`dell_wmi_keymap_type_0012`) cannot already contain the correct values?
>> I don`t quite undestand what you meant
>>
>> what "correct values" should be contained by the dell_wmi_keymap_type_0012?
>>
> My point is that if you had:
> ```
> static const struct key_entry dell_wmi_keymap_type_0012[] = {
> 	{ KE_KEY, 0x00120001, { KEY_MICMUTE } },
> 	{ KE_SW,  0x00120002, { SW_CAMERA_LENS_COVER } },
> 	{ KE_END, 0},
> };
> ```
> then there would be no need to create a copy and the code would be simpler.
>
>
>> [...]
>>>> +static int __init init_dell_privacy(void)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	ret = wmi_has_guid(DELL_PRIVACY_GUID);
>>>> +	if (!ret)
>>>> +		return -ENODEV;
>>> The init function of a module that exports symbols must not fail, otherwise
>>> it'll prevent the loading of dependent modules.
>> Yes, the wmi driver is the entry for whole privacy driver.
>>
>> if wmi guid query failed, the privacy driver will not be registered.
>>
>> dell-laptop driver will get "-ENOVDE" from dell_privacy_valid().
>>
>> it will register its micmute led trigger driver from dell laptop driver.
>>
> My point is that if dell-privacy is a dependency of dell-laptop, and dell-privacy
> fails to load, then dell-laptop cannot be loaded. Effectively, the lack of
> the DELL_PRIVACY_GUID WMI interface will cause the dell-laptop module not be
> able to be loaded.
>
>
>> [...]
>>>> +#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]);
>>> What if `len < 5`?
>> when CONFIG_DELL_PRIVACY is enabled, and dell_privacy_valid return zero which means privacy driver loaded as expected.
>>
>> for example ,the micmute report wmi event data len is "5", it will not be less than 5 words.
> I'm wondering if there are such guarantees, why is the length checked just a
> couple lines below?
>
>
>> Process buffer (04 00 12 00 0e 00 01 00 03 00)
>>
>>
>> #if IS_ENABLED(CONFIG_DELL_PRIVACY)
> One thing I might have forgotten to point out initially, is that there is no need
> for this #if as dell-privacy-wmi.h provides stub definitions for
> `dell_privacy_valid()` and `dell_privacy_process_event()`.
>
>
>>                          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 */
>>                          }
>> [...]
> Another thing I may have forgotten to say: the name `dell_privacy_valid()` is
> misleading in my opinion, as it gives the impression of being a predicate, even
> though it is not. `dell_privacy_state()` or something like that would be better,
> I think.
>
>
> Regards,
> Barnabás Pőcze
>
>
> p.s. please send text emails.

Barnabás, i will need to make above review codes changes to v4 soon 
,most of the review feedback has been made by V3 patch.

Thanks so much for your time and review effort.

BR.

Perry Yuan

Hans de Goede Jan. 12, 2021, 6:37 p.m. UTC | #9

Hi,

I know there already is a v3 out and I will try to get around to reviewing
that soon, still 1 remark about the discussion surrounding v2:

On 1/11/21 2:42 PM, Perry Yuan wrote:

<snip>

>>> *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.
>>>
>> Please proofread the commit message again, and pay attention to capitalization
>> and spacing.
> I want to reformat it and move the commit info to cover letter.

Please also put a copy of this as a comment in either the wmi or the
acpi driver (with a comment pointing to the comment in the other) this is
important info to have for someone reading the code and trying to understand
how this all fits together.

Regards,

Hans

Perry Yuan Jan. 15, 2021, 7:44 a.m. UTC | #10

On 2021/1/13 2:37, Hans de Goede wrote:

> Hi,
>
> I know there already is a v3 out and I will try to get around to reviewing
> that soon, still 1 remark about the discussion surrounding v2:
>
> On 1/11/21 2:42 PM, Perry Yuan wrote:
>
> <snip>
>
>>>> *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.
>>>>
>>> Please proofread the commit message again, and pay attention to capitalization
>>> and spacing.
>> I want to reformat it and move the commit info to cover letter.
> Please also put a copy of this as a comment in either the wmi or the
> acpi driver (with a comment pointing to the comment in the other) this is
> important info to have for someone reading the code and trying to understand
> how this all fits together.
>
> Regards,
>
> Hans

Hi Hans:

Agreed.

I will add this to the driver comments and explain how the acpi/wmi 
driver associated.

Perry Yuan Feb. 16, 2021, 7:24 a.m. UTC | #11

Hi Hans:

On 2021/1/13 2:37, Hans de Goede wrote:
> Hi,
> 
> I know there already is a v3 out and I will try to get around to reviewing
> that soon, still 1 remark about the discussion surrounding v2:
> 
> On 1/11/21 2:42 PM, Perry Yuan wrote:
> 
> <snip>
> 
>>>> *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.
>>>>
>>> Please proofread the commit message again, and pay attention to capitalization
>>> and spacing.
>> I want to reformat it and move the commit info to cover letter.
> 
> Please also put a copy of this as a comment in either the wmi or the
> acpi driver (with a comment pointing to the comment in the other) this is
> important info to have for someone reading the code and trying to understand
> how this all fits together.
> 
> Regards,
> 
> Hans
> 
Hans.
I have added the comments to the dell-privacy  driver file in V4

-----------------------------------------------------------------------------------
drivers/platform/x86/dell-privacy-wmi.c

EXPORT_SYMBOL_GPL(dell_privacy_valid);
/*
  * The flow of privacy event:
  * 1) User presses key. HW does stuff with this key (timeout is started)
  * 2) WMI event is emitted from BIOS
  * 3) WMI event is received by dell-privacy
  * 4) KEY_MICMUTE emitted from dell-privacy
  * 5) Userland picks up key and modifies kcontrol for SW mute
  * 6) Codec kernel driver catches and calls ledtrig_audio_set defined by
  *    dell-privacy-acpi driver.
  *    codec driver will call like this to switch micmute led state.
  *      ledtrig_audio_set(LED_AUDIO_MICMUTE, micmute_led ? LED_ON 
:LED_OFF);
  * 7) If "LED" is set to on dell-privacy notifies EC,and timeout is 
cancelled,
  *      HW mic mute activated.
  */
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);
....

-----------------------------------------------------------------------------------
drivers/platform/x86/dell-privacy-acpi.c

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

.....

Hans de Goede March 4, 2021, 10:49 a.m. UTC | #12

Hi,

On 2/16/21 8:24 AM, Perry Yuan wrote:
> Hi Hans:
> 
> On 2021/1/13 2:37, Hans de Goede wrote:
>> Hi,
>>
>> I know there already is a v3 out and I will try to get around to reviewing
>> that soon, still 1 remark about the discussion surrounding v2:
>>
>> On 1/11/21 2:42 PM, Perry Yuan wrote:
>>
>> <snip>
>>
>>>>> *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.
>>>>>
>>>> Please proofread the commit message again, and pay attention to capitalization
>>>> and spacing.
>>> I want to reformat it and move the commit info to cover letter.
>>
>> Please also put a copy of this as a comment in either the wmi or the
>> acpi driver (with a comment pointing to the comment in the other) this is
>> important info to have for someone reading the code and trying to understand
>> how this all fits together.
>>
>> Regards,
>>
>> Hans
>>
> Hans.
> I have added the comments to the dell-privacy  driver file in V4
> 
> -----------------------------------------------------------------------------------
> drivers/platform/x86/dell-privacy-wmi.c
> 
> EXPORT_SYMBOL_GPL(dell_privacy_valid);
> /*
>  * The flow of privacy event:
>  * 1) User presses key. HW does stuff with this key (timeout is started)
>  * 2) WMI event is emitted from BIOS
>  * 3) WMI event is received by dell-privacy
>  * 4) KEY_MICMUTE emitted from dell-privacy
>  * 5) Userland picks up key and modifies kcontrol for SW mute
>  * 6) Codec kernel driver catches and calls ledtrig_audio_set defined by
>  *    dell-privacy-acpi driver.
>  *    codec driver will call like this to switch micmute led state.
>  *      ledtrig_audio_set(LED_AUDIO_MICMUTE, micmute_led ? LED_ON :LED_OFF);
>  * 7) If "LED" is set to on dell-privacy notifies EC,and timeout is cancelled,
>  *      HW mic mute activated.
>  */
> 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);
> ....
> 
> -----------------------------------------------------------------------------------
> drivers/platform/x86/dell-privacy-acpi.c
> 
> /*
>  * 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;
> 
> .....
> 

This looks good, thank you.

Regards,

Hans

Patch

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 91e6176cdfbd..9d5cc2454b3e 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -491,6 +491,23 @@  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
+	depends on LEDS_TRIGGER_AUDIO
+	select DELL_WMI
+	help
+	This driver provides support for the "Dell Hardware Privacy" feature
+	of Dell laptops.
+	Support for a micmute and camera mute privacy will be provided as
+	hardware button Ctrl+F4 and Ctrl+F9 hotkey
+
+	To compile this driver as a module, choose M here: the module will
+	be called dell_privacy.
+
 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 581475f59819..18c430456de7 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -51,7 +51,9 @@  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_WMI_SYSMAN)		+= dell-wmi-sysman/
-
+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 70edc5bb3a14..ea0c8a8099ff 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");
@@ -1587,10 +1589,10 @@  static ssize_t kbd_led_timeout_store(struct device *dev,
 		switch (unit) {
 		case KBD_TIMEOUT_DAYS:
 			value *= 24;
-			fallthrough;
+			/* fall through */
 		case KBD_TIMEOUT_HOURS:
 			value *= 60;
-			fallthrough;
+			/* fall through */
 		case KBD_TIMEOUT_MINUTES:
 			value *= 60;
 			unit = KBD_TIMEOUT_SECONDS;
@@ -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) {
+			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..fef781555b67
--- /dev/null
+++ b/drivers/platform/x86/dell-privacy-acpi.c
@@ -0,0 +1,165 @@ 
+// 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/bits.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.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;
+	if (acpi_has_method(handle, "ECAK"))
+		acpi_method = "ECAK";
+	else
+		return -ENODEV;
+
+	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);
+	dev_set_drvdata(&pdev->dev, NULL);
+
+	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 void dell_privacy_leds_setup(struct device *dev)
+{
+	struct privacy_acpi_priv *priv = dev_get_drvdata(dev);
+
+	priv->cdev.name = "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);
+	priv->cdev.dev = dev;
+	devm_led_classdev_register(dev, &priv->cdev);
+}
+
+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),
+	},
+	.remove = dell_privacy_acpi_remove,
+};
+
+int dell_privacy_acpi_init(void)
+{
+	int err;
+	struct platform_device *pdev;
+	int privacy_capable = wmi_has_guid(DELL_PRIVACY_GUID);
+
+	if (!privacy_capable)
+		return -ENODEV;
+
+	privacy_acpi = kzalloc(sizeof(struct privacy_acpi_priv), GFP_KERNEL);
+	if (!privacy_acpi)
+		return -ENOMEM;
+
+	pdev = platform_device_register_simple(
+			PRIVACY_PLATFORM_NAME, -1, 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;
+
+	dell_privacy_leds_setup(&pdev->dev);
+
+	return 0;
+
+pdrv_err:
+	platform_device_unregister(pdev);
+pdev_err:
+	kfree(privacy_acpi);
+	return err;
+}
+
+void 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 <[e-mail]>");
+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..80637c7f617c
--- /dev/null
+++ b/drivers/platform/x86/dell-privacy-wmi.c
@@ -0,0 +1,309 @@ 
+// 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 } },
+	{ 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;
+}
+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);
+	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 0x%04x 0x%04x",
+					type, code);
+	}
+error:
+	mutex_unlock(&list_mutex);
+}
+EXPORT_SYMBOL_GPL(dell_privacy_process_event);
+
+static ssize_t devices_supported_show(struct device *dev,
+		struct device_attribute *attr,
+		char *buf)
+{
+	struct privacy_wmi_data *priv = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%d\n", priv->features_present);
+}
+
+static ssize_t current_state_show(struct device *dev,
+		struct device_attribute *attr,
+		char *buf)
+{
+	struct privacy_wmi_data *priv = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%d\n", priv->last_status);
+}
+
+static DEVICE_ATTR_RO(devices_supported);
+static DEVICE_ATTR_RO(current_state);
+
+static struct attribute *platform_attributes[] = {
+	&dev_attr_devices_supported.attr,
+	&dev_attr_current_state.attr,
+	NULL,
+};
+
+static const struct attribute_group privacy_attribute_group = {
+	.attrs = platform_attributes
+};
+
+/*
+ * Describes the Device State class exposed by BIOS which can be consumed by
+ * various applications interested in knowing the Privacy feature capabilities.
+ * class DeviceState
+ * {
+ *  [key, read] string InstanceName;
+ *  [read] boolean ReadOnly;
+ *  [WmiDataId(1), read] uint32 DevicesSupported;
+ *   0 – None, 0x1 – Microphone, 0x2 – Camera, 0x4 -ePrivacy  Screen
+ *  [WmiDataId(2), read] uint32 CurrentState;
+ *   0:Off; 1:On. Bit0 – Microphone, Bit1 – Camera, Bit2 - ePrivacyScreen
+ * };
+ */
+
+static int get_current_status(struct wmi_device *wdev)
+{
+	struct privacy_wmi_data *priv = dev_get_drvdata(&wdev->dev);
+	union acpi_object *obj_present = NULL;
+	u32 *buffer;
+	int ret = 0;
+
+	if (priv == NULL) {
+		pr_err("dell privacy priv is NULL\n");
+		return -EINVAL;
+	}
+	/* check privacy support features and device states */
+	obj_present = wmidev_block_query(wdev, 0);
+	if (obj_present->type != ACPI_TYPE_BUFFER) {
+		dev_err(&wdev->dev, "Dell privacy failed to get device status!\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 = -ENODEV;
+		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(struct privacy_wmi_data),
+			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,
+			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_keymap;
+	ret = sysfs_create_group(&wdev->dev.kobj, &privacy_attribute_group);
+	if (ret)
+		goto err_free_keymap;
+	return 0;
+
+err_free_keymap:
+	privacy_valid = -ENODEV;
+	kfree(keymap);
+err_free_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;
+	sysfs_remove_group(&wdev->dev.kobj, &privacy_attribute_group);
+
+	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;
+
+	ret = wmi_has_guid(DELL_PRIVACY_GUID);
+	if (!ret)
+		return -ENODEV;
+
+	ret = dell_privacy_acpi_init();
+	if (ret) {
+		pr_err("failed to initialize privacy acpi driver: %d\n", ret);
+		goto err_init;
+	}
+
+	ret = wmi_driver_register(&dell_privacy_wmi_driver);
+	if (ret) {
+		pr_err("failed to initialize privacy wmi driver: %d\n", ret);
+		return ret;
+	}
+	return 0;
+
+err_init:
+	wmi_driver_unregister(&dell_privacy_wmi_driver);
+	return ret;
+}
+
+static void dell_privacy_wmi_exit(void)
+{
+	wmi_driver_unregister(&dell_privacy_wmi_driver);
+}
+
+static void __exit exit_dell_privacy(void)
+{
+	dell_privacy_wmi_exit();
+	dell_privacy_acpi_exit();
+}
+
+module_init(init_dell_privacy);
+module_exit(exit_dell_privacy);
+
+MODULE_DEVICE_TABLE(wmi, dell_wmi_privacy_wmi_id_table);
+MODULE_AUTHOR("Perry Yuan <[e-mail]>");
+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..9fa01d084f7d
--- /dev/null
+++ b/drivers/platform/x86/dell-privacy-wmi.h
@@ -0,0 +1,33 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Dell privacy notification driver
+ *
+ * Copyright (C) 2021 Dell Inc. All Rights Reserved.
+ */
+
+#ifndef _DELL_PRIVACY_WMI_H_
+#define _DELL_PRIVACY_WMI_H_
+
+#if IS_ENABLED(CONFIG_DELL_PRIVACY)
+int  dell_privacy_valid(void);
+void dell_privacy_process_event(int type, int code, int status);
+#else /* CONFIG_DELL_PRIVACY */
+static inline int dell_privacy_valid(void)
+{
+	return  -ENODEV;
+}
+
+static inline void dell_privacy_process_event(int type, int code, int status)
+{}
+#endif /* CONFIG_DELL_PRIVACY */
+
+int  dell_privacy_acpi_init(void);
+void dell_privacy_acpi_exit(void);
+
+/* 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..4b22bd21fc42 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 <[e-mail]>");
 MODULE_AUTHOR("Pali Rohár <[e-mail]>");
@@ -381,6 +382,7 @@  static void dell_wmi_notify(struct wmi_device *wdev,
 	u16 *buffer_entry, *buffer_end;
 	acpi_size buffer_size;
 	int len, i;
+	int err;
 
 	if (obj->type != ACPI_TYPE_BUFFER) {
 		pr_warn("bad response type %x\n", obj->type);
@@ -427,18 +429,30 @@  static void dell_wmi_notify(struct wmi_device *wdev,
 
 		switch (buffer_entry[1]) {
 		case 0x0000: /* One key pressed or event occurred */
-		case 0x0012: /* Event with extended data occurred */
-			if (len > 2)
-				dell_wmi_process_key(wdev, buffer_entry[1],
-						     buffer_entry[2]);
-			/* 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;
+		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]);
+			}
+#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]);