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

["Pandruvada, Srinivas" <srinivas.pandruvada-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

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,
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==
> 
>