From: Peng Fan <[email protected]>
There are clocks:
system critical, not allow linux to disable, change rate
allow linux to get rate, because some periphals will use the frequency
to configure periphals.
So introduce an attribute to indicated FIXED clock
Signed-off-by: Peng Fan <[email protected]>
---
drivers/clk/clk-scmi.c | 6 ++++++
drivers/firmware/arm_scmi/clock.c | 5 ++++-
include/linux/scmi_protocol.h | 1 +
3 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
index 8cbe24789c24..a539a35bd45a 100644
--- a/drivers/clk/clk-scmi.c
+++ b/drivers/clk/clk-scmi.c
@@ -182,6 +182,10 @@ static const struct clk_ops scmi_clk_ops = {
.determine_rate = scmi_clk_determine_rate,
};
+static const struct clk_ops scmi_fixed_rate_clk_ops = {
+ .recalc_rate = scmi_clk_recalc_rate,
+};
+
static const struct clk_ops scmi_atomic_clk_ops = {
.recalc_rate = scmi_clk_recalc_rate,
.round_rate = scmi_clk_round_rate,
@@ -293,6 +297,8 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
if (is_atomic &&
sclk->info->enable_latency <= atomic_threshold)
scmi_ops = &scmi_atomic_clk_ops;
+ else if (sclk->info->rate_fixed)
+ scmi_ops = &scmi_fixed_rate_clk_ops;
else
scmi_ops = &scmi_clk_ops;
diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index ddaef34cd88b..8c52db539e54 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -46,6 +46,7 @@ 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 SUPPORTS_FIXED_RATE_CLOCK(x) ((x) & BIT(27))
u8 name[SCMI_SHORT_NAME_MAX_SIZE];
__le32 clock_enable_latency;
};
@@ -326,7 +327,9 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
clk->rate_changed_notifications = true;
if (SUPPORTS_RATE_CHANGE_REQUESTED_NOTIF(attributes))
clk->rate_change_requested_notifications = true;
- if (SUPPORTS_PARENT_CLOCK(attributes))
+ if (SUPPORTS_FIXED_RATE_CLOCK(attributes))
+ clk->rate_fixed = true;
+ else if (SUPPORTS_PARENT_CLOCK(attributes))
scmi_clock_possible_parents(ph, clk_id, clk);
}
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index f2f05fb42d28..e068004c151a 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -47,6 +47,7 @@ struct scmi_clock_info {
bool rate_discrete;
bool rate_changed_notifications;
bool rate_change_requested_notifications;
+ bool rate_fixed;
union {
struct {
int num_rates;
--
2.37.1
On Tue, Oct 10, 2023 at 10:29:11AM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <[email protected]>
>
> There are clocks:
> system critical, not allow linux to disable, change rate
> allow linux to get rate, because some periphals will use the frequency
> to configure periphals.
>
> So introduce an attribute to indicated FIXED clock
>
Hi,
(CCed [email protected])
so AFAIU here you are describing a clock that really is NOT fixed in
general, it is just that the Linux SCMI Agent cannot touch it, but
other SCMI agents on the system CAN change it and so, on one side, you
keep the ability for the Linux agent to read back the current rate with
recalc_rate() and remove all the Clk frameworks callbacks needed to modify
its state, am I right ?
In this scenario, it is really the SCMI platform fw (server) that has to
implement the checks and simply DENY the requests coming from an agent that
is not supposed to touch that clock, while allowing the current rate to
be retrieved.
JUNO/SCP is an example of how the CPUs clocks are visible to Linux BUT
cannot be touched directly via Clock protocol by Linux since in the SCMI
world you are supposed to use the Perf protocol instead to change the OPPs
when you want to modify the performance level of the runnning CPU.
This kind of server-side permissions checks, meant to filter access to
resources based on the requesting agent, are part of the SCMI declared
aim to push the responsibility of such controls out of the kernel into the
platform fw in order to avoid attacks like CLOCK_SCREW by letting the
SCMI firmware be the one and only final arbiter on the requests coming
from the agents; you can ask teh server whatever you like as an agent but
your request can be DENIED or silently ignored (in case of shared resources)
at the will of the platform which has the final say and it is
implemented in a physically distinct code-base.
It seems to me that this patch and the possible associated SCMI specification
change would give back the control to the Linux agent and could allow the
implementation of an SCMI Server that does NOT perform any of these
permission checks.
So, IMO, while this change, on one side, could be certainly useful by
removing a bunch of unused/uneeded callbacks from the CLK SCMI driver when
a fixed clock is identified, it could open the door to a bad implementation
like the one mentioned above which does NOT perform any agent-based
permission check.
Maybe Sudeep or Souvik think differently.
Thanks,
Cristian
> Signed-off-by: Peng Fan <[email protected]>
> ---
> drivers/clk/clk-scmi.c | 6 ++++++
> drivers/firmware/arm_scmi/clock.c | 5 ++++-
> include/linux/scmi_protocol.h | 1 +
> 3 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
> index 8cbe24789c24..a539a35bd45a 100644
> --- a/drivers/clk/clk-scmi.c
> +++ b/drivers/clk/clk-scmi.c
> @@ -182,6 +182,10 @@ static const struct clk_ops scmi_clk_ops = {
> .determine_rate = scmi_clk_determine_rate,
> };
>
> +static const struct clk_ops scmi_fixed_rate_clk_ops = {
> + .recalc_rate = scmi_clk_recalc_rate,
> +};
> +
> static const struct clk_ops scmi_atomic_clk_ops = {
> .recalc_rate = scmi_clk_recalc_rate,
> .round_rate = scmi_clk_round_rate,
> @@ -293,6 +297,8 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
> if (is_atomic &&
> sclk->info->enable_latency <= atomic_threshold)
> scmi_ops = &scmi_atomic_clk_ops;
> + else if (sclk->info->rate_fixed)
> + scmi_ops = &scmi_fixed_rate_clk_ops;
> else
> scmi_ops = &scmi_clk_ops;
>
> diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> index ddaef34cd88b..8c52db539e54 100644
> --- a/drivers/firmware/arm_scmi/clock.c
> +++ b/drivers/firmware/arm_scmi/clock.c
> @@ -46,6 +46,7 @@ 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 SUPPORTS_FIXED_RATE_CLOCK(x) ((x) & BIT(27))
> u8 name[SCMI_SHORT_NAME_MAX_SIZE];
> __le32 clock_enable_latency;
> };
> @@ -326,7 +327,9 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
> clk->rate_changed_notifications = true;
> if (SUPPORTS_RATE_CHANGE_REQUESTED_NOTIF(attributes))
> clk->rate_change_requested_notifications = true;
> - if (SUPPORTS_PARENT_CLOCK(attributes))
> + if (SUPPORTS_FIXED_RATE_CLOCK(attributes))
> + clk->rate_fixed = true;
> + else if (SUPPORTS_PARENT_CLOCK(attributes))
> scmi_clock_possible_parents(ph, clk_id, clk);
> }
>
> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> index f2f05fb42d28..e068004c151a 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -47,6 +47,7 @@ struct scmi_clock_info {
> bool rate_discrete;
> bool rate_changed_notifications;
> bool rate_change_requested_notifications;
> + bool rate_fixed;
> union {
> struct {
> int num_rates;
> --
> 2.37.1
>
> Subject: Re: [RFC] firmware: arm_scmi: clock: add fixed clock attribute
> support
>
> On Tue, Oct 10, 2023 at 10:29:11AM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <[email protected]>
> >
> > There are clocks:
> > system critical, not allow linux to disable, change rate allow linux
> > to get rate, because some periphals will use the frequency to
> > configure periphals.
> >
> > So introduce an attribute to indicated FIXED clock
> >
>
> Hi,
>
> (CCed [email protected])
>
> so AFAIU here you are describing a clock that really is NOT fixed in general, it
> is just that the Linux SCMI Agent cannot touch it, but other SCMI agents on
> the system CAN change it and so, on one side, you keep the ability for the
> Linux agent to read back the current rate with
> recalc_rate() and remove all the Clk frameworks callbacks needed to modify
> its state, am I right ?
Right.
>
> In this scenario, it is really the SCMI platform fw (server) that has to
> implement the checks and simply DENY the requests coming from an agent
> that is not supposed to touch that clock, while allowing the current rate to be
> retrieved.
Linux will try to enable, get rate, runtime disable the clock.
But the server does not allow enable/disable the clock, so the driver probe
will fail.
The SCMI server could bypass enable/disable and only allow get rate,
But this introduces heavy RPC, so just wanna whether it is ok to register
fixed clock and avoid RPC.
>
> JUNO/SCP is an example of how the CPUs clocks are visible to Linux BUT
> cannot be touched directly via Clock protocol by Linux since in the SCMI
> world you are supposed to use the Perf protocol instead to change the OPPs
> when you want to modify the performance level of the runnning CPU.
>
> This kind of server-side permissions checks, meant to filter access to resources
> based on the requesting agent, are part of the SCMI declared aim to push the
> responsibility of such controls out of the kernel into the platform fw in order
> to avoid attacks like CLOCK_SCREW by letting the SCMI firmware be the one
> and only final arbiter on the requests coming from the agents; you can ask
> teh server whatever you like as an agent but your request can be DENIED or
> silently ignored (in case of shared resources) at the will of the platform which
> has the final say and it is implemented in a physically distinct code-base.
>
> It seems to me that this patch and the possible associated SCMI specification
> change would give back the control to the Linux agent and could allow the
> implementation of an SCMI Server that does NOT perform any of these
> permission checks.
>
> So, IMO, while this change, on one side, could be certainly useful by removing
> a bunch of unused/uneeded callbacks from the CLK SCMI driver when a fixed
> clock is identified, it could open the door to a bad implementation like the
> one mentioned above which does NOT perform any agent-based permission
> check.
Thanks for detailed information, let me check whether our SCMI firmware
could do more on the permission side. But if RPC could be removed,
it could save some time.
Thanks,
Peng.
>
> Maybe Sudeep or Souvik think differently.
>
> Thanks,
> Cristian
>
>
> > Signed-off-by: Peng Fan <[email protected]>
> > ---
> > drivers/clk/clk-scmi.c | 6 ++++++
> > drivers/firmware/arm_scmi/clock.c | 5 ++++-
> > include/linux/scmi_protocol.h | 1 +
> > 3 files changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c index
> > 8cbe24789c24..a539a35bd45a 100644
> > --- a/drivers/clk/clk-scmi.c
> > +++ b/drivers/clk/clk-scmi.c
> > @@ -182,6 +182,10 @@ static const struct clk_ops scmi_clk_ops = {
> > .determine_rate = scmi_clk_determine_rate, };
> >
> > +static const struct clk_ops scmi_fixed_rate_clk_ops = {
> > + .recalc_rate = scmi_clk_recalc_rate, };
> > +
> > static const struct clk_ops scmi_atomic_clk_ops = {
> > .recalc_rate = scmi_clk_recalc_rate,
> > .round_rate = scmi_clk_round_rate,
> > @@ -293,6 +297,8 @@ static int scmi_clocks_probe(struct scmi_device
> *sdev)
> > if (is_atomic &&
> > sclk->info->enable_latency <= atomic_threshold)
> > scmi_ops = &scmi_atomic_clk_ops;
> > + else if (sclk->info->rate_fixed)
> > + scmi_ops = &scmi_fixed_rate_clk_ops;
> > else
> > scmi_ops = &scmi_clk_ops;
> >
> > diff --git a/drivers/firmware/arm_scmi/clock.c
> > b/drivers/firmware/arm_scmi/clock.c
> > index ddaef34cd88b..8c52db539e54 100644
> > --- a/drivers/firmware/arm_scmi/clock.c
> > +++ b/drivers/firmware/arm_scmi/clock.c
> > @@ -46,6 +46,7 @@ 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 SUPPORTS_FIXED_RATE_CLOCK(x) ((x) & BIT(27))
> > u8 name[SCMI_SHORT_NAME_MAX_SIZE];
> > __le32 clock_enable_latency;
> > };
> > @@ -326,7 +327,9 @@ static int scmi_clock_attributes_get(const struct
> scmi_protocol_handle *ph,
> > clk->rate_changed_notifications = true;
> > if (SUPPORTS_RATE_CHANGE_REQUESTED_NOTIF(attributes))
> > clk->rate_change_requested_notifications = true;
> > - if (SUPPORTS_PARENT_CLOCK(attributes))
> > + if (SUPPORTS_FIXED_RATE_CLOCK(attributes))
> > + clk->rate_fixed = true;
> > + else if (SUPPORTS_PARENT_CLOCK(attributes))
> > scmi_clock_possible_parents(ph, clk_id, clk);
> > }
> >
> > diff --git a/include/linux/scmi_protocol.h
> > b/include/linux/scmi_protocol.h index f2f05fb42d28..e068004c151a
> > 100644
> > --- a/include/linux/scmi_protocol.h
> > +++ b/include/linux/scmi_protocol.h
> > @@ -47,6 +47,7 @@ struct scmi_clock_info {
> > bool rate_discrete;
> > bool rate_changed_notifications;
> > bool rate_change_requested_notifications;
> > + bool rate_fixed;
> > union {
> > struct {
> > int num_rates;
> > --
> > 2.37.1
> >
On Tue, Oct 10, 2023 at 08:08:01AM +0000, Peng Fan wrote:
> > Subject: Re: [RFC] firmware: arm_scmi: clock: add fixed clock attribute
> > support
> >
> > On Tue, Oct 10, 2023 at 10:29:11AM +0800, Peng Fan (OSS) wrote:
> > > From: Peng Fan <[email protected]>
> > >
> > > There are clocks:
> > > system critical, not allow linux to disable, change rate allow linux
> > > to get rate, because some periphals will use the frequency to
> > > configure periphals.
> > >
> > > So introduce an attribute to indicated FIXED clock
> > >
> >
> > Hi,
> >
> > (CCed [email protected])
> >
> > so AFAIU here you are describing a clock that really is NOT fixed in general, it
> > is just that the Linux SCMI Agent cannot touch it, but other SCMI agents on
> > the system CAN change it and so, on one side, you keep the ability for the
> > Linux agent to read back the current rate with
> > recalc_rate() and remove all the Clk frameworks callbacks needed to modify
> > its state, am I right ?
>
> Right.
>
> >
> > In this scenario, it is really the SCMI platform fw (server) that has to
> > implement the checks and simply DENY the requests coming from an agent
> > that is not supposed to touch that clock, while allowing the current rate to be
> > retrieved.
>
> Linux will try to enable, get rate, runtime disable the clock.
> But the server does not allow enable/disable the clock, so the driver probe
> will fail.
>
Which driver probe ? I have just double checked and when clk-scmi driver
is loaded there are a bunch of SCMI queries to the server BUT no *_SET
command is issued during the clk-scmi probe; indeed JUNO had never had any
issue despite having access to a bunch of CLKs which are visibile BUT not
modifiable from Linux.
> The SCMI server could bypass enable/disable and only allow get rate,
> But this introduces heavy RPC, so just wanna whether it is ok to register
> fixed clock and avoid RPC.
>
> >
> > JUNO/SCP is an example of how the CPUs clocks are visible to Linux BUT
> > cannot be touched directly via Clock protocol by Linux since in the SCMI
> > world you are supposed to use the Perf protocol instead to change the OPPs
> > when you want to modify the performance level of the runnning CPU.
> >
> > This kind of server-side permissions checks, meant to filter access to resources
> > based on the requesting agent, are part of the SCMI declared aim to push the
> > responsibility of such controls out of the kernel into the platform fw in order
> > to avoid attacks like CLOCK_SCREW by letting the SCMI firmware be the one
> > and only final arbiter on the requests coming from the agents; you can ask
> > teh server whatever you like as an agent but your request can be DENIED or
> > silently ignored (in case of shared resources) at the will of the platform which
> > has the final say and it is implemented in a physically distinct code-base.
> >
> > It seems to me that this patch and the possible associated SCMI specification
> > change would give back the control to the Linux agent and could allow the
> > implementation of an SCMI Server that does NOT perform any of these
> > permission checks.
> >
> > So, IMO, while this change, on one side, could be certainly useful by removing
> > a bunch of unused/uneeded callbacks from the CLK SCMI driver when a fixed
> > clock is identified, it could open the door to a bad implementation like the
> > one mentioned above which does NOT perform any agent-based permission
> > check.
>
> Thanks for detailed information, let me check whether our SCMI firmware
> could do more on the permission side. But if RPC could be removed,
> it could save some time.
>
Avoiding to issue SCMI requests destined to fail would be certainly good, even though
it could lead to just quietly implementing a server with no-checks at
all, but why these unneeded calls happen in first place ?
I have never observed anything like that in my setups.
Thanks,
Cristian
> Subject: Re: [RFC] firmware: arm_scmi: clock: add fixed clock attribute
> support
>
> On Tue, Oct 10, 2023 at 08:08:01AM +0000, Peng Fan wrote:
> > > Subject: Re: [RFC] firmware: arm_scmi: clock: add fixed clock
> > > attribute support
> > >
> > > On Tue, Oct 10, 2023 at 10:29:11AM +0800, Peng Fan (OSS) wrote:
> > > > From: Peng Fan <[email protected]>
> > > >
> > > > There are clocks:
> > > > system critical, not allow linux to disable, change rate allow
> > > > linux to get rate, because some periphals will use the frequency
> > > > to configure periphals.
> > > >
> > > > So introduce an attribute to indicated FIXED clock
> > > >
> > >
> > > Hi,
> > >
> > > (CCed [email protected])
> > >
> > > so AFAIU here you are describing a clock that really is NOT fixed in
> > > general, it is just that the Linux SCMI Agent cannot touch it, but
> > > other SCMI agents on the system CAN change it and so, on one side,
> > > you keep the ability for the Linux agent to read back the current
> > > rate with
> > > recalc_rate() and remove all the Clk frameworks callbacks needed to
> > > modify its state, am I right ?
> >
> > Right.
> >
> > >
> > > In this scenario, it is really the SCMI platform fw (server) that
> > > has to implement the checks and simply DENY the requests coming from
> > > an agent that is not supposed to touch that clock, while allowing
> > > the current rate to be retrieved.
> >
> > Linux will try to enable, get rate, runtime disable the clock.
> > But the server does not allow enable/disable the clock, so the driver
> > probe will fail.
> >
>
> Which driver probe ? I have just double checked and when clk-scmi driver is
> loaded there are a bunch of SCMI queries to the server BUT no *_SET
> command is issued during the clk-scmi probe; indeed JUNO had never had
> any issue despite having access to a bunch of CLKs which are visibile BUT not
> modifiable from Linux.
It is my setup, I2C needs CLK BUSAON(this clock is also critical for system),
SCMI server exposes this clock, allow get rate, but when linux try to enable
this clock, SCMI server reply denied, so the i2c driver will probe failure.
Regards,
Peng.
>
> > The SCMI server could bypass enable/disable and only allow get rate,
> > But this introduces heavy RPC, so just wanna whether it is ok to
> > register fixed clock and avoid RPC.
> >
> > >
> > > JUNO/SCP is an example of how the CPUs clocks are visible to Linux
> > > BUT cannot be touched directly via Clock protocol by Linux since in
> > > the SCMI world you are supposed to use the Perf protocol instead to
> > > change the OPPs when you want to modify the performance level of the
> runnning CPU.
> > >
> > > This kind of server-side permissions checks, meant to filter access
> > > to resources based on the requesting agent, are part of the SCMI
> > > declared aim to push the responsibility of such controls out of the
> > > kernel into the platform fw in order to avoid attacks like
> > > CLOCK_SCREW by letting the SCMI firmware be the one and only final
> > > arbiter on the requests coming from the agents; you can ask teh
> > > server whatever you like as an agent but your request can be DENIED
> > > or silently ignored (in case of shared resources) at the will of the platform
> which has the final say and it is implemented in a physically distinct code-
> base.
> > >
> > > It seems to me that this patch and the possible associated SCMI
> > > specification change would give back the control to the Linux agent
> > > and could allow the implementation of an SCMI Server that does NOT
> > > perform any of these permission checks.
> > >
> > > So, IMO, while this change, on one side, could be certainly useful
> > > by removing a bunch of unused/uneeded callbacks from the CLK SCMI
> > > driver when a fixed clock is identified, it could open the door to a
> > > bad implementation like the one mentioned above which does NOT
> > > perform any agent-based permission check.
> >
> > Thanks for detailed information, let me check whether our SCMI
> > firmware could do more on the permission side. But if RPC could be
> > removed, it could save some time.
> >
>
> Avoiding to issue SCMI requests destined to fail would be certainly good,
> even though it could lead to just quietly implementing a server with no-
> checks at all, but why these unneeded calls happen in first place ?
>
> I have never observed anything like that in my setups.
>
> Thanks,
> Cristian
On Tue, Oct 10, 2023 at 10:29:11AM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <[email protected]>
>
> There are clocks:
> system critical, not allow linux to disable, change rate
> allow linux to get rate, because some periphals will use the frequency
> to configure periphals.
>
> So introduce an attribute to indicated FIXED clock
>
> Signed-off-by: Peng Fan <[email protected]>
> ---
> drivers/clk/clk-scmi.c | 6 ++++++
> drivers/firmware/arm_scmi/clock.c | 5 ++++-
> include/linux/scmi_protocol.h | 1 +
> 3 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
> index 8cbe24789c24..a539a35bd45a 100644
> --- a/drivers/clk/clk-scmi.c
> +++ b/drivers/clk/clk-scmi.c
> @@ -182,6 +182,10 @@ static const struct clk_ops scmi_clk_ops = {
> .determine_rate = scmi_clk_determine_rate,
> };
>
> +static const struct clk_ops scmi_fixed_rate_clk_ops = {
> + .recalc_rate = scmi_clk_recalc_rate,
> +};
> +
> static const struct clk_ops scmi_atomic_clk_ops = {
> .recalc_rate = scmi_clk_recalc_rate,
> .round_rate = scmi_clk_round_rate,
> @@ -293,6 +297,8 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
> if (is_atomic &&
> sclk->info->enable_latency <= atomic_threshold)
> scmi_ops = &scmi_atomic_clk_ops;
> + else if (sclk->info->rate_fixed)
> + scmi_ops = &scmi_fixed_rate_clk_ops;
> else
> scmi_ops = &scmi_clk_ops;
>
> diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> index ddaef34cd88b..8c52db539e54 100644
> --- a/drivers/firmware/arm_scmi/clock.c
> +++ b/drivers/firmware/arm_scmi/clock.c
> @@ -46,6 +46,7 @@ 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 SUPPORTS_FIXED_RATE_CLOCK(x) ((x) & BIT(27))
I don't see this in the specification, am I missing something ?
And why do we need it. Can't this be discrete clock with only one clock
rate ? Or step clock with both lowest and highest the same and step being 0.
At-least I don't see the need to change the spec for this and hence no need
to assign any attribute bit-field to represent the same.
--
Regards,
Sudeep
On Tue, Oct 10, 2023 at 10:12:23AM +0100, Sudeep Holla wrote:
> On Tue, Oct 10, 2023 at 10:29:11AM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <[email protected]>
> >
> > There are clocks:
> > system critical, not allow linux to disable, change rate
> > allow linux to get rate, because some periphals will use the frequency
> > to configure periphals.
> >
> > So introduce an attribute to indicated FIXED clock
> >
> > Signed-off-by: Peng Fan <[email protected]>
> > ---
> > drivers/clk/clk-scmi.c | 6 ++++++
> > drivers/firmware/arm_scmi/clock.c | 5 ++++-
> > include/linux/scmi_protocol.h | 1 +
> > 3 files changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
> > index 8cbe24789c24..a539a35bd45a 100644
> > --- a/drivers/clk/clk-scmi.c
> > +++ b/drivers/clk/clk-scmi.c
> > @@ -182,6 +182,10 @@ static const struct clk_ops scmi_clk_ops = {
> > .determine_rate = scmi_clk_determine_rate,
> > };
> >
> > +static const struct clk_ops scmi_fixed_rate_clk_ops = {
> > + .recalc_rate = scmi_clk_recalc_rate,
> > +};
> > +
> > static const struct clk_ops scmi_atomic_clk_ops = {
> > .recalc_rate = scmi_clk_recalc_rate,
> > .round_rate = scmi_clk_round_rate,
> > @@ -293,6 +297,8 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
> > if (is_atomic &&
> > sclk->info->enable_latency <= atomic_threshold)
> > scmi_ops = &scmi_atomic_clk_ops;
> > + else if (sclk->info->rate_fixed)
> > + scmi_ops = &scmi_fixed_rate_clk_ops;
> > else
> > scmi_ops = &scmi_clk_ops;
> >
> > diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> > index ddaef34cd88b..8c52db539e54 100644
> > --- a/drivers/firmware/arm_scmi/clock.c
> > +++ b/drivers/firmware/arm_scmi/clock.c
> > @@ -46,6 +46,7 @@ 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 SUPPORTS_FIXED_RATE_CLOCK(x) ((x) & BIT(27))
>
> I don't see this in the specification, am I missing something ?
>
> And why do we need it. Can't this be discrete clock with only one clock
> rate ? Or step clock with both lowest and highest the same and step being 0.
> At-least I don't see the need to change the spec for this and hence no need
> to assign any attribute bit-field to represent the same.
>
No this is not in the spec, it would require a spec change.
My understanding is that they have clocks that CAN have more than one rate BUT
such clock cannot be changed by Linux, only other agents can
enable/disable/set_rate BUT they still want to be able to query the
current rate for configuration purposes.
Thanks,
Cristian
On Tue, Oct 10, 2023 at 08:08:01AM +0000, Peng Fan wrote:
> > On Tue, Oct 10, 2023 at 08:49:33AM +0100, Cristian Marussi wrote:
> >
> > (CCed [email protected])
> >
> > so AFAIU here you are describing a clock that really is NOT fixed in general, it
> > is just that the Linux SCMI Agent cannot touch it, but other SCMI agents on
> > the system CAN change it and so, on one side, you keep the ability for the
> > Linux agent to read back the current rate with
> > recalc_rate() and remove all the Clk frameworks callbacks needed to modify
> > its state, am I right ?
>
> Right.
>
OK, let me try to understand this better. The SCMI platform provides all the
permissions to change to the Linux(agent) but it is just that you need to
enforce this policy of fixed rate in the kernel ? Or the SCMI platform
restricts the Linux agent to do anything other than fetch the rate ?
> >
> > In this scenario, it is really the SCMI platform fw (server) that has to
> > implement the checks and simply DENY the requests coming from an agent
> > that is not supposed to touch that clock, while allowing the current rate to be
> > retrieved.
+1, that is exactly why I asked the above question.
>
> Linux will try to enable, get rate, runtime disable the clock.
> But the server does not allow enable/disable the clock, so the driver probe
> will fail.
>
Interesting, if you can make the driver work with fixed clock, what can't
you make it work with failure to adjust the clock rates ?
> The SCMI server could bypass enable/disable and only allow get rate,
> But this introduces heavy RPC, so just wanna whether it is ok to register
> fixed clock and avoid RPC.
>
I need to study the clock layer in details, but would this happen if the
clock is present with just one fixed clock rate ?
> >
> > JUNO/SCP is an example of how the CPUs clocks are visible to Linux BUT
> > cannot be touched directly via Clock protocol by Linux since in the SCMI
> > world you are supposed to use the Perf protocol instead to change the OPPs
> > when you want to modify the performance level of the runnning CPU.
> >
Indeed.
> > This kind of server-side permissions checks, meant to filter access to resources
> > based on the requesting agent, are part of the SCMI declared aim to push the
> > responsibility of such controls out of the kernel into the platform fw in order
> > to avoid attacks like CLOCK_SCREW by letting the SCMI firmware be the one
> > and only final arbiter on the requests coming from the agents; you can ask
> > teh server whatever you like as an agent but your request can be DENIED or
> > silently ignored (in case of shared resources) at the will of the platform which
> > has the final say and it is implemented in a physically distinct code-base.
> >
> > It seems to me that this patch and the possible associated SCMI specification
> > change would give back the control to the Linux agent and could allow the
> > implementation of an SCMI Server that does NOT perform any of these
> > permission checks.
> >
Unless I am missing something, I already replied to Peng with reasons why
I don't see the need for change in the specification to represent this case.
> > So, IMO, while this change, on one side, could be certainly useful by removing
> > a bunch of unused/uneeded callbacks from the CLK SCMI driver when a fixed
> > clock is identified, it could open the door to a bad implementation like the
> > one mentioned above which does NOT perform any agent-based permission
> > check.
Agree.
>
> Thanks for detailed information, let me check whether our SCMI firmware
> could do more on the permission side. But if RPC could be removed,
> it could save some time.
I would like to check why would clk framework ask to recalculate the clock
rate if it is fixed clock.
Or you are trying to paper over the issue that it is not a fixed clock. The
other agents can change but not Linux agent but it must always read back the
set clock rate. It is sort of read only clock. That may be an attribute you
want to obtain from the firmware and see if that can be utilised in the clk
framework. But the way you have presented it as fixed is simply wrong to me.
--
Regards,
Sudeep
On Tue, Oct 10, 2023 at 10:22:03AM +0100, Cristian Marussi wrote:
> On Tue, Oct 10, 2023 at 10:12:23AM +0100, Sudeep Holla wrote:
> > On Tue, Oct 10, 2023 at 10:29:11AM +0800, Peng Fan (OSS) wrote:
> > > From: Peng Fan <[email protected]>
> > >
> > > There are clocks:
> > > system critical, not allow linux to disable, change rate
> > > allow linux to get rate, because some periphals will use the frequency
> > > to configure periphals.
> > >
> > > So introduce an attribute to indicated FIXED clock
> > >
> > > Signed-off-by: Peng Fan <[email protected]>
> > > ---
> > > drivers/clk/clk-scmi.c | 6 ++++++
> > > drivers/firmware/arm_scmi/clock.c | 5 ++++-
> > > include/linux/scmi_protocol.h | 1 +
> > > 3 files changed, 11 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
> > > index 8cbe24789c24..a539a35bd45a 100644
> > > --- a/drivers/clk/clk-scmi.c
> > > +++ b/drivers/clk/clk-scmi.c
> > > @@ -182,6 +182,10 @@ static const struct clk_ops scmi_clk_ops = {
> > > .determine_rate = scmi_clk_determine_rate,
> > > };
> > >
> > > +static const struct clk_ops scmi_fixed_rate_clk_ops = {
> > > + .recalc_rate = scmi_clk_recalc_rate,
> > > +};
> > > +
> > > static const struct clk_ops scmi_atomic_clk_ops = {
> > > .recalc_rate = scmi_clk_recalc_rate,
> > > .round_rate = scmi_clk_round_rate,
> > > @@ -293,6 +297,8 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
> > > if (is_atomic &&
> > > sclk->info->enable_latency <= atomic_threshold)
> > > scmi_ops = &scmi_atomic_clk_ops;
> > > + else if (sclk->info->rate_fixed)
> > > + scmi_ops = &scmi_fixed_rate_clk_ops;
> > > else
> > > scmi_ops = &scmi_clk_ops;
> > >
> > > diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> > > index ddaef34cd88b..8c52db539e54 100644
> > > --- a/drivers/firmware/arm_scmi/clock.c
> > > +++ b/drivers/firmware/arm_scmi/clock.c
> > > @@ -46,6 +46,7 @@ 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 SUPPORTS_FIXED_RATE_CLOCK(x) ((x) & BIT(27))
> >
> > I don't see this in the specification, am I missing something ?
> >
> > And why do we need it. Can't this be discrete clock with only one clock
> > rate ? Or step clock with both lowest and highest the same and step being 0.
> > At-least I don't see the need to change the spec for this and hence no need
> > to assign any attribute bit-field to represent the same.
> >
>
> No this is not in the spec, it would require a spec change.
>
> My understanding is that they have clocks that CAN have more than one rate BUT
> such clock cannot be changed by Linux, only other agents can
> enable/disable/set_rate BUT they still want to be able to query the
> current rate for configuration purposes.
>
Fair enough. As I mentioned to Peng on the other thread, it is *not a fixed*
clock. It is read only for this agent.
--
Regards,
Sudeep
From what I see SCMI clock protocol could benefit from an attribute for the clock that describes what operations are possible on a clock.
There are many bus clocks that only the SCMI server manages and the error code DENIED should be handled gracefully by the agent.
In the case of Linux, perhaps this should be handled by the SCMI clock driver, instead of allowing the error to propagate up the Linux clock framework? It seems strange that the SCMI server should swallow the error (silently fail) only for certain agents. I would think this would make debug quite difficult and having a "DENIED" error code not very useful.
Regards,
Ranjani
-----Original Message-----
From: Sudeep Holla <[email protected]>
Sent: Tuesday, October 10, 2023 4:35 AM
To: Cristian Marussi <[email protected]>
Cc: Peng Fan (OSS) <[email protected]>; [email protected]; [email protected]; [email protected]; Ranjani Vaidyanathan <[email protected]>; [email protected]; Glen G Wienecke <[email protected]>; Peng Fan <[email protected]>
Subject: [EXT] Re: [RFC] firmware: arm_scmi: clock: add fixed clock attribute support
Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button
On Tue, Oct 10, 2023 at 10:22:03AM +0100, Cristian Marussi wrote:
> On Tue, Oct 10, 2023 at 10:12:23AM +0100, Sudeep Holla wrote:
> > On Tue, Oct 10, 2023 at 10:29:11AM +0800, Peng Fan (OSS) wrote:
> > > From: Peng Fan <[email protected]>
> > >
> > > There are clocks:
> > > system critical, not allow linux to disable, change rate allow
> > > linux to get rate, because some periphals will use the frequency
> > > to configure periphals.
> > >
> > > So introduce an attribute to indicated FIXED clock
> > >
> > > Signed-off-by: Peng Fan <[email protected]>
> > > ---
> > > drivers/clk/clk-scmi.c | 6 ++++++
> > > drivers/firmware/arm_scmi/clock.c | 5 ++++-
> > > include/linux/scmi_protocol.h | 1 +
> > > 3 files changed, 11 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c index
> > > 8cbe24789c24..a539a35bd45a 100644
> > > --- a/drivers/clk/clk-scmi.c
> > > +++ b/drivers/clk/clk-scmi.c
> > > @@ -182,6 +182,10 @@ static const struct clk_ops scmi_clk_ops = {
> > > .determine_rate = scmi_clk_determine_rate, };
> > >
> > > +static const struct clk_ops scmi_fixed_rate_clk_ops = {
> > > +.recalc_rate = scmi_clk_recalc_rate, };
> > > +
> > > static const struct clk_ops scmi_atomic_clk_ops = {
> > > .recalc_rate = scmi_clk_recalc_rate,
> > > .round_rate = scmi_clk_round_rate, @@ -293,6 +297,8 @@ static
> > > int scmi_clocks_probe(struct scmi_device *sdev)
> > > if (is_atomic &&
> > > sclk->info->enable_latency <= atomic_threshold)
> > > scmi_ops = &scmi_atomic_clk_ops;
> > > + else if (sclk->info->rate_fixed)
> > > + scmi_ops = &scmi_fixed_rate_clk_ops;
> > > else
> > > scmi_ops = &scmi_clk_ops;
> > >
> > > diff --git a/drivers/firmware/arm_scmi/clock.c
> > > b/drivers/firmware/arm_scmi/clock.c
> > > index ddaef34cd88b..8c52db539e54 100644
> > > --- a/drivers/firmware/arm_scmi/clock.c
> > > +++ b/drivers/firmware/arm_scmi/clock.c
> > > @@ -46,6 +46,7 @@ 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 SUPPORTS_FIXED_RATE_CLOCK(x) ((x) & BIT(27))
> >
> > I don't see this in the specification, am I missing something ?
> >
> > And why do we need it. Can't this be discrete clock with only one
> > clock rate ? Or step clock with both lowest and highest the same and step being 0.
> > At-least I don't see the need to change the spec for this and hence
> > no need to assign any attribute bit-field to represent the same.
> >
>
> No this is not in the spec, it would require a spec change.
>
> My understanding is that they have clocks that CAN have more than one
> rate BUT such clock cannot be changed by Linux, only other agents can
> enable/disable/set_rate BUT they still want to be able to query the
> current rate for configuration purposes.
>
Fair enough. As I mentioned to Peng on the other thread, it is *not a fixed* clock. It is read only for this agent.
--
Regards,
Sudeep
On Wed, Oct 11, 2023 at 03:54:59AM +0000, Ranjani Vaidyanathan wrote:
> From what I see SCMI clock protocol could benefit from an attribute for the
> clock that describes what operations are possible on a clock. There are many
> bus clocks that only the SCMI server manages and the error code DENIED
> should be handled gracefully by the agent.
>
Agreed, but we need to understand if we need it per operation basis or
at the higher granularity such as any write or set operations not allowed.
Just my initial thoughts, we can discuss.
> In the case of Linux, perhaps this should be handled by the SCMI clock
> driver, instead of allowing the error to propagate up the Linux clock
> framework?
Yes but even for that we need information from the firmware.
> It seems strange that the SCMI server should swallow the error (silently
> fail) only for certain agents.
I am bit confused by this statement. The SCMI server/platform must not
ignore or fail silently. Since the agent is not allowed, if it attempts
it must return the apt error.
While Linux may choose to ignore the error but I think this is what
we are discussing to figure out what is the best way to avoid it or
worst case handle it gracefully. With any extra info from the firmware,
the former option must be possible and we need not think of the latter
option IMO.
> I would think this would make debug quite difficult and having a "DENIED"
> error code not very useful.
>
Agreed especially if it can and is expected to continue functioning as normal.
--
Regards,
Sudeep