| 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: <473aa749-fcfe-bd38-0d83-95375aeef8a3@collabora.com>
Date: Tue, 12 Jul 2022 12:55:42 +0200
From: AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com>
To: Chen-Yu Tsai <wenst@...omium.org>,
Nícolas F. R. A. Prado
<nfraprado@...labora.com>
Cc: Stephen Boyd <sboyd@...nel.org>, kernel@...labora.com,
Chun-Jie Chen <chun-jie.chen@...iatek.com>,
Ikjoon Jang <ikjn@...omium.org>,
Matthias Brugger <matthias.bgg@...il.com>,
Michael Turquette <mturquette@...libre.com>,
Miles Chen <miles.chen@...iatek.com>,
Ran Jianping <ran.jianping@....com.cn>,
Weiyi Lu <weiyi.lu@...iatek.com>,
linux-arm-kernel@...ts.infradead.org, linux-clk@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH] clk: mediatek: Don't check HW status for mt8192/5's
imp_iic_wrap clocks
Il 12/07/22 12:44, Chen-Yu Tsai ha scritto:
> Hi,
>
> On Tue, Jul 12, 2022 at 4:57 AM Nícolas F. R. A. Prado
> <nfraprado@...labora.com> wrote:
>>
>> The imp_iic_wrap clocks on mt8192/mt8195 require that the i2c_sel parent
>> clock be enabled before their hardware status can be checked. Since this
>> wasn't taken into account, reading from the clk_summary debugfs file
>> would cause the system to completely freeze.
>>
>> Assuming that this clock is managed only by the kernel, and not by any
>> firmware, simply drop the is_enabled() optional callback and instead
>> rely on the enable count for the imp_iic_wrap clocks.
>
> That's the wrong way to go about it.
>
> The I2C clocks already have the CLK_OPS_PARENT_ENABLE flag set. So the
> issue is that somewhere in the clk core, a piece of code is not honoring
> that flag.
>
> And it seems that's in more than one place.
>
Uhm, you're right. I gave my Tested-by, but not a Reviewed-by because I
wasn't really convinced about this solution being the best.
Now that I think of it, the solution may be as simple as:
clk.c
static bool clk_core_is_enabled(struct clk_core *core)
{
bool ret = false;
/*
* If this clock needs parent enabled, but its parent is
* off, we directly return false for two reasons:
* 1. This clock being enabled would be impossible
* 2. The platform may crash for unclocked access while
* reading the status of this clock (where a .is_enabled
* callback is provided).
*/
if (core->flags & CLK_OPS_PARENT_ENABLE &&
!clk_core_is_enabled(core->parent))
return false;
... etc etc etc ...
}
Nícolas, did you try this approach?
> Regards
> ChenYu
>
>> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@...labora.com>
>>
>> ---
>>
>> drivers/clk/mediatek/clk-gate.c | 6 ++++++
>> drivers/clk/mediatek/clk-gate.h | 1 +
>> drivers/clk/mediatek/clk-mt8192-imp_iic_wrap.c | 2 +-
>> drivers/clk/mediatek/clk-mt8195-imp_iic_wrap.c | 2 +-
>> 4 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clk/mediatek/clk-gate.c b/drivers/clk/mediatek/clk-gate.c
>> index 421806236228..8e7c719a69b3 100644
>> --- a/drivers/clk/mediatek/clk-gate.c
>> +++ b/drivers/clk/mediatek/clk-gate.c
>> @@ -124,6 +124,12 @@ static void mtk_cg_disable_inv_no_setclr(struct clk_hw *hw)
>> mtk_cg_clr_bit_no_setclr(hw);
>> }
>>
>> +const struct clk_ops mtk_clk_gate_ops_setclr_counted = {
>> + .enable = mtk_cg_enable,
>> + .disable = mtk_cg_disable,
>> +};
>> +EXPORT_SYMBOL_GPL(mtk_clk_gate_ops_setclr_counted);
>> +
>> const struct clk_ops mtk_clk_gate_ops_setclr = {
>> .is_enabled = mtk_cg_bit_is_cleared,
>> .enable = mtk_cg_enable,
>> diff --git a/drivers/clk/mediatek/clk-gate.h b/drivers/clk/mediatek/clk-gate.h
>> index d9897ef53528..b5502b2911f5 100644
>> --- a/drivers/clk/mediatek/clk-gate.h
>> +++ b/drivers/clk/mediatek/clk-gate.h
>> @@ -19,6 +19,7 @@ extern const struct clk_ops mtk_clk_gate_ops_setclr;
>> extern const struct clk_ops mtk_clk_gate_ops_setclr_inv;
>> extern const struct clk_ops mtk_clk_gate_ops_no_setclr;
>> extern const struct clk_ops mtk_clk_gate_ops_no_setclr_inv;
>> +extern const struct clk_ops mtk_clk_gate_ops_setclr_counted;
>>
>> struct mtk_gate_regs {
>> u32 sta_ofs;
>> diff --git a/drivers/clk/mediatek/clk-mt8192-imp_iic_wrap.c b/drivers/clk/mediatek/clk-mt8192-imp_iic_wrap.c
>> index 700356ac6a58..900ee601169c 100644
>> --- a/drivers/clk/mediatek/clk-mt8192-imp_iic_wrap.c
>> +++ b/drivers/clk/mediatek/clk-mt8192-imp_iic_wrap.c
>> @@ -20,7 +20,7 @@ static const struct mtk_gate_regs imp_iic_wrap_cg_regs = {
>>
>> #define GATE_IMP_IIC_WRAP(_id, _name, _parent, _shift) \
>> GATE_MTK_FLAGS(_id, _name, _parent, &imp_iic_wrap_cg_regs, _shift, \
>> - &mtk_clk_gate_ops_setclr, CLK_OPS_PARENT_ENABLE)
>> + &mtk_clk_gate_ops_setclr_counted, CLK_OPS_PARENT_ENABLE)
>>
>> static const struct mtk_gate imp_iic_wrap_c_clks[] = {
>> GATE_IMP_IIC_WRAP(CLK_IMP_IIC_WRAP_C_I2C10, "imp_iic_wrap_c_i2c10", "infra_i2c0", 0),
>> diff --git a/drivers/clk/mediatek/clk-mt8195-imp_iic_wrap.c b/drivers/clk/mediatek/clk-mt8195-imp_iic_wrap.c
>> index fbc809d05072..e50a77b844f4 100644
>> --- a/drivers/clk/mediatek/clk-mt8195-imp_iic_wrap.c
>> +++ b/drivers/clk/mediatek/clk-mt8195-imp_iic_wrap.c
>> @@ -18,7 +18,7 @@ static const struct mtk_gate_regs imp_iic_wrap_cg_regs = {
>>
>> #define GATE_IMP_IIC_WRAP(_id, _name, _parent, _shift) \
>> GATE_MTK_FLAGS(_id, _name, _parent, &imp_iic_wrap_cg_regs, _shift, \
>> - &mtk_clk_gate_ops_setclr, CLK_OPS_PARENT_ENABLE)
>> + &mtk_clk_gate_ops_setclr_counted, CLK_OPS_PARENT_ENABLE)
>>
>> static const struct mtk_gate imp_iic_wrap_s_clks[] = {
>> GATE_IMP_IIC_WRAP(CLK_IMP_IIC_WRAP_S_I2C5, "imp_iic_wrap_s_i2c5", "top_i2c", 0),
>> --
>> 2.37.0
>>
Powered by blists - more mailing lists