Re: [PATCH v10 04/24] dt-bindings: firmware: arm,scmi: Add support for telemetry protocol

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Dropping info->protocols_mtx in scmi_protocol_release creates a race condition that can permanently remove protocol notifiers while the protocol remains active for concurrent users.
--

--- Patch [4]: [PATCH v10 04/24] dt-bindings: firmware: arm,scmi: Add support for telemetry protocol ---
commit 5a2c3d7356ea4d8a9abff06c827c1b5e1af546f8
Author: Cristian Marussi <[email protected]>
Subject: dt-bindings: firmware: arm,scmi: Add support for telemetry protocol

This commit adds new DeviceTree binding definitions for the SCMI v4.0
Telemetry protocol. It introduces the protocol@1b node referencing the
protocol-node schema.

> diff --git a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> index d06cca9273c48..be733f8d60a02 100644
> --- a/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> +++ b/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
[ ... ]
> @@ -324,6 +324,14 @@ properties:
>      required:
>        - reg
>  
> +  protocol@1b:
> +    $ref: '#/$defs/protocol-node'
> +    unevaluatedProperties: false
> +
> +    properties:
> +      reg:
> +        const: 0x1b
> +
>  unevaluatedProperties: false
>  
>  $defs:

[Severity: High]
Could this regression create a race condition that permanently removes
protocol notifiers while the protocol remains active for concurrent users?

Looking at scmi_protocol_release() in drivers/firmware/arm_scmi/driver.c,
a thread could enter when the refcount is 2 (1 active user and 1 notifier).
It determines it must unregister the notifier, sets proto_notifier_nb, and
exits the scoped_guard, which drops the info->protocols_mtx lock.

Concurrently, another thread calling scmi_get_protocol_instance()
could acquire the mutex, find the protocol in the idr, and increment its
refcount to 3:

drivers/firmware/arm_scmi/driver.c:scmi_get_protocol_instance() {
...
		pi = idr_find(&info->protocols, protocol_id);
		if (pi) {
			refcount_inc(&pi->users);
		} else {
...
}

This returns the protocol to the new user without re-registering the
notifier, since proto_notifier_nb remains null for existing instances.

The first thread then proceeds to completely remove the notifier outside
the lock:

drivers/firmware/arm_scmi/driver.c:scmi_protocol_release() {
...
	if (proto_notifier_nb) {
		if (scmi_protocol_notifier_unregister(pi->handle, &pi->pno))
			dev_err(handle->dev,
				"Failed to release protocol notifier\n");
	}
...
}

Could this result in the new user actively using the protocol while
telemetry or events meant for this protocol are silently dropped?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4