| Message ID | 20241112-fix_childclk_of_roclk_has_been_tampered_with-v2-1-64f8009cdf2a@amlogic.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Neil Armstrong |
| Headers | show |
| Series | [v2] clk: meson: Fix the determine rate error in clk_regmap_divider_ro_ops | expand |
On 11/12/2024 8:57 PM, Chuan Liu via B4 Relay wrote: > [Some people who received this message don't often get email from devnull+chuan.liu.amlogic.com@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > [ EXTERNAL EMAIL ] > > From: Chuan Liu <chuan.liu@amlogic.com> > > The rate determined by calling clk_regmap_divider_ro_ops with > clk_regmap_div_determine_rate is not RO, which will result in the > unexpected modification of the frequency of its children when setting > the rate of a clock that references clk_regmap_divider_ro_ops. > > Fiexs: ea11dda9e091 ("clk: meson: add regmap clocks") typo of "Fiexs", corrected in the next version. > Signed-off-by: Chuan Liu <chuan.liu@amlogic.com> > --- > Background: During the execution of clk_set_rate(), the function > clk_core_round_rate_nolock() is called to calculate the matching rate > and save it to 'core->new_rate'. At the same time, it recalculates and > updates its 'child->newrate'. Finally, clk_change_rate() is called to > set all 'new_rates'. > --- > Changes in v2: > - Remove the CLK_DIVIDER_READ_ONLY judgment logic in > clk_regmap_div_determine_rate(). > - Add clk_regmap_div_ro_determine_rate(). > - Link to v1: https://lore.kernel.org/r/20241111-fix_childclk_of_roclk_has_been_tampered_with-v1-1-f8c1b6ffdcb0@amlogic.com > --- > drivers/clk/meson/clk-regmap.c | 36 ++++++++++++++++++++---------------- > 1 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) { > - 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, > -- > Chuan Liu <chuan.liu@amlogic.com> > >
On 12/11/2024 13:57, Chuan Liu via B4 Relay wrote: > From: Chuan Liu <chuan.liu@amlogic.com> > > The rate determined by calling clk_regmap_divider_ro_ops with > clk_regmap_div_determine_rate is not RO, which will result in the > unexpected modification of the frequency of its children when setting > the rate of a clock that references clk_regmap_divider_ro_ops. > > Fiexs: ea11dda9e091 ("clk: meson: add regmap clocks") > Signed-off-by: Chuan Liu <chuan.liu@amlogic.com> > --- > Background: During the execution of clk_set_rate(), the function > clk_core_round_rate_nolock() is called to calculate the matching rate > and save it to 'core->new_rate'. At the same time, it recalculates and > updates its 'child->newrate'. Finally, clk_change_rate() is called to > set all 'new_rates'. > --- > Changes in v2: > - Remove the CLK_DIVIDER_READ_ONLY judgment logic in > clk_regmap_div_determine_rate(). > - Add clk_regmap_div_ro_determine_rate(). > - Link to v1: https://lore.kernel.org/r/20241111-fix_childclk_of_roclk_has_been_tampered_with-v1-1-f8c1b6ffdcb0@amlogic.com > --- > drivers/clk/meson/clk-regmap.c | 36 ++++++++++++++++++++---------------- > 1 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. 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,
On 11/12/2024 9:24 PM, Neil Armstrong wrote: > [ EXTERNAL EMAIL ] > > On 12/11/2024 13:57, Chuan Liu via B4 Relay wrote: >> From: Chuan Liu <chuan.liu@amlogic.com> >> >> The rate determined by calling clk_regmap_divider_ro_ops with >> clk_regmap_div_determine_rate is not RO, which will result in the >> unexpected modification of the frequency of its children when setting >> the rate of a clock that references clk_regmap_divider_ro_ops. >> >> Fiexs: ea11dda9e091 ("clk: meson: add regmap clocks") >> Signed-off-by: Chuan Liu <chuan.liu@amlogic.com> >> --- >> Background: During the execution of clk_set_rate(), the function >> clk_core_round_rate_nolock() is called to calculate the matching rate >> and save it to 'core->new_rate'. At the same time, it recalculates and >> updates its 'child->newrate'. Finally, clk_change_rate() is called to >> set all 'new_rates'. >> --- >> Changes in v2: >> - Remove the CLK_DIVIDER_READ_ONLY judgment logic in >> clk_regmap_div_determine_rate(). >> - Add clk_regmap_div_ro_determine_rate(). >> - Link to v1: >> https://lore.kernel.org/r/20241111-fix_childclk_of_roclk_has_been_tampered_with-v1-1-f8c1b6ffdcb0@amlogic.com >> --- >> drivers/clk/meson/clk-regmap.c | 36 >> ++++++++++++++++++++---------------- >> 1 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() 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. > > 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, >
On Wed 13 Nov 2024 at 14:55, Chuan Liu <chuan.liu@amlogic.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, >>
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) { - 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);