linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] HID: hid-sensor-hub: do not process feature reports in raw_event
@ 2013-03-21 17:22 daniel.leung
  2013-03-25 15:45 ` Srinivas Pandruvada
  0 siblings, 1 reply; 8+ messages in thread
From: daniel.leung @ 2013-03-21 17:22 UTC (permalink / raw)
  To: jkosina, srinivas.pandruvada; +Cc: linux-input, linux-kernel, Daniel Leung

From: Daniel Leung <daniel.leung@linux.intel.com>

In sensor_hub_raw_event(), HID feature reports are ignored but are
still marked as processed. This causes the in-kernel struct not to be
updated. Any non-updated fields in the feature reports are zero, and
they are being sent to the device. This causes confusion in the
sensor hub firmware, and some sensors are not powered up as a result.

This changes the raw_event rountine to only process input reports,
and let the hid core handle the incoming feature reports.

The issue has been observed on Acer Iconia W700.

Signed-off-by: Daniel Leung <daniel.leung@linux.intel.com>
---
 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 ca749810..3f7df68 100644
--- a/drivers/hid/hid-sensor-hub.c
+++ b/drivers/hid/hid-sensor-hub.c
@@ -413,7 +413,7 @@ static int sensor_hub_raw_event(struct hid_device *hdev,
 			 report->id, size, report->type);
 	hid_dbg(hdev, "maxfield:%d\n", report->maxfield);
 	if (report->type != HID_INPUT_REPORT)
-		return 1;
+		return 0;
 
 	ptr = raw_data;
 	ptr++; /*Skip report id*/
-- 
1.8.1.5


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] HID: hid-sensor-hub: do not process feature reports in raw_event
  2013-03-21 17:22 [PATCH] HID: hid-sensor-hub: do not process feature reports in raw_event daniel.leung
@ 2013-03-25 15:45 ` Srinivas Pandruvada
  2013-03-27 15:09   ` Jiri Kosina
  0 siblings, 1 reply; 8+ messages in thread
From: Srinivas Pandruvada @ 2013-03-25 15:45 UTC (permalink / raw)
  To: daniel.leung; +Cc: jkosina, srinivas.pandruvada, linux-input, linux-kernel

Daniel,

I am looking at 3.9.rc1.
The only place I see the raw_event callback is called from 
hid/hid_input_report(). hid_input_report is called with type 
HID_INPUT_REPORT in all cases, except hid_ctrl(), where it can be 
different depending on xx.report->type. But here, the return value is 
not checked.

Do you know the call chain for HID_FETURE_REPORT, where this is creating 
problem?

Thanks,
Srinivas

On 03/21/2013 10:22 AM, daniel.leung@linux.intel.com wrote:
> From: Daniel Leung <daniel.leung@linux.intel.com>
>
> In sensor_hub_raw_event(), HID feature reports are ignored but are
> still marked as processed. This causes the in-kernel struct not to be
> updated. Any non-updated fields in the feature reports are zero, and
> they are being sent to the device. This causes confusion in the
> sensor hub firmware, and some sensors are not powered up as a result.
>
> This changes the raw_event rountine to only process input reports,
> and let the hid core handle the incoming feature reports.
>
> The issue has been observed on Acer Iconia W700.
>
> Signed-off-by: Daniel Leung <daniel.leung@linux.intel.com>
> ---
>   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 ca749810..3f7df68 100644
> --- a/drivers/hid/hid-sensor-hub.c
> +++ b/drivers/hid/hid-sensor-hub.c
> @@ -413,7 +413,7 @@ static int sensor_hub_raw_event(struct hid_device *hdev,
>   			 report->id, size, report->type);
>   	hid_dbg(hdev, "maxfield:%d\n", report->maxfield);
>   	if (report->type != HID_INPUT_REPORT)
> -		return 1;
> +		return 0;
>   
>   	ptr = raw_data;
>   	ptr++; /*Skip report id*/


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] HID: hid-sensor-hub: do not process feature reports in raw_event
  2013-03-25 15:45 ` Srinivas Pandruvada
