RE: [PATCH v6 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy

[Yuan, Perry]

Hi Hans, Barnabás.

Thanks for your review very much!
I have no concern to change the most of the comments.
Just one sysfs_emit replacement will cause problem , adding comments at below. 

> -----Original Message-----
> From: Hans de Goede <hdegoede@xxxxxxxxxx>
> Sent: Wednesday, April 7, 2021 11:16 PM
> To: Barnabás Pőcze; Yuan, Perry
> Cc: pierre-louis.bossart@xxxxxxxxxxxxxxx; oder_chiou@xxxxxxxxxxx;
> perex@xxxxxxxx; tiwai@xxxxxxxx; mgross@xxxxxxxxxxxxxxx; Limonciello,
> Mario; lgirdwood@xxxxxxxxx; broonie@xxxxxxxxxx; alsa-devel@alsa-
> project.org; linux-kernel@xxxxxxxxxxxxxxx; platform-driver-
> x86@xxxxxxxxxxxxxxx; mario.limonciello@xxxxxxxxxxx; Dell Client Kernel
> Subject: Re: [PATCH v6 1/2] platform/x86: dell-privacy: Add support for Dell
> hardware privacy
> 
> 
> [EXTERNAL EMAIL]
> 
> Hi Perry, Barnabás,
> 
> Barnabás thank you for your review. I agree with all your remarks.
> 
> Perry, Thank you for the new version, this version is looking good.
> This is almost ready for merging.
> 
> Please fix the review remarks from Barnabás. I also have a couple of small
> review remarks myself below.
> 
> On 4/4/21 6:53 PM, Barnabás Pőcze wrote:
> > Hi
> >
> >
> > 2021. április 4., vasárnap 10:31 keltezéssel, Perry Yuan írta:
> >
> >> From: Perry Yuan <perry_yuan@xxxxxxxx>
> >>
> >> add support for Dell privacy driver for the Dell units equipped
> >> hardware privacy design, which protect users privacy of audio and
> >> camera from hardware level. Once the audio or camera privacy mode
> >> activated, any applications will not get any audio or video stream
> >> when user pressed ctrl+F4 hotkey, audio privacy mode will be enabled,
> >> micmute led will be also changed accordingly The micmute led is fully
> >> controlled by hardware & EC(embedded controller) and camera mute
> >> hotkey is Ctrl+F9. Currently design only emits SW_CAMERA_LENS_COVER
> >> event while the camera lens shutter will be changed by EC &
> >> HW(hardware) control
> >>
> >> *The flow is like this:
> >> 1) User presses key. HW does stuff with this key (timeout timer is
> >> started)
> >> 2) WMI event is emitted from BIOS to kernel
> >> 3) WMI event is received by dell-privacy
> >> 4) KEY_MICMUTE emitted from dell-privacy
> >> 5) Userland picks up key and modifies kcontrol for SW mute
> >> 6) Codec kernel driver catches and calls ledtrig_audio_set, like this:
> >>    ledtrig_audio_set(LED_AUDIO_MICMUTE, rt715->micmute_led ?
> LED_ON
> >> :LED_OFF);
> >> 7) If "LED" is set to on dell-privacy notifies EC, and timeout is cancelled,
> >>    HW mic mute activated. If EC not notified, HW mic mute will also be
> >>    activated when timeout used up, it is just later than active ack
> >>
> >> Signed-off-by: Perry Yuan <perry_yuan@xxxxxxxx>
> >> ---
> >> v5 -> v6:
> >> * addressed feedback from Hans
> >> * addressed feedback from Pierre
> >> * optimize some debug format with dev_dbg()
> >> * remove platform driver,combined privacy acpi driver into single wmi
> >>   driver file
> >> * optimize sysfs interface with string added to be more clearly
> >> reading
> >> v4 -> v5:
> >> * addressed feedback from Randy Dunlap
> >> * addressed feedback from Pierre-Louis Bossart
> >> * rebase to latest 5.12 rc4 upstream kernel
> >> * fix some space alignment problem
> >> v3 -> v4:
> >> * fix format for Kconfig
> >> * add sysfs document
> >> * add flow comments to the privacy wmi/acpi driver
> >> * addressed feedback from Barnabás Pőcze[Thanks very much]
> >> * export privacy_valid to make the global state simpler to query
> >> * fix one issue which will block the dell-laptop driver to load when
> >>   privacy driver invalid
> >> * addressed feedback from Pierre-Louis Bossart,remove the EC ID match
> >> v2 -> v3:
> >> * add sysfs attributes doc
> >> v1 -> v2:
> >> * query EC handle from EC driver directly.
> >> * fix some code style.
> >> * add KEY_END to keymap array.
> >> * clean platform device when cleanup called
> >> * use hexadecimal format for log print in dev_dbg
> >> * remove __set_bit for the report keys from probe.
> >> * fix keymap leak
> >> * add err_free_keymap in dell_privacy_wmi_probe
> >> * wmi driver will be unregistered if privacy_acpi_init() fails
> >> * add sysfs attribute files for user space query.
> >> * add leds micmute driver to privacy acpi
> >> * add more design info the commit info
> >> ---
> >> ---
> >>  .../testing/sysfs-platform-dell-privacy-wmi   |  55 +++
> >>  drivers/platform/x86/dell/Kconfig             |  14 +
> >>  drivers/platform/x86/dell/Makefile            |   1 +
> >>  drivers/platform/x86/dell/dell-laptop.c       |  23 +-
> >>  drivers/platform/x86/dell/dell-privacy-wmi.c  | 441
> >> ++++++++++++++++++  drivers/platform/x86/dell/dell-privacy-wmi.h  |  32
> ++
> >>  drivers/platform/x86/dell/dell-wmi.c          |  13 +-
> >>  7 files changed, 572 insertions(+), 7 deletions(-)  create mode
> >> 100644 Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
> >>  create mode 100644 drivers/platform/x86/dell/dell-privacy-wmi.c
> >>  create mode 100644 drivers/platform/x86/dell/dell-privacy-wmi.h
> >>
> >> diff --git
> >> a/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
> >> b/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
> >> new file mode 100644
> >> index 000000000000..a57ddc6a221e
> >> --- /dev/null
> >> +++ b/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
> >> @@ -0,0 +1,55 @@
> >> +What:		/sys/bus/wmi/devices/6932965F-1671-4CEB-B988-
> D3AB0A901919/dell_privacy_supported_type
> >> +Date:		Apr 2021
> >> +KernelVersion:	5.13
> >> +Contact:	"perry.yuan@xxxxxxxx>"
> >> +Description:
> >> +		Display which dell hardware level privacy devices are
> supported
> >> +		“Dell Privacy” is a set of HW, FW, and SW features to
> enhance
> >> +		Dell’s commitment to platform privacy for MIC, Camera, and
> >> +		ePrivacy screens.
> >> +		The supported hardware privacy devices are:
> >> +Attributes:
> >> +		Microphone Mute:
> >> +				Identifies the local microphone can be muted
> by hardware, no applications
> >> +				is available to capture system mic sound
> >> +
> >> +                Camera Shutter:
> >    ^^^^^^^^^^^^^^^^
> >
> > Spaces used instead of tabs.
> >
> >
> >> +				Identifies camera shutter controlled by
> hardware, which is a micromechanical
> >> +				shutter assembly that is built onto the
> camera module to block capturing images
> >> +				from outside the laptop
> >> +
> >> +		supported:
> >> +				The privacy device is supported by this
> system
> >> +
> >> +		unsupported:
> >> +				The privacy device is not supported on this
> system
> >> +
> >> +		For example to check which privacy devices are supported:
> >> +
> >> +		# cat /sys/bus/wmi/drivers/dell-privacy/6932965F-1671-
> 4CEB-B988-D3AB0A901919/dell_privacy_supported_type
> >> +		[Microphone Mute] [supported]
> >> +		[Camera Shutter] [supported]
> >> +		[ePrivacy Screen] [unsupport]
> 
> [unsupport] should be [unsupported] here.
> 
> >> +
> >> +What:		/sys/bus/wmi/devices/6932965F-1671-4CEB-B988-
> D3AB0A901919/dell_privacy_current_state
> >> +Date:		Apr 2021
> >> +KernelVersion:	5.13
> >> +Contact:	"perry.yuan@xxxxxxxx>"
> >> +Description:
> >> +		Allow user space to check current dell privacy device state.
> >> +		Describes the Device State class exposed by BIOS which can
> be
> >> +		consumed by various applications interested in knowing the
> Privacy
> >> +		feature capabilities
> >> +Attributes:
> >> +		muted:
> >> +			Identifies the privacy device is turned off and cannot
> send
> >> +stream to OS applications
> >> +
> >> +		unmuted:
> >> +			Identifies the privacy device is turned on ,audio or
> camera driver can get
> >> +			stream from mic and camera module to OS
> applications
> >> +
> >> +		For example to check all supported current privacy device
> states:
> >> +
> >> +		# cat /sys/bus/wmi/drivers/dell-privacy/6932965F-1671-
> 4CEB-B988-D3AB0A901919/dell_privacy_current_state
> >> +		[Microphone] [unmuted]
> >> +		[Camera Shutter] [unmuted]
> >> diff --git a/drivers/platform/x86/dell/Kconfig
> >> b/drivers/platform/x86/dell/Kconfig
> >> index e0a55337f51a..05d124442b25 100644
> >> --- a/drivers/platform/x86/dell/Kconfig
> >> +++ b/drivers/platform/x86/dell/Kconfig
> >> @@ -204,4 +204,18 @@ config DELL_WMI_SYSMAN
> >>  	  To compile this driver as a module, choose M here: the module will
> >>  	  be called dell-wmi-sysman.
> >>
> >> +config DELL_PRIVACY
> >> +	tristate "Dell Hardware Privacy Support"
> >> +	depends on ACPI
> >> +	depends on ACPI_WMI
> >> +	depends on INPUT
> >> +	depends on DELL_LAPTOP
> >> +	depends on LEDS_TRIGGER_AUDIO
> >> +	select DELL_WMI
> >> +	help
> >> +	  This driver provides integration with the "Dell Hardware Privacy"
> feature
> >> +	  of Dell laptops.
> >> +
> >> +	  To compile this driver as a module, choose M here: the module will
> >> +	  be called dell_privacy.
> >>  endif # X86_PLATFORM_DRIVERS_DELL
> >> diff --git a/drivers/platform/x86/dell/Makefile
> >> b/drivers/platform/x86/dell/Makefile
> >> index d720a3e42ae3..7da0c33dfcca 100644
> >> --- a/drivers/platform/x86/dell/Makefile
> >> +++ b/drivers/platform/x86/dell/Makefile
> >> @@ -19,3 +19,4 @@ obj-$(CONFIG_DELL_WMI_AIO)		+=
> dell-wmi-aio.o
> >>  obj-$(CONFIG_DELL_WMI_DESCRIPTOR)	+= dell-wmi-descriptor.o
> >>  obj-$(CONFIG_DELL_WMI_LED)		+= dell-wmi-led.o
> >>  obj-$(CONFIG_DELL_WMI_SYSMAN)		+= dell-wmi-sysman/
> >> +obj-$(CONFIG_DELL_PRIVACY)              += dell-privacy-wmi.o
> >> diff --git a/drivers/platform/x86/dell/dell-laptop.c
> >> b/drivers/platform/x86/dell/dell-laptop.c
> >> index 70edc5bb3a14..a9b4cf13913c 100644
> >> --- a/drivers/platform/x86/dell/dell-laptop.c
> >> +++ b/drivers/platform/x86/dell/dell-laptop.c
> >> @@ -31,6 +31,8 @@
> >>  #include "dell-rbtn.h"
> >>  #include "dell-smbios.h"
> >>
> >> +#include "dell-privacy-wmi.h"
> >> +
> >>  struct quirk_entry {
> >>  	bool touchpad_led;
> >>  	bool kbd_led_not_present;
> >> @@ -90,6 +92,7 @@ static struct rfkill *wifi_rfkill;  static struct
> >> rfkill *bluetooth_rfkill;  static struct rfkill *wwan_rfkill;  static
> >> bool force_rfkill;
> >> +static bool has_privacy;
> >>
> >>  module_param(force_rfkill, bool, 0444);
> >> MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted
> >> models"); @@ -2206,10 +2209,16 @@ static int __init dell_init(void)
> >>
> >>  	if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
> >>  	    dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) {
> >> -		micmute_led_cdev.brightness =
> ledtrig_audio_get(LED_AUDIO_MICMUTE);
> >> -		ret = led_classdev_register(&platform_device->dev,
> &micmute_led_cdev);
> >> -		if (ret < 0)
> >> -			goto fail_led;
> >> +		if (dell_privacy_has_micmute())
> >> +			has_privacy = true;
> >> +		else
> >> +			has_privacy = false;
> >> +		if (!has_privacy) {
> >> +			micmute_led_cdev.brightness =
> ledtrig_audio_get(LED_AUDIO_MICMUTE);
> >> +			ret = led_classdev_register(&platform_device->dev,
> &micmute_led_cdev);
> >> +			if (ret < 0)
> >> +				goto fail_led;
> >> +		}
> >>  	}
> >>
> >>  	if (acpi_video_get_backlight_type() != acpi_backlight_vendor) @@
> >> -2257,7 +2266,8 @@ static int __init dell_init(void)
> >>  fail_get_brightness:
> >>  	backlight_device_unregister(dell_backlight_device);
> >>  fail_backlight:
> >> -	led_classdev_unregister(&micmute_led_cdev);
> >> +	if (!has_privacy)
> >> +		led_classdev_unregister(&micmute_led_cdev);
> >>  fail_led:
> >>  	dell_cleanup_rfkill();
> >>  fail_rfkill:
> >> @@ -2278,7 +2288,8 @@ static void __exit dell_exit(void)
> >>  		touchpad_led_exit();
> >>  	kbd_led_exit();
> >>  	backlight_device_unregister(dell_backlight_device);
> >> -	led_classdev_unregister(&micmute_led_cdev);
> >> +	if (!has_privacy)
> >> +		led_classdev_unregister(&micmute_led_cdev);
> >>  	dell_cleanup_rfkill();
> >>  	if (platform_device) {
> >>  		platform_device_unregister(platform_device);
> >> diff --git a/drivers/platform/x86/dell/dell-privacy-wmi.c
> >> b/drivers/platform/x86/dell/dell-privacy-wmi.c
> >> new file mode 100644
> >> index 000000000000..3fae8dd077f1
> >> --- /dev/null
> >> +++ b/drivers/platform/x86/dell/dell-privacy-wmi.c
> >> @@ -0,0 +1,441 @@
> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +/*
> >> + * Dell privacy notification driver
> >> + *
> >> + * Copyright (C) 2021 Dell Inc. All Rights Reserved.
> >> + */
> >> +
> >> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >> +
> >> +#include <linux/acpi.h>
> >> +#include <linux/bitops.h>
> >> +#include <linux/input.h>
> >> +#include <linux/input/sparse-keymap.h> #include <linux/list.h>
> >> +#include <linux/leds.h> #include <linux/module.h> #include
> >> +<linux/wmi.h>
> >> +
> >> +#include "dell-privacy-wmi.h"
> >> +
> >> +#define DELL_PRIVACY_GUID "6932965F-1671-4CEB-B988-
> D3AB0A901919"
> >> +#define MICROPHONE_STATUS		BIT(0)
> >> +#define CAMERA_STATUS		        BIT(1)
> >> +#define DELL_PRIVACY_AUDIO_EVENT  0x1 #define
> >> +DELL_PRIVACY_CAMERA_EVENT 0x2
> >> +#define cdev_to_led(c)       container_of(c, struct privacy_wmi_data, cdev)
> >
> > This name is confusing in my opinion. Something like `led_to_priv`
> > could be clearer.
> >
> >
> >> +
> >> +/*
> >> + * The wmi_list is used to store the privacy_priv struct with mutex
> >> +protecting  */ static LIST_HEAD(wmi_list); static
> >> +DEFINE_MUTEX(list_mutex);
> >> +
> >> +struct privacy_wmi_data {
> >> +	struct input_dev *input_dev;
> >> +	struct wmi_device *wdev;
> >> +	struct list_head list;
> >> +	struct led_classdev cdev;
> >> +	u32 features_present;
> >> +	u32 last_status;
> >> +};
> >> +
> >> +/* DELL Privacy Type */
> >> +enum dell_hardware_privacy_type {
> >> +	DELL_PRIVACY_TYPE_AUDIO = 0,
> >> +	DELL_PRIVACY_TYPE_CAMERA,
> >> +	DELL_PRIVACY_TYPE_SCREEN,
> >> +	DELL_PRIVACY_TYPE_MAX,
> >> +};
> >> +
> >> +static const char * const privacy_types[DELL_PRIVACY_TYPE_MAX] = {
> >> +	[DELL_PRIVACY_TYPE_AUDIO] = "Microphone",
> >> +	[DELL_PRIVACY_TYPE_CAMERA] = "Camera Shutter",
> >> +	[DELL_PRIVACY_TYPE_SCREEN] = "ePrivacy Screen", };
> >> +
> >> +/*
> >> + * Keymap for WMI privacy events of type 0x0012  */ static const
> >> +struct key_entry dell_wmi_keymap_type_0012[] = {
> >> +	/* privacy mic mute */
> >> +	{ KE_KEY, 0x0001, { KEY_MICMUTE } },
> >> +	/* privacy camera mute */
> >> +	{ KE_SW,  0x0002, { SW_CAMERA_LENS_COVER } },
> >> +	{ KE_END, 0},
> >> +};
> >> +
> >> +/*
> >> + * global privacy state for other modules to query if the privacy
> >> +driver loaded successfully
> >> + * or no privacy WMI device was detected at initial probe phase  */
> >> +bool dell_privacy_present(void) {
> >> +	struct privacy_wmi_data *priv;
> >> +
> >> +	mutex_lock(&list_mutex);
> >> +	priv = list_first_entry_or_null(&wmi_list,
> >> +			struct privacy_wmi_data,
> >> +			list);
> >> +	mutex_unlock(&list_mutex);
> >> +
> >> +	return priv;
> >> +}
> >> +EXPORT_SYMBOL_GPL(dell_privacy_present);
> 
> As Barnabás also says below this function is not necessary, please remove it.
> 
> >> +
> >> +/*
> >> + * Allow other modules to check current dell privacy mic mute state
> >> +*/ int dell_privacy_has_micmute(void) {
> >> +	struct privacy_wmi_data *priv;
> >> +	int ret;
> >> +
> >> +	mutex_lock(&list_mutex);
> >> +	priv = list_first_entry_or_null(&wmi_list,
> >> +			struct privacy_wmi_data,
> >> +			list);
> >> +	/*
> >> +	 * There is no privacy device node yet, so we need to wait
> >> +	 * until the privacy driver has created software node for the micmute
> device.
> >> +	 * This function will return -EPROBE_DEFER until the privacy device is
> registered.
> >> +	 */
> >> +	if (!priv) {
> >> +		ret = -EPROBE_DEFER;
> >> +		goto out;
> >> +	}
> >> +	ret = priv->features_present & MICROPHONE_STATUS;
> >> +	if (!ret) {
> >> +		dev_err(&priv->wdev->dev, "privacy mic mute feature is not
> supported: %d\n", ret);
> >> +		ret = -ENODEV;
> >> +		goto out;
> >> +	}
> >> +	dev_err(&priv->wdev->dev, "privacy mic mute feature is supported:
> >> +%d\n", ret);
> >
> > I think this could be `dev_info()`, but definitely not `dev_err()`.
> > Although I'd personally move the logging from here to the probe
> > function if you want to log which features are available. `ret` is
> > necessarily 1 here, so I don't think printing it provides additional
> information.
> >
> >
> >> +
> >> +out:
> >> +	mutex_unlock(&list_mutex);
> >> +	return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(dell_privacy_has_micmute);
> >> +
> >> +/*
> >> + * The flow of privacy event:
> >> + * 1) User presses key. HW does stuff with this key (timeout is
> >> +started)
> >> + * 2) WMI event is emitted from BIOS
> >> + * 3) WMI event is received by dell-privacy
> >> + * 4) KEY_MICMUTE emitted from dell-privacy
> >> + * 5) Userland picks up key and modifies kcontrol for SW mute
> >> + * 6) Codec kernel driver catches and calls ledtrig_audio_set defined by
> >> + *    dell-privacy-acpi driver. Codec driver will call like this to switch
> micmute led state.
> >> + *    ledtrig_audio_set(LED_AUDIO_MICMUTE, micmute_led ?
> LED_ON :LED_OFF);
> >> + * 7) If "LED" is set to on dell-privacy notifies EC,and timeout is cancelled,
> >> + *	HW mic mute activated.
> >> + */
> >> +bool dell_privacy_process_event(int type, int code, int status) {
> >> +	struct privacy_wmi_data *priv;
> >> +	const struct key_entry *key;
> >> +	bool ret = false;
> >> +
> >> +	mutex_lock(&list_mutex);
> >> +	priv = list_first_entry_or_null(&wmi_list,
> >> +			struct privacy_wmi_data,
> >> +			list);
> >> +	if (!priv) {
> >> +		dev_err(&priv->wdev->dev, "priv data is NULL\n");
> 
> Please drop the dev_err here, this is expected to happen on devices without
> privacy features, since the intention is to use this function as
> dell_privacy_present() + process_event() functionality in one function. Doing
> things this way is simpler and avoids a race since this function holds the
> mutex the entire time.
> 
> >> +		goto error;
> >> +	}
> >> +
> >
> > I think the rest of the function could be replaced with:
> >
> >   ret = sparse_keymap_report_event(priv->input_dev,
> > DELL_SCAN_CODE(type, code), 1, true)
> >
> >   if (ret)
> >     priv->last_status = status;
> >
> >   error:
> >     [...]
> >
> > (see later a comment for the definition of DELL_SCAN_CODE())
> >
> >
> >> +	key = sparse_keymap_entry_from_scancode(priv->input_dev, (type
> << 16) | code);
> >> +	if (!key) {
> >> +		dev_warn(&priv->wdev->dev, "Unknown key with type
> 0x%04x and code 0x%04x pressed\n",
> >> +			type, code);
> >> +		goto error;
> >> +	}
> >> +	dev_dbg(&priv->wdev->dev, "Key with type 0x%04x and code 0x%04x
> >> +pressed\n", type, code);
> >> +
> >> +	switch (code) {
> >> +	case DELL_PRIVACY_AUDIO_EVENT: /* Mic mute */
> >> +	case DELL_PRIVACY_CAMERA_EVENT: /* Camera mute */
> >> +		priv->last_status = status;
> >> +		sparse_keymap_report_entry(priv->input_dev, key, 1, true);
> >> +		ret = true;
> >> +		break;
> >> +	default:
> >> +		dev_dbg(&priv->wdev->dev, "unknown event type 0x%04x
> 0x%04x",
> >> +type, code);
> >
> > The capitalization is inconsistent. Please either make all messages
> > lowercase or make them all start with an uppercase letter. (And a
> > newline character is missing.)
> >
> >
> >> +	}
> >> +
> >> +error:
> >> +	mutex_unlock(&list_mutex);
> >> +	return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(dell_privacy_process_event);
> >> +
> >> +static ssize_t dell_privacy_supported_type_show(struct device *dev,
> >> +					struct device_attribute *attr,
> >> +					char *buf)
> >> +{
> >> +	enum dell_hardware_privacy_type type;
> >> +	struct privacy_wmi_data *priv = dev_get_drvdata(dev);
> >> +	char *s = buf;
> >> +	u32 privacy_list;
> >> +
> >> +	privacy_list = priv->features_present;
> >> +	for (type = DELL_PRIVACY_TYPE_AUDIO; type <
> DELL_PRIVACY_TYPE_MAX; type++) {
> >> +		if (privacy_types[type]) {
> >
> > Is this check necessary?
> 
> No I don't think it is, this check can be removed, good catch.
> 
> >
> >
> >> +			if (privacy_list & BIT(type))
> >> +				s += sprintf(s, "[%s] [supported]\n",
> privacy_types[type]);
> >> +			else
> >> +				s += sprintf(s, "[%s] [unsupport]\n",
> privacy_types[type]);
> 
> [unsupport] should be [unsupported] here.
> 
> >
> > You can use `sysfs_emit_at()` here.

I found that sysfs_emit_at and sysfs_emit() , will cause the module built failed .

Because it is RO attributes,to save time ,  I will prefer to keep using the sprintf as before. 
Perry 

> >
> >
> >> +		}
> >> +	}
> >> +
> >> +	if (s != buf)
> >> +		/* convert the last space to a newline */
> >> +		*(s-1) = '\n';
> >
> > I believe this is not needed?
> 
> Right this is no longer necessary now that we have 1 line per feature.
> 
> >
> >
> >> +	return (s - buf);
> >> +}
> >> +
> >> +static ssize_t dell_privacy_current_state_show(struct device *dev,
> >> +					struct device_attribute *attr,
> >> +					char *buf)
> >> +{
> >> +	struct privacy_wmi_data *priv = dev_get_drvdata(dev);
> >> +	enum dell_hardware_privacy_type type;
> >> +	u32 privacy_state = priv->last_status;
> >> +	u32 privacy_supported = priv->features_present;
> >> +	char *s = buf;
> >> +
> >> +	for (type = DELL_PRIVACY_TYPE_AUDIO; type <
> DELL_PRIVACY_TYPE_MAX; type++) {
> >> +		if (privacy_supported & BIT(type)) {
> >> +			if (privacy_state & BIT(type))
> >> +				s += sprintf(s, "[%s] [unmuted]\n",
> privacy_types[type]);
> >> +			else
> >> +				s += sprintf(s, "[%s] [muted]\n",
> privacy_types[type]);
> >
> > sysfs_emit_at
> >
> >
> >> +		}
> >> +	}
> >> +
> >> +	if (s != buf)
> >> +		/* convert the last space to a newline */
> >> +		*(s-1) = '\n';
> >
> > not needed?
> 
> Ack.
> 
> >> +	return (s - buf);
> >> +}
> >> +
> >> +static DEVICE_ATTR_RO(dell_privacy_supported_type);
> >> +static DEVICE_ATTR_RO(dell_privacy_current_state);
> >> +
> >> +static struct attribute *privacy_attributes[] = {
> >> +	&dev_attr_dell_privacy_supported_type.attr,
> >> +	&dev_attr_dell_privacy_current_state.attr,
> >> +	NULL,
> >> +};
> >> +
> >> +static const struct attribute_group privacy_attribute_group = {
> >> +	.attrs = privacy_attributes
> >> +};
> >> +
> >> +/*
> >> + * Describes the Device State class exposed by BIOS which can be
> >> +consumed by
> >> + * various applications interested in knowing the Privacy feature
> capabilities.
> >> + * class DeviceState
> >> + * {
> >> + *  [key, read] string InstanceName;
> >> + *  [read] boolean ReadOnly;
> >> + *  [WmiDataId(1), read] uint32 DevicesSupported;
> >> + *   0 – None, 0x1 – Microphone, 0x2 – Camera, 0x4 -ePrivacy  Screen
> >           ^           ^                 ^             ^        ^^
> > Please use a single type of hyphen/dash consistently.     One space is
> enough.
> >
> >
> >> + *  [WmiDataId(2), read] uint32 CurrentState;
> >> + *   0:Off; 1:On. Bit0 – Microphone, Bit1 – Camera, Bit2 - ePrivacyScreen
> >                           ^                  ^              ^
> > Same here.
> >
> >
> >> + * };
> >> + */
> >> +static int get_current_status(struct wmi_device *wdev) {
> >> +	struct privacy_wmi_data *priv = dev_get_drvdata(&wdev->dev);
> >> +	union acpi_object *obj_present;
> >> +	u32 *buffer;
> >> +	int ret = 0;
> >> +
> >> +	if (!priv) {
> >> +		pr_err("dell privacy priv is NULL\n");
> >
> > dev_err?
> 
> Ack.
> 
> >> +		return -EINVAL;
> >> +	}
> >> +	/* check privacy support features and device states */
> >> +	obj_present = wmidev_block_query(wdev, 0);
> >> +	if (!obj_present) {
> >> +		dev_err(&wdev->dev, "failed to read Binary MOF\n");
> >> +		ret = -EIO;
> >> +		return ret;
> >
> > return -EIO ?
> >
> >
> >> +	}
> >> +
> >> +	if (obj_present->type != ACPI_TYPE_BUFFER) {
> >> +		dev_err(&wdev->dev, "Binary MOF is not a buffer!\n");
> >> +		ret = -EIO;
> >> +		goto obj_free;
> >> +	}
> >> +	/*  Although it's not technically a failure, this would lead to
> >> +	 *  unexpected behavior
> >> +	 */
> >> +	if (obj_present->buffer.length != 8) {
> >> +		dev_err(&wdev->dev, "Dell privacy buffer has unexpected
> length (%d)!\n",
> >> +				obj_present->buffer.length);
> >> +		ret = -EINVAL;
> >> +		goto obj_free;
> >> +	}
> >> +	buffer = (u32 *)obj_present->buffer.pointer;
> >> +	priv->features_present = buffer[0];
> >> +	priv->last_status = buffer[1];
> >
> > It's a minor thing, but I still think it'd more explicit and somewhat
> > better to use `get_unaligned_le32()` (or `get_unaligned_cpu32()`).
> >
> >
> >> +
> >> +obj_free:
> >> +	kfree(obj_present);
> >> +	return ret;
> >> +}
> >> +
> >> +static int dell_privacy_micmute_led_set(struct led_classdev *led_cdev,
> >> +					enum led_brightness brightness) {
> >> +	struct privacy_wmi_data *priv = cdev_to_led(led_cdev);
> >> +	static char *acpi_method = (char *)"ECAK";
> >> +	acpi_status status;
> >> +	acpi_handle handle;
> >> +
> >> +	handle = ec_get_handle();
> >> +	if (!handle)
> >> +		return -EIO;
> >> +
> >> +	if (!acpi_has_method(handle, acpi_method))
> >> +		return -EIO;
> >> +
> >> +	status = acpi_evaluate_object(handle, acpi_method, NULL, NULL);
> >> +	if (ACPI_FAILURE(status)) {
> >> +		dev_err(&priv->wdev->dev, "Error setting privacy EC ack
> value: %s\n",
> >> +				acpi_format_exception(status));
> >> +		return -EIO;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +/*
> >> + * Pressing the mute key activates a time delayed circuit to
> >> +physically cut
> >> + * off the mute. The LED is in the same circuit, so it reflects the
> >> +true
> >> + * state of the HW mute.  The reason for the EC "ack" is so that
> >> +software
> >> + * can first invoke a SW mute before the HW circuit is cut off.
> >> +Without SW
> >> + * cutting this off first does not affect the time delayed muting or
> >> +status
> >> + * of the LED but there is a possibility of a "popping" noise.
> >> + *
> >> + * If the EC receives the SW ack, the circuit will be activated
> >> +before the
> >> + * delay completed.
> >> + *
> >> + * Exposing as an LED device allows the codec drivers notification
> >> +path to
> >> + * EC ACK to work
> >> + */
> >> +static int dell_privacy_leds_setup(struct device *dev) {
> >> +	struct privacy_wmi_data *priv = dev_get_drvdata(dev);
> >> +	int ret;
> >> +
> >> +	priv->cdev.name = "dell-privacy::micmute";
> >> +	priv->cdev.max_brightness = 1;
> >> +	priv->cdev.brightness_set_blocking = dell_privacy_micmute_led_set;
> >> +	priv->cdev.default_trigger = "audio-micmute";
> >> +	priv->cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
> >> +	ret = devm_led_classdev_register(dev, &priv->cdev);
> >> +	if (ret)
> >> +		return ret;
> >> +	return 0;
> >
> > You can replace the last four lines with:
> >
> >   return devm_led_classdev_register(...);
> >
> >
> >> +}
> >> +
> >> +static int dell_privacy_wmi_probe(struct wmi_device *wdev, const
> >> +void *context) {
> >> +	struct privacy_wmi_data *priv;
> >> +	struct key_entry *keymap;
> >> +	int ret, i;
> >> +
> >> +	ret = wmi_has_guid(DELL_PRIVACY_GUID);
> >> +	if (!ret)
> >> +		pr_debug("Unable to detect available Dell privacy
> devices: %d\n",
> >> +ret);
> >
> > When this branch is taken, `ret` is necessarily zero, so I don't think
> > printing it provides useful information.
> >
> > And I believe this `wmi_has_guid()` check is unnecessary since the
> > probe method would not be called if the device didn't have such WMI guid
> if I'm not mistaken.
> 
> Ack, Perry please drop this check.
> 
> >> +
> >> +	priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
> >> +	if (!priv)
> >> +		return -ENOMEM;
> >> +
> >> +	dev_set_drvdata(&wdev->dev, priv);
> >> +	priv->wdev = wdev;
> >> +	/* create evdev passing interface */
> >> +	priv->input_dev = devm_input_allocate_device(&wdev->dev);
> >> +	if (!priv->input_dev)
> >> +		return -ENOMEM;
> >> +
> >> +	/* remap the wmi keymap event to new keymap */
> >> +	keymap = kcalloc(ARRAY_SIZE(dell_wmi_keymap_type_0012),
> >> +			sizeof(struct key_entry), GFP_KERNEL);
> >> +	if (!keymap)
> >> +		return -ENOMEM;
> >> +
> >> +	/* remap the keymap code with Dell privacy key type 0x12 as prefix
> >> +	 * KEY_MICMUTE scancode will be reported as 0x120001
> >> +	 */
> >> +	for (i = 0; i < ARRAY_SIZE(dell_wmi_keymap_type_0012); i++) {
> >> +		keymap[i] = dell_wmi_keymap_type_0012[i];
> >> +		keymap[i].code |= (0x0012 << 16);
> >> +	}
> >
> > I still don't see the need for allocating and copying the keymap.
> > Wouldn't the following be sufficient?
> >
> >   #define DELL_SCAN_CODE(type, code) ((type) << 16 | (code))
> >   static const struct key_entry dell_wmi_keymap_type_0012[] = {
> >     { KE_KEY, DELL_SCAN_CODE(0x0012, 0x0001), { KEY_MICMUTE } },
> >     { KE_SW,  DELL_SCAN_CODE(0x0012, 0x0002),
> { SW_CAMERA_LENS_COVER } },
> >     { KE_END, 0},
> >   };
> >
> > Other Dell drivers potentially merge multiple keymaps, so dynamically
> > allocating the key_entry array is justified. Here I see no such need.
> > Can you explain why this copying is done?
> 
> I was wondering the same thing when I reviewed v5, but this is consistent
> with the handling in dell-wmi.c so I'm fine with keeping it as is for consistency.
> But if you (perry) want to change this to the simpler code which Barnabás is
> suggesting that is fine too.
> 
> >
> >
> >> +	ret = sparse_keymap_setup(priv->input_dev, keymap, NULL);
> >> +	if (ret)
> >> +		return ret;
> >
> > `keymap` is leaked if this or any of the later early-returns are taken.
> 
> Right as I mentioned in my review of v5, the "kfree(keymap);" needs to be
> moved before the "if (ret)" here, so the end results looks like this:
> 
> 	ret = sparse_keymap_setup(priv->input_dev, keymap, NULL);
> 	kfree(keymap);
> 	if (ret)
> 		return ret;
> 
> And the kfree(keymap) below goes away.
> 
> >
> >
> >> +
> >> +	priv->input_dev->dev.parent = &wdev->dev;
> >> +	priv->input_dev->name = "Dell Privacy Driver";
> >> +	priv->input_dev->id.bustype = BUS_HOST;
> >> +
> >> +	ret = input_register_device(priv->input_dev);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	ret = get_current_status(priv->wdev);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	ret = devm_device_add_group(&wdev->dev,
> &privacy_attribute_group);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	ret = dell_privacy_leds_setup(&priv->wdev->dev);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	mutex_lock(&list_mutex);
> >> +	list_add_tail(&priv->list, &wmi_list);
> >> +	mutex_unlock(&list_mutex);
> >> +	kfree(keymap);
> 
> This kfree(keymap) needs to be removed. Note that switching to the
> suggested non copied keymap would get rid of all of this.
> 
> >> +	return 0;
> >> +}
> >> +
> >> +static int dell_privacy_wmi_remove(struct wmi_device *wdev) {
> >> +	struct privacy_wmi_data *priv = dev_get_drvdata(&wdev->dev);
> >> +
> >> +	mutex_lock(&list_mutex);
> >> +	list_del(&priv->list);
> >> +	mutex_unlock(&list_mutex);
> >> +	return 0;
> >> +}
> >> +
> >> +static const struct wmi_device_id dell_wmi_privacy_wmi_id_table[] = {
> >> +	{ .guid_string = DELL_PRIVACY_GUID },
> >> +	{ },
> >> +};
> >> +
> >> +static struct wmi_driver dell_privacy_wmi_driver = {
> >> +	.driver = {
> >> +		.name = "dell-privacy",
> >> +	},
> >> +	.probe = dell_privacy_wmi_probe,
> >> +	.remove = dell_privacy_wmi_remove,
> >> +	.id_table = dell_wmi_privacy_wmi_id_table, };
> >> +
> >> +module_wmi_driver(dell_privacy_wmi_driver);
> >> +
> >> +MODULE_DEVICE_TABLE(wmi, dell_wmi_privacy_wmi_id_table);
> >> +MODULE_AUTHOR("Perry Yuan <perry_yuan@xxxxxxxx>");
> >> +MODULE_DESCRIPTION("Dell Privacy WMI Driver");
> >> +MODULE_LICENSE("GPL");
> >> diff --git a/drivers/platform/x86/dell/dell-privacy-wmi.h
> >> b/drivers/platform/x86/dell/dell-privacy-wmi.h
> >> new file mode 100644
> >> index 000000000000..a24893754286
> >> --- /dev/null
> >> +++ b/drivers/platform/x86/dell/dell-privacy-wmi.h
> >> @@ -0,0 +1,32 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-only */
> >> +/*
> >> + * Dell privacy notification driver
> >> + *
> >> + * Copyright (C) 2021 Dell Inc. All Rights Reserved.
> >> + */
> >> +
> >> +#ifndef _DELL_PRIVACY_WMI_H_
> >> +#define _DELL_PRIVACY_WMI_H_
> >> +
> >> +#if IS_ENABLED(CONFIG_DELL_PRIVACY)
> >> +int dell_privacy_has_micmute(void);
> >> +bool dell_privacy_present(void);
> >> +bool dell_privacy_process_event(int type, int code, int status);
> >> +#else /* CONFIG_DELL_PRIVACY */ static inline int
> >> +dell_privacy_has_micmute(void) {
> >> +	return -ENODEV;
> >> +}
> >> +
> >> +static inline int dell_privacy_present(void)
> >                  ^^^
> > It's declared with `bool` return type a couple lines above.
> >
> >
> >> +{
> >> +	return -ENODEV;
> >> +}
> >> +
> >> +static inline void dell_privacy_process_event(int type, int code,
> >> +int status)
> >                  ^^^^
> > It's declared with `bool` return type a couple lines above.
> >
> >
> >> +{}
> >> +#endif /* CONFIG_DELL_PRIVACY */
> >> +
> >> +int  dell_privacy_acpi_init(void);
> >> +void dell_privacy_acpi_exit(void);
> >
> > These two don't seem to be referenced anywhere?
> 
> These no longer exist and should be removed from the header.
> 
> >
> >
> >> +#endif
> >> diff --git a/drivers/platform/x86/dell/dell-wmi.c
> >> b/drivers/platform/x86/dell/dell-wmi.c
> >> index bbdb3e860892..8ef9e22a538f 100644
> >> --- a/drivers/platform/x86/dell/dell-wmi.c
> >> +++ b/drivers/platform/x86/dell/dell-wmi.c
> >> @@ -27,6 +27,7 @@
> >>  #include <acpi/video.h>
> >>  #include "dell-smbios.h"
> >>  #include "dell-wmi-descriptor.h"
> >> +#include "dell-privacy-wmi.h"
> >>
> >>  MODULE_AUTHOR("Matthew Garrett <mjg@xxxxxxxxxx>");
> >> MODULE_AUTHOR("Pali Rohár <pali@xxxxxxxxxx>"); @@ -381,6 +382,7
> @@
> >> static void dell_wmi_notify(struct wmi_device *wdev,
> >>  	u16 *buffer_entry, *buffer_end;
> >>  	acpi_size buffer_size;
> >>  	int len, i;
> >> +	int ret;
> >>
> >>  	if (obj->type != ACPI_TYPE_BUFFER) {
> >>  		pr_warn("bad response type %x\n", obj->type); @@ -427,7
> +429,6 @@
> >> static void dell_wmi_notify(struct wmi_device *wdev,
> >>
> >>  		switch (buffer_entry[1]) {
> >>  		case 0x0000: /* One key pressed or event occurred */
> >> -		case 0x0012: /* Event with extended data occurred */
> >>  			if (len > 2)
> >>  				dell_wmi_process_key(wdev, buffer_entry[1],
> >>  						     buffer_entry[2]);
> >> @@ -439,6 +440,16 @@ static void dell_wmi_notify(struct wmi_device
> *wdev,
> >>  				dell_wmi_process_key(wdev, buffer_entry[1],
> >>  						     buffer_entry[i]);
> >>  			break;
> >> +		case 0x0012:
> >> +			ret = dell_privacy_present();
> >> +			if ((ret) && (len > 2)) {
> >
> > Is the `len > 2` check correct?
> >
> > Moreover, I personally don't see any reason to use a new variable here
> (`ret`).
> >
> > If you incorporate the `dell_privacy_present()` check into
> > `dell_privacy_process_event()`,
> 
> `dell_privacy_process_event()` already incorporates a presency check; and by
> only relying on that check we also avoid a race between the check and
> processing the event.
> 
> > then even
> >
> >   if (len > ?? && dell_privacy_process_event(...))
> >     /* nothing */ ;
> >   else if (len > 2)
> >     dell_wmi_process_key(...);
> >
> > would work as Hans has already pointed it out. And there'd be no need
> > for `dell_privacy_present()` anymore.
> 
> Ack, please change this and remove the entire dell_privacy_present()
> function.
> 
> So you should end up with this here:
> 
> 			if (len > 4 &&
> dell_privacy_process_event(buffer_entry[1], buffer_entry[3], buffer_entry[4]))
> 				/* nothing */ ;
> 			else if (len > 2)
> 				dell_wmi_process_key(wdev, buffer_entry[1],
> dell_wmi_process_key(wdev, buffer_entry[1],
> 			break;
> 
> Regards,
> 
> Hans
> 
> 
> 
> >
> >
> >> +				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]);
> >> +			}
> >> +			break;
> >>  		default: /* Unknown event */
> >>  			pr_info("Unknown WMI event type 0x%x\n",
> >>  				(int)buffer_entry[1]);
> >> --
> >> 2.25.1
> >>
> >
> >
> > Regards,
> > Barnabás Pőcze
> >