linux-kernel - Re: [PATCH 0/3] firmware: arm_scmi: perf/cpufreq: Enable notification only if supported by platform

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aEmFnJVG8lXTDNmO@pluto>
Date: Wed, 11 Jun 2025 14:33:37 +0100
From: Cristian Marussi <cristian.marussi@....com>
To: Sudeep Holla <sudeep.holla@....com>
Cc: "Peng Fan (OSS)" <peng.fan@....nxp.com>,
	Cristian Marussi <cristian.marussi@....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 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 ?

> I wonder if it makes sense to quiesce the warnings from the core if the
> platform doesn't support notifications. I prefer to not add if notification
> supported in all the protocols.
> 

yes

> If the interface can return -EOPNOTSUPP(equivalent to SCMI_ERR_SUPPORT),
> the caller must handle it appropriately(i.e. continue if it can handle
> absence of notification or propagate error).
> 

This is what we do indeed....

> Cristian, Thoughts/opinions ?
>

too many :D ....

> > If platforms not support perf notification, saving some cpu cycles
> > by introducing notify_supported ops.
> > 
> 
> Sure, makes sense to improve where ever possible.
> 

Should be solved as above...

Thanks,
Cristian