Re: [PATCH 08/28] iio: hid: trigger: Balance runtime pm + use pm_runtime_resume_and_get()

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Sun, 16 May 2021 19:19:40 -0700
Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> wrote:

> On Sun, 2021-05-16 at 16:19 +0100, Jonathan Cameron wrote:
> > On Wed, 12 May 2021 15:44:30 +0200
> > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> >   
> > > Em Sun,  9 May 2021 12:33:34 +0100
> > > Jonathan Cameron <jic23@kernel.org> escreveu:
> > >   
> > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > 
> > > > The call to pm_runtime_put_noidle() in remove() callback is not
> > > > balanced by any gets
> > > > 
> > > > Note this doesn't cause any problems beyond reader confusion as
> > > > the runtime
> > > > pm core protects against the reference counter going negative.
> > > > 
> > > > Whilst here, use pm_runtiem_resume_and_get() to simplify code a
> > > > little.
> > > > 
> > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>    
> > > 
> > > RPM get/put logic LGTM.
> > > 
> > > Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>  
> > 
> > After our side discussion on this one I wanted to take another look.
> > I'm not sure I follow the logic behind the runtime_enable() path
> > and why we don't want to enable runtime_pm until first use.  However
> > it's not directly related to this patch and as far as I can tell
> > it's just unsuual rather than broken.  As such, applied this one to
> > the
> > togreg branch of iio.git and pushed out as testing.
> >   
> 
> This was enabled in init path but caused issue with USB sensor hub in
> Thinkpasd Yoga S1, where we can never power off sensor and it wakes up
> device immediately after suspend.
> 
> commit
> ad7532cefd11d11a0814a75fb814c205ee3d9d4c

Ah.  Thanks for the history.  Thanks makes a horrible kind of sense ;)

Jonathan

> 
> 
> Thanks,
> Srinivas
> 
> > Thanks,
> > 
> > Jonathan
> >   
> > >   
> > > > ---
> > > >  drivers/iio/common/hid-sensors/hid-sensor-trigger.c | 8 ++------
> > > >  1 file changed, 2 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> > > > b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> > > > index 5a7b3e253e58..c06537e106e9 100644
> > > > --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> > > > +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> > > > @@ -163,18 +163,15 @@ int hid_sensor_power_state(struct
> > > > hid_sensor_common *st, bool state)
> > > >  
> > > >         if (state) {
> > > >                 atomic_inc(&st->user_requested_state);
> > > > -               ret = pm_runtime_get_sync(&st->pdev->dev);
> > > > +               ret = pm_runtime_resume_and_get(&st->pdev->dev);
> > > >         } else {
> > > >                 atomic_dec(&st->user_requested_state);
> > > >                 pm_runtime_mark_last_busy(&st->pdev->dev);
> > > >                 pm_runtime_use_autosuspend(&st->pdev->dev);
> > > >                 ret = pm_runtime_put_autosuspend(&st->pdev->dev);
> > > >         }
> > > > -       if (ret < 0) {
> > > > -               if (state)
> > > > -                       pm_runtime_put_noidle(&st->pdev->dev);
> > > > +       if (ret < 0)
> > > >                 return ret;
> > > > -       }
> > > >  
> > > >         return 0;
> > > >  #else
> > > > @@ -222,7 +219,6 @@ void hid_sensor_remove_trigger(struct iio_dev
> > > > *indio_dev,
> > > >                 pm_runtime_disable(&attrb->pdev->dev);
> > > >  
> > > >         pm_runtime_set_suspended(&attrb->pdev->dev);
> > > > -       pm_runtime_put_noidle(&attrb->pdev->dev);
> > > >  
> > > >         cancel_work_sync(&attrb->work);
> > > >         iio_trigger_unregister(attrb->trigger);    
> > > 
> > > 
> > > 
> > > Thanks,
> > > Mauro  
> >   
> 
>