| 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
| ||
|
Message-ID: <aKMob3SLE8AofNHw@pluto>
Date: Mon, 18 Aug 2025 14:19:43 +0100
From: Cristian Marussi <cristian.marussi@....com>
To: Dhruva Gole <d-gole@...com>
Cc: Cristian Marussi <cristian.marussi@....com>,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
arm-scmi@...r.kernel.org, linux-pm@...r.kernel.org,
sudeep.holla@....com, james.quinlan@...adcom.com,
f.fainelli@...il.com, vincent.guittot@...aro.org,
quic_sibis@...cinc.com, dan.carpenter@...aro.org,
johan+linaro@...nel.org, rafael@...nel.org, viresh.kumar@...aro.org,
quic_mdtipton@...cinc.com
Subject: Re: [PATCH 1/2] firmware: arm_scmi: Rework quirks framework header
On Mon, Aug 18, 2025 at 11:05:35AM +0530, Dhruva Gole wrote:
> Hi,
>
> On Aug 15, 2025 at 11:27:35 +0100, Cristian Marussi wrote:
Ho Dhruva,
> > Split and relocate the quirks framework header so as to be usable also by
> > SCMI Drivers and not only by the core.
>
> Could you elaborate a bit more on this reasoning? I am not fully
> convinced as to why I shouldn't just include quirks.h in the other scmi
> drivers as well?
>
You can include quirks.h directly BUT you will have to use an ugly
#include ../drivers/firmware/arm_scmi/quirks.h
...AND also you will endup exposing a couple of internal functions used
by the quirk framework like:
void scmi_quirks_initialize(void);
void scmi_quirks_enable(struct device *dev, const char *vend,
const char *subv, const u32 impl);
..so I split out the interface needed for quirking and relocated the
file to include/quirks
> Oh or perhaps you mean to say scmi driver like scmi cpufreq / clk-scmi
> etc.. which lie outside the firmware/arm_scmi folder? If so then that's
> not coming out clearly in this patch commit message.
Yes when I say SCMI drivers I mean general drivers that use the SCMI
stack BUT that are not part of the SCMI core....basically those drivers
that use the API in include/linux/scmi_protocol.h
Anyway...it seems like the only user of quirks in the SCMI drivers has
just disappeared...so maybe this small patch can just wait...
>
> >
> > Signed-off-by: Cristian Marussi <cristian.marussi@....com>
> > ---
> > drivers/firmware/arm_scmi/clock.c | 2 +-
> > drivers/firmware/arm_scmi/driver.c | 1 +
> > drivers/firmware/arm_scmi/quirks.h | 33 +++-------------------
> > include/linux/scmi_quirks.h | 44 ++++++++++++++++++++++++++++++
> > 4 files changed, 50 insertions(+), 30 deletions(-)
> > create mode 100644 include/linux/scmi_quirks.h
> >
> > diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> > index afa7981efe82..5599697de37a 100644
> > --- a/drivers/firmware/arm_scmi/clock.c
> > +++ b/drivers/firmware/arm_scmi/clock.c
> > @@ -7,11 +7,11 @@
> >
> > #include <linux/module.h>
> > #include <linux/limits.h>
> > +#include <linux/scmi_quirks.h>
> > #include <linux/sort.h>
> >
> > #include "protocols.h"
> > #include "notify.h"
> > -#include "quirks.h"
> >
> > /* Updated only after ALL the mandatory features for that version are merged */
> > #define SCMI_PROTOCOL_SUPPORTED_VERSION 0x30000
> > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > index bd56a877fdfc..6f5934cd3a65 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> > @@ -34,6 +34,7 @@
> > #include <linux/processor.h>
> > #include <linux/refcount.h>
> > #include <linux/slab.h>
> > +#include <linux/scmi_quirks.h>
> > #include <linux/xarray.h>
> >
> > #include "common.h"
> > diff --git a/drivers/firmware/arm_scmi/quirks.h b/drivers/firmware/arm_scmi/quirks.h
> > index a71fde85a527..260ae38d617b 100644
> > --- a/drivers/firmware/arm_scmi/quirks.h
> > +++ b/drivers/firmware/arm_scmi/quirks.h
> > @@ -4,49 +4,24 @@
> > *
> > * Copyright (C) 2025 ARM Ltd.
> > */
> > -#ifndef _SCMI_QUIRKS_H
> > -#define _SCMI_QUIRKS_H
> > +#ifndef _SCMI_QUIRKS_INTERNAL_H
> > +#define _SCMI_QUIRKS_INTERNAL_H
>
> Or as per your commit message wording, better to call it
> _SCMI_QUIRKS_CORE_H ?
>
...well mayeb yes....I have not bother to change the filename
anyway ....
> >
> > -#include <linux/static_key.h>
> > +#include <linux/device.h>
> > #include <linux/types.h>
> >
> > #ifdef CONFIG_ARM_SCMI_QUIRKS
> >
> > -#define DECLARE_SCMI_QUIRK(_qn) \
> > - DECLARE_STATIC_KEY_FALSE(scmi_quirk_ ## _qn)
> > -
> > -/*
> > - * A helper to associate the actual code snippet to use as a quirk
> > - * named as _qn.
> > - */
> > -#define SCMI_QUIRK(_qn, _blk) \
> > - do { \
> > - if (static_branch_unlikely(&(scmi_quirk_ ## _qn))) \
> > - (_blk); \
> > - } while (0)
> > -
> > void scmi_quirks_initialize(void);
> > void scmi_quirks_enable(struct device *dev, const char *vend,
> > const char *subv, const u32 impl);
> >
> > #else
> >
> > -#define DECLARE_SCMI_QUIRK(_qn)
> > -/* Force quirks compilation even when SCMI Quirks are disabled */
> > -#define SCMI_QUIRK(_qn, _blk) \
> > - do { \
> > - if (0) \
> > - (_blk); \
> > - } while (0)
> > -
> > static inline void scmi_quirks_initialize(void) { }
> > static inline void scmi_quirks_enable(struct device *dev, const char *vend,
> > const char *sub_vend, const u32 impl) { }
> >
> > #endif /* CONFIG_ARM_SCMI_QUIRKS */
> >
> > -/* Quirk delarations */
> > -DECLARE_SCMI_QUIRK(clock_rates_triplet_out_of_spec);
> > -DECLARE_SCMI_QUIRK(perf_level_get_fc_force);
> > -
> > -#endif /* _SCMI_QUIRKS_H */
> > +#endif /* _SCMI_QUIRKS_INTERNAL_H */
> > diff --git a/include/linux/scmi_quirks.h b/include/linux/scmi_quirks.h
> > new file mode 100644
> > index 000000000000..11657bd91ffc
> > --- /dev/null
> > +++ b/include/linux/scmi_quirks.h
> > @@ -0,0 +1,44 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * System Control and Management Interface (SCMI) Message Protocol Quirks
> > + *
> > + * Copyright (C) 2025 ARM Ltd.
> > + */
> > +#ifndef _SCMI_QUIRKS_H
> > +#define _SCMI_QUIRKS_H
> > +
> > +#include <linux/static_key.h>
> > +#include <linux/types.h>
> > +
> > +#ifdef CONFIG_ARM_SCMI_QUIRKS
> > +
> > +#define DECLARE_SCMI_QUIRK(_qn) \
> > + DECLARE_STATIC_KEY_FALSE(scmi_quirk_ ## _qn)
> > +
> > +/*
> > + * A helper to associate the actual code snippet to use as a quirk
> > + * named as _qn.
> > + */
> > +#define SCMI_QUIRK(_qn, _blk) \
> > + do { \
> > + if (static_branch_unlikely(&(scmi_quirk_ ## _qn))) \
> > + (_blk); \
> > + } while (0)
> > +
> > +#else
> > +
> > +#define DECLARE_SCMI_QUIRK(_qn)
> > +/* Force quirks compilation even when SCMI Quirks are disabled */
> > +#define SCMI_QUIRK(_qn, _blk) \
> > + do { \
> > + if (0) \
> > + (_blk); \
> > + } while (0)
>
> Did you happen to run checkpatch on this?
> 8<---------------------------------------------------------------------------
> WARNING: Argument '_qn' is not used in function-like macro
> #142: FILE: include/linux/scmi_quirks.h:32:
> +#define SCMI_QUIRK(_qn, _blk) \
> + do { \
> + if (0) \
> + (_blk); \
> + } while (0)
>
> total: 0 errors, 2 warnings, 0 checks, 116 lines checked
> --------------------------------------------------------------------------->8
>
Oh yes I did on all the series...BUT this specific error on this branch
of the #if was already present in the original patch that added the
Quirk framework...I did not know how to avoid this warning and given it
is pretty much harmless I just ignored it...
>
> > +
> > +#endif /* CONFIG_ARM_SCMI_QUIRKS */
> > +
> > +/* Quirk delarations */
> > +DECLARE_SCMI_QUIRK(clock_rates_triplet_out_of_spec);
> > +DECLARE_SCMI_QUIRK(perf_level_get_fc_force);
> > +
> > +#endif /* _SCMI_QUIRKS_H */
> > --
> > 2.50.1
> >
>
Thanks for the review.
Cristian
Powered by blists - more mailing lists