Re: [PATCH v2 1/3] iio: light: gp2ap002: Fix unbalanced runtime PM on repeated event writes

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 25 Jul 2026 01:52:45 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Wed, 22 Jul 2026 21:52:45 +0530
> Nikhil Gautam <nikhilgtr@gmail.com> wrote:
> 
> > The IIO core does not filter duplicate writes to the event enable
> > attribute, so writing the same value twice invokes
> > write_event_config() twice. Enabling twice leaks a runtime PM
> > reference, preventing the device from ever suspending again;
> > disabling twice underflows the usage count and triggers a
> > "Runtime PM usage count underflow" warning.
> > 
> > Bail out early when the requested state matches the current state.
> > While at it, switch to pm_runtime_resume_and_get() so a failed
> > resume is propagated to userspace instead of silently marking the
> > event enabled.
> > 
> > Fixes: 97d642e23037c ("iio: light: Add a driver for Sharp GP2AP002x00F")
> > Signed-off-by: Nikhil Gautam <nikhilgtr@gmail.com>  
> 
> https://sashiko.dev/#/patchset/20260722162247.8229-1-nikhilgtr%40gmail.com
> 
> Sashiko raises a point about concurrent accessors. My previous understanding
> was that we were fine if there was only a single file involved.  Having
> dug around a bit I'm fairly sure that is correct.
> The comment here implying to me that we are fine.
> https://elixir.bootlin.com/linux/v7.1.4/source/fs/kernfs/file.c#L340
> 
> Now if we had more than one file then we couldn't rely on this.
> Hence to me it's a bit fragile and we should probably add a local lock
> anyway.
> 
> One other comment inline.
Huh. no idea what that comment was.  Never mind.
> 
> > ---
> >  drivers/iio/light/gp2ap002.c | 13 +++++++++----
> >  1 file changed, 9 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/iio/light/gp2ap002.c b/drivers/iio/light/gp2ap002.c
> > index 42859e5b1089..966459994995 100644
> > --- a/drivers/iio/light/gp2ap002.c
> > +++ b/drivers/iio/light/gp2ap002.c
> > @@ -343,6 +343,10 @@ static int gp2ap002_write_event_config(struct iio_dev *indio_dev,
> >  				       bool state)
> >  {
> >  	struct gp2ap002 *gp2ap002 = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	if (state == gp2ap002->enabled)
> > +		return 0;
> >  
> >  	if (state) {
> >  		/*
> > @@ -350,14 +354,15 @@ static int gp2ap002_write_event_config(struct iio_dev *indio_dev,
> >  		 * already) and reintialize the sensor by using runtime_pm
> >  		 * callbacks.
> >  		 */
> > -		pm_runtime_get_sync(gp2ap002->dev);
> > -		gp2ap002->enabled = true;  
> 
> > +
> > +		ret = pm_runtime_resume_and_get(gp2ap002->dev);
> > +		if (ret)
> > +			return ret;
> >  	} else {
> > -		pm_runtime_mark_last_busy(gp2ap002->dev);
> >  		pm_runtime_put_autosuspend(gp2ap002->dev);
> > -		gp2ap002->enabled = false;
> >  	}
> >  
> > +	gp2ap002->enabled = state;
> >  	return 0;
> >  }
> >    
> 
>