Message-ID: <aFFwmBTPKr_evCnY@pluto>
Date: Tue, 17 Jun 2025 14:41:44 +0100
From: Cristian Marussi <cristian.marussi@....com>
To: Peng Fan <peng.fan@....nxp.com>
Cc: Cristian Marussi <cristian.marussi@....com>,
	Sudeep Holla <sudeep.holla@....com>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Viresh Kumar <viresh.kumar@...aro.org>, arm-scmi@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linux-pm@...r.kernel.org, Peng Fan <peng.fan@....com>
Subject: Re: [PATCH 0/3] firmware: arm_scmi: perf/cpufreq: Enable
 notification only if supported by platform

On Fri, Jun 13, 2025 at 05:50:59PM +0800, Peng Fan wrote:
> Hi Cristian,
> 
> On Thu, Jun 12, 2025 at 11:31:01AM +0100, Cristian Marussi wrote:
> >On Thu, Jun 12, 2025 at 11:43:52AM +0800, Peng Fan wrote:
> >> On Wed, Jun 11, 2025 at 02:33:37PM +0100, Cristian Marussi wrote:
> >> >On Wed, Jun 11, 2025 at 01:17:11PM +0100, Sudeep Holla wrote:
> >> >> On Wed, Jun 11, 2025 at 03:52:42PM +0800, Peng Fan (OSS) wrote:
> >> >> > PERFORMANCE_NOTIFY_LIMITS and PERFORMANCE_NOTIFY_LEVEL are optional
> >> >> > commands. If use these commands on platforms that not support the two,
> >> >> > there is error log:
> >> >> >   SCMI Notifications - Failed to ENABLE events for key:13000008 !
> >> >> >   scmi-cpufreq scmi_dev.4: failed to register for limits change notifier for domain 8
> >> >> > 
> >> >> 
> >> >
> >> >Hi,
> >> >

Hi,

> >> >I had a quick look/refresh to this stuff from years ago...
> >> >
> >> >...wont be so short to explain :P
> >> >
> >> >In general when you register a notifier_block for some SCMI events,
> >> >the assumption was that a driver using proto_X_ops could want to register
> >> >NOT only for proto_X events BUT also for other protos...in such a case you
> >> >are NOT guaranteed that such other proto_Y was initialized when your
> >> >driver probes and tries to register the notifier...indeed it could be
> >> >that such proto_Y could be a module that has still to be loaded !
> >> >
> >> >...in this scenario you can end-up quickly in a hell of probe-dependency
> >> >if you write a driver asking for SCMI events that can or cannot be still
> >> >readily available when the driver probes...
> >> >
> >> >....so the decision was to simply place such notifier registration requests
> >> >on hold on a pending list...whenever the needed missing protocol is
> >> >loaded/inialized the notifier registration is completed...if the proto_Y
> >> >never arrives nothing happens...and your driver caller can probe
> >> >successfully anyway...
> >> >
> >> >This means in such a corner-case the notifier registration is sort of
> >> >asynchonous and eventual errors detected later, when the protocol is
> >> >finally initialized and notifiers finalized, cannot be easily reported
> >> >(BUT I think we could improve on this ... thinking about this...)
> >> >
> >> >...BUT....
> >> >
> >> >....this is NOT our case NOR the most common case...the usual scenario,
> >> >like cpufreq, is that a driver using proto_X_ops tries to register for
> >> >that same proto_X events and in such a case we can detect that such
> >> >domain is unsupported and fail while avoiding to send any message indeed....
> >> >
> >> >....so....:P...while I was going through this rabbit-hole....this issues
> >> >started to feel familiar...O_o....
> >> >
> >> >... indeed I realized that the function that you (Peng) now invoke to
> >> >set the per-domain perf_limit_notify flag was introduced just for these
> >> >reasons to check and avoid such situation for all protocols in the core:
> >> >
> >> >
> >> >commit 8733e86a80f5a7abb7b4b6ca3f417b32c3eb68e3
> >> >Author: Cristian Marussi <cristian.marussi@....com>
> >> >Date:   Mon Feb 12 12:32:23 2024 +0000
> >> >
> >> >    firmware: arm_scmi: Check for notification support
> >> >    
> >> >    When registering protocol events, use the optional .is_notify_supported
> >> >    callback provided by the protocol to check if that specific notification
> >> >    type is available for that particular resource on the running system,
> >> >    marking it as unsupported otherwise.
> >> >    
> >> >    Then, when a notification enable request is received, return an error if
> >> >    it was previously marked as unsuppported, so avoiding to send a needless
> >> >    notification enable command and check the returned value for failure.
> >> >    
> >> >    Signed-off-by: Cristian Marussi <cristian.marussi@....com>
> >> >    Link: https://lore.kernel.org/r/20240212123233.1230090-2-cristian.marussi@arm.com
> >> >    Signed-off-by: Sudeep Holla <sudeep.holla@....com>
> >> >
> >> >
> >> >...so my suspect is that we are ALREADY avoiding to send unneeded
> >> >messages when a domain does NOT support notifications for ALL
> >> >protocols...it is just that we are a bit too noisy...
> >> >
> >> >@Peng do you observe the message being sent instead ? (so maybe the
> >> >above has a bug...) or it is just the message ?
> >> 
> >> Just the message.
> >> 
> >> arm-scmi arm-scmi.0.auto: SCMI Notifications - Notification NOT supported - proto_id:19  evt_id:0  src_id:8
> >> SCMI Notifications - Failed to ENABLE events for key:13000008 !
> >> scmi-cpufreq scmi_dev.4: failed to register for limits change notifier for domain 8
> >> 
> >> It just make user has a feeling that there must be something wrong, especially
> >> those not know the internals.
> >
> >Yes indeed it is too much noisy...
> >
> >> 
> >> And from the error message, "Failed to ENABLE events for key..", we not
> >> know which protocol, and whether notification supported.
> >> 
> >> I was thinking to propogate the error value, but __scmi_enable_evt
> >> always use -EINVAL if not success.
> >> 
> >
> >I suppose, if you want also to save cycles that you could mark internally a
> >protocol, at init time, as NOT suporting notifs (if you can detect that no domain
> >is supported OR the related notfs commands are NOT available) and then
> >bailing out early with -ENOTOPSUPP when trying to register a new
> >notifier (amd muting all the errs to dbgs....) so that the caller can
> >warn if wanted or not...
> 
> Since you have more expertise in this area, do you have plan to improve here?
> 
> If no, I will give a look and see what I could do, but surely needs your
> suggestion.

