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

Commit Message

Yuan, Perry April 12, 2021, 9:19 a.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
activated, any applications will not get any audio or video stream
when user pressed ctrl+F4 hotkey, audio privacy mode will be
enabled, micmute led will be also changed accordingly
The micmute led is fully controlled by hardware & EC(embedded controller)
and camera mute hotkey is Ctrl+F9. Currently design only emits
SW_CAMERA_LENS_COVER event while the camera lens shutter will be
changed by EC & HW(hardware) control

*The flow is like this:
1) User presses key. HW does stuff with this key (timeout timer is started)
2) WMI event is emitted from BIOS to kernel
3) WMI event is received by dell-privacy
4) KEY_MICMUTE emitted from dell-privacy
5) Userland picks up key and modifies kcontrol for SW mute
6) Codec kernel driver catches and calls ledtrig_audio_set, like this:
   ledtrig_audio_set(LED_AUDIO_MICMUTE, rt715->micmute_led ? LED_ON :LED_OFF);
7) If "LED" is set to on dell-privacy notifies EC, and timeout is cancelled,
   HW mic mute activated. If EC not notified, HW mic mute will also be
   activated when timeout used up, it is just later than active ack

Signed-off-by: Perry Yuan <[e-mail]>
---
v5 -> v6:
* addressed feedback from Hans
* addressed feedback from Pierre
* optimize some debug format with dev_dbg()
* remove platform driver,combined privacy acpi driver into single wmi
  driver file
* optimize sysfs interface with string added to be more clearly reading
* remove unused function and clear header file
v4 -> v5:
* addressed feedback from Randy Dunlap
* addressed feedback from Pierre-Louis Bossart
* rebase to latest 5.12 rc4 upstream kernel
* fix some space alignment problem
v3 -> v4:
* fix format for Kconfig
* add sysfs document
* add flow comments to the privacy wmi/acpi driver
* addressed feedback from Barnabás Pőcze[Thanks very much]
* export privacy_valid to make the global state simpler to query
* fix one issue which will block the dell-laptop driver to load when
  privacy driver invalid
* addressed feedback from Pierre-Louis Bossart,remove the EC ID match
v2 -> v3:
* add sysfs attributes doc
v1 -> v2:
* query EC handle from EC driver directly.
* fix some code style.
* add KEY_END to keymap array.
* clean platform device when cleanup called
* use hexadecimal format for log print in dev_dbg
* remove __set_bit for the report keys from probe.
* fix keymap leak
* add err_free_keymap in dell_privacy_wmi_probe
* wmi driver will be unregistered if privacy_acpi_init() fails
* add sysfs attribute files for user space query.
* add leds micmute driver to privacy acpi
* add more design info the commit info
---
---
 .../testing/sysfs-platform-dell-privacy-wmi   |  55 +++
 drivers/platform/x86/dell/Kconfig             |  14 +
 drivers/platform/x86/dell/Makefile            |   1 +
 drivers/platform/x86/dell/dell-laptop.c       |  23 +-
 drivers/platform/x86/dell/dell-privacy-wmi.c  | 394 ++++++++++++++++++
 drivers/platform/x86/dell/dell-privacy-wmi.h  |  23 +
 drivers/platform/x86/dell/dell-wmi.c          |   8 +-
 7 files changed, 511 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
 create mode 100644 drivers/platform/x86/dell/dell-privacy-wmi.c
 create mode 100644 drivers/platform/x86/dell/dell-privacy-wmi.h

Comments

Amadeusz Sławiński April 12, 2021, 10:39 a.m. UTC | #1

On 4/12/2021 11:19 AM, Perry Yuan wrote:
> From: Perry Yuan <[e-mail]>
> 

(...)

> diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
> index 70edc5bb3a14..e7ffc0b81208 100644
> --- a/drivers/platform/x86/dell/dell-laptop.c
> +++ b/drivers/platform/x86/dell/dell-laptop.c
> @@ -31,6 +31,8 @@
>   #include "dell-rbtn.h"
>   #include "dell-smbios.h"
>   
> +#include "dell-privacy-wmi.h"
> +
>   struct quirk_entry {
>   	bool touchpad_led;
>   	bool kbd_led_not_present;
> @@ -90,6 +92,7 @@ static struct rfkill *wifi_rfkill;
>   static struct rfkill *bluetooth_rfkill;
>   static struct rfkill *wwan_rfkill;
>   static bool force_rfkill;
> +static bool has_privacy;
>   
>   module_param(force_rfkill, bool, 0444);
>   MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted models");
> @@ -2206,10 +2209,16 @@ static int __init dell_init(void)
>   
>   	if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
>   	    dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) {
> -		micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
> -		ret = led_classdev_register(&platform_device->dev, &micmute_led_cdev);
> -		if (ret < 0)
> -			goto fail_led;
> +		if (dell_privacy_present())
> +			has_privacy = true;
> +		else
> +			has_privacy = false;

Bit, of nitpicking, but you can write above shorter:
has_privacy = dell_privacy_present();

Yuan, Perry April 13, 2021, 6:35 a.m. UTC | #2

Hi ,
> -----Original Message-----
> From: Amadeusz Sławiński <[e-mail]>
> Sent: 2021年4月12日 18:40
> To: Yuan, Perry; [e-mail]; pierre-
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; Dell Client Kernel; [e-mail]
> Subject: Re: [PATCH v7 1/2] platform/x86: dell-privacy: Add support for Dell
> hardware privacy
> 
> 
> [EXTERNAL EMAIL]
> 
> On 4/12/2021 11:19 AM, Perry Yuan wrote:
> > From: Perry Yuan <[e-mail]>
> >
> 
> (...)
> 
> > diff --git a/drivers/platform/x86/dell/dell-laptop.c
> > b/drivers/platform/x86/dell/dell-laptop.c
> > index 70edc5bb3a14..e7ffc0b81208 100644
> > --- a/drivers/platform/x86/dell/dell-laptop.c
> > +++ b/drivers/platform/x86/dell/dell-laptop.c
> > @@ -31,6 +31,8 @@
> >   #include "dell-rbtn.h"
> >   #include "dell-smbios.h"
> >
> > +#include "dell-privacy-wmi.h"
> > +
> >   struct quirk_entry {
> >   	bool touchpad_led;
> >   	bool kbd_led_not_present;
> > @@ -90,6 +92,7 @@ static struct rfkill *wifi_rfkill;
> >   static struct rfkill *bluetooth_rfkill;
> >   static struct rfkill *wwan_rfkill;
> >   static bool force_rfkill;
> > +static bool has_privacy;
> >
> >   module_param(force_rfkill, bool, 0444);
> >   MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted
> > models"); @@ -2206,10 +2209,16 @@ static int __init dell_init(void)
> >
> >   	if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
> >   	    dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) {
> > -		micmute_led_cdev.brightness =
> ledtrig_audio_get(LED_AUDIO_MICMUTE);
> > -		ret = led_classdev_register(&platform_device->dev,
> &micmute_led_cdev);
> > -		if (ret < 0)
> > -			goto fail_led;
> > +		if (dell_privacy_present())
> > +			has_privacy = true;
> > +		else
> > +			has_privacy = false;
> 
> Bit, of nitpicking, but you can write above shorter:
> has_privacy = dell_privacy_present();

Good point, changed the code as you suggested.
Thank you.
Perry.

Hans de Goede April 13, 2021, 8:48 a.m. UTC | #3

Hi,

On 4/12/21 11:19 AM, 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
> activated, any applications will not get any audio or video stream
> when user pressed ctrl+F4 hotkey, audio privacy mode will be
> enabled, micmute led will be also changed accordingly
> The micmute led is fully controlled by hardware & EC(embedded controller)
> and camera mute hotkey is Ctrl+F9. Currently design only emits
> SW_CAMERA_LENS_COVER event while the camera lens shutter will be
> changed by EC & HW(hardware) control
> 
> *The flow is like this:
> 1) User presses key. HW does stuff with this key (timeout timer is started)
> 2) WMI event is emitted from BIOS to kernel
> 3) WMI event is received by dell-privacy
> 4) KEY_MICMUTE emitted from dell-privacy
> 5) Userland picks up key and modifies kcontrol for SW mute
> 6) Codec kernel driver catches and calls ledtrig_audio_set, like this:
>    ledtrig_audio_set(LED_AUDIO_MICMUTE, rt715->micmute_led ? LED_ON :LED_OFF);
> 7) If "LED" is set to on dell-privacy notifies EC, and timeout is cancelled,
>    HW mic mute activated. If EC not notified, HW mic mute will also be
>    activated when timeout used up, it is just later than active ack
> 
> Signed-off-by: Perry Yuan <[e-mail]>
> ---
> v5 -> v6:
> * addressed feedback from Hans
> * addressed feedback from Pierre
> * optimize some debug format with dev_dbg()
> * remove platform driver,combined privacy acpi driver into single wmi
>   driver file
> * optimize sysfs interface with string added to be more clearly reading
> * remove unused function and clear header file

Thank you, almost there. A few small remarks inline.

> v4 -> v5:
> * addressed feedback from Randy Dunlap
> * addressed feedback from Pierre-Louis Bossart
> * rebase to latest 5.12 rc4 upstream kernel
> * fix some space alignment problem
> v3 -> v4:
> * fix format for Kconfig
> * add sysfs document
> * add flow comments to the privacy wmi/acpi driver
> * addressed feedback from Barnabás Pőcze[Thanks very much]
> * export privacy_valid to make the global state simpler to query
> * fix one issue which will block the dell-laptop driver to load when
>   privacy driver invalid
> * addressed feedback from Pierre-Louis Bossart,remove the EC ID match
> v2 -> v3:
> * add sysfs attributes doc
> v1 -> v2:
> * query EC handle from EC driver directly.
> * fix some code style.
> * add KEY_END to keymap array.
> * clean platform device when cleanup called
> * use hexadecimal format for log print in dev_dbg
> * remove __set_bit for the report keys from probe.
> * fix keymap leak
> * add err_free_keymap in dell_privacy_wmi_probe
> * wmi driver will be unregistered if privacy_acpi_init() fails
> * add sysfs attribute files for user space query.
> * add leds micmute driver to privacy acpi
> * add more design info the commit info
> ---
> ---
>  .../testing/sysfs-platform-dell-privacy-wmi   |  55 +++
>  drivers/platform/x86/dell/Kconfig             |  14 +
>  drivers/platform/x86/dell/Makefile            |   1 +
>  drivers/platform/x86/dell/dell-laptop.c       |  23 +-
>  drivers/platform/x86/dell/dell-privacy-wmi.c  | 394 ++++++++++++++++++
>  drivers/platform/x86/dell/dell-privacy-wmi.h  |  23 +
>  drivers/platform/x86/dell/dell-wmi.c          |   8 +-
>  7 files changed, 511 insertions(+), 7 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
>  create mode 100644 drivers/platform/x86/dell/dell-privacy-wmi.c
>  create mode 100644 drivers/platform/x86/dell/dell-privacy-wmi.h
> 
> diff --git a/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi b/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
> new file mode 100644
> index 000000000000..7f9e18705861
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
> @@ -0,0 +1,55 @@
> +What:		/sys/bus/wmi/devices/6932965F-1671-4CEB-B988-D3AB0A901919/dell_privacy_supported_type
> +Date:		Apr 2021
> +KernelVersion:	5.13
> +Contact:	"[e-mail]>"
> +Description:
> +		Display which dell hardware level privacy devices are supported
> +		“Dell Privacy” is a set of HW, FW, and SW features to enhance
> +		Dell’s commitment to platform privacy for MIC, Camera, and
> +		ePrivacy screens.
> +		The supported hardware privacy devices are:
> +Attributes:
> +		Microphone Mute:
> +				Identifies the local microphone can be muted by hardware, no applications
> +				is available to capture system mic sound
> +
> +		Camera Shutter:
> +				Identifies camera shutter controlled by hardware, which is a micromechanical
> +				shutter assembly that is built onto the camera module to block capturing images
> +				from outside the laptop
> +
> +		supported:
> +				The privacy device is supported by this system
> +
> +		unsupported:
> +				The privacy device is not supported on this system
> +
> +		For example to check which privacy devices are supported:
> +
> +		# cat /sys/bus/wmi/drivers/dell-privacy/6932965F-1671-4CEB-B988-D3AB0A901919/dell_privacy_supported_type
> +		[Microphone Mute] [supported]
> +		[Camera Shutter] [supported]
> +		[ePrivacy Screen] [unsupported]
> +
> +What:		/sys/bus/wmi/devices/6932965F-1671-4CEB-B988-D3AB0A901919/dell_privacy_current_state
> +Date:		Apr 2021
> +KernelVersion:	5.13
> +Contact:	"[e-mail]>"
> +Description:
> +		Allow user space to check current dell privacy device state.
> +		Describes the Device State class exposed by BIOS which can be
> +		consumed by various applications interested in knowing the Privacy
> +		feature capabilities
> +Attributes:
> +		muted:
> +			Identifies the privacy device is turned off and cannot send stream to OS applications
> +
> +		unmuted:
> +			Identifies the privacy device is turned on ,audio or camera driver can get
> +			stream from mic and camera module to OS applications
> +
> +		For example to check all supported current privacy device states:
> +
> +		# cat /sys/bus/wmi/drivers/dell-privacy/6932965F-1671-4CEB-B988-D3AB0A901919/dell_privacy_current_state
> +		[Microphone] [unmuted]
> +		[Camera Shutter] [unmuted]
> diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
> index e0a55337f51a..05d124442b25 100644
> --- a/drivers/platform/x86/dell/Kconfig
> +++ b/drivers/platform/x86/dell/Kconfig
> @@ -204,4 +204,18 @@ config DELL_WMI_SYSMAN
>  	  To compile this driver as a module, choose M here: the module will
>  	  be called dell-wmi-sysman.
>  
> +config DELL_PRIVACY
> +	tristate "Dell Hardware Privacy Support"
> +	depends on ACPI
> +	depends on ACPI_WMI
> +	depends on INPUT
> +	depends on DELL_LAPTOP
> +	depends on LEDS_TRIGGER_AUDIO
> +	select DELL_WMI