@ 2013-03-27 15:09   ` Jiri Kosina
  2013-04-04  7:48     ` Jiri Kosina
  2013-04-08 16:54     ` Daniel Leung
  0 siblings, 2 replies; 8+ messages in thread
From: Jiri Kosina @ 2013-03-27 15:09 UTC (permalink / raw)
  To: Srinivas Pandruvada
  Cc: daniel.leung, srinivas.pandruvada, linux-input, linux-kernel

On Mon, 25 Mar 2013, Srinivas Pandruvada wrote:

> Daniel,
> 
> I am looking at 3.9.rc1.
> The only place I see the raw_event callback is called from
> hid/hid_input_report(). hid_input_report is called with type HID_INPUT_REPORT
> in all cases, except hid_ctrl(), where it can be different depending on
> xx.report->type. But here, the return value is not checked.
> 
> Do you know the call chain for HID_FETURE_REPORT, where this is creating
> problem?

This was exactly what I was wondering about when I have seen the initial 
patch a few weeks ago.

Daniel, could you please elaborate? Is there a out-of-tree driver for Acer 
Iconia W700?

Thanks,

-- 
Jiri Kosina
SUSE Labs

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] HID: hid-sensor-hub: do not process feature reports in raw_event
  2013-03-27 15:09   ` Jiri Kosina
@ 2013-04-04  7:48     ` Jiri Kosina
  2013-04-08 16:54     ` Daniel Leung
  1 sibling, 0 replies; 8+ messages in thread
From: Jiri Kosina @ 2013-04-04  7:48 UTC (permalink / raw)
  To: Srinivas Pandruvada
  Cc: daniel.leung, srinivas.pandruvada, linux-input, linux-kernel

On Wed, 27 Mar 2013, Jiri Kosina wrote:

> > I am looking at 3.9.rc1.
> > The only place I see the raw_event callback is called from
> > hid/hid_input_report(). hid_input_report is called with type HID_INPUT_REPORT
> > in all cases, except hid_ctrl(), where it can be different depending on
> > xx.report->type. But here, the return value is not checked.
> > 
> > Do you know the call chain for HID_FETURE_REPORT, where this is creating
> > problem?
> 
> This was exactly what I was wondering about when I have seen the initial 
> patch a few weeks ago.
> 
> Daniel, could you please elaborate? Is there a out-of-tree driver for Acer 
> Iconia W700?

Daniel ... ?

-- 
Jiri Kosina
SUSE Labs

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] HID: hid-sensor-hub: do not process feature reports in  raw_event
  2013-03-27 15:09   ` Jiri Kosina
  2013-04-04  7:48     ` Jiri Kosina
@ 2013-04-08 16:54     ` Daniel Leung
  2013-04-09 23:55       ` Daniel Leung
  1 sibling, 1 reply; 8+ messages in thread
From: Daniel Leung @ 2013-04-08 16:54 UTC (permalink / raw)
  To: Jiri Kosina
  Cc: Srinivas Pandruvada, daniel.leung, srinivas.pandruvada,
	linux-input, linux-kernel

> On Mon, 25 Mar 2013, Srinivas Pandruvada wrote:
>
>> Daniel,
>>
>> I am looking at 3.9.rc1.
>> The only place I see the raw_event callback is called from
>> hid/hid_input_report(). hid_input_report is called with type
>> HID_INPUT_REPORT
>> in all cases, except hid_ctrl(), where it can be different depending on
>> xx.report->type. But here, the return value is not checked.
>>
>> Do you know the call chain for HID_FETURE_REPORT, where this is creating
>> problem?
>
> This was exactly what I was wondering about when I have seen the initial
> patch a few weeks ago.
>
> Daniel, could you please elaborate? Is there a out-of-tree driver for Acer
> Iconia W700?

Sorry for the late reply. I was out of town and was having difficulties
accessing the mail server.

For the call path, here is what I got (using WARN_ON()):

