Re: [PATCH v12 03/25] firmware: arm_scmi: Introduce protocol instance notifiers

[Cristian Marussi <cristian.marussi@arm.com>]

On Tue, Sep 22, 2026 at 03:44:48PM +0200, David Hildenbrand (Arm) wrote:
> On 9/20/26 11:19, Cristian Marussi wrote:
> > SCMI Protocol notifications are typically used by SCMI drivers to detect
> > and react to particular conditions: this was the assumption and the classic
> > usage scenario upon which the SCMI notification framework was built.
> 
> Was the paragraph supposed to start with "SCMI notifications" ? Because later
> you describe how some protocols might want to reuse the "existing SCMI
> Notifications machinery", and here you talk about the traditional usage.

Indeed.

> 
> > -	mutex_lock(&info->protocols_mtx);
> > -	pi = idr_find(&info->protocols, protocol_id);
> > +	scoped_guard(mutex, &info->protocols_mtx) {
> > +		pi = idr_find(&info->protocols, protocol_id);
> > +		if (pi) {
> > +			refcount_inc(&pi->users);
> > +		} else {
> > +			const struct scmi_protocol *proto;
> >  
> > -	if (pi) {
> > -		refcount_inc(&pi->users);
> > -	} else {
> > -		const struct scmi_protocol *proto;
> > +			/* Fails if protocol not registered on bus */
> > +			proto = scmi_protocol_get(protocol_id, &info->version);
> > +			if (!proto)
> > +				return ERR_PTR(-EPROBE_DEFER);
> >  
> > -		/* Fails if protocol not registered on bus */
> > -		proto = scmi_protocol_get(protocol_id, &info->version);
> > -		if (proto)
> >  			pi = scmi_alloc_init_protocol_instance(info, proto);
> > -		else
> > -			pi = ERR_PTR(-EPROBE_DEFER);
> > +			if (IS_ERR(pi))
> > +				return pi;
> > +
> > +			proto_notifier_nb = READ_ONCE(pi->pno.nb);
> > +		}
> > +	}
> > +
> > +	if (proto_notifier_nb) {
> > +		if (scmi_protocol_notifier_register(pi->handle, &pi->pno))
> > +			dev_warn(handle->dev,
> > +				 "Failed to register protocol notifier\n");
> 
> 
> 	if (proto_notifier_nb &&
> 	    scmi_protocol_notifier_register(pi->handle, &pi->pno))
> 		dev_warn(handle->dev, ...)
> 
> As we dropped the mutex, I assume somebody else could move ahead and
> refcount_inc(&pi->users) + return before the notifier was registered? Is that
> expected?
>

It is more of a problem on the release path...in fact...these limit
condition are an open residual issues...
 
> 
> >  	}
> > -	mutex_unlock(&info->protocols_mtx);
> >  
> >  	return pi;
> >  }
> > @@ -2366,13 +2398,35 @@ int scmi_protocol_acquire(const struct scmi_handle *handle, u8 protocol_id)
> >  void scmi_protocol_release(const struct scmi_handle *handle, u8 protocol_id)
> >  {
> >  	struct scmi_info *info = handle_to_scmi_info(handle);
> > +	struct notifier_block *proto_notifier_nb = NULL;
> >  	struct scmi_protocol_instance *pi;
> >  
> > -	mutex_lock(&info->protocols_mtx);
> > -	pi = idr_find(&info->protocols, protocol_id);
> > -	if (WARN_ON(!pi))
> > -		goto out;
> > +	scoped_guard(mutex, &info->protocols_mtx) {
> > +		pi = idr_find(&info->protocols, protocol_id);
> > +		if (WARN_ON(!pi))
> > +			return;
> >  
> > +		/*
> > +		 * If a protocol notifier was registered and this is the
> > +		 * last istance releasing the protocol, mark the notifier
> 
> s/istance/instance/
> 
> > +		 * for un-registration: note that the notifier itself counts
> > +		 * as one user, as for any other regular notification, so if a
> > +		 * protocol notifier is registered and there are only 2 users
> > +		 * active we can derive that this is the last protocol
> > +		 * instance de-registering.
> > +		 */
> > +		if (scmi_protocol_notifier_registered(&pi->pno) &&
> > +		    refcount_read(&pi->users) == 2)
> > +			proto_notifier_nb = READ_ONCE(pi->pno.nb);
> > +	}
> > +
> 
> We drop the mutex now temporarily. What happens if another instance gets
> registered (incrementing &pi->users) just after we dropped the lock and
> unregister the notifier?
> 
> Shouldn't we care about that or why is it ok?

I'll try a slight different approach for this...till now I was looking for the
least possible invasive ways of dealing with the fact that the core SCMI mechanisms
were NOT originally designed to deal with in-protocol users but just with
external SCMI drivers...

Thanks,
Cristian