DELL_WMI is not a helper library which can be selected, please use depends on here.

More in general I'm a bit worried about the dependencies being added to
dell-laptop.c and dell-wmi.c on the new dell-privacy-wmi.ko module.

What if e.g. dell-laptop.c gets builtin while dell-privacy-wmi.c is a module.

Then we have dell-laptop.c depending on the dell_privacy_present linker-symbol,
but that symbol is in a module, so the main vmlinuz binary will fail to link
due to that missing symbol.

To fix this you need to add:

	depends on DELL_PRIVACY || DELL_PRIVACY = n

To the Kconfig sections for both DELL_WMI and DELL_LAPTOP


> +	help
> +	  This driver provides integration with the "Dell Hardware Privacy" feature
> +	  of Dell laptops.
> +
> +	  To compile this driver as a module, choose M here: the module will
> +	  be called dell_privacy.
>  endif # X86_PLATFORM_DRIVERS_DELL
> diff --git a/drivers/platform/x86/dell/Makefile b/drivers/platform/x86/dell/Makefile
> index d720a3e42ae3..7da0c33dfcca 100644
> --- a/drivers/platform/x86/dell/Makefile
> +++ b/drivers/platform/x86/dell/Makefile
> @@ -19,3 +19,4 @@ obj-$(CONFIG_DELL_WMI_AIO)		+= dell-wmi-aio.o
>  obj-$(CONFIG_DELL_WMI_DESCRIPTOR)	+= dell-wmi-descriptor.o
>  obj-$(CONFIG_DELL_WMI_LED)		+= dell-wmi-led.o
>  obj-$(CONFIG_DELL_WMI_SYSMAN)		+= dell-wmi-sysman/
> +obj-$(CONFIG_DELL_PRIVACY)              += dell-privacy-wmi.o
> diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
> index 70edc5bb3a14..e7ffc0b81208 100644
> --- a/drivers/platform/x86/dell/dell-laptop.c
> +++ b/drivers/platform/x86/dell/dell-laptop.c
> @@ -31,6 +31,8 @@
>  #include "dell-rbtn.h"
>  #include "dell-smbios.h"
>  
> +#include "dell-privacy-wmi.h"
> +
>  struct quirk_entry {
>  	bool touchpad_led;
>  	bool kbd_led_not_present;
> @@ -90,6 +92,7 @@ static struct rfkill *wifi_rfkill;
>  static struct rfkill *bluetooth_rfkill;
>  static struct rfkill *wwan_rfkill;
>  static bool force_rfkill;
> +static bool has_privacy;

There is a problem in the del-laptop.c code with the led_classdev
unregistering which is not introduced by this patch, but this patch is a good
opportunity to fix this.

The problem is that in the dell_init error-exit path as well as in
dell_exit led_classdev_unregister() gets called unconditionally,
while the registering has only happened under the following condition:

	if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
	    dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) {

So this means that the code may end up unregistering a never
registered led_classdev which is not good.

Since you are making the unregistering conditional in this patch,
it would be good to also fix this issue.

Please change:

static bool has_privacy;

To:

static bool micmute_led_registered;



>  
>  module_param(force_rfkill, bool, 0444);
>  MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted models");
> @@ -2206,10 +2209,16 @@ static int __init dell_init(void)
>  
>  	if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
>  	    dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) {
> -		micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
> -		ret = led_classdev_register(&platform_device->dev, &micmute_led_cdev);
> -		if (ret < 0)
> -			goto fail_led;
> +		if (dell_privacy_present())
> +			has_privacy = true;
> +		else
> +			has_privacy = false;
> +		if (!has_privacy) {
> +			micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
> +			ret = led_classdev_register(&platform_device->dev, &micmute_led_cdev);
> +			if (ret < 0)
> +				goto fail_led;
> +		}
>  	}

And change this block to

	if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
	    dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE) &&
	    !dell_privacy_present()) {
		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;
		micmute_led_registered = true;
	}

This also looks cleaner/nicer with one less indent level too :)


>  
>  	if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
> @@ -2257,7 +2266,8 @@ static int __init dell_init(void)
>  fail_get_brightness:
>  	backlight_device_unregister(dell_backlight_device);
>  fail_backlight:
> -	led_classdev_unregister(&micmute_led_cdev);
> +	if (!has_privacy)

Change this to:

	if (micmute_led_registered)
> +		led_classdev_unregister(&micmute_led_cdev);

This way the if condition also is more logical / easier to see
why the if is there.

>  fail_led:
>  	dell_cleanup_rfkill();
>  fail_rfkill:
> @@ -2278,7 +2288,8 @@ static void __exit dell_exit(void)
>  		touchpad_led_exit();
>  	kbd_led_exit();
>  	backlight_device_unregister(dell_backlight_device);
> -	led_classdev_unregister(&micmute_led_cdev);
> +	if (!has_privacy)

And here too, change this to:

	if (micmute_led_registered)

