Re: [PATCH 1/2] devres: add devm_remove_action_nowarn()

[Simona Vetter <simona.vetter@ffwll.ch>]

On Fri, Jan 03, 2025 at 05:44:30PM +0100, Danilo Krummrich wrote:
> devm_remove_action() warns if the action to remove does not exist
> (anymore).
> 
> The Rust devres abstraction, however, has a use-case to call
> devm_remove_action() at a point where it can't be guaranteed that the
> corresponding action hasn't been released yet.
> 
> In particular, an instance of `Devres<T>` may be dropped after the
> action has been released. So far, `Devres<T>` worked around this by
> keeping the inner type alive.
> 
> Hence, add devm_remove_action_nowarn(), which returns an error code if
> the action has been removed already.
> 
> A subsequent patch uses devm_remove_action_nowarn() to remove the action
> when `Devres<T>` is dropped.
> 
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  drivers/base/devres.c  | 17 ++++++++++++-----
>  include/linux/device.h | 18 +++++++++++++++++-
>  2 files changed, 29 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> index 2152eec0c135..d59b8078fc33 100644
> --- a/drivers/base/devres.c
> +++ b/drivers/base/devres.c
> @@ -750,25 +750,32 @@ int __devm_add_action(struct device *dev, void (*action)(void *), void *data, co
>  EXPORT_SYMBOL_GPL(__devm_add_action);
>  
>  /**
> - * devm_remove_action() - removes previously added custom action
> + * devm_remove_action_nowarn() - removes previously added custom action
>   * @dev: Device that owns the action
>   * @action: Function implementing the action
>   * @data: Pointer to data passed to @action implementation
>   *
>   * Removes instance of @action previously added by devm_add_action().
>   * Both action and data should match one of the existing entries.
> + *
> + * In contrast to devm_remove_action(), this function does not WARN() if no
> + * entry could have been found.

I'd put a caution here that most likely, using this is a bad idea. Maybe
something like:

"This should only be used if the action is contained in an object with
independent lifetime management, like the Devres rust abstraction.
Anywhere is the warning most likely indicates a driver bug."

At least I really can't come up with a reasonable design in a C driver
that would ever need this.

Cheers, Sima

> + *
> + * Returns: 0 on success, -ENOENT if no entry could have been found.
>   */
> -void devm_remove_action(struct device *dev, void (*action)(void *), void *data)
> +int devm_remove_action_nowarn(struct device *dev,
> +			      void (*action)(void *),
> +			      void *data)
>  {
>  	struct action_devres devres = {
>  		.data = data,
>  		.action = action,
>  	};
>  
> -	WARN_ON(devres_destroy(dev, devm_action_release, devm_action_match,
> -			       &devres));
> +	return devres_destroy(dev, devm_action_release, devm_action_match,
> +			      &devres);
>  }
> -EXPORT_SYMBOL_GPL(devm_remove_action);
> +EXPORT_SYMBOL_GPL(devm_remove_action_nowarn);
>  
>  /**
>   * devm_release_action() - release previously added custom action
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 667cb6db9019..6879d5e8ac3d 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -399,7 +399,23 @@ void __iomem *devm_of_iomap(struct device *dev,
>  #endif
>  
>  /* allows to add/remove a custom action to devres stack */
> -void devm_remove_action(struct device *dev, void (*action)(void *), void *data);
> +int devm_remove_action_nowarn(struct device *dev, void (*action)(void *), void *data);
> +
> +/**
> + * devm_remove_action() - removes previously added custom action
> + * @dev: Device that owns the action
> + * @action: Function implementing the action
> + * @data: Pointer to data passed to @action implementation
> + *
> + * Removes instance of @action previously added by devm_add_action().
> + * Both action and data should match one of the existing entries.
> + */
> +static inline
> +void devm_remove_action(struct device *dev, void (*action)(void *), void *data)
> +{
> +	WARN_ON(devm_remove_action_nowarn(dev, action, data));
> +}
> +
>  void devm_release_action(struct device *dev, void (*action)(void *), void *data);
>  
>  int __devm_add_action(struct device *dev, void (*action)(void *), void *data, const char *name);
> 
> base-commit: 06e843bbbf2107463249ea6f6b1a736f5647e24a
> -- 
> 2.47.1
> 

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch