linux-kernel - Re: [PATCH v2] clk: meson: Fix the determine rate error in clk_regmap_divider_ro

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1j7c97v8cf.fsf@starbuckisacylon.baylibre.com>
Date: Wed, 13 Nov 2024 09:36:16 +0100
From: Jerome Brunet <jbrunet@...libre.com>
To: Chuan Liu <chuan.liu@...ogic.com>
Cc: neil.armstrong@...aro.org,  Michael Turquette <mturquette@...libre.com>,
  Stephen Boyd <sboyd@...nel.org>,  Kevin Hilman <khilman@...libre.com>,
  Martin Blumenstingl <martin.blumenstingl@...glemail.com>,  Jian Hu
 <jian.hu@...ogic.com>,  Dmitry Rokosov <ddrokosov@...rdevices.ru>,  Yu Tu
 <yu.tu@...ogic.com>,  linux-amlogic@...ts.infradead.org,
  linux-clk@...r.kernel.org,  linux-arm-kernel@...ts.infradead.org,
  linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] clk: meson: Fix the determine rate error in
 clk_regmap_divider_ro_ops

On Wed 13 Nov 2024 at 14:55, Chuan Liu <chuan.liu@...ogic.com> wrote:

 file changed, 20 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/clk/meson/clk-regmap.c
>>> b/drivers/clk/meson/clk-regmap.c
>>> index 07f7e441b916..edf65ca92c7a 100644
>>> --- a/drivers/clk/meson/clk-regmap.c
>>> +++ b/drivers/clk/meson/clk-regmap.c
>>> @@ -80,21 +80,6 @@ static int clk_regmap_div_determine_rate(struct
>>> clk_hw *hw,
>>>   {
>>>       struct clk_regmap *clk = to_clk_regmap(hw);
>>>       struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
>>> -     unsigned int val;
>>> -     int ret;
>>> -
>>> -     /* if read only, just return current value */
>>> -     if (div->flags & CLK_DIVIDER_READ_ONLY) {
>>
>> You're breaking current code by no more checking this flag,
>> the new clk_regmap_div_ro_determine_rate() is fine, but you should call
>> it from here if CLK_DIVIDER_READ_ONLY is set.
>
> My idea is that the newly added clk_regmap_div_ro_determine_rate()

Whatever your idea is, what has been submitted is broken, as Neil
pointed out

> implements the functionality of handling CLK_DIVIDER_READ_ONLY in
> clk_regmap_div_determine_rate(). If we still keep the logic for
> handling CLK_DIVIDER_READ_ONLY here, it will make
> clk_regmap_div_determine_rate() ambiguous and easily confused.

That would just shift the problem from one function to other.
Please apply Neil's suggestion.

>
>>
>> Neil
>>
>>> -             ret = regmap_read(clk->map, div->offset, &val);
>>> -             if (ret)
>>> -                     return ret;
>>> -
>>> -             val >>= div->shift;
>>> -             val &= clk_div_mask(div->width);
>>> -
>>> -             return divider_ro_determine_rate(hw, req, div->table,
>>> -                                              div->width, div->flags,
>>> val);
>>> -     }
>>>
>>>       return divider_determine_rate(hw, req, div->table, div->width,
>>>                                     div->flags);
>>> @@ -127,9 +112,28 @@ const struct clk_ops clk_regmap_divider_ops = {
>>>   };
>>>   EXPORT_SYMBOL_NS_GPL(clk_regmap_divider_ops, CLK_MESON);
>>>
>>> +static int clk_regmap_div_ro_determine_rate(struct clk_hw *hw,
>>> +                                         struct clk_rate_request *req)
>>> +{
>>> +     struct clk_regmap *clk = to_clk_regmap(hw);
>>> +     struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
>>> +     unsigned int val;
>>> +     int ret;
>>> +
>>> +     ret = regmap_read(clk->map, div->offset, &val);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     val >>= div->shift;
>>> +     val &= clk_div_mask(div->width);
>>> +
>>> +     return divider_ro_determine_rate(hw, req, div->table, div->width,
>>> +                                      div->flags, val);
>>> +}
>>> +
>>>   const struct clk_ops clk_regmap_divider_ro_ops = {
>>>       .recalc_rate = clk_regmap_div_recalc_rate,
>>> -     .determine_rate = clk_regmap_div_determine_rate,
>>> +     .determine_rate = clk_regmap_div_ro_determine_rate,
>>>   };
>>>   EXPORT_SYMBOL_NS_GPL(clk_regmap_divider_ro_ops, CLK_MESON);
>>>
>>>
>>> ---
>>> base-commit: 664988eb47dd2d6ae1d9e4188ec91832562f8f26
>>> change-id:
>>> 20241111-fix_childclk_of_roclk_has_been_tampered_with-61dbcc623746
>>>
>>> Best regards,
>>

-- 
Jerome