| 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: <c9385ba8-b225-4c8d-b09f-971d9e6008ab@oss.nxp.com>
Date: Sat, 20 Jan 2024 10:44:06 +0800
From: Peng Fan <peng.fan@....nxp.com>
To: Cristian Marussi <cristian.marussi@....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
在 1/19/2024 2:27 AM, Cristian Marussi 写道:
> 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
It should be fine to use your changes. Do you expect me to use your
patch or make it as a follow up patch?
>
> (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)
This API seems only used by TI gate driver, this change should be in a
standalone change go through clk tree. So I would your changes
as a standalone optimization follow up patch, while not included
in my patchset.
THanks,
Peng.
>
> 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