platform/x86: dell-privacy: Add support for new privacy driver

Commit Message

Yuan, Perry Nov. 3, 2020, 12:55 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
 and camera mute hotkey is ctrl+F9.

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

Comments

kernel test robot Nov. 3, 2020, 4:22 p.m. UTC | #1

Hi Perry,

Thank you for the patch! Perhaps something to improve:

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

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

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

All warnings (new ones prefixed by >>):

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

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

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

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

kernel test robot Nov. 3, 2020, 4:50 p.m. UTC | #2

Hi Perry,

Thank you for the patch! Perhaps something to improve:

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

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

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

All warnings (new ones prefixed by >>):

   drivers/platform/x86/dell-laptop.c: In function 'dell_exit':
>> drivers/platform/x86/dell-laptop.c:2289:5: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
    2289 |     if (!privacy_valid)
         |     ^~
   drivers/platform/x86/dell-laptop.c:2291:2: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if'
    2291 |  dell_cleanup_rfkill();
         |  ^~~~~~~~~~~~~~~~~~~

vim +/if +2289 drivers/platform/x86/dell-laptop.c

  2280	
  2281	static void __exit dell_exit(void)
  2282	{
  2283		dell_laptop_unregister_notifier(&dell_laptop_notifier);
  2284		debugfs_remove_recursive(dell_laptop_dir);
  2285		if (quirks && quirks->touchpad_led)
  2286			touchpad_led_exit();
  2287		kbd_led_exit();
  2288		backlight_device_unregister(dell_backlight_device);
> 2289	    if (!privacy_valid)
  2290	        led_classdev_unregister(&micmute_led_cdev);
  2291		dell_cleanup_rfkill();
  2292		if (platform_device) {
  2293			platform_device_unregister(platform_device);
  2294			platform_driver_unregister(&platform_driver);
  2295		}
  2296	}
  2297	

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

Barnabás Pőcze Nov. 3, 2020, 7:14 p.m. UTC | #3

Hi

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

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

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

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

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

I have also added a couple comments inline.


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

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


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

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


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

`dell_privacy_valid()` returns `bool`.


