* [PATCH 0/3] firmware: arm_scmi: perf/cpufreq: Enable notification only if supported by platform
@ 2025-06-11 7:52 Peng Fan (OSS)
2025-06-11 7:52 ` [PATCH 1/3] firmware: arm_scmi: Fix typo for scmi_perf_proto_ops Peng Fan (OSS)
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Peng Fan (OSS) @ 2025-06-11 7:52 UTC (permalink / raw)
To: Sudeep Holla, Cristian Marussi, Rafael J. Wysocki, Viresh Kumar
Cc: arm-scmi, linux-arm-kernel, linux-kernel, linux-pm, Peng Fan
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
If platforms not support perf notification, saving some cpu cycles
by introducing notify_supported ops.
While at here, patch 1 is a typo fix when doing the patchset.
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
Peng Fan (3):
firmware: arm_scmi: Fix typo for scmi_perf_proto_ops
firmware: arm_scmi: perf: Add notify_supported for scmi_perf_proto_ops
cpufreq: scmi-cpufreq: Enable perf limits notification only supported
drivers/cpufreq/scmi-cpufreq.c | 25 ++++++++++++++++++-------
drivers/firmware/arm_scmi/perf.c | 37 +++++++++++++++++++------------------
include/linux/scmi_protocol.h | 5 ++++-
3 files changed, 41 insertions(+), 26 deletions(-)
---
base-commit: 19a60293b9925080d97f22f122aca3fc46dadaf9
change-id: 20250611-scmi-perf-a0ded8a5a303
Best regards,
--
Peng Fan <peng.fan@nxp.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3] firmware: arm_scmi: Fix typo for scmi_perf_proto_ops
2025-06-11 7:52 [PATCH 0/3] firmware: arm_scmi: perf/cpufreq: Enable notification only if supported by platform Peng Fan (OSS)
@ 2025-06-11 7:52 ` Peng Fan (OSS)
2025-06-11 7:52 ` [PATCH 2/3] firmware: arm_scmi: perf: Add notify_supported " Peng Fan (OSS)
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: Peng Fan (OSS) @ 2025-06-11 7:52 UTC (permalink / raw)
To: Sudeep Holla, Cristian Marussi, Rafael J. Wysocki, Viresh Kumar
Cc: arm-scmi, linux-arm-kernel, linux-kernel, linux-pm, Peng Fan
From: Peng Fan <peng.fan@nxp.com>
The name is power_scale_get, not power_scale_mw_get, correct it.
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
include/linux/scmi_protocol.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 688466a0e816247d24704f7ba109667a14226b67..aafaac1496b06a6e4f0ca32eee58a9edf7d4a70f 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -153,7 +153,7 @@ struct scmi_perf_domain_info {
* for a given device
* @fast_switch_rate_limit: gets the minimum time (us) required between
* successive fast_switching requests
- * @power_scale_mw_get: indicates if the power values provided are in milliWatts
+ * @power_scale_get: indicates if the power values provided are in milliWatts
* or in some other (abstract) scale
*/
struct scmi_perf_proto_ops {
--
2.37.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/3] firmware: arm_scmi: perf: Add notify_supported for scmi_perf_proto_ops
2025-06-11 7:52 [PATCH 0/3] firmware: arm_scmi: perf/cpufreq: Enable notification only if supported by platform Peng Fan (OSS)
2025-06-11 7:52 ` [PATCH 1/3] firmware: arm_scmi: Fix typo for scmi_perf_proto_ops Peng Fan (OSS)
@ 2025-06-11 7:52 ` Peng Fan (OSS)
2025-06-11 7:52 ` [PATCH 3/3] cpufreq: scmi-cpufreq: Enable perf limits notification only supported Peng Fan (OSS)
2025-06-11 12:17 ` [PATCH 0/3] firmware: arm_scmi: perf/cpufreq: Enable notification only if supported by platform Sudeep Holla
3 siblings, 0 replies; 13+ messages in thread
From: Peng Fan (OSS) @ 2025-06-11 7:52 UTC (permalink / raw)
To: Sudeep Holla, Cristian Marussi, Rafael J. Wysocki, Viresh Kumar
Cc: arm-scmi, linux-arm-kernel, linux-kernel, linux-pm, Peng Fan
From: Peng Fan <peng.fan@nxp.com>
PERFORMANCE_NOTIFY_LIMITS and PERFORMANCE_NOTIFY_LEVEL are optional
commands which are not implemented in i.MX95 SCMI firmware. Provide
a hook to query whether they are supported, before invoke
event_notifier_register.
scmi_perf_notify_supported could be directly used here, so just
move scmi_perf_notify_supported above perf_proto_ops and use it
in perf_proto_ops.
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
drivers/firmware/arm_scmi/perf.c | 37 +++++++++++++++++++------------------
include/linux/scmi_protocol.h | 3 +++
2 files changed, 22 insertions(+), 18 deletions(-)
diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index c7e5a34b254bf4e9c51c7be56803b6d851f0e1d6..ba990643edf0bcb5fc25253b4f52a5dc93b62a77 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -1068,24 +1068,6 @@ scmi_power_scale_get(const struct scmi_protocol_handle *ph)
return pi->power_scale;
}
-static const struct scmi_perf_proto_ops perf_proto_ops = {
- .num_domains_get = scmi_perf_num_domains_get,
- .info_get = scmi_perf_info_get,
- .limits_set = scmi_perf_limits_set,
- .limits_get = scmi_perf_limits_get,
- .level_set = scmi_perf_level_set,
- .level_get = scmi_perf_level_get,
- .transition_latency_get = scmi_dvfs_transition_latency_get,
- .rate_limit_get = scmi_dvfs_rate_limit_get,
- .device_opps_add = scmi_dvfs_device_opps_add,
- .freq_set = scmi_dvfs_freq_set,
- .freq_get = scmi_dvfs_freq_get,
- .est_power_get = scmi_dvfs_est_power_get,
- .fast_switch_possible = scmi_fast_switch_possible,
- .fast_switch_rate_limit = scmi_fast_switch_rate_limit,
- .power_scale_get = scmi_power_scale_get,
-};
-
static bool scmi_perf_notify_supported(const struct scmi_protocol_handle *ph,
u8 evt_id, u32 src_id)
{
@@ -1107,6 +1089,25 @@ static bool scmi_perf_notify_supported(const struct scmi_protocol_handle *ph,
return supported;
}
+static const struct scmi_perf_proto_ops perf_proto_ops = {
+ .num_domains_get = scmi_perf_num_domains_get,
+ .info_get = scmi_perf_info_get,
+ .limits_set = scmi_perf_limits_set,
+ .limits_get = scmi_perf_limits_get,
+ .level_set = scmi_perf_level_set,
+ .level_get = scmi_perf_level_get,
+ .transition_latency_get = scmi_dvfs_transition_latency_get,
+ .rate_limit_get = scmi_dvfs_rate_limit_get,
+ .device_opps_add = scmi_dvfs_device_opps_add,
+ .freq_set = scmi_dvfs_freq_set,
+ .freq_get = scmi_dvfs_freq_get,
+ .est_power_get = scmi_dvfs_est_power_get,
+ .fast_switch_possible = scmi_fast_switch_possible,
+ .fast_switch_rate_limit = scmi_fast_switch_rate_limit,
+ .power_scale_get = scmi_power_scale_get,
+ .notify_supported = scmi_perf_notify_supported,
+};
+
static int scmi_perf_set_notify_enabled(const struct scmi_protocol_handle *ph,
u8 evt_id, u32 src_id, bool enable)
{
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index aafaac1496b06a6e4f0ca32eee58a9edf7d4a70f..91865f0ebcbd4b15b55afd8c1a0d0614d6985daf 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -155,6 +155,7 @@ struct scmi_perf_domain_info {
* successive fast_switching requests
* @power_scale_get: indicates if the power values provided are in milliWatts
* or in some other (abstract) scale
+ * @notify_supported: indicates if the event is supported
*/
struct scmi_perf_proto_ops {
int (*num_domains_get)(const struct scmi_protocol_handle *ph);
@@ -185,6 +186,8 @@ struct scmi_perf_proto_ops {
int (*fast_switch_rate_limit)(const struct scmi_protocol_handle *ph,
u32 domain, u32 *rate_limit);
enum scmi_power_scale (*power_scale_get)(const struct scmi_protocol_handle *ph);
+ bool (*notify_supported)(const struct scmi_protocol_handle *ph, u8 evt_id,
+ u32 src_id);
};
/**
--
2.37.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/3] cpufreq: scmi-cpufreq: Enable perf limits notification only supported
2025-06-11 7:52 [PATCH 0/3] firmware: arm_scmi: perf/cpufreq: Enable notification only if supported by platform Peng Fan (OSS)
2025-06-11 7:52 ` [PATCH 1/3] firmware: arm_scmi: Fix typo for scmi_perf_proto_ops Peng Fan (OSS)
2025-06-11 7:52 ` [PATCH 2/3] firmware: arm_scmi: perf: Add notify_supported " Peng Fan (OSS)
@ 2025-06-11 7:52 ` Peng Fan (OSS)
2025-06-11 12:17 ` [PATCH 0/3] firmware: arm_scmi: perf/cpufreq: Enable notification only if supported by platform Sudeep Holla
3 siblings, 0 replies; 13+ messages in thread
From: Peng Fan (OSS) @ 2025-06-11 7:52 UTC (permalink / raw)
To: Sudeep Holla, Cristian Marussi, Rafael J. Wysocki, Viresh Kumar
Cc: arm-scmi, linux-arm-kernel, linux-kernel, linux-pm, Peng Fan
From: Peng Fan <peng.fan@nxp.com>
PERFORMANCE_NOTIFY_LIMITS is optional, so enable perf limits
notification event only when the platform supports it.
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
drivers/cpufreq/scmi-cpufreq.c | 25 ++++++++++++++++++-------
1 file changed, 18 insertions(+), 7 deletions(-)
diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
index ef078426bfd51af6a8a4b803278dfae5d323db48..8999960574a2fc427e934553198584d2aeb14a58 100644
--- a/drivers/cpufreq/scmi-cpufreq.c
+++ b/drivers/cpufreq/scmi-cpufreq.c
@@ -29,6 +29,7 @@ struct scmi_data {
cpumask_var_t opp_shared_cpus;
struct notifier_block limit_notify_nb;
struct freq_qos_request limits_freq_req;
+ bool perf_limit_notify;
};
static struct scmi_protocol_handle *ph;
@@ -310,15 +311,22 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
goto out_free_table;
}
- priv->limit_notify_nb.notifier_call = scmi_limit_notify_cb;
- ret = sdev->handle->notify_ops->event_notifier_register(sdev->handle, SCMI_PROTOCOL_PERF,
+ priv->perf_limit_notify =
+ perf_ops->notify_supported(ph, SCMI_EVENT_PERFORMANCE_LIMITS_CHANGED,
+ priv->domain_id);
+
+ if (priv->perf_limit_notify) {
+ priv->limit_notify_nb.notifier_call = scmi_limit_notify_cb;
+ ret = sdev->handle->notify_ops->event_notifier_register(sdev->handle,
+ SCMI_PROTOCOL_PERF,
SCMI_EVENT_PERFORMANCE_LIMITS_CHANGED,
&priv->domain_id,
&priv->limit_notify_nb);
- if (ret)
- dev_warn(&sdev->dev,
- "failed to register for limits change notifier for domain %d\n",
- priv->domain_id);
+ if (ret)
+ dev_warn(&sdev->dev,
+ "failed to register for limits change notifier for domain %d\n",
+ priv->domain_id);
+ }
return 0;
@@ -341,10 +349,13 @@ static void scmi_cpufreq_exit(struct cpufreq_policy *policy)
struct scmi_data *priv = policy->driver_data;
struct scmi_device *sdev = cpufreq_get_driver_data();
- sdev->handle->notify_ops->event_notifier_unregister(sdev->handle, SCMI_PROTOCOL_PERF,
+ if (priv->perf_limit_notify) {
+ sdev->handle->notify_ops->event_notifier_unregister(sdev->handle,
+ SCMI_PROTOCOL_PERF,
SCMI_EVENT_PERFORMANCE_LIMITS_CHANGED,
&priv->domain_id,
&priv->limit_notify_nb);
+ }
freq_qos_remove_request(&priv->limits_freq_req);
dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table);
dev_pm_opp_remove_all_dynamic(priv->cpu_dev);
--
2.37.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 0/3] firmware: arm_scmi: perf/cpufreq: Enable notification only if supported by platform
2025-06-11 7:52 [PATCH 0/3] firmware: arm_scmi: perf/cpufreq: Enable notification only if supported by platform Peng Fan (OSS)
` (2 preceding siblings ...)
2025-06-11 7:52 ` [PATCH 3/3] cpufreq: scmi-cpufreq: Enable perf limits notification only supported Peng Fan (OSS)
@ 2025-06-11 12:17 ` Sudeep Holla
2025-06-11 13:33 ` Cristian Marussi
3 siblings, 1 reply; 13+ messages in thread
From: Sudeep Holla @ 2025-06-11 12:17 UTC (permalink / raw)
To: Peng Fan (OSS)
Cc: Cristian Marussi, Rafael J. Wysocki, Viresh Kumar, arm-scmi,
linux-arm-kernel, linux-kernel, linux-pm, Sudeep Holla, Peng Fan
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
>
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.
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).
Cristian, Thoughts/opinions ?
> If platforms not support perf notification, saving some cpu cycles
> by introducing notify_supported ops.
>
Sure, makes sense to improve where ever possible.
> While at here, patch 1 is a typo fix when doing the patchset.
>
This one looks OK.
--
Regards,
Sudeep
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/3] firmware: arm_scmi: perf/cpufreq: Enable notification only if supported by platform
2025-06-11 12:17 ` [PATCH 0/3] firmware: arm_scmi: perf/cpufreq: Enable notification only if supported by platform Sudeep Holla
@ 2025-06-11 13:33 ` Cristian Marussi
2025-06-12 3:43 ` Peng Fan
0 siblings, 1 reply; 13+ messages in thread
From: Cristian Marussi @ 2025-06-11 13:33 UTC (permalink / raw)
To: Sudeep Holla
Cc: Peng Fan (OSS), Cristian Marussi, Rafael J. Wysocki, Viresh Kumar,
arm-scmi, linux-arm-kernel, linux-kernel, linux-pm, Peng Fan
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@arm.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@arm.com>
Link: https://lore.kernel.org/r/20240212123233.1230090-2-cristian.marussi@arm.com
Signed-off-by: Sudeep Holla <sudeep.holla@arm.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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/3] firmware: arm_scmi: perf/cpufreq: Enable notification only if supported by platform
2025-06-11 13:33 ` Cristian Marussi
@ 2025-06-12 3:43 ` Peng Fan
2025-06-12 10:31 ` Cristian Marussi
0 siblings, 1 reply; 13+ messages in thread
From: Peng Fan @ 2025-06-12 3:43 UTC (permalink / raw)
To: Cristian Marussi
Cc: Sudeep Holla, Rafael J. Wysocki, Viresh Kumar, arm-scmi,
linux-arm-kernel, linux-kernel, linux-pm, Peng Fan
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@arm.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@arm.com>
> Link: https://lore.kernel.org/r/20240212123233.1230090-2-cristian.marussi@arm.com
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.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.
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 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.."
Thanks,
Peng
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/3] firmware: arm_scmi: perf/cpufreq: Enable notification only if supported by platform
2025-06-12 3:43 ` Peng Fan
@ 2025-06-12 10:31 ` Cristian Marussi
2025-06-13 9:50 ` Peng Fan
0 siblings, 1 reply; 13+ messages in thread
From: Cristian Marussi @ 2025-06-12 10:31 UTC (permalink / raw)
To: Peng Fan
Cc: Cristian Marussi, Sudeep Holla, Rafael J. Wysocki, Viresh Kumar,
arm-scmi, linux-arm-kernel, linux-kernel, linux-pm, Peng Fan
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@arm.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@arm.com>
> > Link: https://lore.kernel.org/r/20240212123233.1230090-2-cristian.marussi@arm.com
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/3] firmware: arm_scmi: perf/cpufreq: Enable notification only if supported by platform
2025-06-12 10:31 ` Cristian Marussi
@ 2025-06-13 9:50 ` Peng Fan
2025-06-17 13:41 ` Cristian Marussi
2025-06-17 13:50 ` [PATCH] [NOT_FOR_MERGE] firmware: arm_scmi: Optimize notifiers registration Cristian Marussi
0 siblings, 2 replies; 13+ messages in thread
From: Peng Fan @ 2025-06-13 9:50 UTC (permalink / raw)
To: Cristian Marussi
Cc: Sudeep Holla, Rafael J. Wysocki, Viresh Kumar, arm-scmi,
linux-arm-kernel, linux-kernel, linux-pm, Peng Fan
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,
>> >
>> >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@arm.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@arm.com>
>> > Link: https://lore.kernel.org/r/20240212123233.1230090-2-cristian.marussi@arm.com
>> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.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.
>
>> >
>> >> 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)
}
/**
-------------------------------
>
>Thanks,
>Cristian
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 0/3] firmware: arm_scmi: perf/cpufreq: Enable notification only if supported by platform
2025-06-13 9:50 ` Peng Fan
@ 2025-06-17 13:41 ` Cristian Marussi
2025-06-17 13:50 ` [PATCH] [NOT_FOR_MERGE] firmware: arm_scmi: Optimize notifiers registration Cristian Marussi
1 sibling, 0 replies; 13+ messages in thread
From: Cristian Marussi @ 2025-06-17 13:41 UTC (permalink / raw)
To: Peng Fan
Cc: Cristian Marussi, Sudeep Holla, Rafael J. Wysocki, Viresh Kumar,
arm-scmi, linux-arm-kernel, linux-kernel, linux-pm, Peng Fan
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@arm.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@arm.com>
> >> > Link: https://lore.kernel.org/r/20240212123233.1230090-2-cristian.marussi@arm.com
> >> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] [NOT_FOR_MERGE] firmware: arm_scmi: Optimize notifiers registration
2025-06-13 9:50 ` Peng Fan
2025-06-17 13:41 ` Cristian Marussi
@ 2025-06-17 13:50 ` Cristian Marussi
2025-06-17 13:59 ` Cristian Marussi
2025-06-20 7:38 ` Peng Fan
1 sibling, 2 replies; 13+ messages in thread
From: Cristian Marussi @ 2025-06-17 13:50 UTC (permalink / raw)
To: peng.fan
Cc: arm-scmi, cristian.marussi, linux-arm-kernel, linux-kernel,
peng.fan, sudeep.holla
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@arm.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 ?)
---
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] [NOT_FOR_MERGE] firmware: arm_scmi: Optimize notifiers registration
2025-06-17 13:50 ` [PATCH] [NOT_FOR_MERGE] firmware: arm_scmi: Optimize notifiers registration Cristian Marussi
@ 2025-06-17 13:59 ` Cristian Marussi
2025-06-20 7:38 ` Peng Fan
1 sibling, 0 replies; 13+ messages in thread
From: Cristian Marussi @ 2025-06-17 13:59 UTC (permalink / raw)
To: peng.fan
Cc: arm-scmi, cristian.marussi, linux-arm-kernel, linux-kernel,
peng.fan, sudeep.holla
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.
>
... btw....not sure even if all of this is worth just to cut down on
needless computation...maybe just reviewing the noise level of the
emitted error message could be enough.
Thanks,
Cristian
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] [NOT_FOR_MERGE] firmware: arm_scmi: Optimize notifiers registration
2025-06-17 13:50 ` [PATCH] [NOT_FOR_MERGE] firmware: arm_scmi: Optimize notifiers registration Cristian Marussi
2025-06-17 13:59 ` Cristian Marussi
@ 2025-06-20 7:38 ` Peng Fan
1 sibling, 0 replies; 13+ messages in thread
From: Peng Fan @ 2025-06-20 7:38 UTC (permalink / raw)
To: Cristian Marussi
Cc: arm-scmi, linux-arm-kernel, linux-kernel, peng.fan, sudeep.holla
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@arm.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
>
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2025-06-20 6:28 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-11 7:52 [PATCH 0/3] firmware: arm_scmi: perf/cpufreq: Enable notification only if supported by platform Peng Fan (OSS)
2025-06-11 7:52 ` [PATCH 1/3] firmware: arm_scmi: Fix typo for scmi_perf_proto_ops Peng Fan (OSS)
2025-06-11 7:52 ` [PATCH 2/3] firmware: arm_scmi: perf: Add notify_supported " Peng Fan (OSS)
2025-06-11 7:52 ` [PATCH 3/3] cpufreq: scmi-cpufreq: Enable perf limits notification only supported Peng Fan (OSS)
2025-06-11 12:17 ` [PATCH 0/3] firmware: arm_scmi: perf/cpufreq: Enable notification only if supported by platform Sudeep Holla
2025-06-11 13:33 ` Cristian Marussi
2025-06-12 3:43 ` Peng Fan
2025-06-12 10:31 ` Cristian Marussi
2025-06-13 9:50 ` Peng Fan
2025-06-17 13:41 ` Cristian Marussi
2025-06-17 13:50 ` [PATCH] [NOT_FOR_MERGE] firmware: arm_scmi: Optimize notifiers registration Cristian Marussi
2025-06-17 13:59 ` Cristian Marussi
2025-06-20 7:38 ` Peng Fan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).