From mboxrd@z Thu Jan 1 00:00:00 1970 From: Srinivas Pandruvada Subject: Re: [PATCH] HID: hid-sensor-hub: do not process feature reports in raw_event Date: Wed, 10 Apr 2013 08:44:34 -0700 Message-ID: <516588E2.4030901@linux.intel.com> References: <1363886546-6517-1-git-send-email-daniel.leung@linux.intel.com> <51507136.4020308@linux.intel.com> <38514.10.7.197.74.1365440092.squirrel@linux.intel.com> <33544.10.7.197.74.1365551736.squirrel@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from mga09.intel.com ([134.134.136.24]:61551 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932747Ab3DJPlJ (ORCPT ); Wed, 10 Apr 2013 11:41:09 -0400 In-Reply-To: <33544.10.7.197.74.1365551736.squirrel@linux.intel.com> Sender: linux-input-owner@vger.kernel.org List-Id: linux-input@vger.kernel.org To: Daniel Leung Cc: Jiri Kosina , srinivas.pandruvada@intel.com, linux-input@vger.kernel.org, linux-kernel@vger.kernel.org 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] [] >> warn_slowpath_common+0x7f/0xc0 >> <7>[ 150.944163] [] warn_slowpath_null+0x1a/0x20 >> <7>[ 150.944173] [] sensor_hub_raw_event+0x2d0/0x3b0 >> [hid_sensor_hub] >> <7>[ 150.944184] [] ? >> _raw_spin_unlock_irqrestore+0x57/0x70 >> <7>[ 150.944195] [] hid_input_report+0x283/0x2d0 >> <7>[ 150.944203] [] hid_ctrl+0x9f/0x180 >> <7>[ 150.944212] [] usb_hcd_giveback_urb+0x77/0x110 >> <7>[ 150.944222] [] xhci_irq+0x60f/0x1530 >> <7>[ 150.944230] [] ? handle_edge_irq+0x1e/0x130 >> <7>[ 150.944239] [] ? >> __lock_acquire.isra.31+0x28f/0xa70 >> <7>[ 150.944247] [] xhci_msi_irq+0x11/0x20 >> <7>[ 150.944258] [] handle_irq_event_percpu+0x55/0x210 >> <7>[ 150.944267] [] handle_irq_event+0x48/0x70 >> <7>[ 150.944273] [] handle_edge_irq+0x77/0x130 >> <7>[ 150.944283] [] handle_irq+0x60/0x150 >> <7>[ 150.944293] [] ? >> atomic_notifier_call_chain+0x16/0x20 >> <7>[ 150.944302] [] do_IRQ+0x5a/0xe0 >> <7>[ 150.944309] [] common_interrupt+0x6f/0x6f >> <7>[ 150.944313] [] ? >> cpuidle_wrap_enter+0x55/0xa0 >> <7>[ 150.944327] [] ? cpuidle_wrap_enter+0x51/0xa0 >> <7>[ 150.944334] [] cpuidle_enter_tk+0x10/0x20 >> <7>[ 150.944341] [] cpuidle_idle_call+0xaf/0x2b0 >> <7>[ 150.944350] [] cpu_idle+0xda/0x130 >> <7>[ 150.944360] [] 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 >