> #endif
>         if (!privacy_valid) {
>             micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
>             ret = led_classdev_register(&platform_device->dev, &micmute_led_cdev);
>             if (ret < 0)
>                 goto fail_led;
>         }
>     }
>
>     if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
>         return 0;
>
>     token = dell_smbios_find_token(BRIGHTNESS_TOKEN);
> 	if (token) {
> 		struct calling_interface_buffer buffer;
>
> @@ -2257,7 +2264,8 @@ static int __init dell_init(void)
> fail_get_brightness:
> 	backlight_device_unregister(dell_backlight_device);
> fail_backlight:
> 	led_classdev_unregister(&micmute_led_cdev);
>     if (!privacy_valid)
>         led_classdev_unregister(&micmute_led_cdev);
> fail_led:
> 	dell_cleanup_rfkill();
> fail_rfkill:
> @@ -2278,7 +2286,8 @@ static void __exit dell_exit(void)
> 		touchpad_led_exit();
> 	kbd_led_exit();
> 	backlight_device_unregister(dell_backlight_device);
> 	led_classdev_unregister(&micmute_led_cdev);
>     if (!privacy_valid)
>         led_classdev_unregister(&micmute_led_cdev);
> 	dell_cleanup_rfkill();
> 	if (platform_device) {
> 		platform_device_unregister(platform_device);
> diff --git a/drivers/platform/x86/dell-privacy-acpi.c b/drivers/platform/x86/dell-privacy-acpi.c
> new file mode 100644
> index 000000000000..516cd99167c3
> --- /dev/null
> +++ b/drivers/platform/x86/dell-privacy-acpi.c
> @@ -0,0 +1,139 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Dell privacy notification driver
> + *
> + * Copyright (C) 2021 Dell Inc. All Rights Reserved.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/acpi.h>
> +#include <linux/device.h>
> +#include <linux/fs.h>
> +#include <linux/kernel.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/string.h>
> +#include <linux/sysfs.h>
> +#include <linux/types.h>
> +#include <linux/wmi.h>
> +#include <linux/slab.h>
> +#include <linux/platform_device.h>
> +#include "dell-privacy-wmi.h"
> +
> +#define PRIVACY_PlATFORM_NAME  "dell-privacy-acpi"
                    ^
should be upper case


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

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


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

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

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

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


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

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


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

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

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


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

The platform device is not cleaned up.


> +
> +MODULE_AUTHOR("Perry Yuan <[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..6c36b7ec44c6
> --- /dev/null
> +++ b/drivers/platform/x86/dell-privacy-wmi.c
> @@ -0,0 +1,259 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Dell privacy notification driver
> + *
> + * Copyright (C) 2021 Dell Inc. All Rights Reserved.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/acpi.h>
> +#include <linux/input.h>
> +#include <linux/input/sparse-keymap.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/wmi.h>
> +#include "dell-privacy-wmi.h"
> +
> +#define DELL_PRIVACY_GUID "6932965F-1671-4CEB-B988-D3AB0A901919"
> +#define MICROPHONE_STATUS		    BIT(0)
> +#define CAMERA_STATUS		        BIT(1)
> +#define PRIVACY_SCREEN_STATUS		BIT(2)

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


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

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


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

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


> +};
> +
> +/*
> + * Keymap for WMI Privacy events of type 0x0012
> + */
> +static const struct key_entry dell_wmi_keymap_type_0012[] = {
> +    /* Privacy MIC Mute */
> +    { KE_KEY, 0x0001, { KEY_MICMUTE } },
> +    /* Privacy Camera Mute */
> +    { KE_SW,  0x0002, { SW_CAMERA_LENS_COVER } },

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


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

The functions returns `bool`.


> +    }
> +    ret = privacy_valid;

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


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

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


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

`if (!priv)`


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

A couple lines above hexadecimal format and capitalization is used.


> +                    type, code);
> +    }
> +error:
> +    mutex_unlock(&list_mutex);
> +    return;
> +}
> +EXPORT_SYMBOL_GPL(dell_privacy_process_event);
> [...]
> +static int dell_privacy_wmi_probe(struct wmi_device *wdev, const void *context)
> +{
> +    struct privacy_wmi_data *priv;
> +    struct key_entry *keymap;
> +    int ret, i, pos = 0;
> +
> +    priv = devm_kzalloc(&wdev->dev, sizeof(struct privacy_wmi_data),
> +            GFP_KERNEL);

`sizeof(*priv)`


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

`sparse_keymap_setup()` takes care of this.


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

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


> +    ret = sparse_keymap_setup(priv->input_dev, keymap, NULL);
> +    if (ret)
> +        return ret;
> +
> +    priv->input_dev->dev.parent = &wdev->dev;
> +    priv->input_dev->name = "Dell Privacy Driver";
> +    priv->input_dev->id.bustype = BUS_HOST;
> +
> +    if (input_register_device(priv->input_dev)) {
> +        pr_debug("input_register_device failed to register! \n");
> +        return -ENODEV;

`keymap` is leaked here.


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

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


> +    }
> +    mutex_unlock(&list_mutex);
> +    kfree(keymap);
> +    return 0;
> +
> +err_free_dev:
> +    input_free_device(priv->input_dev);
> +    kfree(keymap);
> +    return ret;
> +}
> +
> +static int dell_privacy_wmi_remove(struct wmi_device *wdev)
> +{
> +    struct privacy_wmi_data *priv = dev_get_drvdata(&wdev->dev);
> +
> +    mutex_lock(&list_mutex);
> +    list_del(&priv->list);
> +    privacy_valid = -ENODEV;
> +    mutex_unlock(&list_mutex);
> +    return 0;
> +}
> +
> +static const struct wmi_device_id dell_wmi_privacy_wmi_id_table[] = {
> +    { .guid_string = DELL_PRIVACY_GUID },
> +    { },
> +};
> +
> +static struct wmi_driver dell_privacy_wmi_driver = {
> +    .driver = {
> +        .name = "dell-privacy",
> +    },
> +    .probe = dell_privacy_wmi_probe,
> +    .remove = dell_privacy_wmi_remove,
> +    .id_table = dell_wmi_privacy_wmi_id_table,
> +};
> +
> +static int __init init_dell_privacy(void)
> +{
> +    int ret, wmi, acpi;

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

```
int ret;

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

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

...

return 0;


undo_step_2:
  ...
undo_step_1:
  ....

return ret;
```


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

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

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


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

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


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

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


> +MODULE_AUTHOR("Perry Yuan <[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..94af81d76e44
> --- /dev/null
> +++ b/drivers/platform/x86/dell-privacy-wmi.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Dell privacy notification driver
> + *
> + * Copyright (C) 2021 Dell Inc. All Rights Reserved.
> + */
> +
> +#ifndef _DELL_PRIVACY_WMI_H_
> +#define _DELL_PRIVACY_WMI_H_
> +#include <linux/wmi.h>

This include is not needed.


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

These aren't prefixed by `dell_`?


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

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


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


Regards,
Barnabás Pőcze

Matthew Garrett Nov. 4, 2020, 1:49 a.m. UTC | #4

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

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

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

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

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

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

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

kernel test robot Nov. 4, 2020, 6:43 a.m. UTC | #5

Hi Perry,

Thank you for the patch! Perhaps something to improve:

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

url:    https://github.com/0day-ci/linux/commits/Perry-Yuan/platform-x86-dell-privacy-Add-support-for-new-privacy-driver/20201103-205721
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git b7cbaf59f62f8ab8f157698f9e31642bff525bd0
config: x86_64-randconfig-m001-20201104 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0

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

smatch warnings:
drivers/platform/x86/dell-wmi.c:414 dell_wmi_notify() warn: inconsistent indenting
drivers/platform/x86/dell-laptop.c:2207 dell_init() warn: inconsistent indenting
drivers/platform/x86/dell-laptop.c:2289 dell_exit() warn: inconsistent indenting
drivers/platform/x86/dell-laptop.c:2291 dell_exit() warn: curly braces intended?

vim +414 drivers/platform/x86/dell-wmi.c

83fc44c32ad8b8b Pali Rohár      2014-11-11  377  
bff589be59c5092 Andy Lutomirski 2015-11-25  378  static void dell_wmi_notify(struct wmi_device *wdev,
bff589be59c5092 Andy Lutomirski 2015-11-25  379  			    union acpi_object *obj)
0b3f6109f0c9ff9 Matthew Garrett 2009-01-09  380  {
00ebbeb39b70072 Andy Lutomirski 2017-08-01  381  	struct dell_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
83fc44c32ad8b8b Pali Rohár      2014-11-11  382  	u16 *buffer_entry, *buffer_end;
bff589be59c5092 Andy Lutomirski 2015-11-25  383  	acpi_size buffer_size;
83fc44c32ad8b8b Pali Rohár      2014-11-11  384  	int len, i;
0b3f6109f0c9ff9 Matthew Garrett 2009-01-09  385  
83fc44c32ad8b8b Pali Rohár      2014-11-11  386  	if (obj->type != ACPI_TYPE_BUFFER) {
83fc44c32ad8b8b Pali Rohár      2014-11-11  387  		pr_warn("bad response type %x\n", obj->type);
5ea2559726b7862 Rezwanul Kabir  2009-11-02  388  		return;
5ea2559726b7862 Rezwanul Kabir  2009-11-02  389  	}
5ea2559726b7862 Rezwanul Kabir  2009-11-02  390  
83fc44c32ad8b8b Pali Rohár      2014-11-11  391  	pr_debug("Received WMI event (%*ph)\n",
83fc44c32ad8b8b Pali Rohár      2014-11-11  392  		obj->buffer.length, obj->buffer.pointer);
83fc44c32ad8b8b Pali Rohár      2014-11-11  393  
83fc44c32ad8b8b Pali Rohár      2014-11-11  394  	buffer_entry = (u16 *)obj->buffer.pointer;
83fc44c32ad8b8b Pali Rohár      2014-11-11  395  	buffer_size = obj->buffer.length/2;
83fc44c32ad8b8b Pali Rohár      2014-11-11  396  	buffer_end = buffer_entry + buffer_size;
83fc44c32ad8b8b Pali Rohár      2014-11-11  397  
481fe5be821c3d0 Pali Rohár      2016-01-04  398  	/*
481fe5be821c3d0 Pali Rohár      2016-01-04  399  	 * BIOS/ACPI on devices with WMI interface version 0 does not clear
481fe5be821c3d0 Pali Rohár      2016-01-04  400  	 * buffer before filling it. So next time when BIOS/ACPI send WMI event
481fe5be821c3d0 Pali Rohár      2016-01-04  401  	 * which is smaller as previous then it contains garbage in buffer from
481fe5be821c3d0 Pali Rohár      2016-01-04  402  	 * previous event.
481fe5be821c3d0 Pali Rohár      2016-01-04  403  	 *
481fe5be821c3d0 Pali Rohár      2016-01-04  404  	 * BIOS/ACPI on devices with WMI interface version 1 clears buffer and
481fe5be821c3d0 Pali Rohár      2016-01-04  405  	 * sometimes send more events in buffer at one call.
481fe5be821c3d0 Pali Rohár      2016-01-04  406  	 *
481fe5be821c3d0 Pali Rohár      2016-01-04  407  	 * So to prevent reading garbage from buffer we will process only first
481fe5be821c3d0 Pali Rohár      2016-01-04  408  	 * one event on devices with WMI interface version 0.
481fe5be821c3d0 Pali Rohár      2016-01-04  409  	 */
00ebbeb39b70072 Andy Lutomirski 2017-08-01  410  	if (priv->interface_version == 0 && buffer_entry < buffer_end)
481fe5be821c3d0 Pali Rohár      2016-01-04  411  		if (buffer_end > buffer_entry + buffer_entry[0] + 1)
481fe5be821c3d0 Pali Rohár      2016-01-04  412  			buffer_end = buffer_entry + buffer_entry[0] + 1;
481fe5be821c3d0 Pali Rohár      2016-01-04  413  
83fc44c32ad8b8b Pali Rohár      2014-11-11 @414      while (buffer_entry < buffer_end) {
83fc44c32ad8b8b Pali Rohár      2014-11-11  415  
83fc44c32ad8b8b Pali Rohár      2014-11-11  416          len = buffer_entry[0];
83fc44c32ad8b8b Pali Rohár      2014-11-11  417          if (len == 0)
83fc44c32ad8b8b Pali Rohár      2014-11-11  418              break;
83fc44c32ad8b8b Pali Rohár      2014-11-11  419  
83fc44c32ad8b8b Pali Rohár      2014-11-11  420          len++;
83fc44c32ad8b8b Pali Rohár      2014-11-11  421  
83fc44c32ad8b8b Pali Rohár      2014-11-11  422          if (buffer_entry + len > buffer_end) {
83fc44c32ad8b8b Pali Rohár      2014-11-11  423              pr_warn("Invalid length of WMI event\n");
83fc44c32ad8b8b Pali Rohár      2014-11-11  424              break;
0b3f6109f0c9ff9 Matthew Garrett 2009-01-09  425          }
83fc44c32ad8b8b Pali Rohár      2014-11-11  426  
83fc44c32ad8b8b Pali Rohár      2014-11-11  427          pr_debug("Process buffer (%*ph)\n", len*2, buffer_entry);
83fc44c32ad8b8b Pali Rohár      2014-11-11  428  
83fc44c32ad8b8b Pali Rohár      2014-11-11  429          switch (buffer_entry[1]) {
e075b3c898e4055 Pali Rohár      2016-06-15  430              case 0x0000: /* One key pressed or event occurred */
e075b3c898e4055 Pali Rohár      2016-06-15  431                  if (len > 2)
0c026c361be1734 Y Paritcher     2020-06-10  432                      dell_wmi_process_key(wdev, buffer_entry[1],
bff589be59c5092 Andy Lutomirski 2015-11-25  433                              buffer_entry[2]);
83fc44c32ad8b8b Pali Rohár      2014-11-11  434                  break;
e075b3c898e4055 Pali Rohár      2016-06-15  435              case 0x0010: /* Sequence of keys pressed */
e075b3c898e4055 Pali Rohár      2016-06-15  436              case 0x0011: /* Sequence of events occurred */
83fc44c32ad8b8b Pali Rohár      2014-11-11  437                  for (i = 2; i < len; ++i)
bff589be59c5092 Andy Lutomirski 2015-11-25  438                      dell_wmi_process_key(wdev, buffer_entry[1],
e075b3c898e4055 Pali Rohár      2016-06-15  439                              buffer_entry[i]);
83fc44c32ad8b8b Pali Rohár      2014-11-11  440                  break;
cee9f60d7ca58d8 perry_yuan      2020-11-03  441              case 0x0012:
cee9f60d7ca58d8 perry_yuan      2020-11-03  442  #if IS_ENABLED(CONFIG_DELL_PRIVACY)
cee9f60d7ca58d8 perry_yuan      2020-11-03  443                  if (dell_privacy_valid()) {
cee9f60d7ca58d8 perry_yuan      2020-11-03  444                      dell_privacy_process_event(buffer_entry[1], buffer_entry[3], 
cee9f60d7ca58d8 perry_yuan      2020-11-03  445                              buffer_entry[4]);
cee9f60d7ca58d8 perry_yuan      2020-11-03  446                  } else {
cee9f60d7ca58d8 perry_yuan      2020-11-03  447                      if (len > 2)
cee9f60d7ca58d8 perry_yuan      2020-11-03  448                          dell_wmi_process_key(wdev, buffer_entry[1], buffer_entry[2]);
cee9f60d7ca58d8 perry_yuan      2020-11-03  449                  }
cee9f60d7ca58d8 perry_yuan      2020-11-03  450  #else
cee9f60d7ca58d8 perry_yuan      2020-11-03  451                  /* Extended data is currently ignored */
cee9f60d7ca58d8 perry_yuan      2020-11-03  452                  if (len > 2)
cee9f60d7ca58d8 perry_yuan      2020-11-03  453                      dell_wmi_process_key(wdev, buffer_entry[1], buffer_entry[2]);
cee9f60d7ca58d8 perry_yuan      2020-11-03  454  #endif
cee9f60d7ca58d8 perry_yuan      2020-11-03  455                  break;
e075b3c898e4055 Pali Rohár      2016-06-15  456              default: /* Unknown event */
83fc44c32ad8b8b Pali Rohár      2014-11-11  457                  pr_info("Unknown WMI event type 0x%x\n",
83fc44c32ad8b8b Pali Rohár      2014-11-11  458                          (int)buffer_entry[1]);
83fc44c32ad8b8b Pali Rohár      2014-11-11  459                  break;
0b3f6109f0c9ff9 Matthew Garrett 2009-01-09  460          }
83fc44c32ad8b8b Pali Rohár      2014-11-11  461  
83fc44c32ad8b8b Pali Rohár      2014-11-11  462          buffer_entry += len;
83fc44c32ad8b8b Pali Rohár      2014-11-11  463  
83fc44c32ad8b8b Pali Rohár      2014-11-11  464      }
83fc44c32ad8b8b Pali Rohár      2014-11-11  465  

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

Hans de Goede Nov. 9, 2020, 11:16 a.m. UTC | #6

Hi,

On 11/3/20 1:55 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
>  and camera mute hotkey is ctrl+F9.
> 
> Signed-off-by: Perry Yuan  <[e-mail]>
> Signed-off-by: Limonciello Mario <[e-mail]>

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

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

Regards,

Hans


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

Yuan, Perry Nov. 11, 2020, 7:21 a.m. UTC | #7

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

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


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

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

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

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


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

 

 
> 
> --
> Matthew Garrett | [e-mail]

Yuan, Perry Nov. 11, 2020, 7:24 a.m. UTC | #8

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

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

Perry

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

Matthew Garrett Nov. 11, 2020, 7:24 a.m. UTC | #9

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

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

Limonciello, Mario Nov. 11, 2020, 2:30 p.m. UTC | #10

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

Matthew,

Pressing the mute key activates a time delayed circuit to physically cut
off the mute.  The LED is in the same circuit, so it reflects the true
state of the HW mute.  The reason for the EC "ack" is so that software
can first invoke a SW mute before the HW circuit is cut off.  Without SW
cutting this off first does not affect the time delayed muting or status
of the LED but there is a possibility of a "popping" noise leading to a
poor user experience.

If the EC receives the SW ack, the circuit will be activated before the
delay completed.

Exposing as an LED device allows the codec drivers notification path to
EC ACK to work.  Later follow up work is already envisioned that if HW
mute is already enacted but SW mute is modified (IE LED notifier goes
through) that a message can come back out to userspace to tell the user
something along the lines of

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

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

Perry,

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

Enrico Weigelt, metux IT consult Nov. 12, 2020, 3:06 p.m. UTC | #11

On 11.11.20 15:30, Limonciello, Mario wrote:

Hi,

> Pressing the mute key activates a time delayed circuit to physically cut
> off the mute.  The LED is in the same circuit, so it reflects the true
> state of the HW mute.  The reason for the EC "ack" is so that software
> can first invoke a SW mute before the HW circuit is cut off.  Without SW
> cutting this off first does not affect the time delayed muting or status
> of the LED but there is a possibility of a "popping" noise leading to a
> poor user experience.

how long is that timeout ?

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

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


--mtx

Limonciello, Mario Nov. 12, 2020, 3:31 p.m. UTC | #12

> > Pressing the mute key activates a time delayed circuit to physically cut
> > off the mute.  The LED is in the same circuit, so it reflects the true
> > state of the HW mute.  The reason for the EC "ack" is so that software
> > can first invoke a SW mute before the HW circuit is cut off.  Without SW
> > cutting this off first does not affect the time delayed muting or status
> > of the LED but there is a possibility of a "popping" noise leading to a
> > poor user experience.
> 
> how long is that timeout ?

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

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

The flow is like this:

1) User presses key.  HW does stuff with this key (timeout is started)
2) Event is emitted from FW
3) Event received by dell-privacy
4) KEY_MICMUTE emitted from dell-privacy
5) Userland picks up key and modifies kcontrol for SW mute
6) Codec kernel driver catches and calls ledtrig_audio_set, like this:

