Re: [PATCH v7 03/23] firmware: arm_scmi: Introduce protocol instance notifiers

On Mon, Aug 03, 2026 at 11:52:05PM +0100, Fayssal Benmlih wrote:
> Hi Cristian,
> 

Hi,

> A couple of notifier lifetime issues inline.
> 
> > 	scoped_guard(mutex, &info->protocols_mtx) {
> > 		pi = idr_find(&info->protocols, protocol_id);
> > 		if (WARN_ON(!pi))
> > 			return;
> >
> > 		proto_notifier_nb = pi->pno.nb;
> > 		/* Ensure NULL is visible */
> > 		smp_store_mb(pi->pno.nb, NULL);
> > 	}
> >
> > 	if (proto_notifier_nb) {
> > 		int ret;
> >
> > 		ret = scmi_protocol_notifier_unregister(pi->handle,
> > 							&pi->pno);
> > 		if (ret)
> > 			dev_err(handle->dev,
> > 				"Failed to release protocol notifier\n");
> > 	}
> >
> > 	guard(mutex)(&info->protocols_mtx);
> > 	if (refcount_dec_and_test(&pi->users)) {
> 
> The notifier is cleared and unregistered before decrementing the protocol
> users refcount. If the protocol instance has multiple users, the first
> user that releases it removes the protocol implementation's notifier even
> though the instance remains active for the remaining users.
> 

> Should notifier removal happen only when the final protocol reference is
> released?
> 

Yes, but it is not so easy to do given the current notification handlers
design (that I did :P) since notifier were not supposed to be used from
within a protocol, till Telemetry...so the attempt is to fit (cleanly)
this new use-case into the existing SCMI Notification framework..since
99% of the related handling is the same....I have reviewed this logic
in V8, improved I think, but still Sashiko has some complaints...

> The ordering may need to be reworked so the final-reference decision and
> clearing of pno are made under protocols_mtx, while the potentially
> blocking notifier unregister operation is performed without freeing the
> protocol instance underneath it.
> 

Cannot be done holding the mutex with the current design...

> > 	if (proto_notifier_nb) {
> > 		int ret;
> >
> > 		ret = scmi_protocol_notifier_register(pi->handle, &pi->pno);
> > 		if (ret)
> > 			dev_warn(handle->dev,
> > 				 "Failed to register protocol notifier\n");
> > 	}
> 
> If registration fails, pi->pno.nb remains populated. Future acquisitions
> of the existing protocol instance do not retry registration, while release
> later attempts to unregister the notifier even though registration never
> succeeded.
> 
> Please either clear the stored notifier on registration failure or track
> registration state separately and provide a defined retry/error path.
> 
Reviewed all of this in V8.

Thanks,
Cristian
>