| 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: <DU0PR04MB9417977827068BFD4903486988DDA@DU0PR04MB9417.eurprd04.prod.outlook.com>
Date: Thu, 26 Oct 2023 12:20:34 +0000
From: Peng Fan <peng.fan@....com>
To: Cristian Marussi <cristian.marussi@....com>,
"Peng Fan (OSS)" <peng.fan@....nxp.com>
CC: "sudeep.holla@....com" <sudeep.holla@....com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Ranjani Vaidyanathan <ranjani.vaidyanathan@....com>,
Glen G Wienecke <glen.wienecke@....com>,
Nitin Garg <nitin.garg_3@....com>,
Chuck Cannon <chuck.cannon@....com>,
"sboyd@...nel.org" <sboyd@...nel.org>,
Souvik Chakravarty <Souvik.Chakravarty@....com>
Subject: RE: [RFC] firmware: arm_scmi: support clock denied attributes
Hi Cristian
> 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.
Yes, except enable/disable, rate/parent both return DENY.
>
> Does this solve your usecase where your other drivers (callers) were failing to
> probe due to such error being reported from the server ?
We removed the assigned-clock-rates and assigned-clock-parents from device
tree nodes to avoid failure.
The peripheral driver call clk_enable/disable, since it return success, so issue
resolved.
>
> 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 ?
See above, removed the assigned-clock-parents and assigned-clock-rates.
The scmi firmware configured the parent and rate ready.
> 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...)
The last RFC was to register clocks as fixed clock, so no parent/rate call needed.
But actually we may need get new rate because the scmi firmware could
update rate per device performance level setting.
>
> Does this work with upstream drivers, or just with some downstream solution
> properly crafted to handle the EACCESS ?
The patch tested in downstream repo, 6.6.
>
> 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.
Thanks for your suggestions, I will give a look and prepare a new version
patch.
Thanks,
Peng.
>
> 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