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

[sashiko-bot@kernel.org]

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?

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.

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

> +int devm_atomic_notifier_chain_register(struct device *dev,
> +					struct atomic_notifier_head *nh,
> +					struct notifier_block *nb)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260816060648.4030-1-farbere@amazon.com?part=1