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

[David Hildenbrand (Arm)]

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.

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


>  	}
> -	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?

-- 
Cheers,

David