linux-kernel - Re: [PATCH] [NOT_FOR_MERGE] firmware: arm_scmi: Optimize notifiers registration

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20250620073848.GA27519@nxa18884-linux>
Date: Fri, 20 Jun 2025 15:38:48 +0800
From: Peng Fan <peng.fan@....nxp.com>
To: Cristian Marussi <cristian.marussi@....com>
Cc: arm-scmi@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org, peng.fan@....com,
	sudeep.holla@....com
Subject: Re: [PATCH] [NOT_FOR_MERGE] firmware: arm_scmi: Optimize notifiers
 registration

Hi Cristian,
On Tue, Jun 17, 2025 at 02:50:38PM +0100, Cristian Marussi wrote:
>Some platforms could be configured not to support notification events from
>specific sources and such a case is already handled properly by avoiding
>even to attempt to send a notification enable request since it would be
>doomed to fail anyway.
>
>In an extreme scenario, though, a platform could support not even one
>single source on a specific event: in such a case would be meaningless to
>even allow to register a notifier and we can bail-out immediately, saving
>a lot of needless computation.
>
>Flag such condition, when detected at protocol initialization time, and
>reject upfront any attempt to register a notifier for such completely
>unsupported events with -ENOTSUPP.
>
>Signed-off-by: Cristian Marussi <cristian.marussi@....com>
>---
>NOT FOR MERGE until tested properly even with late loaded protocols.
>DOES NOT address the issues with verobosity of messages and lack of
>details about failures (which protos ? which resources ?)


I tested this patch on i.MX95, no error log anymore. Except
the one in cpufreq:
scmi-cpufreq scmi_dev.5: failed to register for limits change notifier for domain 8

The ret is -EOPNOTSUPP.

Thanks,
Peng

>---
> drivers/firmware/arm_scmi/notify.c | 39 +++++++++++++++++++++++-------
> 1 file changed, 30 insertions(+), 9 deletions(-)
>
>diff --git a/drivers/firmware/arm_scmi/notify.c b/drivers/firmware/arm_scmi/notify.c
>index e160ecb22948..dee9f238f6fd 100644
>--- a/drivers/firmware/arm_scmi/notify.c
>+++ b/drivers/firmware/arm_scmi/notify.c
>@@ -318,6 +318,9 @@ struct scmi_registered_events_desc {
>  *	    customized event report
>  * @num_sources: The number of possible sources for this event as stated at
>  *		 events' registration time
>+ * @not_supported_by_platform: A flag to indicate that not even one source was
>+ *			       found to be supported by the platform for this
>+ *			       event
>  * @sources: A reference to a dynamically allocated array used to refcount the
>  *	     events' enable requests for all the existing sources
>  * @sources_mtx: A mutex to serialize the access to @sources
>@@ -334,6 +337,7 @@ struct scmi_registered_event {
> 	const struct scmi_event	*evt;
> 	void		*report;
> 	u32		num_sources;
>+	bool		not_supported_by_platform;
> 	refcount_t	*sources;
> 	/* locking to serialize the access to sources */
> 	struct mutex	sources_mtx;
>@@ -811,10 +815,19 @@ int scmi_register_protocol_events(const struct scmi_handle *handle, u8 proto_id,
> 		if (!r_evt->report)
> 			return -ENOMEM;
> 
>-		for (id = 0; id < r_evt->num_sources; id++)
>-			if (ee->ops->is_notify_supported &&
>-			    !ee->ops->is_notify_supported(ph, r_evt->evt->id, id))
>-				refcount_set(&r_evt->sources[id], NOTIF_UNSUPP);
>+		if (ee->ops->is_notify_supported) {
>+			int supported = 0;
>+
>+			for (id = 0; id < r_evt->num_sources; id++) {
>+				if (!ee->ops->is_notify_supported(ph, r_evt->evt->id, id))
>+					refcount_set(&r_evt->sources[id], NOTIF_UNSUPP);
>+				else
>+					supported++;
>+			}
>+
>+			/* Not even one source has been found to be supported */
>+			r_evt->not_supported_by_platform = !supported;
>+		}
> 
> 		pd->registered_events[i] = r_evt;
> 		/* Ensure events are updated */
>@@ -936,6 +949,11 @@ static inline int scmi_bind_event_handler(struct scmi_notify_instance *ni,
> 	 * of protocol instance.
> 	 */
> 	hash_del(&hndl->hash);
>+
>+	/* Bailout if event is not supported at all */
>+	if (r_evt->not_supported_by_platform)
>+		return -EOPNOTSUPP;
>+
> 	/*
> 	 * Acquire protocols only for NON pending handlers, so as NOT to trigger
> 	 * protocol initialization when a notifier is registered against a still
>@@ -1060,6 +1078,9 @@ __scmi_event_handler_get_ops(struct scmi_notify_instance *ni,
> 	r_evt = SCMI_GET_REVT(ni, KEY_XTRACT_PROTO_ID(evt_key),
> 			      KEY_XTRACT_EVT_ID(evt_key));
> 
>+	if (r_evt && r_evt->not_supported_by_platform)
>+		return ERR_PTR(-EOPNOTSUPP);
>+
> 	mutex_lock(&ni->pending_mtx);
> 	/* Search registered events at first ... if possible at all */
> 	if (r_evt) {
>@@ -1087,7 +1108,7 @@ __scmi_event_handler_get_ops(struct scmi_notify_instance *ni,
> 				hndl->key);
> 			/* this hndl can be only a pending one */
> 			scmi_put_handler_unlocked(ni, hndl);
>-			hndl = NULL;
>+			hndl = ERR_PTR(-EINVAL);
> 		}
> 	}
> 	mutex_unlock(&ni->pending_mtx);
>@@ -1370,8 +1391,8 @@ static int scmi_notifier_register(const struct scmi_handle *handle,
> 	evt_key = MAKE_HASH_KEY(proto_id, evt_id,
> 				src_id ? *src_id : SRC_ID_MASK);
> 	hndl = scmi_get_or_create_handler(ni, evt_key);
>-	if (!hndl)
>-		return -EINVAL;
>+	if (IS_ERR(hndl))
>+		return PTR_ERR(hndl);
> 
> 	blocking_notifier_chain_register(&hndl->chain, nb);
> 
>@@ -1416,8 +1437,8 @@ static int scmi_notifier_unregister(const struct scmi_handle *handle,
> 	evt_key = MAKE_HASH_KEY(proto_id, evt_id,
> 				src_id ? *src_id : SRC_ID_MASK);
> 	hndl = scmi_get_handler(ni, evt_key);
>-	if (!hndl)
>-		return -EINVAL;
>+	if (IS_ERR(hndl))
>+		return PTR_ERR(hndl);
> 
> 	/*
> 	 * Note that this chain unregistration call is safe on its own
>-- 
>2.47.0
>