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

[Danilo Krummrich <dakr@kernel.org>]

On Tue, Jan 07, 2025 at 11:11:20AM +0100, Alice Ryhl wrote:
> On Tue, Jan 7, 2025 at 11:05 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > 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.
> 
> Potentially if there are two cleanup paths that could run in parallel,
> they could use this to avoid needing to synchronize which one removes
> it?

Yeah, I also though if I can make up such a case. But I think the real issue is
that even if we can find one, it's probably an abuse of devres.

Devres is there to indicate that the driver was unbound from the device, which
causes remove() for the driver to cleanup. So, rather than removing the action
from some async path, we can just wait for remove() to clean up. The only
exception can hence be probe().

> 
> Alice