Re: [PATCH RFC 1/2] firmware: arm_scmi: clock: discover clock domains on first use

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] firmware: arm_scmi: refcount underflow in __scmi_enable_evt()
  on clock notify
--

commit 8fa43ba288f69bdf8d875f87946751f895ce422b
Author: Ghennadi Procopciuc <ghennadi.procopciuc@oss.nxp.com>

firmware: arm_scmi: clock: discover clock domains on first use

This commit defers SCMI clock domain discovery from protocol initialization
to the first operation on each clock, freeing the discovered arrays in an
instance_deinit callback.

> diff --git a/drivers/firmware/arm/scmi/clock.c b/drivers/firmware/arm/scmi/clock.c
> index 0278705d809e8..010b63b637030 100644
> --- a/drivers/firmware/arm/scmi/clock.c
> +++ b/drivers/firmware/arm/scmi/clock.c
[ ... ]
> @@ -1113,23 +1176,16 @@ static const struct scmi_clk_proto_ops clk_proto_ops = {
>  static bool scmi_clk_notify_supported(const struct scmi_protocol_handle *ph,
>  				      u8 evt_id, u32 src_id)
>  {
> -	bool supported;
> -	struct scmi_clock_info *clk;
>  	struct clock_info *ci = ph->get_priv(ph);
>  
> -	if (evt_id >= ARRAY_SIZE(evt_2_cmd))
> -		return false;
> -
> -	clk = scmi_clock_domain_lookup(ci, src_id);
> -	if (IS_ERR(clk))
> +	if (evt_id >= ARRAY_SIZE(evt_2_cmd) || src_id >= ci->num_clocks)
>  		return false;
>  
> +	/* No clock is discovered yet, so report the protocol level support */
>  	if (evt_id == SCMI_EVENT_CLOCK_RATE_CHANGED)
> -		supported = clk->rate_changed_notifications;
> -	else
> -		supported = clk->rate_change_requested_notifications;
> +		return ci->notify_rate_changed_cmd;
>  
> -	return supported;
> +	return ci->notify_rate_change_requested_cmd;
>  }
>  
>  static int scmi_clk_rate_notify(const struct scmi_protocol_handle *ph,
> @@ -1157,10 +1213,23 @@ static int scmi_clk_set_notify_enabled(const struct scmi_protocol_handle *ph,
>  				       u8 evt_id, u32 src_id, bool enable)
>  {
>  	int ret, cmd_id;
> +	bool supported;
> +	struct scmi_clock_info *clk;
> +	struct clock_info *ci = ph->get_priv(ph);
>  
>  	if (evt_id >= ARRAY_SIZE(evt_2_cmd))
>  		return -EINVAL;
>  
> +	clk = scmi_clock_domain_lookup(ph, ci, src_id, false);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +
> +	supported = evt_id == SCMI_EVENT_CLOCK_RATE_CHANGED ?
> +		    clk->rate_changed_notifications :
> +		    clk->rate_change_requested_notifications;
> +	if (!supported)
> +		return -EOPNOTSUPP;
> +
>  	cmd_id = evt_2_cmd[evt_id];
>  	ret = scmi_clk_rate_notify(ph, src_id, cmd_id, enable);
>  	if (ret)

[Severity: Medium]
Can this underflow r_evt->sources[src_id] in __scmi_enable_evt() when an
all-sources clock notifier is unregistered?

Because scmi_clk_notify_supported() now returns true for every clock when
the protocol supports the notification command,
scmi_register_protocol_events() leaves r_evt->sources[src_id] at 0 instead
of NOTIF_UNSUPP for clocks that do not support notifications.  When an
event is enabled for all sources (SRC_ID_MASK), __scmi_enable_evt() gets
-EOPNOTSUPP from scmi_clk_set_notify_enabled() for those clocks and leaves
their refcount at 0, while still succeeding overall if at least one clock
supports the event.  When the notifier is later unregistered, the disable
path in __scmi_enable_evt() only skips NOTIF_UNSUPP:

drivers/firmware/arm/scmi/notify.c:__scmi_enable_evt() {
	...
			} else if (refcount_read(sid) == 0) {
				ret = REVT_NOTIFY_ENABLE(r_evt, r_evt->evt->id,
							 src_id);
				if (!ret)
					refcount_set(sid, 1);
			} else {
				refcount_inc(sid);
			}
			retvals += !ret;
		}
	} else {
		for (; num_sources; src_id++, num_sources--) {
			sid = &r_evt->sources[src_id];
			if (refcount_read(sid) == NOTIF_UNSUPP)
				continue;
			if (refcount_dec_and_test(sid))
				REVT_NOTIFY_DISABLE(r_evt,
						    r_evt->evt->id, src_id);
	...
}

Calling refcount_dec_and_test(sid) on the clocks left at 0 triggers a
refcount underflow warning and saturates r_evt->sources[src_id], causing
subsequent enable calls on those sources to hit the refcount_inc(sid) path
and report success without enabling notifications.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-b4-scmi-clk-on-demand-v1-0-7fd162097b57@oss.nxp.com?part=1