Re: [PATCH v5 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
Hi Hans. Thanks for your suggestion helping to improve the patch. On 3/24/21 7:39 AM, Hans de Goede wrote: Hi, On 3/24/21 12:19 PM, Hans de Goede wrote: Hi, On 3/22/21 10:38 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 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 emmits SW_CAMERA_LENS_COVER event while the camera lens shutter will be changed by EC & hw(hadware) 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. Signed-off-by: Perry Yuan One last remark, I think that the dell_privacy_present() function which I suggested in my review should be renamed to: dell_privacy_has_micmute() and then its return should check the micmute feature bit in priv->features_present and this new function should then be used in dell-laptop.c to determine it dell-laptop.c should register its own mic-mute led_classdev. This way if a theoretical future laptop shows up where the micmute feature is not implemented by the privacy interface dell-laptop.c will still register the non-privacy mic-mute led_classdev. Regards, Hans Yes, I rename the micmute checking function to dell_privacy_has_micmute. and i rewrite it to simplify the micmute checking process. I am doing some testing and drive other feedback to be finished. Will post V6 soon for your confirm. Perry. --- 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 | 32 ++ drivers/platform/x86/dell/Kconfig | 16 + drivers/platform/x86/dell/Makefile| 3 + drivers/platform/x86/dell/dell-laptop.c | 23 +- drivers/platform/x86/dell/dell-privacy-acpi.c | 164 + drivers/platform/x86/dell/dell-privacy-wmi.c | 340 ++ drivers/platform/x86/dell/dell-privacy-wmi.h | 35 ++ drivers/platform/x86/dell/dell-wmi.c | 27 +- 8 files changed, 627 insertions(+), 13 deletions(-) create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi create mode 100644 drivers/platform/x86/dell/dell-privacy-acpi.c 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 ..20c15a51ec38 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi @@ -0,0 +1,32 @@ +What: /sys/bus/wmi/devices/6932965F-1671-4CEB-B988-D3AB0A901919/devices_supported +Date: Feb 2021 +KernelVersion: 5.12 +Contact: "perry.y...@dell.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
Re: [PATCH v5 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
Hi Pierre
Thanks for your review .
I changed the patch and explain the review comments as below
(Maybe the mail format has problem, sent from one new system thunderbird :)
On 3/23/21 2:57 PM, Pierre-Louis Bossart wrote:
Minor comments below.
On 3/22/21 4:38 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
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 emmits
typo: emits
fixed
SW_CAMERA_LENS_COVER event while the camera lens shutter will be
changed by EC & hw(hadware) control
typo: hardware
fixed
*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.
what happens if there is timeout? You have an explicit description of
the timer handling in the success case, but not what happens on a
timeout...
add more explicit description for timeout triggered case
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
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 ..20c15a51ec38
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
@@ -0,0 +1,32 @@
+What:
/sys/bus/wmi/devices/6932965F-1671-4CEB-B988-D3AB0A901919/devices_supported
+Date: Feb 2021
+KernelVersion: 5.12
5.13 now?
changed to 5.13
+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;
+ static char *acpi_method = (char *)"ECAK";
+
+ 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->dev, "Error setting privacy EC ack value: %s\n",
+ acpi_format_exception(status));
+ return -EIO;
+ }
+ pr_debug("set dell privacy micmute ec ack event done\n");
can we use dev_dbg() here? Same for all occurrences of pr_debug and
pr_err below, it's cleaner and easier to filter.
I changed some pr_xx to dev_xxx , but below code will be more complex
to use dev_xxx to print the
log , because it need to get the priv->dev, but it is not registered at
this time , I would prefer to keep use the pr_debug here. and some
other cases where "priv->dev" cannot be used.
static int __init init_dell_privacy(void)
{
int ret;
ret = wmi_has_guid(DELL_PRIVACY_GUID);
if (!ret) {
privacy_valid = -ENODEV;
pr_debug("Unable to detect available Dell privacy
devices: %d\n", ret);
return privacy_valid;
}
+ 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(>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;
useless init
Sorry, I do not get this point .
int should be needed to return error code if
RE: [PATCH v5 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
Hi Hans. Reviewing your feedback and working on next version below > -Original Message- > From: Hans de Goede > Sent: Wednesday, March 24, 2021 7:19 PM > To: Yuan, Perry; po...@protonmail.com; pierre- > louis.boss...@linux.intel.com; oder_ch...@realtek.com; pe...@perex.cz; > ti...@suse.com; mgr...@linux.intel.com; Limonciello, Mario > Cc: lgirdw...@gmail.com; broo...@kernel.org; alsa-de...@alsa-project.org; > linux-kernel@vger.kernel.org; platform-driver-...@vger.kernel.org > Subject: Re: [PATCH v5 1/2] platform/x86: dell-privacy: Add support for Dell > hardware privacy > > > [EXTERNAL EMAIL] > > Hi, > > On 3/22/21 10:38 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 > > 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 emmits SW_CAMERA_LENS_COVER > > event while the camera lens shutter will be changed by EC & > > hw(hadware) 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. > > > > Signed-off-by: Perry Yuan > > --- > > 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 | 32 ++ > > drivers/platform/x86/dell/Kconfig | 16 + > > drivers/platform/x86/dell/Makefile| 3 + > > drivers/platform/x86/dell/dell-laptop.c | 23 +- > > drivers/platform/x86/dell/dell-privacy-acpi.c | 164 + > > drivers/platform/x86/dell/dell-privacy-wmi.c | 340 ++ > > drivers/platform/x86/dell/dell-privacy-wmi.h | 35 ++ > > drivers/platform/x86/dell/dell-wmi.c | 27 +- > > 8 files changed, 627 insertions(+), 13 deletions(-) create mode > > 100644 Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi > > create mode 100644 drivers/platform/x86/dell/dell-privacy-acpi.c > > 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 ..20c15a51ec38 > > --- /dev/null > > +++ b/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi > > @@ -0,0 +1,32 @@ > > +What:
Re: [PATCH v5 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
Hi,
On 3/25/21 8:52 AM, Yuan, Perry wrote:
> Hi Hans.
>
>> -Original Message-
>> From: Hans de Goede
>> Sent: Wednesday, March 24, 2021 3:40 AM
>> To: Pierre-Louis Bossart; Yuan, Perry; po...@protonmail.com;
>> oder_ch...@realtek.com; pe...@perex.cz; ti...@suse.com;
>> mgr...@linux.intel.com; Limonciello, Mario
>> Cc: alsa-de...@alsa-project.org; linux-kernel@vger.kernel.org;
>> lgirdw...@gmail.com; platform-driver-...@vger.kernel.org;
>> broo...@kernel.org
>> Subject: Re: [PATCH v5 1/2] platform/x86: dell-privacy: Add support for Dell
>> hardware privacy
>>
>>
>> [EXTERNAL EMAIL]
>>
>> Hi,
>>
>> On 3/23/21 7:57 PM, Pierre-Louis Bossart wrote:
>>> Minor comments below.
>>
>> >
>>>> +int __init dell_privacy_acpi_init(void)
>>>
>>> is the __init necessary? You call this routine from another which already
>>> has
>> this qualifier.
>>
>> Yes this is necessary, all functions which are only used during module_load /
>> from the module's init function must be marked as __init so that the kernel
>> can unload them after module loading is done.
>>
>> I do wonder if this one should not be static though (I've not looked at this
>> patch in detail yet).
>>
>>>
>>>> +{
>>>> + int err;
>>>> + struct platform_device *pdev;
>>>> +
>>>> + if (!wmi_has_guid(DELL_PRIVACY_GUID))
>>>> + return -ENODEV;
>>>> +
>>>> + privacy_acpi = kzalloc(sizeof(*privacy_acpi), GFP_KERNEL);
>>>> + if (!privacy_acpi)
>>>> + return -ENOMEM;
>>>> +
>>>> + err = platform_driver_register(_privacy_platform_drv);
>>>> + if (err)
>>>> + goto pdrv_err;
>>>> +
>>>> + pdev = platform_device_register_simple(
>>>> + PRIVACY_PLATFORM_NAME, PLATFORM_DEVID_NONE, NULL, 0);
>>>> + if (IS_ERR(pdev)) {
>>>> + err = PTR_ERR(pdev);
>>>> + goto pdev_err;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +
>>>> +pdev_err:
>>>> + platform_device_unregister(pdev);
>>>> +pdrv_err:
>>>> + kfree(privacy_acpi);
>>>> + return err;
>>>> +}
>>>> +
>>>> +void __exit dell_privacy_acpi_exit(void)
>>>
>>> is the __exit required here?
>>
>> Idem. Also static ?
>>
>> Regards,
>>
>> Hans
>>
>
> If adding static to the function, I will be worried about that the init and
> exit cannot be called by another file .
That is right, which is why I added the "?".
But this is no longer relevant after my detailed review of the patch,
so lets discuss things further in the detailed review email-thread.
Regards,
Hans
RE: [PATCH v5 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
Hi Hans.
> -Original Message-
> From: Hans de Goede
> Sent: Wednesday, March 24, 2021 3:40 AM
> To: Pierre-Louis Bossart; Yuan, Perry; po...@protonmail.com;
> oder_ch...@realtek.com; pe...@perex.cz; ti...@suse.com;
> mgr...@linux.intel.com; Limonciello, Mario
> Cc: alsa-de...@alsa-project.org; linux-kernel@vger.kernel.org;
> lgirdw...@gmail.com; platform-driver-...@vger.kernel.org;
> broo...@kernel.org
> Subject: Re: [PATCH v5 1/2] platform/x86: dell-privacy: Add support for Dell
> hardware privacy
>
>
> [EXTERNAL EMAIL]
>
> Hi,
>
> On 3/23/21 7:57 PM, Pierre-Louis Bossart wrote:
> > Minor comments below.
>
>
> >> +int __init dell_privacy_acpi_init(void)
> >
> > is the __init necessary? You call this routine from another which already
> > has
> this qualifier.
>
> Yes this is necessary, all functions which are only used during module_load /
> from the module's init function must be marked as __init so that the kernel
> can unload them after module loading is done.
>
> I do wonder if this one should not be static though (I've not looked at this
> patch in detail yet).
>
> >
> >> +{
> >> + int err;
> >> + struct platform_device *pdev;
> >> +
> >> + if (!wmi_has_guid(DELL_PRIVACY_GUID))
> >> + return -ENODEV;
> >> +
> >> + privacy_acpi = kzalloc(sizeof(*privacy_acpi), GFP_KERNEL);
> >> + if (!privacy_acpi)
> >> + return -ENOMEM;
> >> +
> >> + err = platform_driver_register(_privacy_platform_drv);
> >> + if (err)
> >> + goto pdrv_err;
> >> +
> >> + pdev = platform_device_register_simple(
> >> + PRIVACY_PLATFORM_NAME, PLATFORM_DEVID_NONE, NULL, 0);
> >> + if (IS_ERR(pdev)) {
> >> + err = PTR_ERR(pdev);
> >> + goto pdev_err;
> >> + }
> >> +
> >> + return 0;
> >> +
> >> +pdev_err:
> >> + platform_device_unregister(pdev);
> >> +pdrv_err:
> >> + kfree(privacy_acpi);
> >> + return err;
> >> +}
> >> +
> >> +void __exit dell_privacy_acpi_exit(void)
> >
> > is the __exit required here?
>
> Idem. Also static ?
>
> Regards,
>
> Hans
>
If adding static to the function, I will be worried about that the init and
exit cannot be called by another file .
>
>
> >
> >> +{
> >> + struct platform_device *pdev =
> >> +to_platform_device(privacy_acpi->dev);
> >> +
> >> + platform_device_unregister(pdev);
> >> + platform_driver_unregister(_privacy_platform_drv);
> >> + kfree(privacy_acpi);
> >> +}
> >> +
> >> +MODULE_AUTHOR("Perry Yuan ");
> >> +MODULE_DESCRIPTION("DELL Privacy ACPI Driver");
> >> +MODULE_LICENSE("GPL");
> >
> >
Re: [PATCH v5 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
Hi, On 3/24/21 12:19 PM, Hans de Goede wrote: > Hi, > > On 3/22/21 10:38 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 >> 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 emmits >> SW_CAMERA_LENS_COVER event while the camera lens shutter will be >> changed by EC & hw(hadware) 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. >> >> Signed-off-by: Perry Yuan One last remark, I think that the dell_privacy_present() function which I suggested in my review should be renamed to: dell_privacy_has_micmute() and then its return should check the micmute feature bit in priv->features_present and this new function should then be used in dell-laptop.c to determine it dell-laptop.c should register its own mic-mute led_classdev. This way if a theoretical future laptop shows up where the micmute feature is not implemented by the privacy interface dell-laptop.c will still register the non-privacy mic-mute led_classdev. Regards, Hans >> --- >> 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 | 32 ++ >> drivers/platform/x86/dell/Kconfig | 16 + >> drivers/platform/x86/dell/Makefile| 3 + >> drivers/platform/x86/dell/dell-laptop.c | 23 +- >> drivers/platform/x86/dell/dell-privacy-acpi.c | 164 + >> drivers/platform/x86/dell/dell-privacy-wmi.c | 340 ++ >> drivers/platform/x86/dell/dell-privacy-wmi.h | 35 ++ >> drivers/platform/x86/dell/dell-wmi.c | 27 +- >> 8 files changed, 627 insertions(+), 13 deletions(-) >> create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi >> create mode 100644 drivers/platform/x86/dell/dell-privacy-acpi.c >> 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 ..20c15a51ec38 >> --- /dev/null >> +++ b/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi >> @@ -0,0 +1,32 @@ >> +What: >> /sys/bus/wmi/devices/6932965F-1671-4CEB-B988-D3AB0A901919/devices_supported >> +Date: Feb 2021 >> +KernelVersion: 5.12 >> +Contact:"perry.y...@dell.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: >> +- 0 = Not Supported >> +- 1 = Supported >> +- Bit0 -> Microphone >> +
Re: [PATCH v5 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
Hi, On 3/22/21 10:38 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 > 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 emmits > SW_CAMERA_LENS_COVER event while the camera lens shutter will be > changed by EC & hw(hadware) 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. > > Signed-off-by: Perry Yuan > --- > 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 | 32 ++ > drivers/platform/x86/dell/Kconfig | 16 + > drivers/platform/x86/dell/Makefile| 3 + > drivers/platform/x86/dell/dell-laptop.c | 23 +- > drivers/platform/x86/dell/dell-privacy-acpi.c | 164 + > drivers/platform/x86/dell/dell-privacy-wmi.c | 340 ++ > drivers/platform/x86/dell/dell-privacy-wmi.h | 35 ++ > drivers/platform/x86/dell/dell-wmi.c | 27 +- > 8 files changed, 627 insertions(+), 13 deletions(-) > create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi > create mode 100644 drivers/platform/x86/dell/dell-privacy-acpi.c > 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 ..20c15a51ec38 > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi > @@ -0,0 +1,32 @@ > +What: > /sys/bus/wmi/devices/6932965F-1671-4CEB-B988-D3AB0A901919/devices_supported > +Date:Feb 2021 > +KernelVersion: 5.12 > +Contact: "perry.y...@dell.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: > + - 0 = Not Supported > + - 1 = Supported > + - Bit0 -> Microphone > + - Bit1 -> Camera > + - Bit2 -> ePrivacy Screen > + > +What: > /sys/bus/wmi/devices/6932965F-1671-4CEB-B988-D3AB0A901919/current_state > +Date:Feb 2021 > +KernelVersion: 5.12 > +Contact: "perry.y...@dell.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 > + There are three Bits for available states: > + - 0 = Enabled > + - 1 = Disabled What is the
Re: [PATCH v5 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
Hi,
On 3/23/21 7:57 PM, Pierre-Louis Bossart wrote:
> Minor comments below.
> +int __init dell_privacy_acpi_init(void)
>
> is the __init necessary? You call this routine from another which already has
> this qualifier.
Yes this is necessary, all functions which are only used during module_load /
from the
module's init function must be marked as __init so that the kernel can unload
them
after module loading is done.
I do wonder if this one should not be static though (I've not looked at this
patch in
detail yet).
>
>> +{
>> + int err;
>> + struct platform_device *pdev;
>> +
>> + if (!wmi_has_guid(DELL_PRIVACY_GUID))
>> + return -ENODEV;
>> +
>> + privacy_acpi = kzalloc(sizeof(*privacy_acpi), GFP_KERNEL);
>> + if (!privacy_acpi)
>> + return -ENOMEM;
>> +
>> + err = platform_driver_register(_privacy_platform_drv);
>> + if (err)
>> + goto pdrv_err;
>> +
>> + pdev = platform_device_register_simple(
>> + PRIVACY_PLATFORM_NAME, PLATFORM_DEVID_NONE, NULL, 0);
>> + if (IS_ERR(pdev)) {
>> + err = PTR_ERR(pdev);
>> + goto pdev_err;
>> + }
>> +
>> + return 0;
>> +
>> +pdev_err:
>> + platform_device_unregister(pdev);
>> +pdrv_err:
>> + kfree(privacy_acpi);
>> + return err;
>> +}
>> +
>> +void __exit dell_privacy_acpi_exit(void)
>
> is the __exit required here?
Idem. Also static ?
Regards,
Hans
>
>> +{
>> + struct platform_device *pdev = to_platform_device(privacy_acpi->dev);
>> +
>> + platform_device_unregister(pdev);
>> + platform_driver_unregister(_privacy_platform_drv);
>> + kfree(privacy_acpi);
>> +}
>> +
>> +MODULE_AUTHOR("Perry Yuan ");
>> +MODULE_DESCRIPTION("DELL Privacy ACPI Driver");
>> +MODULE_LICENSE("GPL");
>
>
Re: [PATCH v5 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
Minor comments below.
On 3/22/21 4:38 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
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 emmits
typo: emits
SW_CAMERA_LENS_COVER event while the camera lens shutter will be
changed by EC & hw(hadware) control
typo: hardware
*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.
what happens if there is timeout? You have an explicit description of
the timer handling in the success case, but not what happens on a timeout...
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 ..20c15a51ec38
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
@@ -0,0 +1,32 @@
+What:
/sys/bus/wmi/devices/6932965F-1671-4CEB-B988-D3AB0A901919/devices_supported
+Date: Feb 2021
+KernelVersion: 5.12
5.13 now?
+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;
+ static char *acpi_method = (char *)"ECAK";
+
+ 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->dev, "Error setting privacy EC ack value: %s\n",
+ acpi_format_exception(status));
+ return -EIO;
+ }
+ pr_debug("set dell privacy micmute ec ack event done\n");
can we use dev_dbg() here? Same for all occurrences of pr_debug and
pr_err below, it's cleaner and easier to filter.
+ 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(>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;
useless init
+
+ 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, >cdev);
+ if (ret)
+ return ret;
+ return 0;
+}
+
+static int dell_privacy_acpi_probe(struct platform_device *pdev)
+{
+ int ret;
+
+ platform_set_drvdata(pdev, privacy_acpi);
+ privacy_acpi->dev = >dev;
+ privacy_acpi->platform_device = pdev;
+
+ ret = dell_privacy_leds_setup(>dev);
+ if (ret)
+ return -EIO;
any reason to hard-code -EIO, woud 'return ret' be enough?
+
+ return 0;
+}
+
+static struct platform_driver dell_privacy_platform_drv = {
+ .driver = {
+ .name = PRIVACY_PLATFORM_NAME,
+ },
+ .probe = dell_privacy_acpi_probe,
+ .remove = dell_privacy_acpi_remove,
+};
+
+int __init dell_privacy_acpi_init(void)
is the __init necessary? You call this routine
RE: [PATCH v5 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
Hi Randy. > -Original Message- > From: Randy Dunlap > Sent: Tuesday, March 23, 2021 1:15 AM > To: Yuan, Perry; po...@protonmail.com; pierre- > louis.boss...@linux.intel.com; oder_ch...@realtek.com; pe...@perex.cz; > ti...@suse.com; hdego...@redhat.com; mgr...@linux.intel.com; > Limonciello, Mario > Cc: lgirdw...@gmail.com; broo...@kernel.org; alsa-de...@alsa-project.org; > linux-kernel@vger.kernel.org; platform-driver-...@vger.kernel.org > Subject: Re: [PATCH v5 1/2] platform/x86: dell-privacy: Add support for Dell > hardware privacy > > > [EXTERNAL EMAIL] > > On 3/22/21 2:38 AM, Perry Yuan wrote: > > From: Perry Yuan > > > > > diff --git a/drivers/platform/x86/dell/Kconfig > > b/drivers/platform/x86/dell/Kconfig > > index e0a55337f51a..0e0f1eb35bd6 100644 > > --- a/drivers/platform/x86/dell/Kconfig > > +++ b/drivers/platform/x86/dell/Kconfig > > @@ -204,4 +204,20 @@ 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 support for the "Dell Hardware Privacy" feature > > + of Dell laptops. > > + Support for a micmute and camera mute privacy will be provided as > > better: > are provided as > > > + hardware button Ctrl+F4 and Ctrl+F9 hotkey. > > Does that say that Ctrl+F4 is a hardware button and that Ctrl+F9 is a hotkey? > If so, what's the difference? and why? Are they different hardware > implementations? Does the user care? They all use the Ctrl button, Ctrl +F4 requires that the Ctrl button and F4 button are pressed at the same time. F9+ctrl also have the same meaning. They are belonging to same hardware privacy solution, just use different keys combination to activate different privacy function . > > > + > > + To compile this driver as a module, choose M here: the module will > > + be called dell_privacy. > > endif # X86_PLATFORM_DRIVERS_DELL > thanks. > -- > ~Randy
Re: [PATCH v5 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
On 3/22/21 2:38 AM, Perry Yuan wrote: > From: Perry Yuan > > diff --git a/drivers/platform/x86/dell/Kconfig > b/drivers/platform/x86/dell/Kconfig > index e0a55337f51a..0e0f1eb35bd6 100644 > --- a/drivers/platform/x86/dell/Kconfig > +++ b/drivers/platform/x86/dell/Kconfig > @@ -204,4 +204,20 @@ 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 support for the "Dell Hardware Privacy" feature > + of Dell laptops. > + Support for a micmute and camera mute privacy will be provided as better: are provided as > + hardware button Ctrl+F4 and Ctrl+F9 hotkey. Does that say that Ctrl+F4 is a hardware button and that Ctrl+F9 is a hotkey? If so, what's the difference? and why? Are they different hardware implementations? Does the user care? > + > + To compile this driver as a module, choose M here: the module will > + be called dell_privacy. > endif # X86_PLATFORM_DRIVERS_DELL thanks. -- ~Randy
[PATCH v5 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
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 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 emmits SW_CAMERA_LENS_COVER event while the camera lens shutter will be changed by EC & hw(hadware) 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. Signed-off-by: Perry Yuan --- 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 | 32 ++ drivers/platform/x86/dell/Kconfig | 16 + drivers/platform/x86/dell/Makefile| 3 + drivers/platform/x86/dell/dell-laptop.c | 23 +- drivers/platform/x86/dell/dell-privacy-acpi.c | 164 + drivers/platform/x86/dell/dell-privacy-wmi.c | 340 ++ drivers/platform/x86/dell/dell-privacy-wmi.h | 35 ++ drivers/platform/x86/dell/dell-wmi.c | 27 +- 8 files changed, 627 insertions(+), 13 deletions(-) create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi create mode 100644 drivers/platform/x86/dell/dell-privacy-acpi.c 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 ..20c15a51ec38 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi @@ -0,0 +1,32 @@ +What: /sys/bus/wmi/devices/6932965F-1671-4CEB-B988-D3AB0A901919/devices_supported +Date: Feb 2021 +KernelVersion: 5.12 +Contact: "perry.y...@dell.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: + - 0 = Not Supported + - 1 = Supported + - Bit0 -> Microphone + - Bit1 -> Camera + - Bit2 -> ePrivacy Screen + +What: /sys/bus/wmi/devices/6932965F-1671-4CEB-B988-D3AB0A901919/current_state +Date: Feb 2021 +KernelVersion: 5.12 +Contact: "perry.y...@dell.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 + There are three Bits for available states: + - 0 = Enabled + - 1 = Disabled + - Bit0 -> Microphone + - Bit1 -> Camera + - Bit2 -> ePrivacyScreen + diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig index e0a55337f51a..0e0f1eb35bd6 100644 ---
