| 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: <ZaltgIGyx1al-F9x@pluto>
Date: Thu, 18 Jan 2024 18:27:12 +0000
From: Cristian Marussi <cristian.marussi@....com>
To: "Peng Fan (OSS)" <peng.fan@....nxp.com>
Cc: sudeep.holla@....com, mturquette@...libre.com, sboyd@...nel.org,
linux-arm-kernel@...ts.infradead.org, linux-clk@...r.kernel.org,
linux-kernel@...r.kernel.org, Peng Fan <peng.fan@....com>
Subject: Re: [PATCH V3 2/2] clk: scmi: support state_ctrl_forbidden
On Mon, Jan 15, 2024 at 02:02:03PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@....com>
>
> Some clocks may exported to linux, while those clocks are not allowed
> to configure by Linux. For example:
>
> SYS_CLK1-----
> \
> --MUX--->MMC1_CLK
> /
> SYS_CLK2-----
>
> MMC1 needs set parent, so SYS_CLK1 and SYS_CLK2 are exported to Linux,
> then the clk propagation will touch SYS_CLK1 or SYS_CLK2.
> So we need bypass the failure for SYS_CLK1 or SYS_CLK2 when enable
> the clock of MMC1.
>
Hi,
so this looks good to me and apparently (as noted) the CLK framework is OK
with a driver swallowing the -EACCESS when a clock is immutable, BUT at the
end of the day do we even need to try this SCMI call and hide the failure in
case of immutable clocks ?
I mean, what if we just dont provide any callback for enable/disable...I can
see plenty of drivers not providing those callbacks ?
Maybe this is probably more of a question for Stephen...
IOW what about doing something like below...does it make any difference
in your setup ? works fine in my emulated env
(Note that last snippet in clk_gate_restore_context() is probably a fix
that needs to be added anyway by looking at the code in clk.c)
Thanks,
Cristian
--->8----
diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
index 5327e0547741..a669a2f2f78b 100644
--- a/drivers/clk/clk-scmi.c
+++ b/drivers/clk/clk-scmi.c
@@ -121,11 +121,7 @@ static int scmi_clk_enable(struct clk_hw *hw)
struct scmi_clk *clk = to_scmi_clk(hw);
int ret;
- ret = scmi_proto_clk_ops->enable(clk->ph, clk->id, NOT_ATOMIC);
- if (ret == -EACCES && clk->info->state_ctrl_forbidden)
- return 0;
-
- return ret;
+ return scmi_proto_clk_ops->enable(clk->ph, clk->id, NOT_ATOMIC);
}
static void scmi_clk_disable(struct clk_hw *hw)
@@ -140,11 +136,7 @@ static int scmi_clk_atomic_enable(struct clk_hw *hw)
struct scmi_clk *clk = to_scmi_clk(hw);
int ret;
- ret = scmi_proto_clk_ops->enable(clk->ph, clk->id, ATOMIC);
- if (ret == -EACCES && clk->info->state_ctrl_forbidden)
- return 0;
-
- return ret;
+ return scmi_proto_clk_ops->enable(clk->ph, clk->id, ATOMIC);
}
static void scmi_clk_atomic_disable(struct clk_hw *hw)
@@ -204,6 +196,15 @@ static const struct clk_ops scmi_atomic_clk_ops = {
.determine_rate = scmi_clk_determine_rate,
};
+static const struct clk_ops scmi_no_state_ctrl_clk_ops = {
+ .recalc_rate = scmi_clk_recalc_rate,
+ .round_rate = scmi_clk_round_rate,
+ .set_rate = scmi_clk_set_rate,
+ .set_parent = scmi_clk_set_parent,
+ .get_parent = scmi_clk_get_parent,
+ .determine_rate = scmi_clk_determine_rate,
+};
+
static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk,
const struct clk_ops *scmi_ops)
{
@@ -300,8 +301,10 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
* specify (or support) an enable_latency associated with a
* clock, we default to use atomic operations mode.
*/
- if (is_atomic &&
- sclk->info->enable_latency <= atomic_threshold)
+ if (sclk->info->state_ctrl_forbidden)
+ scmi_ops = &scmi_no_state_ctrl_clk_ops;
+ else if (is_atomic &&
+ sclk->info->enable_latency <= atomic_threshold)
scmi_ops = &scmi_atomic_clk_ops;
else
scmi_ops = &scmi_clk_ops;
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index f0940af485a5..79b90a8099d7 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1200,9 +1200,11 @@ void clk_gate_restore_context(struct clk_hw *hw)
struct clk_core *core = hw->core;
if (core->enable_count)
- core->ops->enable(hw);
+ if (core->ops->enable)
+ core->ops->enable(hw);
else
- core->ops->disable(hw);
+ if (core->ops->disable)
+ core->ops->disable(hw);
}
EXPORT_SYMBOL_GPL(clk_gate_restore_context);
---8<---
> Signed-off-by: Peng Fan <peng.fan@....com>
> ---
>
> V3:
> Add check in atomic enable
>
> V2:
> New. Take Cristian's suggestion
>
> drivers/clk/clk-scmi.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
> index 8cbe24789c24..5327e0547741 100644
> --- a/drivers/clk/clk-scmi.c
> +++ b/drivers/clk/clk-scmi.c
> @@ -119,8 +119,13 @@ 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);
> + int ret;
> +
> + ret = scmi_proto_clk_ops->enable(clk->ph, clk->id, NOT_ATOMIC);
> + if (ret == -EACCES && clk->info->state_ctrl_forbidden)
> + return 0;
>
> - return scmi_proto_clk_ops->enable(clk->ph, clk->id, NOT_ATOMIC);
> + return ret;
> }
>
> static void scmi_clk_disable(struct clk_hw *hw)
> @@ -133,8 +138,13 @@ 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);
> + int ret;
> +
> + ret = scmi_proto_clk_ops->enable(clk->ph, clk->id, ATOMIC);
> + if (ret == -EACCES && clk->info->state_ctrl_forbidden)
> + return 0;
>
> - return scmi_proto_clk_ops->enable(clk->ph, clk->id, ATOMIC);
> + return ret;
> }
>
> static void scmi_clk_atomic_disable(struct clk_hw *hw)
> --
> 2.37.1
>
Powered by blists - more mailing lists