<4>[  150.943775] ------------[ cut here ]------------
<4>[  150.943861] WARNING: at
../../../../../../../../../../home/dleung/android/ia/private/jb-mr1-internal/kernel/intel/drivers/hid/hid-sensor-hub.c:418
sensor_hub_raw_event+0x2d0/0x3b0 [hid_sensor_hub]()
<4>[  150.944028] Hardware name: ICONIA W700
<7>[  150.944071] Modules linked in: hid_sensor_als hid_sensor_gyro_3d
hid_sensor_accel_3d uvcvideo videobuf2_vmalloc videobuf2_memops
videobuf2_core btusb bluetooth hid_multitouch ath9k ath9k_common ath9k_hw
ath snd_hda_codec_hdmi snd_hda_codec_realtek ip6table_raw snd_hda_intel
iptable_raw snd_hda_codec snd_hwdep snd_pcm snd_page_alloc snd_timer snd
soundcore hid_sensor_trigger hid_sensor_iio_common hid_sensor_hub
industrialio_triggered_buffer kfifo_buf industrialio coretemp
<7>[  150.944132] Pid: 0, comm: swapper/1 Not tainted
3.8.0-aia1-00043-gd7fd780-dirty #2
<7>[  150.944136] Call Trace:
<7>[  150.944141]  <IRQ>  [<ffffffff8105b5bf>] warn_slowpath_common+0x7f/0xc0
<7>[  150.944163]  [<ffffffff8105b61a>] warn_slowpath_null+0x1a/0x20
<7>[  150.944173]  [<ffffffffa00315e0>] sensor_hub_raw_event+0x2d0/0x3b0
[hid_sensor_hub]
<7>[  150.944184]  [<ffffffff819021e7>] ?
_raw_spin_unlock_irqrestore+0x57/0x70
<7>[  150.944195]  [<ffffffff816ccce3>] hid_input_report+0x283/0x2d0
<7>[  150.944203]  [<ffffffff816d9faf>] hid_ctrl+0x9f/0x180
<7>[  150.944212]  [<ffffffff815b2087>] usb_hcd_giveback_urb+0x77/0x110
<7>[  150.944222]  [<ffffffff8160073f>] xhci_irq+0x60f/0x1530
<7>[  150.944230]  [<ffffffff810df0fe>] ? handle_edge_irq+0x1e/0x130
<7>[  150.944239]  [<ffffffff810af04f>] ? __lock_acquire.isra.31+0x28f/0xa70
<7>[  150.944247]  [<ffffffff81601671>] xhci_msi_irq+0x11/0x20
<7>[  150.944258]  [<ffffffff810dc395>] handle_irq_event_percpu+0x55/0x210
<7>[  150.944267]  [<ffffffff810dc598>] handle_irq_event+0x48/0x70
<7>[  150.944273]  [<ffffffff810df157>] handle_edge_irq+0x77/0x130
<7>[  150.944283]  [<ffffffff81004180>] handle_irq+0x60/0x150
<7>[  150.944293]  [<ffffffff81906406>] ?
atomic_notifier_call_chain+0x16/0x20
<7>[  150.944302]  [<ffffffff8190c38a>] do_IRQ+0x5a/0xe0
<7>[  150.944309]  [<ffffffff8190266f>] common_interrupt+0x6f/0x6f
<7>[  150.944313]  <EOI>  [<ffffffff816a97f5>] ? cpuidle_wrap_enter+0x55/0xa0
<7>[  150.944327]  [<ffffffff816a97f1>] ? cpuidle_wrap_enter+0x51/0xa0
<7>[  150.944334]  [<ffffffff816a9850>] cpuidle_enter_tk+0x10/0x20
<7>[  150.944341]  [<ffffffff816a940f>] cpuidle_idle_call+0xaf/0x2b0
<7>[  150.944350]  [<ffffffff8100b3aa>] cpu_idle+0xda/0x130
<7>[  150.944360]  [<ffffffff818f081d>] start_secondary+0x266/0x268
<4>[  150.944365] ---[ end trace 32319d39ebee5616 ]---


You can see that it went through hid_ctrl().

>From what I can gather using usbmon, the sensor hub sends the feature
report correctly. However, the in-kernel struct is not updated when the
report comes in. Therefore, the report from sensor_hub_report() is always
zero-filled. When calling sensor_hub_set_feature(), those zeros are being
sent back to the device. The feature report contains fields for power
state and reporting state. If they are set to zero, the firmware does not
know what to do as zero means undefined state. Both power and reporting
states have to be set for sensor to report values. Given the zero-filled
report, and we can only deal with one field in sensor_hub_set_feature(),
setting power state to non-zero also sends zero (i.e. undefined state) to
the reporting state field, and vice versa.

The sensor hub on Acer Iconia W700 is using drivers/hid/hid-sensor-hub.c.
I also played with Sony Vaio Duo 11 a while back (before having this
patch), and was having trouble turning on gyroscope. I suspect it is
caused by the same issue. The sensor hub on Acer is from STMicro, while
the one in Vaio Duo is from Freescale.



