| 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: <ZTpCAgUxjbVnFEkB@pluto>
Date: Thu, 26 Oct 2023 11:40:02 +0100
From: Cristian Marussi <cristian.marussi@....com>
To: "Peng Fan (OSS)" <peng.fan@....nxp.com>
Cc: sudeep.holla@....com, linux-arm-kernel@...ts.infradead.org,
linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org,
ranjani.vaidyanathan@....com, glen.wienecke@....com,
nitin.garg@....com, chuck.cannon@....com, sboyd@...nel.org,
Peng Fan <peng.fan@....com>,
Souvik Chakravarty <Souvik.Chakravarty@....com>
Subject: Re: [RFC] firmware: arm_scmi: support clock denied attributes
On Thu, Oct 26, 2023 at 11:41:25AM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@....com>
>
> This is not in SPEC and is just defined by NXP. This patch is
> to start the discussion to start include them in SPEC.
>
> Signed-off-by: Peng Fan <peng.fan@....com>
> ---
Hi Peng,
thanks for validating this scenario.
[CC Souvik]
So at the end, you are returning -EACCESS anyway, it is just that you
are avoiding to send any SCMI message at all if the flag reports that
you cannot touch this and you will get back a DENY.
Does this solve your usecase where your other drivers (callers) were
failing to probe due to such error being reported from the server ?
>From our offline discussions my understanding was that, beside un-needed
SCMI msg exchanges, your main issue was receiving a DENY error from the
server when trying to modify a clock, how does this solves that ?
Basically You are just returning the same error from the clk driver,
just avoiding (rightly so) needless SCMI exchanges.
(..in your last RFC patch you attempt was indeed different, to refrain
from registering any clk framework callbacks at all for untocuhable
clocks...)
Does this work with upstream drivers, or just with some downstream
solution properly crafted to handle the EACCESS ?
Anyway IMO, these changes in this scenario are certainly valuable in
general since they avoid needless exchanges with the server around
clocks that we know upfront we cannot touch.
BUT, hvaing said that, if this series goes further as it is and the
spec is changed accordingly, please move all of this logic inside the
protocol layer: there is no reason to change the clk-scmi driver at
all for this.
You can just check that same new flags (that you set in clk discovery)
upfront inside the related clock operations in drivers/firmware/arm/clock.c
and just return EACCESS from there avoiding any SCMI exchanges if the
flags are set.
This way you dont either need to re-define and expose such new flags in
scmi_protocol.h, it can just all handled inside the SCMI protocol layer.
Thank,
Cristian
> drivers/clk/clk-scmi.c | 39 +++++++++++++++++++++++++------
> drivers/firmware/arm_scmi/clock.c | 9 +++++++
> include/linux/scmi_protocol.h | 4 ++++
> 3 files changed, 45 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
> index 8cbe24789c24..303f8a8ec8e0 100644
> --- a/drivers/clk/clk-scmi.c
> +++ b/drivers/clk/clk-scmi.c
> @@ -75,15 +75,13 @@ static int scmi_clk_set_rate(struct clk_hw *hw, unsigned long rate,
> unsigned long parent_rate)
> {
> struct scmi_clk *clk = to_scmi_clk(hw);
> + const struct scmi_clock_info *info = clk->info;
> + u64 rate1 = 0;
>
> - return scmi_proto_clk_ops->rate_set(clk->ph, clk->id, rate);
> -}
> -
> -static int scmi_clk_set_parent(struct clk_hw *hw, u8 parent_index)
> -{
> - struct scmi_clk *clk = to_scmi_clk(hw);
> + if (info->flags & SCMI_CLOCK_SET_RATE_DENIED)
> + return -EACCES;
>
> - return scmi_proto_clk_ops->parent_set(clk->ph, clk->id, parent_index);
> + return scmi_proto_clk_ops->rate_set(clk->ph, clk->id, rate);
> }
>
> static u8 scmi_clk_get_parent(struct clk_hw *hw)
> @@ -107,6 +105,17 @@ static u8 scmi_clk_get_parent(struct clk_hw *hw)
> return p_idx;
> }
>
> +static int scmi_clk_set_parent(struct clk_hw *hw, u8 parent_index)
> +{
> + struct scmi_clk *clk = to_scmi_clk(hw);
> + const struct scmi_clock_info *info = clk->info;
> +
> + if (info->flags & SCMI_CLOCK_SET_ENABLE_DENIED)
> + return -EACCES;
> +
> + return scmi_proto_clk_ops->parent_set(clk->ph, clk->id, parent_index);
> +}
> +
> static int scmi_clk_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
> {
> /*
> @@ -119,6 +128,10 @@ static int scmi_clk_determine_rate(struct clk_hw *hw, struct clk_rate_request *r
> static int scmi_clk_enable(struct clk_hw *hw)
> {
> struct scmi_clk *clk = to_scmi_clk(hw);
> + const struct scmi_clock_info *info = clk->info;
> +
> + if (info->flags & SCMI_CLOCK_SET_ENABLE_DENIED)
> + return 0;
>
> return scmi_proto_clk_ops->enable(clk->ph, clk->id, NOT_ATOMIC);
> }
> @@ -126,6 +139,10 @@ static int scmi_clk_enable(struct clk_hw *hw)
> static void scmi_clk_disable(struct clk_hw *hw)
> {
> struct scmi_clk *clk = to_scmi_clk(hw);
> + const struct scmi_clock_info *info = clk->info;
> +
> + if (info->flags & SCMI_CLOCK_SET_ENABLE_DENIED)
> + return;
>
> scmi_proto_clk_ops->disable(clk->ph, clk->id, NOT_ATOMIC);
> }
> @@ -133,6 +150,10 @@ static void scmi_clk_disable(struct clk_hw *hw)
> static int scmi_clk_atomic_enable(struct clk_hw *hw)
> {
> struct scmi_clk *clk = to_scmi_clk(hw);
> + const struct scmi_clock_info *info = clk->info;
> +
> + if (info->flags & SCMI_CLOCK_SET_ENABLE_DENIED)
> + return 0;
>
> return scmi_proto_clk_ops->enable(clk->ph, clk->id, ATOMIC);
> }
> @@ -140,6 +161,10 @@ static int scmi_clk_atomic_enable(struct clk_hw *hw)
> static void scmi_clk_atomic_disable(struct clk_hw *hw)
> {
> struct scmi_clk *clk = to_scmi_clk(hw);
> + const struct scmi_clock_info *info = clk->info;
> +
> + if (info->flags & SCMI_CLOCK_SET_ENABLE_DENIED)
> + return;
>
> scmi_proto_clk_ops->disable(clk->ph, clk->id, ATOMIC);
> }
> diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> index 42b81c181d68..1a62e3b82d34 100644
> --- a/drivers/firmware/arm_scmi/clock.c
> +++ b/drivers/firmware/arm_scmi/clock.c
> @@ -46,6 +46,9 @@ struct scmi_msg_resp_clock_attributes {
> #define SUPPORTS_RATE_CHANGE_REQUESTED_NOTIF(x) ((x) & BIT(30))
> #define SUPPORTS_EXTENDED_NAMES(x) ((x) & BIT(29))
> #define SUPPORTS_PARENT_CLOCK(x) ((x) & BIT(28))
> +#define SETS_ENABLE_DENIED(x) ((x) & BIT(15))
> +#define SETS_RATE_DENIED(x) ((x) & BIT(14))
> +#define SETS_PARENT_DENIED(x) ((x) & BIT(13))
> u8 name[SCMI_SHORT_NAME_MAX_SIZE];
> __le32 clock_enable_latency;
> };
> @@ -327,6 +330,12 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
> clk->rate_change_requested_notifications = true;
> if (SUPPORTS_PARENT_CLOCK(attributes))
> scmi_clock_possible_parents(ph, clk_id, clk);
> + if (SETS_PARENT_DENIED(attributes))
> + clk->flags |= SCMI_CLOCK_SET_PARENT_DENIED;
> + if (SETS_RATE_DENIED(attributes))
> + clk->flags |= SCMI_CLOCK_SET_RATE_DENIED;
> + if (SETS_ENABLE_DENIED(attributes))
> + clk->flags |= SCMI_CLOCK_SET_ENABLE_DENIED;
> }
>
> return ret;
> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> index f2f05fb42d28..71911dcd8117 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -41,12 +41,16 @@ struct scmi_revision_info {
> char sub_vendor_id[SCMI_SHORT_NAME_MAX_SIZE];
> };
>
> +#define SCMI_CLOCK_SET_PARENT_DENIED BIT(13)
> +#define SCMI_CLOCK_SET_RATE_DENIED BIT(14)
> +#define SCMI_CLOCK_SET_ENABLE_DENIED BIT(15)
> struct scmi_clock_info {
> char name[SCMI_MAX_STR_SIZE];
> unsigned int enable_latency;
> bool rate_discrete;
> bool rate_changed_notifications;
> bool rate_change_requested_notifications;
> + unsigned int flags;
> union {
> struct {
> int num_rates;
> --
> 2.37.1
>
Powered by blists - more mailing lists