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

[Marco Giunta]

Hi Huang,

Thanks for the review and the useful feedback.

> I went through the patch and the approach looks right to me - matching
> what lenovo-wmi-camera does with SW_CAMERA_LENS_COVER is the correct
> call, and reporting the initial state before input_register_device()
> is the right way for EVIOCGSW to pick it up.

Good to know the approach makes sense, thanks for confirming it.

> I initially wondered whether the lazy device creation could race
> against a concurrent notification, but ideapad_wmi_notify() holds
> ideapad_shared_mutex across the whole handler, so the init is
> serialized. It might be worth a comment on the init function saying
> it relies on that, since it's not obvious from the code itself - I
> had to go and check.

This is a fair point. Indeed the patch as it is relies *implicitly* on
the pre-existing mutex infrastructure.

I think there are at least two possible approaches to act on this.

  1.  A comment above ideapad_camera_switch_init() stating that the caller
      holds ideapad_shared_mutex (or something along these lines);

  2.  using something like lockdep_assert_held(&ideapad_shared_mutex) in
      ideapad_camera_switch_report(). This documents the requirement and
      checks it on PROVE_LOCKING kernels, and is the approach recommended
      by Documentation/locking/lockdep-design.rst (sec. "Annotations") and
      Documentation/process/maintainer-tip.rst (sec. "Documenting locking
      requirements").

As for 2, the documentation above is not specific to this subsystem, but
the same rationale seems to apply here, and the annotation is already used
elsewhere in platform/x86, e.g. in intel/sdsi.c and amd/hsmp/.
It would be one line that compiles away without CONFIG_LOCKDEP and warns
with CONFIG_PROVE_LOCKING.
On the other hand, I may be missing some context regarding how this class
of annotations is meant to be used. And of course 1 & 2 could be used
together.

Any preference or other thoughts?

> One small thing: if ideapad_camera_switch_init() fails, the state
> carried by that event is dropped, and every later event retries the
> registration and warns again. Probably fine in practice, but a
> persistent failure would log one warning per toggle.

That's another fair point. I admit I copy-pasted that dev_warn() from the
lenovo-wmi-camera driver without too much thought.
Do you think it's a good idea e.g. to replace dev_warn() with
dev_warn_once()? Or something else entirely?

> Thanks also for the notes on the alternatives and the
> suspend-staleness limitation, that context is really helpful for
> review.

Thank you for pointing this out, I have never included such a section
before and was wondering about its usefulness.

I will collect any further comment before sending out a v2.

Thanks again for your review and feedback.

Best regards,
Marco