Thanks,
Daniel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] HID: hid-sensor-hub: do not process feature reports in  raw_event
  2013-04-08 16:54     ` Daniel Leung
@ 2013-04-09 23:55       ` Daniel Leung
  2013-04-10 15:44         ` Srinivas Pandruvada
  0 siblings, 1 reply; 8+ messages in thread
From: Daniel Leung @ 2013-04-09 23:55 UTC (permalink / raw)
  To: Daniel Leung
  Cc: Jiri Kosina, Srinivas Pandruvada, daniel.leung,
	srinivas.pandruvada, linux-input, linux-kernel

>> On Mon, 25 Mar 2013, Srinivas Pandruvada wrote:
>>
>>> Daniel,
>>>
>>> I am looking at 3.9.rc1.
>>> The only place I see the raw_event callback is called from
>>> hid/hid_input_report(). hid_input_report is called with type
>>> HID_INPUT_REPORT
>>> in all cases, except hid_ctrl(), where it can be different depending on
>>> xx.report->type. But here, the return value is not checked.
>>>
>>> Do you know the call chain for HID_FETURE_REPORT, where this is
>>> creating
>>> problem?
>>
>> This was exactly what I was wondering about when I have seen the initial
>> patch a few weeks ago.
>>
>> Daniel, could you please elaborate? Is there a out-of-tree driver for
>> Acer
>> Iconia W700?
>
> Sorry for the late reply. I was out of town and was having difficulties
> accessing the mail server.
>
> For the call path, here is what I got (using WARN_ON()):
>
> <4>[  150.943775] ------------[ cut here ]------------
> <4>[  150.943861] WARNING: at
> ../../../../../../../../../../home/dleung/android/ia/private/jb-mr1-internal/kernel/intel/drivers/hid/hid-sensor-hub.c:418
> sensor_hub_raw_event+0x2d0/0x3b0 [hid_sensor_hub]()
> <4>[  150.944028] Hardware name: ICONIA W700
> <7>[  150.944071] Modules linked in: hid_sensor_als hid_sensor_gyro_3d
> hid_sensor_accel_3d uvcvideo videobuf2_vmalloc videobuf2_memops
> videobuf2_core btusb bluetooth hid_multitouch ath9k ath9k_common ath9k_hw
> ath snd_hda_codec_hdmi snd_hda_codec_realtek ip6table_raw snd_hda_intel
> iptable_raw snd_hda_codec snd_hwdep snd_pcm snd_page_alloc snd_timer snd
> soundcore hid_sensor_trigger hid_sensor_iio_common hid_sensor_hub
> industrialio_triggered_buffer kfifo_buf industrialio coretemp
> <7>[  150.944132] Pid: 0, comm: swapper/1 Not tainted
> 3.8.0-aia1-00043-gd7fd780-dirty #2
> <7>[  150.944136] Call Trace:
> <7>[  150.944141]  <IRQ>  [<ffffffff8105b5bf>]
> warn_slowpath_common+0x7f/0xc0
> <7>[  150.944163]  [<ffffffff8105b61a>] warn_slowpath_null+0x1a/0x20
> <7>[  150.944173]  [<ffffffffa00315e0>] sensor_hub_raw_event+0x2d0/0x3b0
> [hid_sensor_hub]
> <7>[  150.944184]  [<ffffffff819021e7>] ?
> _raw_spin_unlock_irqrestore+0x57/0x70
> <7>[  150.944195]  [<ffffffff816ccce3>] hid_input_report+0x283/0x2d0
> <7>[  150.944203]  [<ffffffff816d9faf>] hid_ctrl+0x9f/0x180
> <7>[  150.944212]  [<ffffffff815b2087>] usb_hcd_giveback_urb+0x77/0x110
> <7>[  150.944222]  [<ffffffff8160073f>] xhci_irq+0x60f/0x1530
> <7>[  150.944230]  [<ffffffff810df0fe>] ? handle_edge_irq+0x1e/0x130
> <7>[  150.944239]  [<ffffffff810af04f>] ?
> __lock_acquire.isra.31+0x28f/0xa70
> <7>[  150.944247]  [<ffffffff81601671>] xhci_msi_irq+0x11/0x20
> <7>[  150.944258]  [<ffffffff810dc395>] handle_irq_event_percpu+0x55/0x210
> <7>[  150.944267]  [<ffffffff810dc598>] handle_irq_event+0x48/0x70
> <7>[  150.944273]  [<ffffffff810df157>] handle_edge_irq+0x77/0x130
> <7>[  150.944283]  [<ffffffff81004180>] handle_irq+0x60/0x150
> <7>[  150.944293]  [<ffffffff81906406>] ?
> atomic_notifier_call_chain+0x16/0x20
> <7>[  150.944302]  [<ffffffff8190c38a>] do_IRQ+0x5a/0xe0
> <7>[  150.944309]  [<ffffffff8190266f>] common_interrupt+0x6f/0x6f
> <7>[  150.944313]  <EOI>  [<ffffffff816a97f5>] ?
> cpuidle_wrap_enter+0x55/0xa0
> <7>[  150.944327]  [<ffffffff816a97f1>] ? cpuidle_wrap_enter+0x51/0xa0
> <7>[  150.944334]  [<ffffffff816a9850>] cpuidle_enter_tk+0x10/0x20
> <7>[  150.944341]  [<ffffffff816a940f>] cpuidle_idle_call+0xaf/0x2b0
> <7>[  150.944350]  [<ffffffff8100b3aa>] cpu_idle+0xda/0x130
> <7>[  150.944360]  [<ffffffff818f081d>] start_secondary+0x266/0x268
> <4>[  150.944365] ---[ end trace 32319d39ebee5616 ]---
>
>
> You can see that it went through hid_ctrl().
>
> From what I can gather using usbmon, the sensor hub sends the feature
> report correctly. However, the in-kernel struct is not updated when the
> report comes in. Therefore, the report from sensor_hub_report() is always
> zero-filled. When calling sensor_hub_set_feature(), those zeros are being
> sent back to the device. The feature report contains fields for power
> state and reporting state. If they are set to zero, the firmware does not
> know what to do as zero means undefined state. Both power and reporting
> states have to be set for sensor to report values. Given the zero-filled
> report, and we can only deal with one field in sensor_hub_set_feature(),
> setting power state to non-zero also sends zero (i.e. undefined state) to
> the reporting state field, and vice versa.
>
> The sensor hub on Acer Iconia W700 is using drivers/hid/hid-sensor-hub.c.
> I also played with Sony Vaio Duo 11 a while back (before having this
> patch), and was having trouble turning on gyroscope. I suspect it is
> caused by the same issue. The sensor hub on Acer is from STMicro, while
> the one in Vaio Duo is from Freescale.

Just to clarify a little bit on the issue.

Although hid_ctrl() does not care the return value of hid_input_report(),
the issue is within hid_input_report(). In drivers/hid/hid-core.c:1317
where raw_event() is called, if return is non-zero, the following call of
hid_report_raw_event() is skipped. This call is needed to have the
in-kernel struct updated with the actual feature report from device.


Daniel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] HID: hid-sensor-hub: do not process feature reports in raw_event
  2013-04-09 23:55       ` Daniel Leung
