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

[Danilo Krummrich <dakr@kernel.org>]

On Mon, Jan 06, 2025 at 12:47:52PM +0100, Simona Vetter wrote:
> 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."

Yes, I thought of something similar too, but wasn't quite sure if it's needed.
At least for me, if something has the postfix "nowarn", it already makes me
wonder if I should really use it.

I'll add a paragraph.

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

I tried, but couldn't either. The only thing I could think of was a revocable
thing in C.

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