linux-kernel - Re: [PATCH] clk: qcom: clk-alpha-pll: Replace divide operator with comparison

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <kh4diauo5u63mldchmd66pbnqxwnbqfoqcpxsw6wwocbadygvz@3diccu2xt4kj>
Date: Wed, 28 Aug 2024 23:44:20 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Jon Hunter <jonathanh@...dia.com>
Cc: Vladimir Zapolskiy <vladimir.zapolskiy@...aro.org>, 
	Satya Priya Kakitapalli <quic_skakitap@...cinc.com>, Bjorn Andersson <andersson@...nel.org>, 
	Michael Turquette <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>, linux-arm-msm@...r.kernel.org, 
	linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Ajit Pandey <quic_ajipan@...cinc.com>, Imran Shaik <quic_imrashai@...cinc.com>, 
	Taniya Das <quic_tdas@...cinc.com>, Jagadeesh Kona <quic_jkona@...cinc.com>, 
	kernel test robot <lkp@...el.com>, Dan Carpenter <dan.carpenter@...aro.org>, 
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>
Subject: Re: [PATCH] clk: qcom: clk-alpha-pll: Replace divide operator with
 comparison

On Wed, Aug 28, 2024 at 02:47:05PM GMT, Jon Hunter wrote:
> Hi Satya, Vladimir,
> 
> On 13/08/2024 21:01, Vladimir Zapolskiy wrote:
> > On 8/13/24 12:40, Satya Priya Kakitapalli wrote:
> > > In zonda_pll_adjust_l_val() replace the divide operator with comparison
> > > operator since comparisons are faster than divisions.
> > > 
> > > Fixes: f4973130d255 ("clk: qcom: clk-alpha-pll: Update set_rate for
> > > Zonda PLL")
> > 
> > Apparently the change is not a fix, therefore I believe the Fixes tag
> > shall be removed.
> 
> 
> From the commit message it is not clear that this is a fix, but I
> believe that it is. With the current -next I am seeing the following
> build error (with GCC 7.3.1) on ARM ...
> 
> drivers/clk/qcom/clk-alpha-pll.o: In function `clk_zonda_pll_set_rate':
> clk-alpha-pll.c:(.text+0x45dc): undefined reference to `__aeabi_uldivmod'

This should be a part of the commit message

> > > Reported-by: kernel test robot <lkp@...el.com>
> > > Reported-by: Dan Carpenter <dan.carpenter@...aro.org>
> > > Closes: https://lore.kernel.org/r/202408110724.8pqbpDiD-lkp@intel.com/

this Closes tag must come after lkp's Reported-by. Please also add
Closes with the link to Dan's report.

> 
> There is also the above smatch warning that was reported.

And the Smatch warning too should be a part of the commit message.

Last, but not least, as it is a fix, there should be a Fixes: tag and
optionally a cc:stable.

> 
> > > Signed-off-by: Satya Priya Kakitapalli <quic_skakitap@...cinc.com>
> > > ---
> > >   drivers/clk/qcom/clk-alpha-pll.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/clk/qcom/clk-alpha-pll.c
> > > b/drivers/clk/qcom/clk-alpha-pll.c
> > > index 2f620ccb41cb..fd8a82bb3690 100644
> > > --- a/drivers/clk/qcom/clk-alpha-pll.c
> > > +++ b/drivers/clk/qcom/clk-alpha-pll.c
> > > @@ -2126,7 +2126,7 @@ static void zonda_pll_adjust_l_val(unsigned
> > > long rate, unsigned long prate, u32
> > >       remainder = do_div(quotient, prate);
> > >       *l = quotient;
> > 
> > Since it's not a fix, but a simplification, you may wish to remove
> > an unnecessary 'quotient' local variable:
> > 
> > remainder = do_div(rate, prate);
> > 
> > > -    if ((remainder * 2) / prate)
> > > +    if ((remainder * 2) >= prate)
> > >           *l = *l + 1;
> > 
> > *l = rate + (u32)(remainder * 2 >= prate);
> 
> 
> The above change does fix this build error for me.
> 
> Satya, did you intend this to be a fix? Can we get this into -next?
> 
> Thanks
> Jon
> 
> -- 
> nvpublic

-- 
With best wishes
Dmitry