Message-ID: <aEqsZWSlq9wKv10a@pluto>
Date: Thu, 12 Jun 2025 11:31:01 +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 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,
> >
> >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...

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

Thanks,
Cristian

Powered by blists - more mailing lists