Re: [PATCH v2] platform/x86: ideapad-laptop: Report camera switch as SW_CAMERA_LENS_COVER

[Marco Giunta]

Hi Rong,

Thank you so much for your detailed reply and the much improved patch.

Let me immediately go on record and say that, apart from a small addition
to the SMI driver and one line in ideapad-laptop, I am mostly active in
the sound system, and therefore consider myself a platform/x86 newbie.
So I really appreciate you sharing your knowledge here.

> > * I considered a couple of alternatives to the approach in this patch:
> >
> >   1.  Simply map 0x0c and 0x0d to KEY_CAMERA_ACCESS_ENABLE/DISABLE:
> >
> >       { KE_KEY,	0x0c | IDEAPAD_WMI_KEY, { KEY_CAMERA_ACCESS_ENABLE } },
> >       { KE_KEY,	0x0d | IDEAPAD_WMI_KEY, { KEY_CAMERA_ACCESS_DISABLE } },
> >
> >       similar to the touchpad off/on hkey events (66/67) this driver emits
> >       after the firmware has toggled the touchpad state. However, HUTRR72
> >       describes these hkey events as asserting a camera access state for
> >       the host to apply, rather than reporting one the firmware has
> >       already applied.
> >
> 
> Well, it really doesn't matter how the specification says; what matters
> is how devices in reality do: whenever the HID Usage Table doesn't define
> a usage for hardware/firmware to report the already-applied value but
> defines a usage for notifying the host to apply a specific value, a lot
> of devices will emerge, using the latter even when the former is
> intended.
> 
> For example, many USB Audio Class devices come with volume control
> buttons or knobs. Some of them tune the volume themselves as well as
> emitting Volume Increment/Decrement HID events.
> 
> In this case, the audio stack, in response to the HID events,
> intentionally overrides the volume set by hardware, effectively
> synchronizing the hardware and software volume value.
> 
> The pattern has become so common that many devices like that also
> implement an internal timeout mechanism. They only set the volume
> themselves when reaching the timeout with no UAC volume control request
> received.
> 
> IOW, it doesn't matter whether the hardware/firmware has done its job as
> long as the subsequent software control is harmless. That's why we can
> safely use KEY_TOUCHPAD_ON/OFF even if the firmware has toggled the
> touchpad state. They cause the desktop environment to mask/unmask
> touchpad input, which is essentially a no-op (thus a safe operation) when
> the firmware has already disabled/enabled the touchpad.
> 
> Fundamentally speaking, using KEY_CAMERA_ACCESS_ENABLE/DISABLE is also
> OK. It's just...
> 
> > Furthermore, lenovo-wmi-camera moved away from
> >       these hkey events in favor of SW_CAMERA_LENS_COVER in the quoted
> >       commit, which is arguably the closest relevant precedent.
> 
> ...relatively new and isn't widely used compared to SW_CAMERA_LENS_COVER.
> 
> Thus, I agreed that using SW_CAMERA_LENS_COVER is a better approach as it
> aligns with other drivers' behavior. The patch seems over-engineered
> though, see below.

