linux-kernel - Re: [PATCH] cpufreq: scmi: Add quirk to disable checks in scmi_dev_used_by

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <aJ7rBgce5eWSkkk3@pluto>
Date: Fri, 15 Aug 2025 09:08:38 +0100
From: Cristian Marussi <cristian.marussi@....com>
To: Florian Fainelli <florian.fainelli@...adcom.com>
Cc: linux-kernel@...r.kernel.org, james.quinlan@...adcom.com,
	Cristian Marussi <cristian.marussi@....com>,
	Sudeep Holla <sudeep.holla@....com>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Viresh Kumar <viresh.kumar@...aro.org>, Peng Fan <peng.fan@....com>,
	Mike Tipton <quic_mdtipton@...cinc.com>, arm-scmi@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH] cpufreq: scmi: Add quirk to disable checks in
 scmi_dev_used_by_cpus()

On Thu, Aug 14, 2025 at 03:51:55PM -0700, Florian Fainelli wrote:
> Broadcom STB platforms were early adopters of the SCMI framework and as
> a result, not all deployed systems have a Device Tree entry where SCMI
> protocol 0x13 (PERFORMANCE) is declared as a clock provider, nor are the
> CPU Device Tree node(s) referencing protocol 0x13 as their clock
> provider.
> 
> Leverage the quirks framework recently introduce to match on the
> Broadcom SCMI vendor and in that case, disable the Device Tree
> properties checks being done by scmi_dev_used_by_cpus().
> 

Hi,

> Suggested-by: Cristian Marussi <cristian.marussi@....com>
> Fixes: 6c9bb8692272 ("cpufreq: scmi: Skip SCMI devices that aren't used by the CPUs")
> Signed-off-by: Florian Fainelli <florian.fainelli@...adcom.com>
> ---
>  drivers/cpufreq/scmi-cpufreq.c     | 13 +++++++++++++
>  drivers/firmware/arm_scmi/quirks.c |  2 ++
>  drivers/firmware/arm_scmi/quirks.h |  1 +
>  3 files changed, 16 insertions(+)
> 
> diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
> index ef078426bfd5..80647511d3c3 100644
> --- a/drivers/cpufreq/scmi-cpufreq.c
> +++ b/drivers/cpufreq/scmi-cpufreq.c
> @@ -22,6 +22,8 @@
>  #include <linux/types.h>
>  #include <linux/units.h>
>  
> +#include "../drivers/firmware/arm_scmi/quirks.h"
> +

I will post a patch to move this header up to avoid the uglyness of this
include....

>  struct scmi_data {
>  	int domain_id;
>  	int nr_opp;
> @@ -34,6 +36,7 @@ struct scmi_data {
>  static struct scmi_protocol_handle *ph;
>  static const struct scmi_perf_proto_ops *perf_ops;
>  static struct cpufreq_driver scmi_cpufreq_driver;
> +static bool __maybe_unused scmi_cpufreq_dt_props_check_disable;
>  

Not sure why you introduce an intermediate global bool to check...this
defeats a bit the whole idea of the quirks framework which is based on
static_keys and is supposed to be mostly transarent when quirks are not
enabled....

Couldn't you just move the quirk inside the get_rate ?
(maybe I am missing something around compiler behaviours..)
 
#define QUIRK_SCMI_CPUFREQ_CHECK_DT_PROPS		\
({							\
	if (true)					\
		return true;				\
})

>  static unsigned int scmi_cpufreq_get_rate(unsigned int cpu)
>  {
> @@ -400,6 +403,9 @@ static bool scmi_dev_used_by_cpus(struct device *scmi_dev)
>  	struct device *cpu_dev;
>  	int cpu, idx;
>  

+	SCMI_QUIRK(scmi_cpufreq_no_check_dt_props, QUIRK_SCMI_CPUFREQ_CHECK_DT_PROPS);

>  	if (!scmi_np)
>  		return false;
>  
> @@ -427,12 +433,19 @@ static bool scmi_dev_used_by_cpus(struct device *scmi_dev)
>  	return false;
>  }
>  
> +#define QUIRK_SCMI_CPUFREQ_CHECK_DT_PROPS			\
> +	({							\
> +		scmi_cpufreq_dt_props_check_disable = true;	\
> +	})
> +
>  static int scmi_cpufreq_probe(struct scmi_device *sdev)
>  {
>  	int ret;
>  	struct device *dev = &sdev->dev;
>  	const struct scmi_handle *handle;
>  

> +	SCMI_QUIRK(scmi_cpufreq_no_check_dt_props, QUIRK_SCMI_CPUFREQ_CHECK_DT_PROPS);
> +

...removing this of course

>  	handle = sdev->handle;
>  
>  	if (!handle || !scmi_dev_used_by_cpus(dev))
> diff --git a/drivers/firmware/arm_scmi/quirks.c b/drivers/firmware/arm_scmi/quirks.c
> index 03960aca3610..aafc7b4b3294 100644
> --- a/drivers/firmware/arm_scmi/quirks.c
> +++ b/drivers/firmware/arm_scmi/quirks.c
> @@ -171,6 +171,7 @@ struct scmi_quirk {
>  /* Global Quirks Definitions */
>  DEFINE_SCMI_QUIRK(clock_rates_triplet_out_of_spec, NULL, NULL, NULL);
>  DEFINE_SCMI_QUIRK(perf_level_get_fc_force, "Qualcomm", NULL, "0x20000-");
> +DEFINE_SCMI_QUIRK_EXPORTED(scmi_cpufreq_no_check_dt_props, "brcm-scmi", NULL, "0x2");

Also, are you sure about using version as "0x2" ? That is supposed to
indicate the (optional) SCMI FW Version to which this quirk will
apply...and with that it means whatever FW versioning you use in
Broadcom to identify build versions....it is NOT the SCMI Protocol
Version, so that also means that if/when you will change the advertised
version, this quirk wont apply anymore...or equally if there are older
version than 0x2 that are buggy they wont be quirked...

One more doubt I have (despite me having suggested this solution) is
that here you are quirking against a malformed deployed DT really,
not against some SCMI FW anomaly in the Broadcom FW, but using the
SCMI Quirks framework you are tying the quirk to the SCMI FW Vendor
and maybe some specific SCMI FW Version....

...so what will happen when you will update/fix your DT in the future ?
Will you also take care to bump the BRCM SCMI FW version to disable the
quirk in the DT deployed by your FW binary ?

Thanks,
Cristian