@ 2013-04-10 15:44         ` Srinivas Pandruvada
  2013-05-29 14:19           ` Jiri Kosina
  0 siblings, 1 reply; 8+ messages in thread
From: Srinivas Pandruvada @ 2013-04-10 15:44 UTC (permalink / raw)
  To: Daniel Leung; +Cc: Jiri Kosina, srinivas.pandruvada, linux-input, linux-kernel

Thanks Daniel.

I don't know if this should be fixed in client drivers since other 
drivers may have this issue.

I interpret the return value of hdrv->raw_event as :
"<0"     : Error. The driver has parsed the message and found issue
"==0"  :    No error and driver has consumed report
">0" : Not an error and driver has not parsed or consumed the event

In this case hid-sensor-hub didn't parse message to return any error or 
to say consumed.

May be hid-core.c

        if (hdrv && hdrv->raw_event && hid_match_report(hid, report)) {
                 ret = hdrv->raw_event(hid, report, data, size);
                 if (ret < 0) {
                         goto unlock;
                 }
         }


I think Jiri can have better idea.

Thanks,
Srinivas


On 04/09/2013 04:55 PM, Daniel Leung wrote:
>>> On Mon, 25 Mar 2013, Srinivas Pandruvada wrote:
>>>
>>>> Daniel,
>>>>
>>>> I am looking at 3.9.rc1.
>>>> The only place I see the raw_event callback is called from
>>>> hid/hid_input_report(). hid_input_report is called with type
>>>> HID_INPUT_REPORT
>>>> in all cases, except hid_ctrl(), where it can be different depending on
>>>> xx.report->type. But here, the return value is not checked.
>>>>
>>>> Do you know the call chain for HID_FETURE_REPORT, where this is
>>>> creating
>>>> problem?
>>> This was exactly what I was wondering about when I have seen the initial
>>> patch a few weeks ago.
>>>
>>> Daniel, could you please elaborate? Is there a out-of-tree driver for
>>> Acer
>>> Iconia W700?
>> Sorry for the late reply. I was out of town and was having difficulties
>> accessing the mail server.
>>
>> For the call path, here is what I got (using WARN_ON()):
>>
>> <4>[  150.943775] ------------[ cut here ]------------
>> <4>[  150.943861] WARNING: at
>> ../../../../../../../../../../home/dleung/android/ia/private/jb-mr1-internal/kernel/intel/drivers/hid/hid-sensor-hub.c:418
>> sensor_hub_raw_event+0x2d0/0x3b0 [hid_sensor_hub]()
>> <4>[  150.944028] Hardware name: ICONIA W700
>> <7>[  150.944071] Modules linked in: hid_sensor_als hid_sensor_gyro_3d
>> hid_sensor_accel_3d uvcvideo videobuf2_vmalloc videobuf2_memops
>> videobuf2_core btusb bluetooth hid_multitouch ath9k ath9k_common ath9k_hw
>> ath snd_hda_codec_hdmi snd_hda_codec_realtek ip6table_raw snd_hda_intel
>> iptable_raw snd_hda_codec snd_hwdep snd_pcm snd_page_alloc snd_timer snd
>> soundcore hid_sensor_trigger hid_sensor_iio_common hid_sensor_hub
>> industrialio_triggered_buffer kfifo_buf industrialio coretemp
>> <7>[  150.944132] Pid: 0, comm: swapper/1 Not tainted
>> 3.8.0-aia1-00043-gd7fd780-dirty #2
>> <7>[  150.944136] Call Trace:
>> <7>[  150.944141]  <IRQ>  [<ffffffff8105b5bf>]
>> warn_slowpath_common+0x7f/0xc0
>> <7>[  150.944163]  [<ffffffff8105b61a>] warn_slowpath_null+0x1a/0x20
>> <7>[  150.944173]  [<ffffffffa00315e0>] sensor_hub_raw_event+0x2d0/0x3b0
>> [hid_sensor_hub]
>> <7>[  150.944184]  [<ffffffff819021e7>] ?
>> _raw_spin_unlock_irqrestore+0x57/0x70
>> <7>[  150.944195]  [<ffffffff816ccce3>] hid_input_report+0x283/0x2d0
>> <7>[  150.944203]  [<ffffffff816d9faf>] hid_ctrl+0x9f/0x180
>> <7>[  150.944212]  [<ffffffff815b2087>] usb_hcd_giveback_urb+0x77/0x110
>> <7>[  150.944222]  [<ffffffff8160073f>] xhci_irq+0x60f/0x1530
>> <7>[  150.944230]  [<ffffffff810df0fe>] ? handle_edge_irq+0x1e/0x130
>> <7>[  150.944239]  [<ffffffff810af04f>] ?
>> __lock_acquire.isra.31+0x28f/0xa70
>> <7>[  150.944247]  [<ffffffff81601671>] xhci_msi_irq+0x11/0x20
>> <7>[  150.944258]  [<ffffffff810dc395>] handle_irq_event_percpu+0x55/0x210
>> <7>[  150.944267]  [<ffffffff810dc598>] handle_irq_event+0x48/0x70
>> <7>[  150.944273]  [<ffffffff810df157>] handle_edge_irq+0x77/0x130
>> <7>[  150.944283]  [<ffffffff81004180>] handle_irq+0x60/0x150
>> <7>[  150.944293]  [<ffffffff81906406>] ?
>> atomic_notifier_call_chain+0x16/0x20
>> <7>[  150.944302]  [<ffffffff8190c38a>] do_IRQ+0x5a/0xe0
>> <7>[  150.944309]  [<ffffffff8190266f>] common_interrupt+0x6f/0x6f
>> <7>[  150.944313]  <EOI>  [<ffffffff816a97f5>] ?
>> cpuidle_wrap_enter+0x55/0xa0
>> <7>[  150.944327]  [<ffffffff816a97f1>] ? cpuidle_wrap_enter+0x51/0xa0
>> <7>[  150.944334]  [<ffffffff816a9850>] cpuidle_enter_tk+0x10/0x20
>> <7>[  150.944341]  [<ffffffff816a940f>] cpuidle_idle_call+0xaf/0x2b0
>> <7>[  150.944350]  [<ffffffff8100b3aa>] cpu_idle+0xda/0x130
>> <7>[  150.944360]  [<ffffffff818f081d>] start_secondary+0x266/0x268
>> <4>[  150.944365] ---[ end trace 32319d39ebee5616 ]---
>>
>>
>> You can see that it went through hid_ctrl().
>>
>>  From what I can gather using usbmon, the sensor hub sends the feature
>> report correctly. However, the in-kernel struct is not updated when the
>> report comes in. Therefore, the report from sensor_hub_report() is always
>> zero-filled. When calling sensor_hub_set_feature(), those zeros are being
>> sent back to the device. The feature report contains fields for power
>> state and reporting state. If they are set to zero, the firmware does not
>> know what to do as zero means undefined state. Both power and reporting
>> states have to be set for sensor to report values. Given the zero-filled
>> report, and we can only deal with one field in sensor_hub_set_feature(),
>> setting power state to non-zero also sends zero (i.e. undefined state) to
>> the reporting state field, and vice versa.
>>
>> The sensor hub on Acer Iconia W700 is using drivers/hid/hid-sensor-hub.c.
>> I also played with Sony Vaio Duo 11 a while back (before having this
>> patch), and was having trouble turning on gyroscope. I suspect it is
>> caused by the same issue. The sensor hub on Acer is from STMicro, while
>> the one in Vaio Duo is from Freescale.
> Just to clarify a little bit on the issue.
>
> Although hid_ctrl() does not care the return value of hid_input_report(),
> the issue is within hid_input_report(). In drivers/hid/hid-core.c:1317
> where raw_event() is called, if return is non-zero, the following call of
> hid_report_raw_event() is skipped. This call is needed to have the
> in-kernel struct updated with the actual feature report from device.
>
>
> Daniel
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] HID: hid-sensor-hub: do not process feature reports in raw_event
  2013-04-10 15:44         ` Srinivas Pandruvada
@ 2013-05-29 14:19           ` Jiri Kosina
  0 siblings, 0 replies; 8+ messages in thread
