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

["Uwe Kleine-König" <ukleinek@kernel.org>]

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

Hello Andy,

On Mon, Aug 10, 2026 at 12:12:41PM +0300, Andy Shevchenko wrote:
> On Mon, Aug 10, 2026 at 12:11:40PM +0300, Andy Shevchenko wrote:
> > On Mon, Aug 10, 2026 at 06:37:25AM +0200, Uwe Kleine-König wrote:
> > > On Sun, Jul 26, 2026 at 10:17:27AM +0000, Eliav Farber wrote:
> 
> ...
> 
> > > > +{
> > > > +	struct atomic_notifier_chain_devres *dr;
> > > > +	int ret;
> > > > +
> > > > +	dr = devres_alloc(devm_atomic_notifier_chain_unregister,
> > > > +			  sizeof(*dr), GFP_KERNEL);
> > > > +	if (!dr)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	ret = atomic_notifier_chain_register(nh, n);
> > > > +	if (ret) {
> > > > +		devres_free(dr);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	dr->nh = nh;
> > > > +	dr->nb = n;
> > > > +	devres_add(dev, dr);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(devm_atomic_notifier_chain_register);
> > > 
> > > IMHO devm_atomic_notifier_chain_register() should look as follows:
> > > 
> > > 	ret = atomic_notifier_chain_register(nh, n);
> > > 	if (ret)
> > > 		return ret;
> > > 
> > > 	return devm_add_action_or_reset(dev, devm_atomic_notifier_chain_unregister, dr)
> > > 
> > > which is much easier and includes less details from the inner workings
> > > of devm. Same for the blocking variant.
> > 
> > Won't work. We need more than one parameter and hence the whole devres_alloc().

Oh indeed, justified concern.

> > Please, look closer to the code.
> 
> And just answering ahead, no, devm_kmalloc() is not good bandaid as it makes code
> not anyhow better to read and have a downside of memory fragmentation.

My idea would be to introduce a variant of devm_add_action_or_reset()
that copies the data (in a way to prevent the fragmentation), such that
the function could look as follows:

int devm_atomic_notifier_chain_register(struct device *dev,
                                       struct atomic_notifier_head *nh,
                                       struct notifier_block *n)
{
	struct atomic_notifier_chain_devres dr = {
		.nh = nh,
		/*
		 * Maybe rename n to nb (or vice-versa) to make the
		 * names match here?
		 */
		.nb = n,
	};
	int ret;
	
	ret = atomic_notifier_chain_register(nh, n);
	if (ret)
		return ret;
	
	return devm_copy_data_and_add_action_or_reset(dev,
						      devm_atomic_notifier_chain_unregister,
						      &dr, sizeof(dr));
}

The name is ugly and long, but IMHO the semantics are useful and the new
devm_atomic_notifier_chain_register() looks IMHO nicer that the original
from Eliav's patch.

But that would be a separate topic, so going with the currently
suggested approach is fine for me.

If you rename n to nb, you can have my ack.

Best regards
Uwe

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