Re: [PATCH] iio: hid-sensor-trigger: Change get poll value function order to avoid sensor properties losing after resume from S3

[Jonathan Cameron <jic23@kernel.org>]

On 27/02/17 01:37, Pandruvada, Srinivas wrote:
> On Mon, 2017-02-27 at 01:00 +0000, Song, Hongyan wrote:
>> Hi Jonathan,
>> 	I add more information about the patch:
>> 	
>> Bug found: 
>> When we cat sensor incli sensor data with hysteresis value is 0,
>> there always have streamed sensor data output,
>> But when system enter S3 and resume back, the streaming thread is
>> still on but there is not streaming sensor data output.
>> Test data as below:
>> 279: 2017-01-24 01:16:00.279385 *5* incli_3d: Min Delta: 99.779ms,
>> Max Delta: 100.316ms, Received: 10/s, Overall: 170/16s; Sample values
>> ,0.010546,-0.006712,2.797579
>> 280: 2017-01-24 01:16:41.211385 *5* incli_3d: Min Delta: 100.16ms,
>> Max Delta: 40924.6ms, Received: 2/s, Overall: 172/19s; Sample values
>> ,0.010546,-0.006712,2.797579
>> 281: 2017-01-24 01:16:42.219892 *5* incli_3d: Min Delta: 0.023ms, Max
>> Delta: 543.28ms, Received: 2/s, Overall: 174/20s; Sample values
>> ,0.010546,-0.006712,2.797579
>> 282: 2017-01-24 01:16:42.755548 *4*
>> CIIOSensorInform::sensor_wait_event poll timeout
>> 283: 2017-01-24 01:16:43.223062 *5* incli_3d: Min Delta: --, Max
>> Delta: --, Received: 0/s, Overall: 174/21s
>> 284: 2017-01-24 01:16:43.756980 *4*
>> CIIOSensorInform::sensor_wait_event poll timeout
>> 285: 2017-01-24 01:16:44.225527 *5* incli_3d: Min Delta: --, Max
>> Delta: --, Received: 0/s, Overall: 174/22s
>> 286: 2017-01-24 01:16:44.758467 *4*
>> CIIOSensorInform::sensor_wait_event poll timeout
>> 287: 2017-01-24 01:16:45.228082 *5* incli_3d: Min Delta: --, Max
>> Delta: --, Received: 0/s, Overall: 174/23s
>> 288: 2017-01-24 01:16:45.759958 *4*
>> CIIOSensorInform::sensor_wait_event poll timeout
>>
>> Root cause: 
>> when sensor resume back from S3, the sensor property user set is
>> lost.
>> For example, if we set sensor hysteresis value to be 0, then let
>> system enter S3, when resume back, the hysteresis value will be not 0
>> but the default value 1.
>>
>> this is not what we want, resume back from S3 should not lose the
>> properties which is set before S3, just because the property setting
>> sequence lead to this lose.
>>
>> This is patch is a fix patch and needed for stable.
> But not urgent.
Thanks for the info.

Applied to the fixes-togreg branch of iio.git.

Thanks,

Jonathan
> 
> Thanks,
> Srinivas
> 
>>
>> Thanks a lot!
>>
>> BR
>> Song Hongyan
>>
>>
>> -----Original Message-----
>> From: Jonathan Cameron [mailto:jic23@kernel.org] 
>> Sent: Sunday, February 26, 2017 12:52 AM
>> To: Pandruvada, Srinivas <srinivas.pandruvada@intel.com>; linux-input
>> @vger.kernel.org; Song, Hongyan <hongyan.song@intel.com>; linux-iio@v
>> ger.kernel.org
>> Cc: jikos@kernel.org
>> Subject: Re: [PATCH] iio: hid-sensor-trigger: Change get poll value
>> function order to avoid sensor properties losing after resume from S3
>>
>> On 22/02/17 05:40, Pandruvada, Srinivas wrote:
>>> On Wed, 2017-02-22 at 17:17 +0800, Song Hongyan wrote:
>>>> In function _hid_sensor_power_state(), when
>>>> hid_sensor_read_poll_value()
>>>> is called, sensor's all properties will be updated by the value
>>>> from 
>>>> sensor hardware/firmware.
>>>> In some implementation, sensor hardware/firmware will do a power 
>>>> cycle during S3. In this case, after resume, once
>>>> hid_sensor_read_poll_value()
>>>> is called, sensor's all properties which are kept by driver
>>>> during S3 
>>>> will be changed to default value.
>>>> But instead, if a set feature function is called first, sensor 
>>>> hardware/firmware will be recovered to the last status. So change
>>>> the
>>>> sensor_hub_set_feature() calling order to behind of set feature 
>>>> function to avoid sensor properties lose.
>>>>
>>>> Signed-off-by: Song Hongyan <hongyan.song@intel.com>
>>>
>>> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>>
>> Hi Song,
>>
>> The patch message, whilst excellent on the technical detail, gives me
>> no sense of urgency on this. Is this a fix we want to have heading
>> for stable ASAP or are we looking at something seen in some
>> developmental hardware for example?
>> Links to bug reports, or examples of hardware suffering from the
>> issue are always helpful as well.
>>
>> Also for something like this a 'fixes' tag is very helpful when
>> people are looking to back port it.
>>
>> Anyhow, I'm going to hold off on taking this one until I have more
>> information. Right now it makes little difference as the next fixes
>> pull request from me will probably not be until next weekend.
>>
>> Thanks,
>>
>> Jonathan
>>>
>>>> ---
>>>>  drivers/iio/common/hid-sensors/hid-sensor-trigger.c | 6 +++---
>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
>>>> b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
>>>> index a3cce3a..ecf592d 100644
>>>> --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
>>>> +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
>>>> @@ -51,8 +51,6 @@ static int _hid_sensor_power_state(struct 
>>>> hid_sensor_common *st, bool state)
>>>>  			st->report_state.report_id,
>>>>  			st->report_state.index,
>>>>  			HID_USAGE_SENSOR_PROP_REPORTING_STATE_AL
>>>> L_EV
>>>> ENTS_ENUM);
>>>> -
>>>> -		poll_value = hid_sensor_read_poll_value(st);
>>>>  	} else {
>>>>  		int val;
>>>>  
>>>> @@ -89,7 +87,9 @@ static int _hid_sensor_power_state(struct 
>>>> hid_sensor_common *st, bool state)
>>>>  	sensor_hub_get_feature(st->hsdev, st-
>>>>> power_state.report_id,
>>>>  			       st->power_state.index,
>>>>  			       sizeof(state_val), &state_val);
>>>> -	if (state && poll_value)
>>>> +	if (state)
>>>> +		poll_value = hid_sensor_read_poll_value(st);
>>>> +	if (poll_value > 0)
>>>>  		msleep_interruptible(poll_value * 2);
>>>>  
>>>>  	return 
>>>> 0;N     r  y   b X  ǧv ^ )޺{.n +    {  *"  ^n r   z \x1a  h    &  \x1e
>>>> G   
>>>> h \x03( 階 ݢj"  \x1a ^[m     z ޖ   f   h   ~ mml==
>>
>> N�����r��y���b�X��ǧv�^�)޺{.n�+����{��*"��^n�r���z�\x1a��h����&��\x1e�G���h�\x03(�階�ݢj"��\x1a�^[m�����z�ޖ���f���h���~�mml==