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

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.

To the layman I would say dev_info is too noisy actually. I think debugging (dev_dbg) would be just fine. If there is a function not working, adding dynamic debugging on kernel command line of modprobe line will be plenty sufficient to get this 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@dell.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@redhat.com"); MODULE_AUTHOR("Pali Rohár pali@kernel.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