linux-kernel - Re: [RFC PATCH] clk: fractional-divider: Correct max_{m,n} handed over to rational_best

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]

Date:   Wed, 14 Jul 2021 18:10:46 +0800
From:   Liu Ying <victor.liu@....com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Dong Aisheng <aisheng.dong@....com>,
        NXP Linux Team <linux-imx@....com>,
        Jacky Bai <ping.bai@....com>
Subject: Re: [RFC PATCH] clk: fractional-divider: Correct max_{m,n} handed
 over to rational_best_approximation()

On Wed, 2021-07-14 at 12:12 +0300, Andy Shevchenko wrote:
> On Wed, Jul 14, 2021 at 02:41:29PM +0800, Liu Ying wrote:
> > If a fractional divider clock has the flag
> > CLK_FRAC_DIVIDER_ZERO_BASED set, the maximum
> > numerator and denominator handed over to
> > rational_best_approximation(), in this case
> > max_m and max_n, should be increased by one
> > comparing to those have the flag unset.  Without
> > this patch, a zero based fractional divider
> > with 1-bit mwidth and 3-bit nwidth would wrongly
> > generate 96MHz clock rate if the parent clock
> > rate is 288MHz, while the expected clock rate
> > is 115.2MHz with m = 2 and n = 5.
> 
> Make sure that your editor is configured to allow you to have lines ~70-72
> characters long.

Alright, I'll see if I can improve this in v2 if necessary.

> 
> ...
> 
> > The patch is RFC, because the rationale behind the below snippet in
> > clk_fd_general_approximation() is unclear to Jacky and me and we are
> > not sure if there is any room to improve this patch due to the snippet.
> > Maybe, Andy may help shed some light here.  Thanks.
> > 
> > -----------------------------------8<---------------------------------
> > /*
> >  * Get rate closer to *parent_rate to guarantee there is no overflow
> >  * for m and n. In the result it will be the nearest rate left shifted
> >  * by (scale - fd->nwidth) bits.
> >  */
> 
> I don't know how to rephrase above comment better.
> 
> > scale = fls_long(*parent_rate / rate - 1);
> > if (scale > fd->nwidth)
> > 	rate <<= scale - fd->nwidth;
> 
> This takes an advantage of the numbers be in a form of
> 
> 	n = k * 2^m, (1)
> 
> where m will be scale in the snippet above. Thus, if n can be represented by
> (1), we opportunistically reduce amount of bits needed for it by shifting right
> by m bits.
> 
> Does it make sense?

Thanks for your explaination.
But, sorry, Jacky and I still don't understand this.

> 
> The code looks good to me, btw, although I dunno if you need to call the newly
> introduced function before or after the above mentioned snippet.

Assuming that snippet is fully orthogonal to this patch, then it
doesn't matter if it's before or after.

So, enlightenment/comments/ideas are welcome.

Thanks,
Liu Ying


>