diff mbox series

HID: sensor-hub: Enable hid core report processing for all devices

Message ID 20231219231503.1506801-1-jekhor@gmail.com
State New
Headers
Series HID: sensor-hub: Enable hid core report processing for all devices |

Commit Message

Yauhen Kharuzhy Dec. 19, 2023, 11:15 p.m. UTC 
  After the commit 666cf30a589a ("HID: sensor-hub: Allow multi-function
sensor devices") hub devices are claimed by hidraw driver in hid_connect().
This causes stoppping of processing HID reports by hid core due to
optimization.

In such case, the hid-sensor-custom driver cannot match a known custom
sensor in hid_sensor_custom_get_known() because it try to check custom
properties which weren't filled from the report because hid core didn't
parsed it.

As result, custom sensors like hinge angle sensor and LISS sensors
don't work.

Mark the sensor hub devices claimed by some driver to avoid hidraw-related
optimizations.

Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
---
 drivers/hid/hid-sensor-hub.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jonathan Cameron Dec. 20, 2023, 2:52 p.m. UTC | #1 
On Wed, 20 Dec 2023 01:15:03 +0200
Yauhen Kharuzhy <jekhor@gmail.com> wrote:

> After the commit 666cf30a589a ("HID: sensor-hub: Allow multi-function
> sensor devices") hub devices are claimed by hidraw driver in hid_connect().
> This causes stoppping of processing HID reports by hid core due to
> optimization.
> 
> In such case, the hid-sensor-custom driver cannot match a known custom
> sensor in hid_sensor_custom_get_known() because it try to check custom
> properties which weren't filled from the report because hid core didn't
> parsed it.
> 
> As result, custom sensors like hinge angle sensor and LISS sensors
> don't work.
> 
> Mark the sensor hub devices claimed by some driver to avoid hidraw-related
> optimizations.
> 
> Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
Fixes tag?

> ---
>  drivers/hid/hid-sensor-hub.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
> index 2eba152e8b90..26e93a331a51 100644
> --- a/drivers/hid/hid-sensor-hub.c
> +++ b/drivers/hid/hid-sensor-hub.c
> @@ -632,7 +632,7 @@ static int sensor_hub_probe(struct hid_device *hdev,
>  	}
>  	INIT_LIST_HEAD(&hdev->inputs);
>  
> -	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> +	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT | HID_CONNECT_DRIVER);
>  	if (ret) {
>  		hid_err(hdev, "hw start failed\n");
>  		return ret;
Yauhen Kharuzhy Dec. 20, 2023, 3:04 p.m. UTC | #2 
ср, 20 дек. 2023 г. в 16:52, Jonathan Cameron <jic23@kernel.org>:
>
> On Wed, 20 Dec 2023 01:15:03 +0200
> Yauhen Kharuzhy <jekhor@gmail.com> wrote:
>
> > After the commit 666cf30a589a ("HID: sensor-hub: Allow multi-function
> > sensor devices") hub devices are claimed by hidraw driver in hid_connect().
> > This causes stoppping of processing HID reports by hid core due to
> > optimization.
> >
> > In such case, the hid-sensor-custom driver cannot match a known custom
> > sensor in hid_sensor_custom_get_known() because it try to check custom
> > properties which weren't filled from the report because hid core didn't
> > parsed it.
> >
> > As result, custom sensors like hinge angle sensor and LISS sensors
> > don't work.
> >
> > Mark the sensor hub devices claimed by some driver to avoid hidraw-related
> > optimizations.
> >
> > Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
> Fixes tag?

Fixes: 666cf30a589a ("HID: sensor-hub: Allow multi-function sensor devices")

>
> > ---
> >  drivers/hid/hid-sensor-hub.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
> > index 2eba152e8b90..26e93a331a51 100644
> > --- a/drivers/hid/hid-sensor-hub.c
> > +++ b/drivers/hid/hid-sensor-hub.c
> > @@ -632,7 +632,7 @@ static int sensor_hub_probe(struct hid_device *hdev,
> >       }
> >       INIT_LIST_HEAD(&hdev->inputs);
> >
> > -     ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> > +     ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT | HID_CONNECT_DRIVER);
> >       if (ret) {
> >               hid_err(hdev, "hw start failed\n");
> >               return ret;
>
srinivas pandruvada Dec. 22, 2023, 12:43 p.m. UTC | #3 
On Wed, 2023-12-20 at 17:04 +0200, Yauhen Kharuzhy wrote:
> ср, 20 дек. 2023 г. в 16:52, Jonathan Cameron <jic23@kernel.org>:
> > 
> > On Wed, 20 Dec 2023 01:15:03 +0200
> > Yauhen Kharuzhy <jekhor@gmail.com> wrote:
> > 
> > > After the commit 666cf30a589a ("HID: sensor-hub: Allow multi-
> > > function
> > > sensor devices") hub devices are claimed by hidraw driver in
> > > hid_connect().
> > > This causes stoppping of processing HID reports by hid core due
> > > to
> > > optimization.
> > > 
> > > In such case, the hid-sensor-custom driver cannot match a known
> > > custom
> > > sensor in hid_sensor_custom_get_known() because it try to check
> > > custom
> > > properties which weren't filled from the report because hid core
> > > didn't
> > > parsed it.
> > > 
> > > As result, custom sensors like hinge angle sensor and LISS
> > > sensors
> > > don't work.
> > > 
> > > Mark the sensor hub devices claimed by some driver to avoid
> > > hidraw-related
> > > optimizations.
> > > 
> > > Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
> > Fixes tag?
> 
> Fixes: 666cf30a589a ("HID: sensor-hub: Allow multi-function sensor
> devices")
> 
This flag causes
 		hdev->claimed |= HID_CLAIMED_DRIVER;
I don't see the flag is used anywhere after this assignment in hid
core. Only two other drivers are setting this flag. We need Jiri's help
here why this is a special case.

Thanks,
Srinivas

> > 
> > > ---
> > >  drivers/hid/hid-sensor-hub.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-
> > > sensor-hub.c
> > > index 2eba152e8b90..26e93a331a51 100644
> > > --- a/drivers/hid/hid-sensor-hub.c
> > > +++ b/drivers/hid/hid-sensor-hub.c
> > > @@ -632,7 +632,7 @@ static int sensor_hub_probe(struct hid_device
> > > *hdev,
> > >       }
> > >       INIT_LIST_HEAD(&hdev->inputs);
> > > 
> > > -     ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> > > +     ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT |
> > > HID_CONNECT_DRIVER);
> > >       if (ret) {
> > >               hid_err(hdev, "hw start failed\n");
> > >               return ret;
> > 
> 
>
Benjamin Tissoires Dec. 22, 2023, 1:28 p.m. UTC | #4 
On Fri, Dec 22, 2023 at 1:44 PM srinivas pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
>
> On Wed, 2023-12-20 at 17:04 +0200, Yauhen Kharuzhy wrote:
> > ср, 20 дек. 2023 г. в 16:52, Jonathan Cameron <jic23@kernel.org>:
> > >
> > > On Wed, 20 Dec 2023 01:15:03 +0200
> > > Yauhen Kharuzhy <jekhor@gmail.com> wrote:
> > >
> > > > After the commit 666cf30a589a ("HID: sensor-hub: Allow multi-
> > > > function
> > > > sensor devices") hub devices are claimed by hidraw driver in
> > > > hid_connect().
> > > > This causes stoppping of processing HID reports by hid core due
> > > > to
> > > > optimization.
> > > >
> > > > In such case, the hid-sensor-custom driver cannot match a known
> > > > custom
> > > > sensor in hid_sensor_custom_get_known() because it try to check
> > > > custom
> > > > properties which weren't filled from the report because hid core
> > > > didn't
> > > > parsed it.
> > > >
> > > > As result, custom sensors like hinge angle sensor and LISS
> > > > sensors
> > > > don't work.
> > > >
> > > > Mark the sensor hub devices claimed by some driver to avoid
> > > > hidraw-related
> > > > optimizations.
> > > >
> > > > Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
> > > Fixes tag?
> >
> > Fixes: 666cf30a589a ("HID: sensor-hub: Allow multi-function sensor
> > devices")
> >
> This flag causes
>                 hdev->claimed |= HID_CLAIMED_DRIVER;
> I don't see the flag is used anywhere after this assignment in hid
> core. Only two other drivers are setting this flag. We need Jiri's help
> here why this is a special case.

It's used in hid_report_raw_event()[0]:
```
    if (hid->claimed != HID_CLAIMED_HIDRAW && report->maxfield) {
        hid_process_report(hid, report, cdata, interrupt);
        hdrv = hid->driver;
        if (hdrv && hdrv->report)
            hdrv->report(hid, report);
    }
```

The whole point of setting HID_CLAIMED_DRIVER is to have hid->claimed
not equal to HID_CLAIMED_HIDRAW, in case we need the hid core
processing.

Cheers,
Benjamin


[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hid/hid-core.c#n2015

>
> Thanks,
> Srinivas
>
> > >
> > > > ---
> > > >  drivers/hid/hid-sensor-hub.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-
> > > > sensor-hub.c
> > > > index 2eba152e8b90..26e93a331a51 100644
> > > > --- a/drivers/hid/hid-sensor-hub.c
> > > > +++ b/drivers/hid/hid-sensor-hub.c
> > > > @@ -632,7 +632,7 @@ static int sensor_hub_probe(struct hid_device
> > > > *hdev,
> > > >       }
> > > >       INIT_LIST_HEAD(&hdev->inputs);
> > > >
> > > > -     ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> > > > +     ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT |
> > > > HID_CONNECT_DRIVER);
> > > >       if (ret) {
> > > >               hid_err(hdev, "hw start failed\n");
> > > >               return ret;
> > >
> >
> >
>
srinivas pandruvada Dec. 22, 2023, 2:24 p.m. UTC | #5 
On Fri, 2023-12-22 at 14:28 +0100, Benjamin Tissoires wrote:
> On Fri, Dec 22, 2023 at 1:44 PM srinivas pandruvada
> <srinivas.pandruvada@linux.intel.com> wrote:
> > 
> > On Wed, 2023-12-20 at 17:04 +0200, Yauhen Kharuzhy wrote:
> > > ср, 20 дек. 2023 г. в 16:52, Jonathan Cameron <jic23@kernel.org>:
> > > > 
> > > > On Wed, 20 Dec 2023 01:15:03 +0200
> > > > Yauhen Kharuzhy <jekhor@gmail.com> wrote:
> > > > 
> > > > > After the commit 666cf30a589a ("HID: sensor-hub: Allow multi-
> > > > > function
> > > > > sensor devices") hub devices are claimed by hidraw driver in
> > > > > hid_connect().
> > > > > This causes stoppping of processing HID reports by hid core
> > > > > due
> > > > > to
> > > > > optimization.
> > > > > 
> > > > > In such case, the hid-sensor-custom driver cannot match a
> > > > > known
> > > > > custom
> > > > > sensor in hid_sensor_custom_get_known() because it try to
> > > > > check
> > > > > custom
> > > > > properties which weren't filled from the report because hid
> > > > > core
> > > > > didn't
> > > > > parsed it.
> > > > > 
> > > > > As result, custom sensors like hinge angle sensor and LISS
> > > > > sensors
> > > > > don't work.
> > > > > 
> > > > > Mark the sensor hub devices claimed by some driver to avoid
> > > > > hidraw-related
> > > > > optimizations.
> > > > > 
> > > > > Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
> > > > Fixes tag?
> > > 
> > > Fixes: 666cf30a589a ("HID: sensor-hub: Allow multi-function
> > > sensor
> > > devices")
> > > 
> > This flag causes
> >                 hdev->claimed |= HID_CLAIMED_DRIVER;
> > I don't see the flag is used anywhere after this assignment in hid
> > core. Only two other drivers are setting this flag. We need Jiri's
> > help
> > here why this is a special case.
> 
> It's used in hid_report_raw_event()[0]:
> ```
>     if (hid->claimed != HID_CLAIMED_HIDRAW && report->maxfield) {
>         hid_process_report(hid, report, cdata, interrupt);
>         hdrv = hid->driver;
>         if (hdrv && hdrv->report)
>             hdrv->report(hid, report);
>     }
> ```
> 
> The whole point of setting HID_CLAIMED_DRIVER is to have hid->claimed
> not equal to HID_CLAIMED_HIDRAW, in case we need the hid core
> processing.
Thanks Benjamin for explaining.
Then this change looks fine as sensor hub driver will claim this device
and it needs hid core to process report.

Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

Thanks,
Srinivas

> 
> Cheers,
> Benjamin
> 
> 
> [0]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hid/hid-core.c#n2015
> 
> > 
> > Thanks,
> > Srinivas
> > 
> > > > 
> > > > > ---
> > > > >  drivers/hid/hid-sensor-hub.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-
> > > > > sensor-hub.c
> > > > > index 2eba152e8b90..26e93a331a51 100644
> > > > > --- a/drivers/hid/hid-sensor-hub.c
> > > > > +++ b/drivers/hid/hid-sensor-hub.c
> > > > > @@ -632,7 +632,7 @@ static int sensor_hub_probe(struct
> > > > > hid_device
> > > > > *hdev,
> > > > >       }
> > > > >       INIT_LIST_HEAD(&hdev->inputs);
> > > > > 
> > > > > -     ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> > > > > +     ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT |
> > > > > HID_CONNECT_DRIVER);
> > > > >       if (ret) {
> > > > >               hid_err(hdev, "hw start failed\n");
> > > > >               return ret;
> > > > 
> > > 
> > > 
> > 
>
Daniel Thompson Dec. 22, 2023, 5:16 p.m. UTC | #6 
On Wed, Dec 20, 2023 at 01:15:03AM +0200, Yauhen Kharuzhy wrote:
> After the commit 666cf30a589a ("HID: sensor-hub: Allow multi-function
> sensor devices") hub devices are claimed by hidraw driver in hid_connect().
> This causes stoppping of processing HID reports by hid core due to
> optimization.
>
> In such case, the hid-sensor-custom driver cannot match a known custom
> sensor in hid_sensor_custom_get_known() because it try to check custom
> properties which weren't filled from the report because hid core didn't
> parsed it.
>
> As result, custom sensors like hinge angle sensor and LISS sensors
> don't work.
>
> Mark the sensor hub devices claimed by some driver to avoid hidraw-related
> optimizations.
>
> Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>

I dusted off the Yoga C630 that provoked me into posting
666cf30a589a ("HID: sensor-hub: Allow multi-function sensor devices")
in the first place. Keyboard is still going strong so isn't not
comprehensive but for whatever it is worth this is:
Tested-by: Daniel Thompson <daniel.thompson@linaro.org>


Daniel.
Benjamin Tissoires Dec. 22, 2023, 6:27 p.m. UTC | #7 
On Wed, 20 Dec 2023 01:15:03 +0200, Yauhen Kharuzhy wrote:
> After the commit 666cf30a589a ("HID: sensor-hub: Allow multi-function
> sensor devices") hub devices are claimed by hidraw driver in hid_connect().
> This causes stoppping of processing HID reports by hid core due to
> optimization.
> 
> In such case, the hid-sensor-custom driver cannot match a known custom
> sensor in hid_sensor_custom_get_known() because it try to check custom
> properties which weren't filled from the report because hid core didn't
> parsed it.
> 
> [...]

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git (for-6.8/sensor-hub), thanks!

[1/1] HID: sensor-hub: Enable hid core report processing for all devices
      https://git.kernel.org/hid/hid/c/8e2f79f41a5d

Cheers,
diff mbox series

Patch

diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
index 2eba152e8b90..26e93a331a51 100644
--- a/drivers/hid/hid-sensor-hub.c
+++ b/drivers/hid/hid-sensor-hub.c
@@ -632,7 +632,7 @@  static int sensor_hub_probe(struct hid_device *hdev,
 	}
 	INIT_LIST_HEAD(&hdev->inputs);
 
-	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT | HID_CONNECT_DRIVER);
 	if (ret) {
 		hid_err(hdev, "hw start failed\n");
 		return ret;