From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 40B45C433E0 for ; Tue, 12 Jan 2021 19:04:05 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id D319523122 for ; Tue, 12 Jan 2021 19:04:04 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2392852AbhALTDt (ORCPT ); Tue, 12 Jan 2021 14:03:49 -0500 Received: from mga14.intel.com ([192.55.52.115]:31276 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726387AbhALTDs (ORCPT ); Tue, 12 Jan 2021 14:03:48 -0500 IronPort-SDR: JX/rBr9WQ1yCacpnUAaNoRxGb72yswiJxEI5H5y0JjQGh4sl+LcjysMRp3ica1HQVWfL+EyJKg mhhDqqj8xJmA== X-IronPort-AV: E=McAfee;i="6000,8403,9862"; a="177317642" X-IronPort-AV: E=Sophos;i="5.79,342,1602572400"; d="scan'208";a="177317642" Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Jan 2021 11:01:59 -0800 IronPort-SDR: yljuQCMVq7Z6/YY/mdG4ErUM0tX7fGuFxOjqJpD5sj9h6wHrBC0b1/g0iYJy/98SBCg0yTGj4m PPhDUQffDrRw== X-IronPort-AV: E=Sophos;i="5.79,342,1602572400"; d="scan'208";a="404573572" Received: from clinton1-mobl1.amr.corp.intel.com (HELO [10.212.214.129]) ([10.212.214.129]) by fmsmga003-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Jan 2021 11:01:57 -0800 Subject: Re: [PATCH v3 1/3] platform/x86: dell-privacy: Add support for Dell hardware privacy To: Perry Yuan , oder_chiou@realtek.com, perex@perex.cz, tiwai@suse.com, hdegoede@redhat.com, mgross@linux.intel.com Cc: alsa-devel@alsa-project.org, Mario.Limonciello@dell.com, linux-kernel@vger.kernel.org, lgirdwood@gmail.com, platform-driver-x86@vger.kernel.org, broonie@kernel.org References: <20210112171723.19484-1-Perry_Yuan@Dell.com> From: Pierre-Louis Bossart Message-ID: Date: Tue, 12 Jan 2021 13:00:55 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <20210112171723.19484-1-Perry_Yuan@Dell.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 1/12/21 11:17 AM, Perry Yuan wrote: > From: Perry Yuan > > 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 > enabled, 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 emmit > SW_CAMERA_LENS_COVER event while the camera lens shutter will be > changed by EC & hw(hadware) control It wouldn't hurt to use capital letters and punctuation, it helps with readility.. [...] > diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c > index 70edc5bb3a14..2fea1f34fcf9 100644 > --- a/drivers/platform/x86/dell-laptop.c > +++ b/drivers/platform/x86/dell-laptop.c > @@ -30,6 +30,7 @@ > #include > #include "dell-rbtn.h" > #include "dell-smbios.h" > +#include "dell-privacy-wmi.h" > > struct quirk_entry { > bool touchpad_led; > @@ -90,6 +91,7 @@ static struct rfkill *wifi_rfkill; > static struct rfkill *bluetooth_rfkill; > static struct rfkill *wwan_rfkill; > static bool force_rfkill; > +static bool privacy_valid; why is this variable needed? Was the intent to have a kernel parameter here? > module_param(force_rfkill, bool, 0444); > MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted models"); > @@ -2205,11 +2207,18 @@ static int __init dell_init(void) > dell_laptop_register_notifier(&dell_laptop_notifier); > > 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; > + dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) { > +#if IS_ENABLED(CONFIG_DELL_PRIVACY) > + ret = dell_privacy_valid(); > + if (!ret) > + privacy_valid = true; > +#endif > + if (!privacy_valid) { if it was intended to be used as a kernel parameter it's not done the right way: the value set by the user would be ignored... > + 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 (!privacy_valid) > + 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 (!privacy_valid) > + led_classdev_unregister(&micmute_led_cdev); > dell_cleanup_rfkill(); > if (platform_device) { > platform_device_unregister(platform_device); > diff --git a/drivers/platform/x86/dell-privacy-acpi.c b/drivers/platform/x86/dell-privacy-acpi.c > new file mode 100644 > index 000000000000..df6a86e1345c > --- /dev/null > +++ b/drivers/platform/x86/dell-privacy-acpi.c > @@ -0,0 +1,167 @@ > +// 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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "dell-privacy-wmi.h" > + > +#define PRIVACY_PLATFORM_NAME "dell-privacy-acpi" > +#define DELL_PRIVACY_GUID "6932965F-1671-4CEB-B988-D3AB0A901919" > + > +struct privacy_acpi_priv { > + struct device *dev; > + struct platform_device *platform_device; > + struct led_classdev cdev; > +}; > +static struct privacy_acpi_priv *privacy_acpi; > + > +static int dell_privacy_micmute_led_set(struct led_classdev *led_cdev, > + enum led_brightness brightness) > +{ > + struct privacy_acpi_priv *priv = privacy_acpi; > + acpi_status status; > + acpi_handle handle; > + char *acpi_method; > + > + handle = ec_get_handle(); > + if (!handle) > + return -EIO; > + acpi_method = "ECAK"; > + status = acpi_evaluate_object(handle, acpi_method, NULL, NULL); > + if (ACPI_FAILURE(status)) { > + dev_err(priv->dev, "Error setting privacy EC ack value: %s\n", > + acpi_format_exception(status)); > + return -EIO; > + } > + return 0; > +} > + > +static int dell_privacy_acpi_remove(struct platform_device *pdev) > +{ > + struct privacy_acpi_priv *priv = dev_get_drvdata(privacy_acpi->dev); > + > + led_classdev_unregister(&priv->cdev); > + > + 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_acpi_priv *priv = dev_get_drvdata(dev); > + int ret = 0; > + > + 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 < 0) > + return ret; > + return 0; > +} > + > +static int dell_privacy_acpi_probe(struct platform_device *pdev) > +{ > + platform_set_drvdata(pdev, privacy_acpi); > + privacy_acpi->dev = &pdev->dev; > + privacy_acpi->platform_device = pdev; > + return 0; > +} > + > +static const struct acpi_device_id privacy_acpi_device_ids[] = { > + {"PNP0C09", 0}, > + { }, > +}; > +MODULE_DEVICE_TABLE(acpi, privacy_acpi_device_ids); > + > +static struct platform_driver dell_privacy_platform_drv = { > + .driver = { > + .name = PRIVACY_PLATFORM_NAME, > + .acpi_match_table = ACPI_PTR(privacy_acpi_device_ids), > + }, no .probe? > + .remove = dell_privacy_acpi_remove, > +}; > + > +int __init dell_privacy_acpi_init(void) > +{ > + int err; > + struct platform_device *pdev; > + int privacy_capable = wmi_has_guid(DELL_PRIVACY_GUID); > + > + if (!wmi_has_guid(DELL_PRIVACY_GUID)) > + return -ENODEV; > + > + privacy_acpi = kzalloc(sizeof(*privacy_acpi), GFP_KERNEL); > + if (!privacy_acpi) > + return -ENOMEM; > + > + pdev = platform_device_register_simple( > + PRIVACY_PLATFORM_NAME, PLATFORM_DEVID_NONE, NULL, 0); > + if (IS_ERR(pdev)) { > + err = PTR_ERR(pdev); > + goto pdev_err; > + } > + err = platform_driver_probe(&dell_privacy_platform_drv, > + dell_privacy_acpi_probe); > + if (err) > + goto pdrv_err; why is the probe done here? Put differently, what prevents you from using a 'normal' platform driver, and do the leds_setup in the .probe()? > + > + err = dell_privacy_leds_setup(&pdev->dev); > + if (err < 0) > + goto pdrv_err; > + > + return 0; > + > +pdrv_err: > + platform_device_unregister(pdev); > +pdev_err: > + kfree(privacy_acpi); > + return err; > +} > + > +void __exit dell_privacy_acpi_exit(void) > +{ > + struct platform_device *pdev = to_platform_device(privacy_acpi->dev); > + > + platform_device_unregister(pdev); > + platform_driver_unregister(&dell_privacy_platform_drv); > + kfree(privacy_acpi); > +} > + > +MODULE_AUTHOR("Perry Yuan "); > +MODULE_DESCRIPTION("DELL Privacy ACPI Driver"); > +MODULE_LICENSE("GPL"); > diff --git a/drivers/platform/x86/dell-privacy-wmi.c b/drivers/platform/x86/dell-privacy-wmi.c > new file mode 100644 > index 000000000000..432a3f4ed226 > --- /dev/null > +++ b/drivers/platform/x86/dell-privacy-wmi.c > @@ -0,0 +1,320 @@ > +// 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 > +#include > +#include > +#include > +#include > +#include > +#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 PRIVACY_SCREEN_STATUS BIT(2) > + > +static int privacy_valid = -EPROBE_DEFER; this is set to -ENODEV on remove. that looks odd to me. > +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; > + u32 features_present; > + u32 last_status; > +}; > + > +/* > + * 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}, > +}; > + > +int dell_privacy_valid(void) > +{ > + int ret; > + > + ret = wmi_has_guid(DELL_PRIVACY_GUID); > + if (!ret) > + return -ENODEV; > + ret = privacy_valid; > + return ret; return privacy_valid? > +} > +EXPORT_SYMBOL_GPL(dell_privacy_valid); > + > +void dell_privacy_process_event(int type, int code, int status) > +{ > + struct privacy_wmi_data *priv; > + const struct key_entry *key; > + > + mutex_lock(&list_mutex); > + priv = list_first_entry_or_null(&wmi_list, > + struct privacy_wmi_data, > + list); > + if (!priv) { > + pr_err("dell privacy priv is NULL\n"); > + goto error; > + } > + key = sparse_keymap_entry_from_scancode(priv->input_dev, (type << 16)|code); missing spaces, does this even pass with checkpatch.pl? > + if (!key) { > + dev_dbg(&priv->wdev->dev, "Unknown key with type 0x%04x and code 0x%04x pressed\n", > + type, code); > + goto error; > + } > + switch (code) { > + case DELL_PRIVACY_TYPE_AUDIO: /* Mic mute */ > + priv->last_status = status; > + sparse_keymap_report_entry(priv->input_dev, key, 1, true); > + break; > + case DELL_PRIVACY_TYPE_CAMERA: /* Camera mute */ Never seen anyone 'mute' their camera? 'switch off' or 'disable'? > + priv->last_status = status; > + sparse_keymap_report_entry(priv->input_dev, key, 1, true); > + break; > + default: > + dev_dbg(&priv->wdev->dev, "unknown event type 0x%04x 0x%04x", > + type, code); > + } > +error: > + mutex_unlock(&list_mutex); > +} > +EXPORT_SYMBOL_GPL(dell_privacy_process_event); [...] > +/* > + * 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 > + * [WmiDataId(2), read] uint32 CurrentState; > + * 0:Off; 1:On. Bit0 – Microphone, Bit1 – Camera, Bit2 - ePrivacyScreen > + * }; > + */ > + > +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"); > + return -EINVAL; > + } can this happen? when would this pointer be NULL? > + /* 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; > + privacy_valid = ret; > + return ret; > + } > + > + if (obj_present->type != ACPI_TYPE_BUFFER) { > + dev_err(&wdev->dev, "Binary MOF is not a buffer!\n"); > + ret = -EIO; > + privacy_valid = ret; > + 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; > + privacy_valid = ret; > + goto obj_free; > + } > + buffer = (u32 *)obj_present->buffer.pointer; > + priv->features_present = buffer[0]; > + priv->last_status = buffer[1]; > + privacy_valid = 0; > + > +obj_free: > + kfree(obj_present); > + return ret; > +} > + > +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, pos = 0; > + > + 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) + > + 1, same line? > + sizeof(struct key_entry), GFP_KERNEL); > + if (!keymap) { > + ret = -ENOMEM; > + goto err_free_dev; > + } > + /* 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[pos] = dell_wmi_keymap_type_0012[i]; > + keymap[pos].code |= (0x0012 << 16); > + pos++; > + } > + ret = sparse_keymap_setup(priv->input_dev, keymap, NULL); > + if (ret) > + return ret; > + priv->input_dev->dev.parent = &wdev->dev; > + priv->input_dev->name = "Dell Privacy Driver"; > + priv->input_dev->id.bustype = BUS_HOST; > + if (input_register_device(priv->input_dev)) { > + pr_debug("input_register_device failed to register!\n"); > + goto err_free_keymap; > + } > + mutex_lock(&list_mutex); > + list_add_tail(&priv->list, &wmi_list); > + mutex_unlock(&list_mutex); > + if (get_current_status(priv->wdev)) > + goto err_free_input; > + ret = devm_device_add_group(&wdev->dev, &privacy_attribute_group); > + if (ret) > + goto err_free_input; > + kfree(keymap); > + return 0; having a set of newlines wouldn't hurt, thsi is not very easy to read and split in logical sections... > + > +err_free_input: > + input_unregister_device(priv->input_dev); > +err_free_keymap: > + privacy_valid = -ENODEV; > + kfree(keymap); > +err_free_dev: > + input_free_device(priv->input_dev); > + return ret; > +} > + > +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); > + privacy_valid = -ENODEV; > + input_unregister_device(priv->input_dev); > + > + return 0; > +} [...] > + case 0x0012: > +#if IS_ENABLED(CONFIG_DELL_PRIVACY) > + err = dell_privacy_valid(); > + if (err == 0) { > + 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]); > + /* Extended data is currently ignored */ > + } > +#else > + if (len > 2) > + dell_wmi_process_key(wdev, buffer_entry[1], > + buffer_entry[2]); > + /* Extended data is currently ignored */ > +#endif this doesn't look very nice, can we avoid the duplication? > + break; > default: /* Unknown event */ > pr_info("Unknown WMI event type 0x%x\n", > (int)buffer_entry[1]); >