Re: [PATCH 07/13] greybus: Discard pm_runtime_put_autosuspend() return value

[Brian Norris <briannorris@chromium.org>]

Hi Alex,

On Wed, Oct 07, 2026 at 09:28:45AM -0500, Alex Elder wrote:
> On 10/6/26 6:13 PM, Brian Norris wrote:
> > Returning an error code from pm_runtime_put_autosuspend() merely means
> > that it has not queued up a timer or work item to check whether or not
> > the device can be suspended and there are many perfectly valid
> > situations in which that can happen, like after writing "on" to the
> > device's runtime PM "control" attribute in sysfs for one example.
> 
> If I understand this right, "put autosuspend" is basically an
> unconditional "put" with the assumption that auto-suspend will
> ensure the eventual suspend occurs sometime after the reference
> count reaches zero.

Yes. Similar pattern for any other pm_runtime_put... variant -- they
always decrement the reference counter, even if they otherwise return
non-zero/error.

It definitely can be confusing, and it's one reason for $subject series
-- to remove confusing API surface when it doesn't help. (Similar story
for pm_runtime_get_sync() -- it increments the reference count, even on
error. That's why pm_runtime_resume_and_get() was invented.)

> > Modify gb_pm_runtime_put_autosuspend() to discard the
> > pm_runtime_put_autosuspend() return value, change its return type to
> > void, and update its caller in the Greybus power supply driver
> > accordingly.  Also drop the redundant pm_runtime_mark_last_busy() call
> > from gb_pm_runtime_put_autosuspend() while we're here, as this is
> > already part of pm_runtime_put_autosuspend().
> 
> I support including this fix in this patch.
> 
> > Note that this resolves a bug in the power_supply driver: in tracking
> > 'gbpsy->pm_acquired', it erroneously assumed that
> > pm_runtime_put_autosuspend() would not release a refcount when it
> > returned a non-zero value. That's a false assumption.
> 
> So is it safe for this code to assume the reference count has
> been decremented in this case?

Correct.

> The comments say we're trying
> to ensure there's exactly one get/put pair.  (I don't know the
> reasoning behind that though.)

I didn't look too far into this, but on first glance, it looks like
gb_power_supply_state_change() can receieve a number of different
properties, several of which mean "off". We only want the first "off" to
drop a reference count.

That seems like a fairly typical sort of pattern, and the right answer
is to ignore the return code, because the reference count was
decremented unconditionally.

> > This will facilitate a planned change of the
> > pm_runtime_put_autosuspend() return type to void in the future, similar
> > to commit 3afd8df02433 ("PM: runtime: Change pm_runtime_put() return
> > type to void").
> > 
> > Signed-off-by: Brian Norris <briannorris@chromium.org>
> 
> Despite my questions/comments I think this looks good.
> 
> Reviewed-by: Alex Elder <elder@riscstar.com>

Thanks,
Brian

> > ---
> > This patch is independent of the rest of the series, except for the end
> > (changing the return type). I expect it can be applied by individual
> > maintainers, and we pick up the end once the dust is settled.
> > 
> >   drivers/staging/greybus/power_supply.c |  8 ++------
> >   include/linux/greybus/bundle.h         | 12 +++---------
> >   2 files changed, 5 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/staging/greybus/power_supply.c b/drivers/staging/greybus/power_supply.c
> > index 44bd8a72fa50..beefcbaf3681 100644
> > --- a/drivers/staging/greybus/power_supply.c
> > +++ b/drivers/staging/greybus/power_supply.c
> > @@ -377,12 +377,8 @@ static void gb_power_supply_state_change(struct gb_power_supply *gbpsy,
> >   			gbpsy->pm_acquired = true;
> >   	} else {
> >   		if (gbpsy->pm_acquired) {
> > -			ret = gb_pm_runtime_put_autosuspend(connection->bundle);
> > -			if (ret)
> > -				dev_err(&connection->bundle->dev,
> > -					"Fail to set wake unlock for none charging\n");
> > -			else
> > -				gbpsy->pm_acquired = false;
> > +			gb_pm_runtime_put_autosuspend(connection->bundle);
> > +			gbpsy->pm_acquired = false;
> >   		}
> >   	}
> > diff --git a/include/linux/greybus/bundle.h b/include/linux/greybus/bundle.h
> > index df8d88424cb7..361a94d3499b 100644
> > --- a/include/linux/greybus/bundle.h
> > +++ b/include/linux/greybus/bundle.h
> > @@ -59,14 +59,9 @@ static inline int gb_pm_runtime_get_sync(struct gb_bundle *bundle)
> >   	return 0;
> >   }
> > -static inline int gb_pm_runtime_put_autosuspend(struct gb_bundle *bundle)
> > +static inline void gb_pm_runtime_put_autosuspend(struct gb_bundle *bundle)
> >   {
> > -	int retval;
> > -
> > -	pm_runtime_mark_last_busy(&bundle->dev);
> > -	retval = pm_runtime_put_autosuspend(&bundle->dev);
> > -
> > -	return retval;
> > +	pm_runtime_put_autosuspend(&bundle->dev);
> >   }
> >   static inline void gb_pm_runtime_get_noresume(struct gb_bundle *bundle)
> > @@ -82,8 +77,7 @@ static inline void gb_pm_runtime_put_noidle(struct gb_bundle *bundle)
> >   #else
> >   static inline int gb_pm_runtime_get_sync(struct gb_bundle *bundle)
> >   { return 0; }
> > -static inline int gb_pm_runtime_put_autosuspend(struct gb_bundle *bundle)
> > -{ return 0; }
> > +static inline void gb_pm_runtime_put_autosuspend(struct gb_bundle *bundle) {}
> >   static inline void gb_pm_runtime_get_noresume(struct gb_bundle *bundle) {}
> >   static inline void gb_pm_runtime_put_noidle(struct gb_bundle *bundle) {}
>