> +		led_classdev_unregister(&micmute_led_cdev);
>  	dell_cleanup_rfkill();
>  	if (platform_device) {
>  		platform_device_unregister(platform_device);
> diff --git a/drivers/platform/x86/dell/dell-privacy-wmi.c b/drivers/platform/x86/dell/dell-privacy-wmi.c
> new file mode 100644
> index 000000000000..4be7548d1a03
> --- /dev/null
> +++ b/drivers/platform/x86/dell/dell-privacy-wmi.c
> @@ -0,0 +1,394 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Dell privacy notification driver
> + *
> + * Copyright (C) 2021 Dell Inc. All Rights Reserved.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/acpi.h>
> +#include <linux/bitops.h>
> +#include <linux/input.h>
> +#include <linux/input/sparse-keymap.h>
> +#include <linux/list.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/wmi.h>
> +
> +#include "dell-privacy-wmi.h"
> +
> +#define DELL_PRIVACY_GUID "6932965F-1671-4CEB-B988-D3AB0A901919"
> +#define MICROPHONE_STATUS		BIT(0)
> +#define CAMERA_STATUS		        BIT(1)
> +#define DELL_PRIVACY_AUDIO_EVENT  0x1
> +#define DELL_PRIVACY_CAMERA_EVENT 0x2
> +#define led_to_priv(c)       container_of(c, struct privacy_wmi_data, cdev)
> +
> +/*
> + * The wmi_list is used to store the privacy_priv struct with mutex protecting
> + */
> +static LIST_HEAD(wmi_list);
> +static DEFINE_MUTEX(list_mutex);
> +
> +struct privacy_wmi_data {
> +	struct input_dev *input_dev;
> +	struct wmi_device *wdev;
> +	struct list_head list;
> +	struct led_classdev cdev;
> +	u32 features_present;
> +	u32 last_status;
> +};
> +
> +/* DELL Privacy Type */
> +enum dell_hardware_privacy_type {
> +	DELL_PRIVACY_TYPE_AUDIO = 0,
> +	DELL_PRIVACY_TYPE_CAMERA,
> +	DELL_PRIVACY_TYPE_SCREEN,
> +	DELL_PRIVACY_TYPE_MAX,
> +};
> +
> +static const char * const privacy_types[DELL_PRIVACY_TYPE_MAX] = {
> +	[DELL_PRIVACY_TYPE_AUDIO] = "Microphone",
> +	[DELL_PRIVACY_TYPE_CAMERA] = "Camera Shutter",
> +	[DELL_PRIVACY_TYPE_SCREEN] = "ePrivacy Screen",
> +};
> +
> +/*
> + * Keymap for WMI privacy events of type 0x0012
> + */
> +static const struct key_entry dell_wmi_keymap_type_0012[] = {
> +	/* privacy mic mute */
> +	{ KE_KEY, 0x0001, { KEY_MICMUTE } },
> +	/* privacy camera mute */
> +	{ KE_SW,  0x0002, { SW_CAMERA_LENS_COVER } },
> +	{ KE_END, 0},
> +};
> +
> +/*
> + * global privacy state for other modules to query if the privacy driver loaded successfully
> + * or no privacy WMI device was detected at initial probe phase
> + */
> +bool dell_privacy_present(void)
> +{
> +	struct privacy_wmi_data *priv;
> +
> +	mutex_lock(&list_mutex);
> +	priv = list_first_entry_or_null(&wmi_list,
> +			struct privacy_wmi_data,
> +			list);
> +	mutex_unlock(&list_mutex);
> +
> +	return priv;
> +}
> +EXPORT_SYMBOL_GPL(dell_privacy_present);

I thought that we had chaned this to dell_privacy_has_mic_mute ?

Returning:

	return priv && (priv->features_present & BIT(DELL_PRIVACY_TYPE_AUDIO));

?

What has happened to that change, please change that back, the only user of
dell_privacy_present() is dell-laptop.c and that wants to know if
dell-privacy.c will be handling the mute-led or not, so only checking
for dell-privacy presence is not enough.



> +
> +/*
> + * The flow of privacy event:
> + * 1) User presses key. HW does stuff with this key (timeout is started)
> + * 2) WMI event is emitted from BIOS
> + * 3) WMI event is received by dell-privacy
> + * 4) KEY_MICMUTE emitted from dell-privacy
> + * 5) Userland picks up key and modifies kcontrol for SW mute
> + * 6) Codec kernel driver catches and calls ledtrig_audio_set defined by
> + *    dell-privacy-acpi driver. Codec driver will call like this to switch micmute led state.
> + *    ledtrig_audio_set(LED_AUDIO_MICMUTE, micmute_led ? LED_ON :LED_OFF);
> + * 7) If "LED" is set to on dell-privacy notifies EC,and timeout is cancelled,
> + *	HW mic mute activated.
> + */
> +bool dell_privacy_process_event(int type, int code, int status)
> +{
> +	struct privacy_wmi_data *priv;
> +	const struct key_entry *key;
> +	bool ret = false;
> +
> +	mutex_lock(&list_mutex);
> +	priv = list_first_entry_or_null(&wmi_list,
> +			struct privacy_wmi_data,
> +			list);
> +	if (!priv) {
> +		goto error;
> +	}
> +
> +	key = sparse_keymap_entry_from_scancode(priv->input_dev, (type << 16) | code);
> +	if (!key) {
> +		dev_warn(&priv->wdev->dev, "Unknown key with type 0x%04x and code 0x%04x pressed\n",
> +			type, code);
> +		goto error;
> +	}
> +	dev_dbg(&priv->wdev->dev, "Key with type 0x%04x and code 0x%04x pressed\n", type, code);
> +
> +	switch (code) {
> +	case DELL_PRIVACY_AUDIO_EVENT: /* Mic mute */
> +	case DELL_PRIVACY_CAMERA_EVENT: /* Camera mute */
> +		priv->last_status = status;
> +		sparse_keymap_report_entry(priv->input_dev, key, 1, true);
> +		ret = true;
> +		break;
> +	default:
> +		dev_dbg(&priv->wdev->dev, "unknown event type 0x%04x 0x%04x\n", type, code);
> +	}
> +
> +error:
> +	mutex_unlock(&list_mutex);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(dell_privacy_process_event);
> +
> +static ssize_t dell_privacy_supported_type_show(struct device *dev,
> +					struct device_attribute *attr,
> +					char *buf)
> +{
> +	struct privacy_wmi_data *priv = dev_get_drvdata(dev);
> +	enum dell_hardware_privacy_type type;
> +	u32 privacy_list;
> +	char *s = buf;
> +
> +	privacy_list = priv->features_present;
> +	for (type = DELL_PRIVACY_TYPE_AUDIO; type < DELL_PRIVACY_TYPE_MAX; type++) {
> +		if (privacy_list & BIT(type))
> +			s += sprintf(s, "[%s] [supported]\n", privacy_types[type]);
> +		else
> +			s += sprintf(s, "[%s] [unsupported]\n", privacy_types[type]);
> +	}
> +
> +	return (s - buf);
> +}

As mentioned before I think (not sure), please use sysfs_emit_at here, like this:

static ssize_t dell_privacy_supported_type_show(struct device *dev,
					struct device_attribute *attr,
					char *buf)
{
	struct privacy_wmi_data *priv = dev_get_drvdata(dev);
	enum dell_hardware_privacy_type type;
	u32 privacy_list;
	int len = 0;

	privacy_list = priv->features_present;
	for (type = DELL_PRIVACY_TYPE_AUDIO; type < DELL_PRIVACY_TYPE_MAX; type++) {
		if (privacy_list & BIT(type))
			len += sysfs_emit_at(buf, len, "[%s] [supported]\n", privacy_types[type]);
		else
			len += sysfs_emit_at(buf, len,  "[%s] [unsupported]\n", privacy_types[type]);
	}

	return len;
}

This will ensure that we never overrun the size of the passed in buffer.



> +
> +static ssize_t dell_privacy_current_state_show(struct device *dev,
> +					struct device_attribute *attr,
> +					char *buf)
> +{
> +	struct privacy_wmi_data *priv = dev_get_drvdata(dev);
> +	u32 privacy_supported = priv->features_present;
> +	enum dell_hardware_privacy_type type;
> +	u32 privacy_state = priv->last_status;
> +	char *s = buf;
> +
> +	for (type = DELL_PRIVACY_TYPE_AUDIO; type < DELL_PRIVACY_TYPE_MAX; type++) {
> +		if (privacy_supported & BIT(type)) {
> +			if (privacy_state & BIT(type))
> +				s += sprintf(s, "[%s] [unmuted]\n", privacy_types[type]);
> +			else
> +				s += sprintf(s, "[%s] [muted]\n", privacy_types[type]);
> +		}
> +	}
> +
> +	return (s - buf);
> +}

And the same here.


> +
> +static DEVICE_ATTR_RO(dell_privacy_supported_type);
> +static DEVICE_ATTR_RO(dell_privacy_current_state);
> +
> +static struct attribute *privacy_attributes[] = {
> +	&dev_attr_dell_privacy_supported_type.attr,
> +	&dev_attr_dell_privacy_current_state.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group privacy_attribute_group = {
> +	.attrs = privacy_attributes
> +};
> +
> +/*
> + * Describes the Device State class exposed by BIOS which can be consumed by
> + * various applications interested in knowing the Privacy feature capabilities.
> + * class DeviceState
> + * {
> + *  [key, read] string InstanceName;
> + *  [read] boolean ReadOnly;
> + *
> + *  [WmiDataId(1), read] uint32 DevicesSupported;
> + *   0 - None; 0x1 - Microphone; 0x2 - Camera; 0x4 - ePrivacy  Screen
> + *
> + *  [WmiDataId(2), read] uint32 CurrentState;
> + *   0 - Off; 1 - On; Bit0 - Microphone; Bit1 - Camera; Bit2 - ePrivacyScreen
> + * };
> + */
> +static int get_current_status(struct wmi_device *wdev)
> +{
> +	struct privacy_wmi_data *priv = dev_get_drvdata(&wdev->dev);
> +	union acpi_object *obj_present;
> +	u32 *buffer;
> +	int ret = 0;
> +
> +	if (!priv) {
> +		dev_err(&wdev->dev, "dell privacy priv is NULL\n");
> +		return -EINVAL;
> +	}
> +	/* check privacy support features and device states */
> +	obj_present = wmidev_block_query(wdev, 0);
> +	if (!obj_present) {
> +		dev_err(&wdev->dev, "failed to read Binary MOF\n");
> +		return -EIO;
> +	}
> +
> +	if (obj_present->type != ACPI_TYPE_BUFFER) {
> +		dev_err(&wdev->dev, "Binary MOF is not a buffer!\n");
> +		ret = -EIO;
> +		goto obj_free;
> +	}
> +	/*  Although it's not technically a failure, this would lead to
> +	 *  unexpected behavior
> +	 */
> +	if (obj_present->buffer.length != 8) {
> +		dev_err(&wdev->dev, "Dell privacy buffer has unexpected length (%d)!\n",
> +				obj_present->buffer.length);
> +		ret = -EINVAL;
> +		goto obj_free;
> +	}
> +	buffer = (u32 *)obj_present->buffer.pointer;
> +	priv->features_present = buffer[0];
> +	priv->last_status = buffer[1];
> +
> +obj_free:
> +	kfree(obj_present);
> +	return ret;
> +}
> +
> +static int dell_privacy_micmute_led_set(struct led_classdev *led_cdev,
> +					enum led_brightness brightness)
> +{
> +	struct privacy_wmi_data *priv = led_to_priv(led_cdev);
> +	static char *acpi_method = (char *)"ECAK";
> +	acpi_status status;
> +	acpi_handle handle;
> +
> +	handle = ec_get_handle();
> +	if (!handle)
> +		return -EIO;
> +
> +	if (!acpi_has_method(handle, acpi_method))
> +		return -EIO;
> +
> +	status = acpi_evaluate_object(handle, acpi_method, NULL, NULL);
> +	if (ACPI_FAILURE(status)) {
> +		dev_err(&priv->wdev->dev, "Error setting privacy EC ack value: %s\n",
> +				acpi_format_exception(status));
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * Pressing the mute key activates a time delayed circuit to physically cut
> + * off the mute. The LED is in the same circuit, so it reflects the true
> + * state of the HW mute.  The reason for the EC "ack" is so that software
> + * can first invoke a SW mute before the HW circuit is cut off.  Without SW
> + * cutting this off first does not affect the time delayed muting or status
> + * of the LED but there is a possibility of a "popping" noise.
> + *
> + * If the EC receives the SW ack, the circuit will be activated before the
> + * delay completed.
> + *
> + * Exposing as an LED device allows the codec drivers notification path to
> + * EC ACK to work
> + */
> +static int dell_privacy_leds_setup(struct device *dev)
> +{
> +	struct privacy_wmi_data *priv = dev_get_drvdata(dev);
> +
> +	priv->cdev.name = "dell-privacy::micmute";
> +	priv->cdev.max_brightness = 1;
> +	priv->cdev.brightness_set_blocking = dell_privacy_micmute_led_set;
> +	priv->cdev.default_trigger = "audio-micmute";
> +	priv->cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
> +	return devm_led_classdev_register(dev, &priv->cdev);
> +}
> +
> +static int dell_privacy_wmi_probe(struct wmi_device *wdev, const void *context)
> +{
> +	struct privacy_wmi_data *priv;
> +	struct key_entry *keymap;
> +	int ret, i;
> +
> +	ret = wmi_has_guid(DELL_PRIVACY_GUID);
> +	if (!ret)
> +		pr_debug("Unable to detect available Dell privacy devices!\n");
> +
> +	priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(&wdev->dev, priv);
> +	priv->wdev = wdev;
> +	/* create evdev passing interface */
> +	priv->input_dev = devm_input_allocate_device(&wdev->dev);
> +	if (!priv->input_dev)
> +		return -ENOMEM;
> +
> +	/* remap the wmi keymap event to new keymap */
> +	keymap = kcalloc(ARRAY_SIZE(dell_wmi_keymap_type_0012),
> +			sizeof(struct key_entry), GFP_KERNEL);
> +	if (!keymap)
> +		return -ENOMEM;
> +
> +	/* remap the keymap code with Dell privacy key type 0x12 as prefix
> +	 * KEY_MICMUTE scancode will be reported as 0x120001
> +	 */
> +	for (i = 0; i < ARRAY_SIZE(dell_wmi_keymap_type_0012); i++) {
> +		keymap[i] = dell_wmi_keymap_type_0012[i];
> +		keymap[i].code |= (0x0012 << 16);
> +	}
> +	ret = sparse_keymap_setup(priv->input_dev, keymap, NULL);
> +	kfree(keymap);
> +	if (ret)
> +		return ret;
> +
> +	priv->input_dev->dev.parent = &wdev->dev;
> +	priv->input_dev->name = "Dell Privacy Driver";
> +	priv->input_dev->id.bustype = BUS_HOST;
> +
> +	ret = input_register_device(priv->input_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = get_current_status(priv->wdev);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_device_add_group(&wdev->dev, &privacy_attribute_group);
> +	if (ret)
> +		return ret;
> +
> +	ret = dell_privacy_leds_setup(&priv->wdev->dev);
> +	if (ret)
> +		return ret;

This should be conditional on the audio privacy feature being supported:

	if (priv->features_present & BIT(DELL_PRIVACY_TYPE_AUDIO)) {
		ret = dell_privacy_leds_setup(&priv->wdev->dev);
		if (ret)
			return ret;
	}


> +
> +	mutex_lock(&list_mutex);
> +	list_add_tail(&priv->list, &wmi_list);
> +	mutex_unlock(&list_mutex);
> +	return 0;
> +}
> +
> +static int dell_privacy_wmi_remove(struct wmi_device *wdev)
> +{
> +	struct privacy_wmi_data *priv = dev_get_drvdata(&wdev->dev);
> +
> +	mutex_lock(&list_mutex);
> +	list_del(&priv->list);
> +	mutex_unlock(&list_mutex);
> +	return 0;
> +}
> +
> +static const struct wmi_device_id dell_wmi_privacy_wmi_id_table[] = {
> +	{ .guid_string = DELL_PRIVACY_GUID },
> +	{ },
> +};
> +
> +static struct wmi_driver dell_privacy_wmi_driver = {
> +	.driver = {
> +		.name = "dell-privacy",
> +	},
> +	.probe = dell_privacy_wmi_probe,
> +	.remove = dell_privacy_wmi_remove,
> +	.id_table = dell_wmi_privacy_wmi_id_table,
> +};
> +
> +module_wmi_driver(dell_privacy_wmi_driver);
> +
> +MODULE_DEVICE_TABLE(wmi, dell_wmi_privacy_wmi_id_table);
> +MODULE_AUTHOR("Perry Yuan <[e-mail]>");
> +MODULE_DESCRIPTION("Dell Privacy WMI Driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/platform/x86/dell/dell-privacy-wmi.h b/drivers/platform/x86/dell/dell-privacy-wmi.h
> new file mode 100644
> index 000000000000..04c49d2fa924
> --- /dev/null
> +++ b/drivers/platform/x86/dell/dell-privacy-wmi.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Dell privacy notification driver
> + *
> + * Copyright (C) 2021 Dell Inc. All Rights Reserved.
> + */
> +
> +#ifndef _DELL_PRIVACY_WMI_H_
> +#define _DELL_PRIVACY_WMI_H_
> +
> +#if IS_ENABLED(CONFIG_DELL_PRIVACY)
> +bool dell_privacy_present(void);
> +bool dell_privacy_process_event(int type, int code, int status);
> +#else /* CONFIG_DELL_PRIVACY */
> +static inline bool dell_privacy_present(void)
> +{
> +	return -ENODEV;
> +}
> +
> +static inline bool dell_privacy_process_event(int type, int code, int status)
> +{}

This is missing a return false; between the {}.

> +#endif /* CONFIG_DELL_PRIVACY */
> +#endif
> diff --git a/drivers/platform/x86/dell/dell-wmi.c b/drivers/platform/x86/dell/dell-wmi.c
> index bbdb3e860892..228e85a427f5 100644
> --- a/drivers/platform/x86/dell/dell-wmi.c
> +++ b/drivers/platform/x86/dell/dell-wmi.c
> @@ -27,6 +27,7 @@
>  #include <acpi/video.h>
>  #include "dell-smbios.h"
>  #include "dell-wmi-descriptor.h"
> +#include "dell-privacy-wmi.h"
>  
>  MODULE_AUTHOR("Matthew Garrett <[e-mail]>");
>  MODULE_AUTHOR("Pali Rohár <[e-mail]>");
> @@ -427,7 +428,6 @@ static void dell_wmi_notify(struct wmi_device *wdev,
>  
>  		switch (buffer_entry[1]) {
>  		case 0x0000: /* One key pressed or event occurred */
> -		case 0x0012: /* Event with extended data occurred */
>  			if (len > 2)
>  				dell_wmi_process_key(wdev, buffer_entry[1],
>  						     buffer_entry[2]);
> @@ -439,6 +439,12 @@ static void dell_wmi_notify(struct wmi_device *wdev,
>  				dell_wmi_process_key(wdev, buffer_entry[1],
>  						     buffer_entry[i]);
>  			break;
> +		case 0x0012:
> +			if ((len > 4) && dell_privacy_process_event(buffer_entry[1], buffer_entry[3], buffer_entry[4]))
> +				/* privacy hotkey handle */;

Please change this comment to:

				/* dell_privacy_process_event() has handled the event */ ;


> +			else if (len > 2)
> +				dell_wmi_process_key(wdev, buffer_entry[1], buffer_entry[2]);
> +			break;
>  		default: /* Unknown event */
>  			pr_info("Unknown WMI event type 0x%x\n",
>  				(int)buffer_entry[1]);
> 



Regards,

Hans

Yuan, Perry May 6, 2021, 9:48 a.m. UTC | #4

Hi Hans.
I changed the driver in V8 as your comments.
Just one Kconfig change , It will cause some built error .

> -----Original Message-----
> From: Hans de Goede <[e-mail]>
> Sent: 2021年4月13日 16:48
> To: Yuan, Perry; [e-mail]; pierre-
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; Dell Client Kernel
> Subject: Re: [PATCH v7 1/2] platform/x86: dell-privacy: Add support for Dell
> hardware privacy
> 
> 
> [EXTERNAL EMAIL]
> 
> Hi,
> 
> On 4/12/21 11:19 AM, 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
> > activated, any applications will not get any audio or video stream
> > when user pressed ctrl+F4 hotkey, audio privacy mode will be enabled,
> > micmute led will be also changed accordingly The micmute led is fully
> > controlled by hardware & EC(embedded controller) and camera mute
> > hotkey is Ctrl+F9. Currently design only emits SW_CAMERA_LENS_COVER
> > event while the camera lens shutter will be changed by EC &
> > HW(hardware) control
> >
> > *The flow is like this:
> > 1) User presses key. HW does stuff with this key (timeout timer is
> > started)
> > 2) WMI event is emitted from BIOS to kernel
> > 3) WMI event is received by dell-privacy
> > 4) KEY_MICMUTE emitted from dell-privacy
> > 5) Userland picks up key and modifies kcontrol for SW mute
> > 6) Codec kernel driver catches and calls ledtrig_audio_set, like this:
> >    ledtrig_audio_set(LED_AUDIO_MICMUTE, rt715->micmute_led ? LED_ON
> > :LED_OFF);
> > 7) If "LED" is set to on dell-privacy notifies EC, and timeout is cancelled,
> >    HW mic mute activated. If EC not notified, HW mic mute will also be
> >    activated when timeout used up, it is just later than active ack
> >
> > Signed-off-by: Perry Yuan <[e-mail]>
> > ---
> > v5 -> v6:
> > * addressed feedback from Hans
> > * addressed feedback from Pierre
> > * optimize some debug format with dev_dbg()
> > * remove platform driver,combined privacy acpi driver into single wmi
> >   driver file
> > * optimize sysfs interface with string added to be more clearly
> > reading
> > * remove unused function and clear header file
> 
> Thank you, almost there. A few small remarks inline.
> 
> > v4 -> v5:
> > * addressed feedback from Randy Dunlap
> > * addressed feedback from Pierre-Louis Bossart
> > * rebase to latest 5.12 rc4 upstream kernel
> > * fix some space alignment problem
> > v3 -> v4:
> > * fix format for Kconfig
> > * add sysfs document
> > * add flow comments to the privacy wmi/acpi driver
> > * addressed feedback from Barnabás Pőcze[Thanks very much]
> > * export privacy_valid to make the global state simpler to query
> > * fix one issue which will block the dell-laptop driver to load when
> >   privacy driver invalid
> > * addressed feedback from Pierre-Louis Bossart,remove the EC ID match
> > v2 -> v3:
> > * add sysfs attributes doc
> > v1 -> v2:
> > * query EC handle from EC driver directly.
> > * fix some code style.
> > * add KEY_END to keymap array.
> > * clean platform device when cleanup called
> > * use hexadecimal format for log print in dev_dbg
> > * remove __set_bit for the report keys from probe.
> > * fix keymap leak
> > * add err_free_keymap in dell_privacy_wmi_probe
> > * wmi driver will be unregistered if privacy_acpi_init() fails
> > * add sysfs attribute files for user space query.
> > * add leds micmute driver to privacy acpi
> > * add more design info the commit info
> > ---
> > ---
> >  .../testing/sysfs-platform-dell-privacy-wmi   |  55 +++
> >  drivers/platform/x86/dell/Kconfig             |  14 +
> >  drivers/platform/x86/dell/Makefile            |   1 +
> >  drivers/platform/x86/dell/dell-laptop.c       |  23 +-
> >  drivers/platform/x86/dell/dell-privacy-wmi.c  | 394
> > ++++++++++++++++++  drivers/platform/x86/dell/dell-privacy-wmi.h  |  23 +
> >  drivers/platform/x86/dell/dell-wmi.c          |   8 +-
> >  7 files changed, 511 insertions(+), 7 deletions(-)  create mode
> > 100644 Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
> >  create mode 100644 drivers/platform/x86/dell/dell-privacy-wmi.c
> >  create mode 100644 drivers/platform/x86/dell/dell-privacy-wmi.h
> >
> > diff --git a/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
> > b/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
> > new file mode 100644
> > index 000000000000..7f9e18705861
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
> > @@ -0,0 +1,55 @@
> > +What:		/sys/bus/wmi/devices/6932965F-1671-4CEB-B988-
> D3AB0A901919/dell_privacy_supported_type
> > +Date:		Apr 2021
> > +KernelVersion:	5.13
> > +Contact:	"[e-mail]>"
> > +Description:
> > +		Display which dell hardware level privacy devices are
> supported
> > +		“Dell Privacy” is a set of HW, FW, and SW features to enhance
> > +		Dell’s commitment to platform privacy for MIC, Camera, and
> > +		ePrivacy screens.
> > +		The supported hardware privacy devices are:
> > +Attributes:
> > +		Microphone Mute:
> > +				Identifies the local microphone can be muted
> by hardware, no applications
> > +				is available to capture system mic sound
> > +
> > +		Camera Shutter:
> > +				Identifies camera shutter controlled by
> hardware, which is a micromechanical
> > +				shutter assembly that is built onto the camera
> module to block capturing images
> > +				from outside the laptop
> > +
> > +		supported:
> > +				The privacy device is supported by this system
> > +
> > +		unsupported:
> > +				The privacy device is not supported on this
> system
> > +
> > +		For example to check which privacy devices are supported:
> > +
> > +		# cat /sys/bus/wmi/drivers/dell-privacy/6932965F-1671-
> 4CEB-B988-D3AB0A901919/dell_privacy_supported_type
> > +		[Microphone Mute] [supported]
> > +		[Camera Shutter] [supported]
> > +		[ePrivacy Screen] [unsupported]
> > +
> > +What:		/sys/bus/wmi/devices/6932965F-1671-4CEB-B988-
> D3AB0A901919/dell_privacy_current_state
> > +Date:		Apr 2021
> > +KernelVersion:	5.13
> > +Contact:	"[e-mail]>"
> > +Description:
> > +		Allow user space to check current dell privacy device state.
> > +		Describes the Device State class exposed by BIOS which can
> be
> > +		consumed by various applications interested in knowing the
> Privacy
> > +		feature capabilities
> > +Attributes:
> > +		muted:
> > +			Identifies the privacy device is turned off and cannot
> send stream
> > +to OS applications
> > +
> > +		unmuted:
> > +			Identifies the privacy device is turned on ,audio or
> camera driver can get
> > +			stream from mic and camera module to OS
> applications
> > +
> > +		For example to check all supported current privacy device
> states:
> > +
> > +		# cat /sys/bus/wmi/drivers/dell-privacy/6932965F-1671-
> 4CEB-B988-D3AB0A901919/dell_privacy_current_state
> > +		[Microphone] [unmuted]
> > +		[Camera Shutter] [unmuted]
> > diff --git a/drivers/platform/x86/dell/Kconfig
> > b/drivers/platform/x86/dell/Kconfig
> > index e0a55337f51a..05d124442b25 100644
> > --- a/drivers/platform/x86/dell/Kconfig
> > +++ b/drivers/platform/x86/dell/Kconfig
> > @@ -204,4 +204,18 @@ config DELL_WMI_SYSMAN
> >  	  To compile this driver as a module, choose M here: the module will
> >  	  be called dell-wmi-sysman.
> >
> > +config DELL_PRIVACY
> > +	tristate "Dell Hardware Privacy Support"
> > +	depends on ACPI
> > +	depends on ACPI_WMI
> > +	depends on INPUT
> > +	depends on DELL_LAPTOP
> > +	depends on LEDS_TRIGGER_AUDIO
> > +	select DELL_WMI
> 
> DELL_WMI is not a helper library which can be selected, please use depends
> on here.
> 
> More in general I'm a bit worried about the dependencies being added to dell-
> laptop.c and dell-wmi.c on the new dell-privacy-wmi.ko module.
> 
> What if e.g. dell-laptop.c gets builtin while dell-privacy-wmi.c is a module.
> 
> Then we have dell-laptop.c depending on the dell_privacy_present linker-
> symbol, but that symbol is in a module, so the main vmlinuz binary will fail to
> link due to that missing symbol.
> 
> To fix this you need to add:
> 
> 	depends on DELL_PRIVACY || DELL_PRIVACY = n
> 
> To the Kconfig sections for both DELL_WMI and DELL_LAPTOP

If I add "depends on DELL_PRIVACY || DELL_PRIVACY = n" to both DELL_WMI and DELL_LAPTOP
The compile will report error "recursive dependency detected"
I do not think the dell-laptop will be builtin option as we know.

I am confused that why the symbol will be failed to link like that ?
because the compiler can find the dell_privacy_present which is defined in one common header file.



> 
> 
> > +	help
> > +	  This driver provides integration with the "Dell Hardware Privacy"
> feature
> > +	  of Dell laptops.
> > +
> > +	  To compile this driver as a module, choose M here: the module will
> > +	  be called dell_privacy.
> >  endif # X86_PLATFORM_DRIVERS_DELL
> > diff --git a/drivers/platform/x86/dell/Makefile
> > b/drivers/platform/x86/dell/Makefile
> > index d720a3e42ae3..7da0c33dfcca 100644
> > --- a/drivers/platform/x86/dell/Makefile
> > +++ b/drivers/platform/x86/dell/Makefile
> > @@ -19,3 +19,4 @@ obj-$(CONFIG_DELL_WMI_AIO)		+= dell-wmi-
> aio.o
> >  obj-$(CONFIG_DELL_WMI_DESCRIPTOR)	+= dell-wmi-descriptor.o
> >  obj-$(CONFIG_DELL_WMI_LED)		+= dell-wmi-led.o
> >  obj-$(CONFIG_DELL_WMI_SYSMAN)		+= dell-wmi-sysman/
> > +obj-$(CONFIG_DELL_PRIVACY)              += dell-privacy-wmi.o
> > diff --git a/drivers/platform/x86/dell/dell-laptop.c
> > b/drivers/platform/x86/dell/dell-laptop.c
> > index 70edc5bb3a14..e7ffc0b81208 100644
> > --- a/drivers/platform/x86/dell/dell-laptop.c
> > +++ b/drivers/platform/x86/dell/dell-laptop.c
> > @@ -31,6 +31,8 @@
> >  #include "dell-rbtn.h"
> >  #include "dell-smbios.h"
> >
> > +#include "dell-privacy-wmi.h"
> > +
> >  struct quirk_entry {
> >  	bool touchpad_led;
> >  	bool kbd_led_not_present;
> > @@ -90,6 +92,7 @@ static struct rfkill *wifi_rfkill;  static struct
> > rfkill *bluetooth_rfkill;  static struct rfkill *wwan_rfkill;  static
> > bool force_rfkill;
> > +static bool has_privacy;
> 
> There is a problem in the del-laptop.c code with the led_classdev
> unregistering which is not introduced by this patch, but this patch is a good
> opportunity to fix this.
> 
> The problem is that in the dell_init error-exit path as well as in dell_exit
> led_classdev_unregister() gets called unconditionally, while the registering
> has only happened under the following condition:
> 
> 	if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
> 	    dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) {
> 
> So this means that the code may end up unregistering a never registered
> led_classdev which is not good.
> 
> Since you are making the unregistering conditional in this patch, it would be
> good to also fix this issue.
> 
> Please change:
> 
> static bool has_privacy;
> 
> To:
> 
> static bool micmute_led_registered;
> 
> 
> 
> >
> >  module_param(force_rfkill, bool, 0444);
> > MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted
> > models"); @@ -2206,10 +2209,16 @@ static int __init dell_init(void)
> >
> >  	if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
> >  	    dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) {
> > -		micmute_led_cdev.brightness =
> ledtrig_audio_get(LED_AUDIO_MICMUTE);
> > -		ret = led_classdev_register(&platform_device->dev,
> &micmute_led_cdev);
> > -		if (ret < 0)
> > -			goto fail_led;
> > +		if (dell_privacy_present())
> > +			has_privacy = true;
> > +		else
> > +			has_privacy = false;
> > +		if (!has_privacy) {
> > +			micmute_led_cdev.brightness =
> ledtrig_audio_get(LED_AUDIO_MICMUTE);
> > +			ret = led_classdev_register(&platform_device->dev,
> &micmute_led_cdev);
> > +			if (ret < 0)
> > +				goto fail_led;
> > +		}
> >  	}
> 
> And change this block to
> 
> 	if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
> 	    dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE) &&
> 	    !dell_privacy_present()) {
> 		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;
> 		micmute_led_registered = true;
> 	}
> 
> This also looks cleaner/nicer with one less indent level too :)
> 
> 
> >
> >  	if (acpi_video_get_backlight_type() != acpi_backlight_vendor) @@
> > -2257,7 +2266,8 @@ static int __init dell_init(void)
> >  fail_get_brightness:
> >  	backlight_device_unregister(dell_backlight_device);
> >  fail_backlight:
> > -	led_classdev_unregister(&micmute_led_cdev);
> > +	if (!has_privacy)
> 
> Change this to:
> 
> 	if (micmute_led_registered)
> > +		led_classdev_unregister(&micmute_led_cdev);
> 
> This way the if condition also is more logical / easier to see why the if is there.
> 
> >  fail_led:
> >  	dell_cleanup_rfkill();
> >  fail_rfkill:
> > @@ -2278,7 +2288,8 @@ static void __exit dell_exit(void)
> >  		touchpad_led_exit();
> >  	kbd_led_exit();
> >  	backlight_device_unregister(dell_backlight_device);
> > -	led_classdev_unregister(&micmute_led_cdev);
> > +	if (!has_privacy)
> 
> And here too, change this to:
> 
> 	if (micmute_led_registered)
> 
> > +		led_classdev_unregister(&micmute_led_cdev);
> >  	dell_cleanup_rfkill();
> >  	if (platform_device) {
> >  		platform_device_unregister(platform_device);
> > diff --git a/drivers/platform/x86/dell/dell-privacy-wmi.c
> > b/drivers/platform/x86/dell/dell-privacy-wmi.c
> > new file mode 100644
> > index 000000000000..4be7548d1a03
> > --- /dev/null
> > +++ b/drivers/platform/x86/dell/dell-privacy-wmi.c
> > @@ -0,0 +1,394 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Dell privacy notification driver
> > + *
> > + * Copyright (C) 2021 Dell Inc. All Rights Reserved.
> > + */
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/bitops.h>
> > +#include <linux/input.h>
> > +#include <linux/input/sparse-keymap.h> #include <linux/list.h>
> > +#include <linux/leds.h> #include <linux/module.h> #include
> > +<linux/wmi.h>
> > +
> > +#include "dell-privacy-wmi.h"
> > +
> > +#define DELL_PRIVACY_GUID "6932965F-1671-4CEB-B988-
> D3AB0A901919"
> > +#define MICROPHONE_STATUS		BIT(0)
> > +#define CAMERA_STATUS		        BIT(1)
> > +#define DELL_PRIVACY_AUDIO_EVENT  0x1 #define
> > +DELL_PRIVACY_CAMERA_EVENT 0x2
> > +#define led_to_priv(c)       container_of(c, struct privacy_wmi_data, cdev)
> > +
> > +/*
> > + * The wmi_list is used to store the privacy_priv struct with mutex
> > +protecting  */ static LIST_HEAD(wmi_list); static
> > +DEFINE_MUTEX(list_mutex);
> > +
> > +struct privacy_wmi_data {
> > +	struct input_dev *input_dev;
> > +	struct wmi_device *wdev;
> > +	struct list_head list;
> > +	struct led_classdev cdev;
> > +	u32 features_present;
> > +	u32 last_status;
> > +};
> > +
> > +/* DELL Privacy Type */
> > +enum dell_hardware_privacy_type {
> > +	DELL_PRIVACY_TYPE_AUDIO = 0,
> > +	DELL_PRIVACY_TYPE_CAMERA,
> > +	DELL_PRIVACY_TYPE_SCREEN,
> > +	DELL_PRIVACY_TYPE_MAX,
> > +};
> > +
> > +static const char * const privacy_types[DELL_PRIVACY_TYPE_MAX] = {
> > +	[DELL_PRIVACY_TYPE_AUDIO] = "Microphone",
> > +	[DELL_PRIVACY_TYPE_CAMERA] = "Camera Shutter",
> > +	[DELL_PRIVACY_TYPE_SCREEN] = "ePrivacy Screen", };
> > +
> > +/*
> > + * Keymap for WMI privacy events of type 0x0012  */ static const
> > +struct key_entry dell_wmi_keymap_type_0012[] = {
> > +	/* privacy mic mute */
> > +	{ KE_KEY, 0x0001, { KEY_MICMUTE } },
> > +	/* privacy camera mute */
> > +	{ KE_SW,  0x0002, { SW_CAMERA_LENS_COVER } },
> > +	{ KE_END, 0},
> > +};
> > +
> > +/*
> > + * global privacy state for other modules to query if the privacy
> > +driver loaded successfully
> > + * or no privacy WMI device was detected at initial probe phase  */
> > +bool dell_privacy_present(void) {
> > +	struct privacy_wmi_data *priv;
> > +
> > +	mutex_lock(&list_mutex);
> > +	priv = list_first_entry_or_null(&wmi_list,
> > +			struct privacy_wmi_data,
> > +			list);
> > +	mutex_unlock(&list_mutex);
> > +
> > +	return priv;
> > +}
> > +EXPORT_SYMBOL_GPL(dell_privacy_present);
> 
> I thought that we had chaned this to dell_privacy_has_mic_mute ?
> 
> Returning:
> 
> 	return priv && (priv->features_present &
> BIT(DELL_PRIVACY_TYPE_AUDIO));
> 
> ?
> 
> What has happened to that change, please change that back, the only user of
> dell_privacy_present() is dell-laptop.c and that wants to know if dell-privacy.c
> will be handling the mute-led or not, so only checking for dell-privacy
> presence is not enough.
> 
> 
> 
> > +
> > +/*
> > + * The flow of privacy event:
> > + * 1) User presses key. HW does stuff with this key (timeout is
> > +started)
> > + * 2) WMI event is emitted from BIOS
> > + * 3) WMI event is received by dell-privacy
> > + * 4) KEY_MICMUTE emitted from dell-privacy
> > + * 5) Userland picks up key and modifies kcontrol for SW mute
> > + * 6) Codec kernel driver catches and calls ledtrig_audio_set defined by
> > + *    dell-privacy-acpi driver. Codec driver will call like this to switch
> micmute led state.
> > + *    ledtrig_audio_set(LED_AUDIO_MICMUTE, micmute_led ?
> LED_ON :LED_OFF);
> > + * 7) If "LED" is set to on dell-privacy notifies EC,and timeout is cancelled,
> > + *	HW mic mute activated.
> > + */
> > +bool dell_privacy_process_event(int type, int code, int status) {
> > +	struct privacy_wmi_data *priv;
> > +	const struct key_entry *key;
> > +	bool ret = false;
> > +
> > +	mutex_lock(&list_mutex);
> > +	priv = list_first_entry_or_null(&wmi_list,
> > +			struct privacy_wmi_data,
> > +			list);
> > +	if (!priv) {
> > +		goto error;
> > +	}
> > +
> > +	key = sparse_keymap_entry_from_scancode(priv->input_dev, (type <<
> 16) | code);
> > +	if (!key) {
> > +		dev_warn(&priv->wdev->dev, "Unknown key with type
> 0x%04x and code 0x%04x pressed\n",
> > +			type, code);
> > +		goto error;
> > +	}
> > +	dev_dbg(&priv->wdev->dev, "Key with type 0x%04x and code 0x%04x
> > +pressed\n", type, code);
> > +
> > +	switch (code) {
> > +	case DELL_PRIVACY_AUDIO_EVENT: /* Mic mute */
> > +	case DELL_PRIVACY_CAMERA_EVENT: /* Camera mute */
> > +		priv->last_status = status;
> > +		sparse_keymap_report_entry(priv->input_dev, key, 1, true);
> > +		ret = true;
> > +		break;
> > +	default:
> > +		dev_dbg(&priv->wdev->dev, "unknown event type 0x%04x
> 0x%04x\n", type, code);
> > +	}
> > +
> > +error:
> > +	mutex_unlock(&list_mutex);
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(dell_privacy_process_event);
> > +
> > +static ssize_t dell_privacy_supported_type_show(struct device *dev,
> > +					struct device_attribute *attr,
> > +					char *buf)
> > +{
> > +	struct privacy_wmi_data *priv = dev_get_drvdata(dev);
> > +	enum dell_hardware_privacy_type type;
> > +	u32 privacy_list;
> > +	char *s = buf;
> > +
> > +	privacy_list = priv->features_present;
> > +	for (type = DELL_PRIVACY_TYPE_AUDIO; type <
> DELL_PRIVACY_TYPE_MAX; type++) {
> > +		if (privacy_list & BIT(type))
> > +			s += sprintf(s, "[%s] [supported]\n",
> privacy_types[type]);
> > +		else
> > +			s += sprintf(s, "[%s] [unsupported]\n",
> privacy_types[type]);
> > +	}
> > +
> > +	return (s - buf);
> > +}
> 
> As mentioned before I think (not sure), please use sysfs_emit_at here, like this:
> 
> static ssize_t dell_privacy_supported_type_show(struct device *dev,
> 					struct device_attribute *attr,
> 					char *buf)
> {
> 	struct privacy_wmi_data *priv = dev_get_drvdata(dev);
> 	enum dell_hardware_privacy_type type;
> 	u32 privacy_list;
> 	int len = 0;
> 
> 	privacy_list = priv->features_present;
> 	for (type = DELL_PRIVACY_TYPE_AUDIO; type <
> DELL_PRIVACY_TYPE_MAX; type++) {
> 		if (privacy_list & BIT(type))
> 			len += sysfs_emit_at(buf, len, "[%s] [supported]\n",
> privacy_types[type]);
> 		else
> 			len += sysfs_emit_at(buf, len,  "[%s] [unsupported]\n",
> privacy_types[type]);
> 	}
> 
> 	return len;
> }
> 
> This will ensure that we never overrun the size of the passed in buffer.
> 
> 
> 
> > +
> > +static ssize_t dell_privacy_current_state_show(struct device *dev,
> > +					struct device_attribute *attr,
> > +					char *buf)
> > +{
> > +	struct privacy_wmi_data *priv = dev_get_drvdata(dev);
> > +	u32 privacy_supported = priv->features_present;
> > +	enum dell_hardware_privacy_type type;
> > +	u32 privacy_state = priv->last_status;
> > +	char *s = buf;
> > +
> > +	for (type = DELL_PRIVACY_TYPE_AUDIO; type <
> DELL_PRIVACY_TYPE_MAX; type++) {
> > +		if (privacy_supported & BIT(type)) {
> > +			if (privacy_state & BIT(type))
> > +				s += sprintf(s, "[%s] [unmuted]\n",
> privacy_types[type]);
> > +			else
> > +				s += sprintf(s, "[%s] [muted]\n",
> privacy_types[type]);
> > +		}
> > +	}
> > +
> > +	return (s - buf);
> > +}
> 
> And the same here.
> 
> 
> > +
> > +static DEVICE_ATTR_RO(dell_privacy_supported_type);
> > +static DEVICE_ATTR_RO(dell_privacy_current_state);
> > +
> > +static struct attribute *privacy_attributes[] = {
> > +	&dev_attr_dell_privacy_supported_type.attr,
> > +	&dev_attr_dell_privacy_current_state.attr,
> > +	NULL,
> > +};
> > +
> > +static const struct attribute_group privacy_attribute_group = {
> > +	.attrs = privacy_attributes
> > +};
> > +
> > +/*
> > + * Describes the Device State class exposed by BIOS which can be
> > +consumed by
> > + * various applications interested in knowing the Privacy feature
> capabilities.
> > + * class DeviceState
> > + * {
> > + *  [key, read] string InstanceName;
> > + *  [read] boolean ReadOnly;
> > + *
> > + *  [WmiDataId(1), read] uint32 DevicesSupported;
> > + *   0 - None; 0x1 - Microphone; 0x2 - Camera; 0x4 - ePrivacy  Screen
> > + *
> > + *  [WmiDataId(2), read] uint32 CurrentState;
> > + *   0 - Off; 1 - On; Bit0 - Microphone; Bit1 - Camera; Bit2 - ePrivacyScreen
> > + * };
> > + */
> > +static int get_current_status(struct wmi_device *wdev) {
> > +	struct privacy_wmi_data *priv = dev_get_drvdata(&wdev->dev);
> > +	union acpi_object *obj_present;
> > +	u32 *buffer;
> > +	int ret = 0;
> > +
> > +	if (!priv) {
> > +		dev_err(&wdev->dev, "dell privacy priv is NULL\n");
> > +		return -EINVAL;
> > +	}
> > +	/* check privacy support features and device states */
> > +	obj_present = wmidev_block_query(wdev, 0);
> > +	if (!obj_present) {
> > +		dev_err(&wdev->dev, "failed to read Binary MOF\n");
> > +		return -EIO;
> > +	}
> > +
> > +	if (obj_present->type != ACPI_TYPE_BUFFER) {
> > +		dev_err(&wdev->dev, "Binary MOF is not a buffer!\n");
> > +		ret = -EIO;
> > +		goto obj_free;
> > +	}
> > +	/*  Although it's not technically a failure, this would lead to
> > +	 *  unexpected behavior
> > +	 */
> > +	if (obj_present->buffer.length != 8) {
> > +		dev_err(&wdev->dev, "Dell privacy buffer has unexpected
> length (%d)!\n",
> > +				obj_present->buffer.length);
> > +		ret = -EINVAL;
> > +		goto obj_free;
> > +	}
> > +	buffer = (u32 *)obj_present->buffer.pointer;
> > +	priv->features_present = buffer[0];
> > +	priv->last_status = buffer[1];
> > +
> > +obj_free:
> > +	kfree(obj_present);
> > +	return ret;
> > +}
> > +
> > +static int dell_privacy_micmute_led_set(struct led_classdev *led_cdev,
> > +					enum led_brightness brightness)
> > +{
> > +	struct privacy_wmi_data *priv = led_to_priv(led_cdev);
> > +	static char *acpi_method = (char *)"ECAK";
> > +	acpi_status status;
> > +	acpi_handle handle;
> > +
> > +	handle = ec_get_handle();
> > +	if (!handle)
> > +		return -EIO;
> > +
> > +	if (!acpi_has_method(handle, acpi_method))
> > +		return -EIO;
> > +
> > +	status = acpi_evaluate_object(handle, acpi_method, NULL, NULL);
> > +	if (ACPI_FAILURE(status)) {
> > +		dev_err(&priv->wdev->dev, "Error setting privacy EC ack
> value: %s\n",
> > +				acpi_format_exception(status));
> > +		return -EIO;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Pressing the mute key activates a time delayed circuit to
> > +physically cut
> > + * off the mute. The LED is in the same circuit, so it reflects the
> > +true
> > + * state of the HW mute.  The reason for the EC "ack" is so that
> > +software
> > + * can first invoke a SW mute before the HW circuit is cut off.
> > +Without SW
> > + * cutting this off first does not affect the time delayed muting or
> > +status
> > + * of the LED but there is a possibility of a "popping" noise.
> > + *
> > + * If the EC receives the SW ack, the circuit will be activated
> > +before the
> > + * delay completed.
> > + *
> > + * Exposing as an LED device allows the codec drivers notification
> > +path to
> > + * EC ACK to work
> > + */
> > +static int dell_privacy_leds_setup(struct device *dev) {
> > +	struct privacy_wmi_data *priv = dev_get_drvdata(dev);
> > +
> > +	priv->cdev.name = "dell-privacy::micmute";
> > +	priv->cdev.max_brightness = 1;
> > +	priv->cdev.brightness_set_blocking = dell_privacy_micmute_led_set;
> > +	priv->cdev.default_trigger = "audio-micmute";
> > +	priv->cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
> > +	return devm_led_classdev_register(dev, &priv->cdev); }
> > +
> > +static int dell_privacy_wmi_probe(struct wmi_device *wdev, const void
> > +*context) {
> > +	struct privacy_wmi_data *priv;
> > +	struct key_entry *keymap;
> > +	int ret, i;
> > +
> > +	ret = wmi_has_guid(DELL_PRIVACY_GUID);
> > +	if (!ret)
> > +		pr_debug("Unable to detect available Dell privacy
> devices!\n");
> > +
> > +	priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	dev_set_drvdata(&wdev->dev, priv);
> > +	priv->wdev = wdev;
> > +	/* create evdev passing interface */
> > +	priv->input_dev = devm_input_allocate_device(&wdev->dev);
> > +	if (!priv->input_dev)
> > +		return -ENOMEM;
> > +
> > +	/* remap the wmi keymap event to new keymap */
> > +	keymap = kcalloc(ARRAY_SIZE(dell_wmi_keymap_type_0012),
> > +			sizeof(struct key_entry), GFP_KERNEL);
> > +	if (!keymap)
> > +		return -ENOMEM;
> > +
> > +	/* remap the keymap code with Dell privacy key type 0x12 as prefix
> > +	 * KEY_MICMUTE scancode will be reported as 0x120001
> > +	 */
> > +	for (i = 0; i < ARRAY_SIZE(dell_wmi_keymap_type_0012); i++) {
> > +		keymap[i] = dell_wmi_keymap_type_0012[i];
> > +		keymap[i].code |= (0x0012 << 16);
> > +	}
> > +	ret = sparse_keymap_setup(priv->input_dev, keymap, NULL);
> > +	kfree(keymap);
> > +	if (ret)
> > +		return ret;
> > +
> > +	priv->input_dev->dev.parent = &wdev->dev;
> > +	priv->input_dev->name = "Dell Privacy Driver";
> > +	priv->input_dev->id.bustype = BUS_HOST;
> > +
> > +	ret = input_register_device(priv->input_dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = get_current_status(priv->wdev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = devm_device_add_group(&wdev->dev,
> &privacy_attribute_group);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = dell_privacy_leds_setup(&priv->wdev->dev);
> > +	if (ret)
> > +		return ret;
> 
> This should be conditional on the audio privacy feature being supported:
> 
> 	if (priv->features_present & BIT(DELL_PRIVACY_TYPE_AUDIO)) {
> 		ret = dell_privacy_leds_setup(&priv->wdev->dev);
> 		if (ret)
> 			return ret;
> 	}
> 
> 
> > +
> > +	mutex_lock(&list_mutex);
> > +	list_add_tail(&priv->list, &wmi_list);
> > +	mutex_unlock(&list_mutex);
> > +	return 0;
> > +}
> > +
> > +static int dell_privacy_wmi_remove(struct wmi_device *wdev) {
> > +	struct privacy_wmi_data *priv = dev_get_drvdata(&wdev->dev);
> > +
> > +	mutex_lock(&list_mutex);
> > +	list_del(&priv->list);
> > +	mutex_unlock(&list_mutex);
> > +	return 0;
> > +}
> > +
> > +static const struct wmi_device_id dell_wmi_privacy_wmi_id_table[] = {
> > +	{ .guid_string = DELL_PRIVACY_GUID },
> > +	{ },
> > +};
> > +
> > +static struct wmi_driver dell_privacy_wmi_driver = {
> > +	.driver = {
> > +		.name = "dell-privacy",
> > +	},
> > +	.probe = dell_privacy_wmi_probe,
> > +	.remove = dell_privacy_wmi_remove,
> > +	.id_table = dell_wmi_privacy_wmi_id_table, };
> > +
> > +module_wmi_driver(dell_privacy_wmi_driver);
> > +
> > +MODULE_DEVICE_TABLE(wmi, dell_wmi_privacy_wmi_id_table);
> > +MODULE_AUTHOR("Perry Yuan <[e-mail]>");
> > +MODULE_DESCRIPTION("Dell Privacy WMI Driver");
> MODULE_LICENSE("GPL");
> > diff --git a/drivers/platform/x86/dell/dell-privacy-wmi.h
> > b/drivers/platform/x86/dell/dell-privacy-wmi.h
> > new file mode 100644
> > index 000000000000..04c49d2fa924
> > --- /dev/null
> > +++ b/drivers/platform/x86/dell/dell-privacy-wmi.h
> > @@ -0,0 +1,23 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Dell privacy notification driver
> > + *
> > + * Copyright (C) 2021 Dell Inc. All Rights Reserved.
> > + */
> > +
> > +#ifndef _DELL_PRIVACY_WMI_H_
> > +#define _DELL_PRIVACY_WMI_H_
> > +
> > +#if IS_ENABLED(CONFIG_DELL_PRIVACY)
> > +bool dell_privacy_present(void);
> > +bool dell_privacy_process_event(int type, int code, int status);
> > +#else /* CONFIG_DELL_PRIVACY */ static inline bool
> > +dell_privacy_present(void) {
> > +	return -ENODEV;
> > +}
> > +
> > +static inline bool dell_privacy_process_event(int type, int code, int
> > +status) {}
> 
> This is missing a return false; between the {}.
> 
> > +#endif /* CONFIG_DELL_PRIVACY */
> > +#endif
> > diff --git a/drivers/platform/x86/dell/dell-wmi.c
> > b/drivers/platform/x86/dell/dell-wmi.c
> > index bbdb3e860892..228e85a427f5 100644
> > --- a/drivers/platform/x86/dell/dell-wmi.c
> > +++ b/drivers/platform/x86/dell/dell-wmi.c
> > @@ -27,6 +27,7 @@
> >  #include <acpi/video.h>
> >  #include "dell-smbios.h"
> >  #include "dell-wmi-descriptor.h"
> > +#include "dell-privacy-wmi.h"
> >
> >  MODULE_AUTHOR("Matthew Garrett <[e-mail]>");
> > MODULE_AUTHOR("Pali Rohár <[e-mail]>"); @@ -427,7 +428,6
> @@
> > static void dell_wmi_notify(struct wmi_device *wdev,
> >
> >  		switch (buffer_entry[1]) {
> >  		case 0x0000: /* One key pressed or event occurred */
> > -		case 0x0012: /* Event with extended data occurred */
> >  			if (len > 2)
> >  				dell_wmi_process_key(wdev, buffer_entry[1],
> >  						     buffer_entry[2]);
> > @@ -439,6 +439,12 @@ static void dell_wmi_notify(struct wmi_device
> *wdev,
> >  				dell_wmi_process_key(wdev, buffer_entry[1],
> >  						     buffer_entry[i]);
> >  			break;
> > +		case 0x0012:
> > +			if ((len > 4) &&
> dell_privacy_process_event(buffer_entry[1], buffer_entry[3], buffer_entry[4]))
> > +				/* privacy hotkey handle */;
> 
> Please change this comment to:
> 
> 				/* dell_privacy_process_event() has handled
> the event */ ;
> 
> 
> > +			else if (len > 2)
> > +				dell_wmi_process_key(wdev, buffer_entry[1],
> buffer_entry[2]);
> > +			break;
> >  		default: /* Unknown event */
> >  			pr_info("Unknown WMI event type 0x%x\n",
> >  				(int)buffer_entry[1]);
> >
> 
> 
> 
> Regards,
> 
> Hans

Hans de Goede May 18, 2021, 10:49 a.m. UTC | #5

Hi Perry,

On 5/6/21 11:48 AM, Yuan, Perry wrote:
> Hi Hans.
> I changed the driver in V8 as your comments.
> Just one Kconfig change , It will cause some built error .

<snip>

>>> diff --git a/drivers/platform/x86/dell/Kconfig
>>> b/drivers/platform/x86/dell/Kconfig
>>> index e0a55337f51a..05d124442b25 100644
>>> --- a/drivers/platform/x86/dell/Kconfig
>>> +++ b/drivers/platform/x86/dell/Kconfig
>>> @@ -204,4 +204,18 @@ config DELL_WMI_SYSMAN
>>>  	  To compile this driver as a module, choose M here: the module will
>>>  	  be called dell-wmi-sysman.
>>>
>>> +config DELL_PRIVACY
>>> +	tristate "Dell Hardware Privacy Support"
>>> +	depends on ACPI
>>> +	depends on ACPI_WMI
>>> +	depends on INPUT
>>> +	depends on DELL_LAPTOP
>>> +	depends on LEDS_TRIGGER_AUDIO
>>> +	select DELL_WMI
>>
>> DELL_WMI is not a helper library which can be selected, please use depends
>> on here.
>>
>> More in general I'm a bit worried about the dependencies being added to dell-
>> laptop.c and dell-wmi.c on the new dell-privacy-wmi.ko module.
>>
>> What if e.g. dell-laptop.c gets builtin while dell-privacy-wmi.c is a module.
>>
>> Then we have dell-laptop.c depending on the dell_privacy_present linker-
>> symbol, but that symbol is in a module, so the main vmlinuz binary will fail to
>> link due to that missing symbol.
>>
>> To fix this you need to add:
>>
>> 	depends on DELL_PRIVACY || DELL_PRIVACY = n
>>
>> To the Kconfig sections for both DELL_WMI and DELL_LAPTOP
> 
> If I add "depends on DELL_PRIVACY || DELL_PRIVACY = n" to both DELL_WMI and DELL_LAPTOP
> The compile will report error "recursive dependency detected"
> I do not think the dell-laptop will be builtin option as we know.
> 
> I am confused that why the symbol will be failed to link like that ?
> because the compiler can find the dell_privacy_present which is defined in one common header file.

The issue is that e.g the dell-laptop code may be builtin into the kernel
(so part of the vmlinuz file) while the dell-privacy code could be build
as a module (so as a dell-privacy.ko file) in this case building the vmlinuz
file will fail at the linking stage since the dell_privacy_present() symbol
is not part of vmlinuz where as the dell-laptop code which needs that
symbol is part of vmlinuz.

The reason why these circular dependency issues trigger is because the
dell-privacy.ko module actually does not depend (at a symbol level) on
dell-laptop / dell-wmi at all. It is the other way around dell-laptop
and dell-wmi use symbols from dell-privacy, where as dell-privacy can
be loaded into the kernel without dell-wmi / dell-laptop being loaded
just fine.

Now there is a functional dependency where dell-privacy does not do much
when it is not called form the event handler in dell-wmi, but this is not
a code-level dependency.

I've been thinking a bit about this and I've come to the following
conclusions:

1. dell-privacy should not depend on dell-laptop at all, the only reason
dell-laptop calls into dell-privacy is to not register a mic-mute LED if
dell-laptop is not build at all then it will also not register the mic-mute
LED, so the dependency of dell-privacy on dell-laptop can be dropped.

2. Building dell-privacy without also building dell-wmi is a different story
this will work fine, but the dell-privacy module will the mostly just sit 
there (it will provide the sysfs files) without really doing anything.

Also since dell-wmi will depend on dell-privacy when it is enabled,
dell-privacy will always need to be loaded when dell-wmi is loaded.

The dell-privacy code really is an extension to / plugin of the dell-wmi
code; and since the 2 always need to be loaded together anyways, it would
be better to put the code in a single kernel-module (less overhead loading
modules that way) and this also neatly solves the builtin vs module
dependency issue.

This way we can simply make DELL_PRIVACY a boolean option which controls if
privacy support gets added to the dell-wmi module or not, but since it is
now built into the same module (if enabled) we can never have the case where
one part is built into the kernel and the other into a .ko file.

I'm currently preparing a set of changes which implements this, because
this is sort of hard to describe with words and I hope that providing a
patch implemeting the suggested change make things a bit more clear.

I'll send another email when the changes are ready.

Regards,

Hans

Patch

diff --git a/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi b/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
new file mode 100644
index 000000000000..7f9e18705861
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
@@ -0,0 +1,55 @@ 
+What:		/sys/bus/wmi/devices/6932965F-1671-4CEB-B988-D3AB0A901919/dell_privacy_supported_type
+Date:		Apr 2021
+KernelVersion:	5.13
+Contact:	"[e-mail]>"
+Description:
+		Display which dell hardware level privacy devices are supported
+		“Dell Privacy” is a set of HW, FW, and SW features to enhance
+		Dell’s commitment to platform privacy for MIC, Camera, and
+		ePrivacy screens.
+		The supported hardware privacy devices are:
+Attributes:
+		Microphone Mute:
+				Identifies the local microphone can be muted by hardware, no applications
+				is available to capture system mic sound
+
+		Camera Shutter:
+				Identifies camera shutter controlled by hardware, which is a micromechanical
+				shutter assembly that is built onto the camera module to block capturing images
+				from outside the laptop
+
+		supported:
+				The privacy device is supported by this system
+
+		unsupported:
+				The privacy device is not supported on this system
+
+		For example to check which privacy devices are supported:
+
+		# cat /sys/bus/wmi/drivers/dell-privacy/6932965F-1671-4CEB-B988-D3AB0A901919/dell_privacy_supported_type
+		[Microphone Mute] [supported]
+		[Camera Shutter] [supported]
+		[ePrivacy Screen] [unsupported]
+
+What:		/sys/bus/wmi/devices/6932965F-1671-4CEB-B988-D3AB0A901919/dell_privacy_current_state
+Date:		Apr 2021
+KernelVersion:	5.13
+Contact:	"[e-mail]>"
+Description:
+		Allow user space to check current dell privacy device state.
+		Describes the Device State class exposed by BIOS which can be
+		consumed by various applications interested in knowing the Privacy
+		feature capabilities
+Attributes:
+		muted:
+			Identifies the privacy device is turned off and cannot send stream to OS applications
+
+		unmuted:
+			Identifies the privacy device is turned on ,audio or camera driver can get
+			stream from mic and camera module to OS applications
+
+		For example to check all supported current privacy device states:
+
+		# cat /sys/bus/wmi/drivers/dell-privacy/6932965F-1671-4CEB-B988-D3AB0A901919/dell_privacy_current_state
+		[Microphone] [unmuted]
+		[Camera Shutter] [unmuted]
diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
index e0a55337f51a..05d124442b25 100644
--- a/drivers/platform/x86/dell/Kconfig
+++ b/drivers/platform/x86/dell/Kconfig
@@ -204,4 +204,18 @@  config DELL_WMI_SYSMAN
 	  To compile this driver as a module, choose M here: the module will
 	  be called dell-wmi-sysman.
 
+config DELL_PRIVACY
+	tristate "Dell Hardware Privacy Support"
+	depends on ACPI
+	depends on ACPI_WMI
+	depends on INPUT
+	depends on DELL_LAPTOP
+	depends on LEDS_TRIGGER_AUDIO
+	select DELL_WMI
+	help
+	  This driver provides integration with the "Dell Hardware Privacy" feature
+	  of Dell laptops.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called dell_privacy.
 endif # X86_PLATFORM_DRIVERS_DELL
diff --git a/drivers/platform/x86/dell/Makefile b/drivers/platform/x86/dell/Makefile
index d720a3e42ae3..7da0c33dfcca 100644
--- a/drivers/platform/x86/dell/Makefile
+++ b/drivers/platform/x86/dell/Makefile
@@ -19,3 +19,4 @@  obj-$(CONFIG_DELL_WMI_AIO)		+= dell-wmi-aio.o
 obj-$(CONFIG_DELL_WMI_DESCRIPTOR)	+= dell-wmi-descriptor.o
 obj-$(CONFIG_DELL_WMI_LED)		+= dell-wmi-led.o
 obj-$(CONFIG_DELL_WMI_SYSMAN)		+= dell-wmi-sysman/
+obj-$(CONFIG_DELL_PRIVACY)              += dell-privacy-wmi.o
diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
index 70edc5bb3a14..e7ffc0b81208 100644
--- a/drivers/platform/x86/dell/dell-laptop.c
+++ b/drivers/platform/x86/dell/dell-laptop.c
@@ -31,6 +31,8 @@ 
 #include "dell-rbtn.h"
 #include "dell-smbios.h"
 
+#include "dell-privacy-wmi.h"
+
 struct quirk_entry {
 	bool touchpad_led;
 	bool kbd_led_not_present;
@@ -90,6 +92,7 @@  static struct rfkill *wifi_rfkill;
 static struct rfkill *bluetooth_rfkill;
 static struct rfkill *wwan_rfkill;
 static bool force_rfkill;
+static bool has_privacy;
 
 module_param(force_rfkill, bool, 0444);
 MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted models");
@@ -2206,10 +2209,16 @@  static int __init dell_init(void)
 
 	if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
 	    dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) {
-		micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
-		ret = led_classdev_register(&platform_device->dev, &micmute_led_cdev);
-		if (ret < 0)
-			goto fail_led;
+		if (dell_privacy_present())
+			has_privacy = true;
+		else
+			has_privacy = false;
+		if (!has_privacy) {
+			micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
+			ret = led_classdev_register(&platform_device->dev, &micmute_led_cdev);
+			if (ret < 0)
+				goto fail_led;
+		}
 	}
 
 	if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
@@ -2257,7 +2266,8 @@  static int __init dell_init(void)
 fail_get_brightness:
 	backlight_device_unregister(dell_backlight_device);
 fail_backlight:
-	led_classdev_unregister(&micmute_led_cdev);
+	if (!has_privacy)
+		led_classdev_unregister(&micmute_led_cdev);
 fail_led:
 	dell_cleanup_rfkill();
 fail_rfkill:
@@ -2278,7 +2288,8 @@  static void __exit dell_exit(void)
 		touchpad_led_exit();
 	kbd_led_exit();
 	backlight_device_unregister(dell_backlight_device);
-	led_classdev_unregister(&micmute_led_cdev);
+	if (!has_privacy)
+		led_classdev_unregister(&micmute_led_cdev);
 	dell_cleanup_rfkill();
 	if (platform_device) {
 		platform_device_unregister(platform_device);
diff --git a/drivers/platform/x86/dell/dell-privacy-wmi.c b/drivers/platform/x86/dell/dell-privacy-wmi.c
new file mode 100644
index 000000000000..4be7548d1a03
--- /dev/null
+++ b/drivers/platform/x86/dell/dell-privacy-wmi.c
@@ -0,0 +1,394 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Dell privacy notification driver
+ *
+ * Copyright (C) 2021 Dell Inc. All Rights Reserved.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/acpi.h>
+#include <linux/bitops.h>
+#include <linux/input.h>
+#include <linux/input/sparse-keymap.h>
+#include <linux/list.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/wmi.h>
+
+#include "dell-privacy-wmi.h"
+
+#define DELL_PRIVACY_GUID "6932965F-1671-4CEB-B988-D3AB0A901919"
+#define MICROPHONE_STATUS		BIT(0)
+#define CAMERA_STATUS		        BIT(1)
+#define DELL_PRIVACY_AUDIO_EVENT  0x1
+#define DELL_PRIVACY_CAMERA_EVENT 0x2
+#define led_to_priv(c)       container_of(c, struct privacy_wmi_data, cdev)
+
+/*
+ * The wmi_list is used to store the privacy_priv struct with mutex protecting
+ */
+static LIST_HEAD(wmi_list);
+static DEFINE_MUTEX(list_mutex);
+
+struct privacy_wmi_data {
+	struct input_dev *input_dev;
+	struct wmi_device *wdev;
+	struct list_head list;
+	struct led_classdev cdev;
+	u32 features_present;
+	u32 last_status;
+};
+
+/* DELL Privacy Type */
+enum dell_hardware_privacy_type {
+	DELL_PRIVACY_TYPE_AUDIO = 0,
+	DELL_PRIVACY_TYPE_CAMERA,
+	DELL_PRIVACY_TYPE_SCREEN,
+	DELL_PRIVACY_TYPE_MAX,
+};
+
+static const char * const privacy_types[DELL_PRIVACY_TYPE_MAX] = {
+	[DELL_PRIVACY_TYPE_AUDIO] = "Microphone",
+	[DELL_PRIVACY_TYPE_CAMERA] = "Camera Shutter",
+	[DELL_PRIVACY_TYPE_SCREEN] = "ePrivacy Screen",
+};
+
+/*
+ * Keymap for WMI privacy events of type 0x0012
+ */
+static const struct key_entry dell_wmi_keymap_type_0012[] = {
+	/* privacy mic mute */
+	{ KE_KEY, 0x0001, { KEY_MICMUTE } },
+	/* privacy camera mute */
+	{ KE_SW,  0x0002, { SW_CAMERA_LENS_COVER } },
+	{ KE_END, 0},
+};
+
+/*
+ * global privacy state for other modules to query if the privacy driver loaded successfully
+ * or no privacy WMI device was detected at initial probe phase
+ */
+bool dell_privacy_present(void)
+{
+	struct privacy_wmi_data *priv;
+
+	mutex_lock(&list_mutex);
+	priv = list_first_entry_or_null(&wmi_list,
+			struct privacy_wmi_data,
+			list);
+	mutex_unlock(&list_mutex);
+
+	return priv;
+}
+EXPORT_SYMBOL_GPL(dell_privacy_present);
+
+/*
+ * The flow of privacy event:
+ * 1) User presses key. HW does stuff with this key (timeout is started)
+ * 2) WMI event is emitted from BIOS
+ * 3) WMI event is received by dell-privacy
+ * 4) KEY_MICMUTE emitted from dell-privacy
+ * 5) Userland picks up key and modifies kcontrol for SW mute
+ * 6) Codec kernel driver catches and calls ledtrig_audio_set defined by
+ *    dell-privacy-acpi driver. Codec driver will call like this to switch micmute led state.
+ *    ledtrig_audio_set(LED_AUDIO_MICMUTE, micmute_led ? LED_ON :LED_OFF);
+ * 7) If "LED" is set to on dell-privacy notifies EC,and timeout is cancelled,
+ *	HW mic mute activated.
+ */
+bool dell_privacy_process_event(int type, int code, int status)
+{
+	struct privacy_wmi_data *priv;
+	const struct key_entry *key;
+	bool ret = false;
+
+	mutex_lock(&list_mutex);
+	priv = list_first_entry_or_null(&wmi_list,
+			struct privacy_wmi_data,
+			list);
+	if (!priv) {
+		goto error;
+	}
+
+	key = sparse_keymap_entry_from_scancode(priv->input_dev, (type << 16) | code);
+	if (!key) {
+		dev_warn(&priv->wdev->dev, "Unknown key with type 0x%04x and code 0x%04x pressed\n",
+			type, code);
+		goto error;
+	}
+	dev_dbg(&priv->wdev->dev, "Key with type 0x%04x and code 0x%04x pressed\n", type, code);
+
+	switch (code) {
+	case DELL_PRIVACY_AUDIO_EVENT: /* Mic mute */
+	case DELL_PRIVACY_CAMERA_EVENT: /* Camera mute */
+		priv->last_status = status;
+		sparse_keymap_report_entry(priv->input_dev, key, 1, true);
+		ret = true;
+		break;
+	default:
+		dev_dbg(&priv->wdev->dev, "unknown event type 0x%04x 0x%04x\n", type, code);
+	}
+
+error:
+	mutex_unlock(&list_mutex);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dell_privacy_process_event);
+
+static ssize_t dell_privacy_supported_type_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	struct privacy_wmi_data *priv = dev_get_drvdata(dev);
+	enum dell_hardware_privacy_type type;
+	u32 privacy_list;
+	char *s = buf;
+
+	privacy_list = priv->features_present;
+	for (type = DELL_PRIVACY_TYPE_AUDIO; type < DELL_PRIVACY_TYPE_MAX; type++) {
+		if (privacy_list & BIT(type))
+			s += sprintf(s, "[%s] [supported]\n", privacy_types[type]);
+		else
+			s += sprintf(s, "[%s] [unsupported]\n", privacy_types[type]);
+	}
+
+	return (s - buf);
+}
+
+static ssize_t dell_privacy_current_state_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	struct privacy_wmi_data *priv = dev_get_drvdata(dev);
+	u32 privacy_supported = priv->features_present;
+	enum dell_hardware_privacy_type type;
+	u32 privacy_state = priv->last_status;
+	char *s = buf;
+
+	for (type = DELL_PRIVACY_TYPE_AUDIO; type < DELL_PRIVACY_TYPE_MAX; type++) {
+		if (privacy_supported & BIT(type)) {
+			if (privacy_state & BIT(type))
+				s += sprintf(s, "[%s] [unmuted]\n", privacy_types[type]);
+			else
+				s += sprintf(s, "[%s] [muted]\n", privacy_types[type]);
+		}
+	}
+
+	return (s - buf);
+}
+
+static DEVICE_ATTR_RO(dell_privacy_supported_type);
+static DEVICE_ATTR_RO(dell_privacy_current_state);
+
+static struct attribute *privacy_attributes[] = {
+	&dev_attr_dell_privacy_supported_type.attr,
+	&dev_attr_dell_privacy_current_state.attr,
+	NULL,
+};
+
+static const struct attribute_group privacy_attribute_group = {
+	.attrs = privacy_attributes
+};
+
+/*
+ * Describes the Device State class exposed by BIOS which can be consumed by
+ * various applications interested in knowing the Privacy feature capabilities.
+ * class DeviceState
+ * {
+ *  [key, read] string InstanceName;
+ *  [read] boolean ReadOnly;
+ *
+ *  [WmiDataId(1), read] uint32 DevicesSupported;
+ *   0 - None; 0x1 - Microphone; 0x2 - Camera; 0x4 - ePrivacy  Screen
+ *
+ *  [WmiDataId(2), read] uint32 CurrentState;
+ *   0 - Off; 1 - On; Bit0 - Microphone; Bit1 - Camera; Bit2 - ePrivacyScreen
+ * };
+ */
+static int get_current_status(struct wmi_device *wdev)
+{
+	struct privacy_wmi_data *priv = dev_get_drvdata(&wdev->dev);
+	union acpi_object *obj_present;
+	u32 *buffer;
+	int ret = 0;
+
+	if (!priv) {
+		dev_err(&wdev->dev, "dell privacy priv is NULL\n");
+		return -EINVAL;
+	}
+	/* check privacy support features and device states */
+	obj_present = wmidev_block_query(wdev, 0);
+	if (!obj_present) {
+		dev_err(&wdev->dev, "failed to read Binary MOF\n");
+		return -EIO;
+	}
+
+	if (obj_present->type != ACPI_TYPE_BUFFER) {
+		dev_err(&wdev->dev, "Binary MOF is not a buffer!\n");
+		ret = -EIO;
+		goto obj_free;
+	}
+	/*  Although it's not technically a failure, this would lead to
+	 *  unexpected behavior
+	 */
+	if (obj_present->buffer.length != 8) {
+		dev_err(&wdev->dev, "Dell privacy buffer has unexpected length (%d)!\n",
+				obj_present->buffer.length);
+		ret = -EINVAL;
+		goto obj_free;
+	}
+	buffer = (u32 *)obj_present->buffer.pointer;
+	priv->features_present = buffer[0];
+	priv->last_status = buffer[1];
+
+obj_free:
+	kfree(obj_present);
+	return ret;
+}
+
+static int dell_privacy_micmute_led_set(struct led_classdev *led_cdev,
+					enum led_brightness brightness)
+{
+	struct privacy_wmi_data *priv = led_to_priv(led_cdev);
+	static char *acpi_method = (char *)"ECAK";
+	acpi_status status;
+	acpi_handle handle;
+
+	handle = ec_get_handle();
+	if (!handle)
+		return -EIO;
+
+	if (!acpi_has_method(handle, acpi_method))
+		return -EIO;
+
+	status = acpi_evaluate_object(handle, acpi_method, NULL, NULL);
+	if (ACPI_FAILURE(status)) {
+		dev_err(&priv->wdev->dev, "Error setting privacy EC ack value: %s\n",
+				acpi_format_exception(status));
+		return -EIO;
+	}
+
+	return 0;
+}
+
+/*
+ * Pressing the mute key activates a time delayed circuit to physically cut
+ * off the mute. The LED is in the same circuit, so it reflects the true
+ * state of the HW mute.  The reason for the EC "ack" is so that software
+ * can first invoke a SW mute before the HW circuit is cut off.  Without SW
+ * cutting this off first does not affect the time delayed muting or status
+ * of the LED but there is a possibility of a "popping" noise.
+ *
+ * If the EC receives the SW ack, the circuit will be activated before the
+ * delay completed.
+ *
+ * Exposing as an LED device allows the codec drivers notification path to
+ * EC ACK to work
+ */
+static int dell_privacy_leds_setup(struct device *dev)
+{
+	struct privacy_wmi_data *priv = dev_get_drvdata(dev);
+
+	priv->cdev.name = "dell-privacy::micmute";
+	priv->cdev.max_brightness = 1;
+	priv->cdev.brightness_set_blocking = dell_privacy_micmute_led_set;
+	priv->cdev.default_trigger = "audio-micmute";
+	priv->cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
+	return devm_led_classdev_register(dev, &priv->cdev);
+}
+
+static int dell_privacy_wmi_probe(struct wmi_device *wdev, const void *context)
+{
+	struct privacy_wmi_data *priv;
+	struct key_entry *keymap;
+	int ret, i;
+
+	ret = wmi_has_guid(DELL_PRIVACY_GUID);
+	if (!ret)
+		pr_debug("Unable to detect available Dell privacy devices!\n");
+
+	priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	dev_set_drvdata(&wdev->dev, priv);
+	priv->wdev = wdev;
+	/* create evdev passing interface */
+	priv->input_dev = devm_input_allocate_device(&wdev->dev);
+	if (!priv->input_dev)
+		return -ENOMEM;
+
+	/* remap the wmi keymap event to new keymap */
+	keymap = kcalloc(ARRAY_SIZE(dell_wmi_keymap_type_0012),
+			sizeof(struct key_entry), GFP_KERNEL);
+	if (!keymap)
+		return -ENOMEM;
+
+	/* remap the keymap code with Dell privacy key type 0x12 as prefix
+	 * KEY_MICMUTE scancode will be reported as 0x120001
+	 */
+	for (i = 0; i < ARRAY_SIZE(dell_wmi_keymap_type_0012); i++) {
+		keymap[i] = dell_wmi_keymap_type_0012[i];
+		keymap[i].code |= (0x0012 << 16);
+	}
+	ret = sparse_keymap_setup(priv->input_dev, keymap, NULL);
+	kfree(keymap);
+	if (ret)
+		return ret;
+
+	priv->input_dev->dev.parent = &wdev->dev;
+	priv->input_dev->name = "Dell Privacy Driver";
+	priv->input_dev->id.bustype = BUS_HOST;
+
+	ret = input_register_device(priv->input_dev);
+	if (ret)
+		return ret;
+
+	ret = get_current_status(priv->wdev);
+	if (ret)
+		return ret;
+
+	ret = devm_device_add_group(&wdev->dev, &privacy_attribute_group);
+	if (ret)
+		return ret;
+
+	ret = dell_privacy_leds_setup(&priv->wdev->dev);
+	if (ret)
+		return ret;
+
+	mutex_lock(&list_mutex);
+	list_add_tail(&priv->list, &wmi_list);
+	mutex_unlock(&list_mutex);
+	return 0;
+}
+
+static int dell_privacy_wmi_remove(struct wmi_device *wdev)
+{
+	struct privacy_wmi_data *priv = dev_get_drvdata(&wdev->dev);
+
+	mutex_lock(&list_mutex);
+	list_del(&priv->list);
+	mutex_unlock(&list_mutex);
+	return 0;
+}
+
+static const struct wmi_device_id dell_wmi_privacy_wmi_id_table[] = {
+	{ .guid_string = DELL_PRIVACY_GUID },
+	{ },
+};
+
+static struct wmi_driver dell_privacy_wmi_driver = {
+	.driver = {
+		.name = "dell-privacy",
+	},
+	.probe = dell_privacy_wmi_probe,
+	.remove = dell_privacy_wmi_remove,
+	.id_table = dell_wmi_privacy_wmi_id_table,
+};
+
+module_wmi_driver(dell_privacy_wmi_driver);
+
+MODULE_DEVICE_TABLE(wmi, dell_wmi_privacy_wmi_id_table);
+MODULE_AUTHOR("Perry Yuan <[e-mail]>");
+MODULE_DESCRIPTION("Dell Privacy WMI Driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/platform/x86/dell/dell-privacy-wmi.h b/drivers/platform/x86/dell/dell-privacy-wmi.h
new file mode 100644
index 000000000000..04c49d2fa924
--- /dev/null
+++ b/drivers/platform/x86/dell/dell-privacy-wmi.h
@@ -0,0 +1,23 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Dell privacy notification driver
+ *
+ * Copyright (C) 2021 Dell Inc. All Rights Reserved.
+ */
+
+#ifndef _DELL_PRIVACY_WMI_H_
+#define _DELL_PRIVACY_WMI_H_
+
+#if IS_ENABLED(CONFIG_DELL_PRIVACY)
+bool dell_privacy_present(void);
+bool dell_privacy_process_event(int type, int code, int status);
+#else /* CONFIG_DELL_PRIVACY */
+static inline bool dell_privacy_present(void)
+{
+	return -ENODEV;
+}
+
+static inline bool dell_privacy_process_event(int type, int code, int status)
+{}
+#endif /* CONFIG_DELL_PRIVACY */
+#endif
diff --git a/drivers/platform/x86/dell/dell-wmi.c b/drivers/platform/x86/dell/dell-wmi.c
index bbdb3e860892..228e85a427f5 100644
--- a/drivers/platform/x86/dell/dell-wmi.c
+++ b/drivers/platform/x86/dell/dell-wmi.c
@@ -27,6 +27,7 @@ 
 #include <acpi/video.h>
 #include "dell-smbios.h"
 #include "dell-wmi-descriptor.h"
+#include "dell-privacy-wmi.h"
 
 MODULE_AUTHOR("Matthew Garrett <[e-mail]>");
 MODULE_AUTHOR("Pali Rohár <[e-mail]>");
@@ -427,7 +428,6 @@  static void dell_wmi_notify(struct wmi_device *wdev,
 
 		switch (buffer_entry[1]) {
 		case 0x0000: /* One key pressed or event occurred */
-		case 0x0012: /* Event with extended data occurred */
 			if (len > 2)
 				dell_wmi_process_key(wdev, buffer_entry[1],
 						     buffer_entry[2]);
@@ -439,6 +439,12 @@  static void dell_wmi_notify(struct wmi_device *wdev,
 				dell_wmi_process_key(wdev, buffer_entry[1],
 						     buffer_entry[i]);
 			break;
+		case 0x0012:
+			if ((len > 4) && dell_privacy_process_event(buffer_entry[1], buffer_entry[3], buffer_entry[4]))
+				/* privacy hotkey handle */;
+			else if (len > 2)
+				dell_wmi_process_key(wdev, buffer_entry[1], buffer_entry[2]);
+			break;
 		default: /* Unknown event */
 			pr_info("Unknown WMI event type 0x%x\n",
 				(int)buffer_entry[1]);