Date:   Sun, 04 Apr 2021 16:53:32 +0000
From:   Barnabás Pőcze <pobrn@...tonmail.com>
To:     Perry Yuan <Perry.Yuan@...l.com>
Cc:     "pierre-louis.bossart@...ux.intel.com" 
        <pierre-louis.bossart@...ux.intel.com>,
        "oder_chiou@...ltek.com" <oder_chiou@...ltek.com>,
        "perex@...ex.cz" <perex@...ex.cz>,
        "tiwai@...e.com" <tiwai@...e.com>,
        "hdegoede@...hat.com" <hdegoede@...hat.com>,
        "mgross@...ux.intel.com" <mgross@...ux.intel.com>,
        "Mario.Limonciello@...l.com" <Mario.Limonciello@...l.com>,
        "lgirdwood@...il.com" <lgirdwood@...il.com>,
        "broonie@...nel.org" <broonie@...nel.org>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "platform-driver-x86@...r.kernel.org" 
        <platform-driver-x86@...r.kernel.org>,
        "mario.limonciello@...look.com" <mario.limonciello@...look.com>,
        "Dell.Client.Kernel@...l.com" <Dell.Client.Kernel@...l.com>
Subject: Re: [PATCH v6 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy

Hi


2021. április 4., vasárnap 10:31 keltezéssel, Perry Yuan írta:

> From: Perry Yuan <perry_yuan@...l.com>
>
> add support for Dell privacy driver for the Dell units equipped
> hardware privacy design, which protect users privacy of audio and
> camera from hardware level. Once the audio or camera privacy mode
> 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@...l.com>
> ---
> 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@...l.com>"
> +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]
> +
> +What:		/sys/bus/wmi/devices/6932965F-1671-4CEB-B988-D3AB0A901919/dell_privacy_current_state
> +Date:		Apr 2021
> +KernelVersion:	5.13
> +Contact:	"perry.yuan@...l.com>"
> +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);
> +
> +/*
> + * 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");
> +		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?


> +			if (privacy_list & BIT(type))
> +				s += sprintf(s, "[%s] [supported]\n", privacy_types[type]);
> +			else
> +				s += sprintf(s, "[%s] [unsupport]\n", privacy_types[type]);

You can use `sysfs_emit_at()` here.


> +		}
> +	}
> +
> +	if (s != buf)
> +		/* convert the last space to a newline */
> +		*(s-1) = '\n';

I believe this is not needed?


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


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


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


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


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


> +
> +	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);
> +	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@...l.com>");
> +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?


> +#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@...hat.com>");
>  MODULE_AUTHOR("Pali Rohár <pali@...nel.org>");
> @@ -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()`, 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.


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

Powered by blists - more mailing lists