Message-ID: <SA1PR19MB492678E19C1403E656F9382BFA779@SA1PR19MB4926.namprd19.prod.outlook.com>
Date:   Mon, 5 Apr 2021 15:17:19 +0000
From:   "Limonciello, Mario" <Mario.Limonciello@...l.com>
To:     Barnabás Pőcze <pobrn@...tonmail.com>,
        "Yuan, Perry" <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>,
        "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 <Dell.Client.Kernel@...l.com>
Subject: 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@...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