From: Jiri Kosina @ 2013-05-29 14:19 UTC (permalink / raw)
  To: Srinivas Pandruvada
  Cc: Daniel Leung, srinivas.pandruvada, linux-input, linux-kernel

On Wed, 10 Apr 2013, Srinivas Pandruvada wrote:

> I don't know if this should be fixed in client drivers since other drivers may
> have this issue.

Agreed.

Srinivas, how about the patch below, could you please test it in your 
scenario to see whether it actually fixes the issue?

Thanks.


diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 264f550..65879b9 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1293,7 +1293,7 @@ int hid_input_report(struct hid_device *hid, int type, u8 *data, int size, int i
 
 	if (hdrv && hdrv->raw_event && hid_match_report(hid, report)) {
 		ret = hdrv->raw_event(hid, report, data, size);
-		if (ret != 0) {
+		if (ret < 0) {
 			ret = ret < 0 ? ret : 0;
 			goto unlock;
 		}
-- 
Jiri Kosina
SUSE Labs

^ permalink raw reply related	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2013-05-29 14:19 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-03-21 17:22 [PATCH] HID: hid-sensor-hub: do not process feature reports in raw_event daniel.leung
2013-03-25 15:45 ` Srinivas Pandruvada
2013-03-27 15:09   ` Jiri Kosina
2013-04-04  7:48     ` Jiri Kosina
2013-04-08 16:54     ` Daniel Leung
2013-04-09 23:55       ` Daniel Leung
2013-04-10 15:44         ` Srinivas Pandruvada
2013-05-29 14:19           ` Jiri Kosina

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).