ledtrig_audio_set(LED_AUDIO_MICMUTE, rt715->micmute_led ? LED_ON : LED_OFF);

7) If "LED" is set to on dell-privacy notifies ec, and timeout is cancelled,
HW mic mute activated.

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

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

Hans de Goede Nov. 12, 2020, 3:55 p.m. UTC | #13

Hi,

On 11/12/20 4:31 PM, Limonciello, Mario wrote:
>>> Pressing the mute key activates a time delayed circuit to physically cut
>>> off the mute.  The LED is in the same circuit, so it reflects the true
>>> state of the HW mute.  The reason for the EC "ack" is so that software
>>> can first invoke a SW mute before the HW circuit is cut off.  Without SW
>>> cutting this off first does not affect the time delayed muting or status
>>> of the LED but there is a possibility of a "popping" noise leading to a
>>> poor user experience.
>>
>> how long is that timeout ?
> 
> The exact duration is controlled by component selection in the circuit.
> Linux is typically able to respond faster than Windows in this case.
> 
>>
>>> Exposing as an LED device allows the codec drivers notification path to
>>> EC ACK to work.
>>
>> Which driver exactly ? Who's gonna access this LED ?
> 
> The flow is like this:
> 
> 1) User presses key.  HW does stuff with this key (timeout is started)
> 2) Event is emitted from FW
> 3) Event received by dell-privacy
> 4) KEY_MICMUTE emitted from dell-privacy
> 5) Userland picks up key and modifies kcontrol for SW mute
> 6) Codec kernel driver catches and calls ledtrig_audio_set, like this:
> 
> ledtrig_audio_set(LED_AUDIO_MICMUTE, rt715->micmute_led ? LED_ON : LED_OFF);
> 
> 7) If "LED" is set to on dell-privacy notifies ec, and timeout is cancelled,
> HW mic mute activated.
> 
> Again, if anything in this flow doesn't happen HW mic mute is still activated,
> just will take longer (for duration of timeout) and have popping noise.

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

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

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

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

Regards,

Hans



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

Limonciello, Mario Nov. 12, 2020, 3:57 p.m. UTC | #14

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

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

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

Yes - exactly.

Patch

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