... (would be) Happy to help but I dont have so much bandwidth as of now...I will
send you in reply to this an half-baked/UNTESTED patch to express what I
meant....

> 
> >
> >> >
> >> >> I wonder if it makes sense to quiesce the warnings from the core if the
> >> >> platform doesn't support notifications.
> >> 
> >> If not quiesce, we might need to make it clear from the error message,
> >> saying whether X events are supported for Y protocols or not, not just
> >> a "Failed to ENABLE events for key.."
> >> 
> >
> >Yes that was a very early and primitve errors message of mine...my bad :D
> 
> How about this?
> -------------------------------
> diff --git a/drivers/firmware/arm_scmi/notify.c b/drivers/firmware/arm_scmi/notify.c
> index e160ecb22948..1e5a34dc36ab 100644
> --- a/drivers/firmware/arm_scmi/notify.c
> +++ b/drivers/firmware/arm_scmi/notify.c
> @@ -1184,6 +1184,11 @@ static inline int __scmi_enable_evt(struct scmi_registered_event *r_evt,
>  							 src_id);
>  				if (!ret)
>  					refcount_set(sid, 1);
> +				else
> +					dev_err(r_evt->proto->ph->dev,
> +						"Enable Notification failed - proto_id:%d  evt_id:%d  src_id:%d, %pe",
> +						r_evt->proto->id, r_evt->evt->id,
> +						src_id, ret);
>  			} else {
>  				refcount_inc(sid);
>  			}
> @@ -1313,12 +1318,7 @@ static void scmi_put_active_handler(struct scmi_notify_instance *ni,
>   */
>  static int scmi_event_handler_enable_events(struct scmi_event_handler *hndl)
>  {
> -	if (scmi_enable_events(hndl)) {
> -		pr_err("Failed to ENABLE events for key:%X !\n", hndl->key);
> -		return -EINVAL;
> -	}
> -
> -	return 0;
> +	return scmi_enable_events(hndl)
>  }

I was thinking more about something to cut way before the notifier
registration process...

I'll send you something I played with last week..

Thanks,
Cristian

Powered by blists - more mailing lists