Re: [PATCH v5 01/13] notifier: add device-managed registration APIs

["Uwe Kleine-König" <u.kleine-koenig@baylibre.com>]

[-- Attachment #1: Type: text/plain, Size: 4917 bytes --]

Hello Eliav,

On Sun, Aug 16, 2026 at 06:19:06AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Medium] The patch omits explicitly callable devm_*_unregister() functions, preventing safe early unregistration in drivers.
> - [Medium] devm_atomic_notifier_chain_register() is implicitly restricted to process context but its documentation does not state this.
> - [Low] Device-managed registration for SRCU notifiers is omitted without justification.
> --
> 
> --- Patch [1]: [PATCH v5 01/13] notifier: add device-managed registration APIs ---
> commit 123931abf8d80f91fce35443259e0df9e2736a4e
> Author: Eliav Farber <farbere@amazon.com>
> 
> notifier: add device-managed registration APIs
> 
> This patch adds devm_atomic_notifier_chain_register() and
> devm_blocking_notifier_chain_register() that automatically unregister
> the notifier when the device is unbound. This reduces boilerplate in
> drivers by eliminating per-driver unregister callbacks.
> 
> > diff --git a/include/linux/notifier.h b/include/linux/notifier.h
> > index 01b6c9d9956f9..4eeae9741a6e8 100644
> > --- a/include/linux/notifier.h
> > +++ b/include/linux/notifier.h
> 
> [ ... ]
> 
> > @@ -145,8 +146,14 @@ extern void srcu_init_notifier_head(struct srcu_notifier_head *nh);
> >  
> >  extern int atomic_notifier_chain_register(struct atomic_notifier_head *nh,
> >  		struct notifier_block *nb);
> > +int devm_atomic_notifier_chain_register(struct device *dev,
> > +		struct atomic_notifier_head *nh,
> > +		struct notifier_block *nb);
> 
> [Severity: Medium]
> Should there be corresponding devm_atomic_notifier_chain_unregister() and
> devm_blocking_notifier_chain_unregister() functions provided?

I think these can be added once they are needed.
 
> If a driver needs to unregister early to quiesce hardware, manually calling
> the non-devm unregister function leaves a stale devres action. This performs
> a useless double-unregister during devres_release_all(), which
> unconditionally hits the slow synchronize_rcu() and delays unbind.
> 
> >  extern int blocking_notifier_chain_register(struct blocking_notifier_head *nh,
> >  		struct notifier_block *nb);
> > +int devm_blocking_notifier_chain_register(struct device *dev,
> > +		struct blocking_notifier_head *nh,
> > +		struct notifier_block *nb);
> >  extern int raw_notifier_chain_register(struct raw_notifier_head *nh,
> >  		struct notifier_block *nb);
> >  extern int srcu_notifier_chain_register(struct srcu_notifier_head *nh,
> 
> [Severity: Low]
> Is there a reason the devm variant for SRCU notifiers was omitted?
> 
> The commit message justifies omitting raw notifiers due to locking
> constraints, but SRCU notifiers manage their own internal mutex which should
> be safe to lock in the devres teardown context.

🤷, I don't care much.

> 
> > diff --git a/kernel/notifier.c b/kernel/notifier.c
> > index 2f9fe7c30287f..0c39627ea69c5 100644
> > --- a/kernel/notifier.c
> > +++ b/kernel/notifier.c
> 
> [ ... ]
> 
> > @@ -197,6 +198,56 @@ int atomic_notifier_chain_unregister(struct atomic_notifier_head *nh,
> >  }
> >  EXPORT_SYMBOL_GPL(atomic_notifier_chain_unregister);
> >  
> > +struct atomic_notifier_chain_devres {
> > +	struct atomic_notifier_head *nh;
> > +	struct notifier_block *nb;
> > +};
> > +
> > +static void devm_atomic_notifier_chain_unregister(struct device *dev, void *res)
> > +{
> > +	struct atomic_notifier_chain_devres *dr = res;
> > +
> > +	atomic_notifier_chain_unregister(dr->nh, dr->nb);
> > +}
> > +
> > +/**
> > + *	devm_atomic_notifier_chain_register - Device-managed atomic notifier registration
> > + *	@dev: Device to tie the notifier lifetime to
> > + *	@nh: Pointer to head of the atomic notifier chain
> > + *	@nb: New entry in notifier chain
> > + *
> > + *	Adds a notifier to an atomic notifier chain and registers a cleanup
> > + *	action to automatically unregister it when @dev is unbound.
> > + *
> > + *	Return:
> > + *	0 on success, negative errno on error.
> > + */
> 
> [Severity: Medium]
> Should this kerneldoc include a context warning like "Must be called in
> process context."?
> 
> Unlike atomic_notifier_chain_register() which is safe in atomic context,
> this function uses devres_alloc() with GFP_KERNEL which can sleep, making it
> unsafe to call from atomic context.

That sounds like a good suggestion.

You didn't specify a merge plan for this series. Given there is no
explicit maintainer for kernel/notifier.c, I can apply patch #1 (with
adding the comment suggested by Sashiko) and provide an immutable branch
for subsystem maintainers to pull into their tree as base to apply their
patches. I'd wait till say Wednesday with that to maybe let reviews
tickle in.

Sounds good?

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]