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

[Alex Elder]

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.

> 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?  The comments say we're trying
to ensure there's exactly one get/put pair.  (I don't know the
reasoning behind that though.)

> 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@xxxxxxxxxxxx>

Despite my questions/comments I think this looks good.

Reviewed-by: Alex Elder <elder@xxxxxxxxxxxx>
> ---
> 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) {}