Hi
2021. április 4., vasárnap 10:31 keltezéssel, Perry Yuan írta: > From: Perry Yuan <[email protected]> > > 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 <[email protected]> > --- > 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: "[email protected]>" > +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: "[email protected]>" > +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 <[email protected]>"); > +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 <[email protected]>"); > MODULE_AUTHOR("Pali Rohár <[email protected]>"); > @@ -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
