Re: [PATCH v2] HID: i2c-hid: fix handling of unpopulated devices

[Doug Anderson]

Hi,

On Thu, Oct 5, 2023 at 12:10 AM Benjamin Tissoires <bentiss@xxxxxxxxxx> wrote:
>
> On Oct 02 2023, Johan Hovold wrote:
> > A recent commit reordered probe so that the interrupt line is now
> > requested before making sure that the device exists.
> >
> > This breaks machines like the Lenovo ThinkPad X13s which rely on the
> > HID driver to probe second-source devices and only register the variant
> > that is actually populated. Specifically, the interrupt line may now
> > already be (temporarily) claimed when doing asynchronous probing of the
> > touchpad:
> >
> >       genirq: Flags mismatch irq 191. 00082008 (hid-over-i2c) vs. 00082008 (hid-over-i2c)
> >       i2c_hid_of 21-0015: Could not register for hid-over-i2c interrupt, irq = 191, ret = -16
> >       i2c_hid_of: probe of 21-0015 failed with error -16
> >
> > Fix this by restoring the old behaviour of first making sure the device
> > exists before requesting the interrupt line.
> >
> > Note that something like this should probably be implemented also for
> > "panel followers", whose actual probe is currently effectively deferred
> > until the DRM panel is probed (e.g. by powering down the device after
> > making sure it exists and only then register it as a follower).
> >
> > Fixes: 675cd877c952 ("HID: i2c-hid: Rearrange probe() to power things up later")
> > Cc: Douglas Anderson <dianders@xxxxxxxxxxxx>
> > Cc: Maxime Ripard <mripard@xxxxxxxxxx>
> > Signed-off-by: Johan Hovold <johan+linaro@xxxxxxxxxx>
> > ---
> >
> > Changes in v2
> >  - initialise ihid->is_panel_follower sooner to avoid repeated property
> >    lookups and so that it can be used consistently throughout the driver
> >    for code that differs for "panel followers"
>
> Patches looks good to me, but I can not test it unfortunately.
>
> Doug, would you mind sending us your Ack or tested-by?

Sure. Patches look OK to me, so if you're good with them then I'm good
with them too.

Reviewed-by: Douglas Anderson <dianders@xxxxxxxxxxxx>

I tested this on a board by adding a second i2c-hid device in the
device tree and confirming that things appeared to work OK. I also
tried this with a board setup to use panel-follower (but _not_ two
sources of touchscreens) and that also worked OK. Thus:

Tested-by: Douglas Anderson <dianders@xxxxxxxxxxxx>

As expected, combining panel-follower with two sources of touchscreen
_didn't_ work because only one of them is able to acquire the
interrupt. This is fine with me as there is nobody currently doing
that. I'm still of the belief that we need a more complete solution
for that and I'll continue to work on it.

Thanks!

-Doug