This seems pretty reasonable to me, I figured that a single standard was
hard to enforce and also not strictly needed in practice (the "harmless
no-op" argument above).
I was mostly wondering about certain discussions I saw online while doing
research for this patch of people arguing that such a no-op should be
avoided, see e.g.:
https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/merge_requests/181

Still, your point that userspace duplicating an action already applied by
the hardware is common practice, and fine as long as it is harmless, makes
perfect sense to me. So overall I have no objections to your reasoning here.

> >
> >   2.  Ignore these events like the firmware-handled FnLock events:
> >
> > 			{ KE_IGNORE,	0x0c | IDEAPAD_WMI_KEY },
> > 			{ KE_IGNORE,	0x0d | IDEAPAD_WMI_KEY },
> >
> >       This fixes the KEY_UNKNOWN issue, but userspace gets no information.
> >
> >   Overall, in the end I copied the approach of the lenovo-wmi-camera
> >   driver. Kindly let me know if there are other/better solutions.
> >
> > * Unlike the touchpad, whose state this driver reads with VPCCMD_R_TOUCHPAD
> >   at probe, on resume and on each event,
> >
> 
> At least for the input device, the synchronization on probe or on resume
> does not matter, as no input event is emitted due to !send_events.

Good point, I was conflating stuff.

> > the camera switch state is only
> >   available in EC-private fields, and the two tested laptops even use
> >   different ones. Hence the lenovo-wmi-camera approach: the switch appears
> >   on the first event, and a change made while suspended is not reported,
> >   leaving the state wrong until the switch is toggled again.
> 
> So the difference compared to the lenovo-wmi-camera approach is only
> about when and how the input device is registered.
> 
> On recent models, the firmware neither emits VPCCMD_R_TOUCHPAD nor
> toggles the touchpad. It leaves the job to software by emitting WMI key
> event 0x29, which is converted to KEY_TOUCHPAD_TOGGLE via ideapad_keymap.
> It'd be tidier to take that approach and reuse the existing input device.
> Sparse keymap supports KEY_SW, so it's viable to map the two WMI events
> to SW_CAMERA_LENS_COVER with appropriate switch value.

This makes a lot of sense to me. I appreciate the simplicity of this
approach.

Part of the reason why I initially adopted the "separate input device"
approach was this comment from
drivers/platform/x86/dell/dell-wmi-privacy.c:

/*
 * Unlike keys where only presses matter, userspace may act
 * on switches in both of their positions. Only register
 * SW_CAMERA_LENS_COVER if it is actually there.
 */

and so I implemented a conditional registration (which was then easy to do
by copying the overall approach of the lenovo-wmi-camera driver).

However, I understand that with your patch the switch will simply be
advertised by the existing input device with a default state of 0,
i.e. "camera enabled", which never changes on a device without a
privacy switch, and is indeed a correct description of it. So I
don't think there's any harm in going with your approach.

> I've attached a patch implementing the approach. Could you test if it
> works on your devices?

I compiled it on my Legion and it works as expected. Right after booting,
evtest says:

Input driver version is 1.0.1
Input device ID: bus 0x19 vendor 0x0 product 0x0 version 0x0
Input device name: "Ideapad extra buttons"
Supported events:
  Event type 0 (EV_SYN)
  Event type 1 (EV_KEY)
    Event code 1 (KEY_ESC)
    Event code 138 (KEY_HELP)
    Event code 148 (KEY_PROG1)
    Event code 149 (KEY_PROG2)
    Event code 186 (KEY_F16)
    Event code 202 (KEY_PROG3)
    Event code 203 (KEY_PROG4)
    Event code 212 (KEY_CAMERA)
    Event code 227 (KEY_SWITCHVIDEOMODE)
    Event code 240 (KEY_UNKNOWN)
    Event code 247 (KEY_RFKILL)
    Event code 248 (KEY_MICMUTE)
    Event code 364 (KEY_FAVORITES)
    Event code 445 (KEY_PICKUP_PHONE)
    Event code 446 (KEY_HANGUP_PHONE)
    Event code 447 (?) # commit bc39af8c2c43 was included in this kernel
    Event code 530 (KEY_TOUCHPAD_TOGGLE)
    Event code 531 (KEY_TOUCHPAD_ON)
    Event code 532 (KEY_TOUCHPAD_OFF)
    Event code 562 (KEY_REFRESH_RATE_TOGGLE)
    Event code 618 (KEY_ROOT_MENU)
    Event code 634 (KEY_SELECTIVE_SCREENSHOT)
  Event type 4 (EV_MSC)
    Event code 4 (MSC_SCAN)
  Event type 5 (EV_SW)
    Event code 9 (SW_CAMERA_LENS_COVER) state 0

Starting from the "camera enabled" state and toggling the privacy switch
twice causes:

Event: time 1790013067.697647, type 5 (EV_SW), code 9 (SW_CAMERA_LENS_COVER), value 1
Event: time 1790013067.697647, -------------- SYN_REPORT ------------
Event: time 1790013069.208581, type 5 (EV_SW), code 9 (SW_CAMERA_LENS_COVER), value 0
Event: time 1790013069.208581, -------------- SYN_REPORT ------------

This is the same behavior as my original patch, just on the same input
device as everything else (as expected).

The switch starts on state 0 at boot even if the camera was disabled
while the device was turned off, in which case a new event is reported only
after toggling the switch twice. However this is expected and is the same
behavior as in the original patch if e.g. the switch is toggled during
suspend, so this is not a critique, only a confirmation that your patch
works as I expected.

> If so, you may submit my patch yourself by adding
> a From: pseudo header with my identity and swapping the two Signed-off-
> by: tags. I can also submit it myself as long as you permit me to do so.

I am happy to have you submit the new patch as its main author; it's your
code now!

I'll only add that, apart from

Co-developed-by: Marco Giunta <marco_giunta@xxxxxxxxxx>
Signed-off-by: Marco Giunta <marco_giunta@xxxxxxxxxx>

I think it's also appropriate to add

Tested-by: Marco Giunta <marco_giunta@xxxxxxxxxx>

> >
> > * For reference, on KDE Plasma 6.7.5, using the Legion's touchpad toggle
> >   button shows an OSD, but nothing on a SW_CAMERA_LENS_COVER change or if
> >   sending KEY_CAMERA_ACCESS_ENABLE/DISABLE events. I am not sure whether
> >   this is simply a case of "not implemented yet" or if I misunderstood how
> >   these events are supposed to be used to communicate with userspace.
> >   Please feel free to correct me here.
> 
> I believe KDE Plasma has not implemented it yet. You may raise an issue
> on their Bugzilla.
> 
Noted, thanks.

> > Hi Macro,
>
> Whoops, I mistyped your name. Sorry for that. My fingers definitely have
> their own thoughts.

No worries!!

Thank you again for your very informative review and much simpler patch.
And please feel free to correct me again if there are further inaccuracies
in my understanding of the problem.

Best regards,
Marco