| Message ID | 20240229-freq-table-terminator-v1-1-074334f0905c@gmail.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream, archived |
| Headers | show |
| Series | clk: qcom: fix frequency table termination in several drivers | expand |
Quoting Gabor Juhos (2024-02-29 10:07:46) > The frequency table arrays are supposed to be terminated with an > empty element. Add such entry to the end of the arrays where it > is missing in order to avoid possible out-of-bound access when > the table is traversed by functions like qcom_find_freq() or > qcom_find_freq_floor(). > > Fixes: e3fdbef1bab8 ("clk: qcom: Add Global Clock controller (GCC) driver for IPQ5018") > Signed-off-by: Gabor Juhos <j4g8y7@gmail.com> > --- Reviewed-by: Stephen Boyd <sboyd@kernel.org>
On Thu, Feb 29, 2024 at 07:07:46PM +0100, Gabor Juhos wrote: > The frequency table arrays are supposed to be terminated with an > empty element. Add such entry to the end of the arrays where it > is missing in order to avoid possible out-of-bound access when > the table is traversed by functions like qcom_find_freq() or > qcom_find_freq_floor(). > > Fixes: e3fdbef1bab8 ("clk: qcom: Add Global Clock controller (GCC) driver for IPQ5018") Good find! Looks like these should be backported to the stable kernels as well so someone should add: Cc: stable@vger.kernel.org to all patches except possibly the sc8280xp one (that camera clock controller was added in 6.8-rc1 so that patch does not need it in case you can these fixes in before 6.8 is released). Johan
Hi Johan, 2024. 03. 01. 10:40 keltezéssel, Johan Hovold írta: > On Thu, Feb 29, 2024 at 07:07:46PM +0100, Gabor Juhos wrote: >> The frequency table arrays are supposed to be terminated with an >> empty element. Add such entry to the end of the arrays where it >> is missing in order to avoid possible out-of-bound access when >> the table is traversed by functions like qcom_find_freq() or >> qcom_find_freq_floor(). >> >> Fixes: e3fdbef1bab8 ("clk: qcom: Add Global Clock controller (GCC) driver for IPQ5018") > > Good find! > > Looks like these should be backported to the stable kernels as well so > someone should add: > > Cc: stable@vger.kernel.org > > to all patches except possibly the sc8280xp one (that camera clock > controller was added in 6.8-rc1 so that patch does not need it in case > you can these fixes in before 6.8 is released). You are right maybe, although I did not find strong enough reasons for adding the stable tags. Only the changes of the gcc-ipq5018 driver has been tested on real hardware the others are not. So those does not fit into the "It must be obviously correct and tested." rule. Since the support for the IPQ5018 platform is quite incomplete as several drivers are missing even in 6.8-rc6, it is quite unlikely that anyone uses a stable kernel for something serious on such boards. Hence backporting the gcc-ipq5018 changes would not help too much in itself. Regards, Gabor
On Fri, Mar 01, 2024 at 02:37:01PM +0100, Gabor Juhos wrote: > Hi Johan, > > 2024. 03. 01. 10:40 keltezéssel, Johan Hovold írta: > > On Thu, Feb 29, 2024 at 07:07:46PM +0100, Gabor Juhos wrote: > >> The frequency table arrays are supposed to be terminated with an > >> empty element. Add such entry to the end of the arrays where it > >> is missing in order to avoid possible out-of-bound access when > >> the table is traversed by functions like qcom_find_freq() or > >> qcom_find_freq_floor(). > >> > >> Fixes: e3fdbef1bab8 ("clk: qcom: Add Global Clock controller (GCC) driver for IPQ5018") > > > > Good find! > > > > Looks like these should be backported to the stable kernels as well so > > someone should add: > > > > Cc: stable@vger.kernel.org > > > > to all patches except possibly the sc8280xp one (that camera clock > > controller was added in 6.8-rc1 so that patch does not need it in case > > you can these fixes in before 6.8 is released). > > You are right maybe, although I did not find strong enough reasons for adding > the stable tags. > > Only the changes of the gcc-ipq5018 driver has been tested on real hardware the > others are not. So those does not fit into the "It must be obviously correct and > tested." rule. Since this looks like a straight-forward and obviously correct fix for a bug which could have bad consequences, not being able to test each patch on actual hardware is not a problem. Johan
On Fri, Mar 01, 2024 at 03:03:15PM +0100, Johan Hovold wrote: > On Fri, Mar 01, 2024 at 02:37:01PM +0100, Gabor Juhos wrote: > > Hi Johan, > > > > 2024. 03. 01. 10:40 keltezéssel, Johan Hovold írta: > > > On Thu, Feb 29, 2024 at 07:07:46PM +0100, Gabor Juhos wrote: > > >> The frequency table arrays are supposed to be terminated with an > > >> empty element. Add such entry to the end of the arrays where it > > >> is missing in order to avoid possible out-of-bound access when > > >> the table is traversed by functions like qcom_find_freq() or > > >> qcom_find_freq_floor(). > > >> > > >> Fixes: e3fdbef1bab8 ("clk: qcom: Add Global Clock controller (GCC) driver for IPQ5018") > > > > > > Good find! > > > > > > Looks like these should be backported to the stable kernels as well so > > > someone should add: > > > > > > Cc: stable@vger.kernel.org > > > > > > to all patches except possibly the sc8280xp one (that camera clock > > > controller was added in 6.8-rc1 so that patch does not need it in case > > > you can these fixes in before 6.8 is released). > > > > You are right maybe, although I did not find strong enough reasons for adding > > the stable tags. > > > > Only the changes of the gcc-ipq5018 driver has been tested on real hardware the > > others are not. So those does not fit into the "It must be obviously correct and > > tested." rule. > > Since this looks like a straight-forward and obviously correct fix for a > bug which could have bad consequences, not being able to test each patch > on actual hardware is not a problem. > I agree, and I'm adding the Cc: stable while applying the patches. Thanks, Bjorn
diff --git a/drivers/clk/qcom/gcc-ipq5018.c b/drivers/clk/qcom/gcc-ipq5018.c index 4aba47e8700d2..856a2b30e2a8e 100644 --- a/drivers/clk/qcom/gcc-ipq5018.c +++ b/drivers/clk/qcom/gcc-ipq5018.c @@ -857,6 +857,7 @@ static struct clk_rcg2 lpass_sway_clk_src = { static const struct freq_tbl ftbl_pcie0_aux_clk_src[] = { F(2000000, P_XO, 12, 0, 0), + { } }; static struct clk_rcg2 pcie0_aux_clk_src = { @@ -1099,6 +1100,7 @@ static const struct freq_tbl ftbl_qpic_io_macro_clk_src[] = { F(100000000, P_GPLL0, 8, 0, 0), F(200000000, P_GPLL0, 4, 0, 0), F(320000000, P_GPLL0, 2.5, 0, 0), + { } }; static struct clk_rcg2 qpic_io_macro_clk_src = { @@ -1194,6 +1196,7 @@ static struct clk_rcg2 ubi0_axi_clk_src = { static const struct freq_tbl ftbl_ubi0_core_clk_src[] = { F(850000000, P_UBI32_PLL, 1, 0, 0), F(1000000000, P_UBI32_PLL, 1, 0, 0), + { } }; static struct clk_rcg2 ubi0_core_clk_src = {
The frequency table arrays are supposed to be terminated with an empty element. Add such entry to the end of the arrays where it is missing in order to avoid possible out-of-bound access when the table is traversed by functions like qcom_find_freq() or qcom_find_freq_floor(). Fixes: e3fdbef1bab8 ("clk: qcom: Add Global Clock controller (GCC) driver for IPQ5018") Signed-off-by: Gabor Juhos <j4g8y7@gmail.com> --- drivers/clk/qcom/gcc-ipq5018.c | 3 +++ 1 file changed, 3 insertions(+)