Re: [V3] tty: serial: qcom-geni-serial: Fix get_clk_div_rate() which otherwise could return a sub-optimal clock rate.

[Vijaya Krishna Nivarthi]

On 7/8/2022 6:41 PM, Greg KH wrote:
> On Fri, Jul 08, 2022 at 12:47:37AM +0530, Vijaya Krishna Nivarthi wrote:
> > In the logic around call to clk_round_rate(), for some corner conditions,
> > get_clk_div_rate() could return an sub-optimal clock rate. Also, if an
> > exact clock rate was not found lowest clock was being returned.
> >
> > Search for suitable clock rate in 2 steps
> > a) exact match or within 2% tolerance
> > b) within 5% tolerance
> > This also takes care of corner conditions.
> >
> > Fixes: c2194bc999d4 ("tty: serial: qcom-geni-serial: Remove uart frequency table. Instead, find suitable frequency with call to clk_round_rate")
> > Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@xxxxxxxxxxx>
> > ---
> > v3: simplified algorithm further, fixed robot compile warnings
> > v2: removed minor optimisations to make more readable
> > v1: intial patch contained slightly complicated logic
> > ---
> >   drivers/tty/serial/qcom_geni_serial.c | 88 +++++++++++++++++++++--------------
> >   1 file changed, 53 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> > index 2e23b65..ac2df1c 100644
> > --- a/drivers/tty/serial/qcom_geni_serial.c
> > +++ b/drivers/tty/serial/qcom_geni_serial.c
> > @@ -943,52 +943,71 @@ static int qcom_geni_serial_startup(struct uart_port *uport)
> >   	return 0;
> >   }
> >   
> > -static unsigned long get_clk_div_rate(struct clk *clk, unsigned int baud,
> > -			unsigned int sampling_rate, unsigned int *clk_div)
> > +static unsigned long find_clk_rate_in_tol(struct clk *clk, unsigned int desired_clk,
> > +			unsigned int *clk_div, unsigned int percent_tol)
> >   {
> > -	unsigned long ser_clk;
> > -	unsigned long desired_clk;
> > -	unsigned long freq, prev;
> > +	unsigned long freq;
> >   	unsigned long div, maxdiv;
> > -	int64_t mult;
> > -
> > -	desired_clk = baud * sampling_rate;
> > -	if (!desired_clk) {
> > -		pr_err("%s: Invalid frequency\n", __func__);
> > -		return 0;
> > -	}
> > +	u64 mult;
> > +	unsigned long offset, abs_tol, achieved;
> >   
> > +	abs_tol = div_u64((u64)desired_clk * percent_tol, 100);
> >   	maxdiv = CLK_DIV_MSK >> CLK_DIV_SHFT;
> > -	prev = 0;
> > -
> > -	for (div = 1; div <= maxdiv; div++) {
> > -		mult = div * desired_clk;
> > -		if (mult > ULONG_MAX)
> > +	div = 1;
> > +	while (div <= maxdiv) {
> > +		mult = (u64)div * desired_clk;
> > +		if (mult != (unsigned long)mult)
> >   			break;
> >   
> > -		freq = clk_round_rate(clk, (unsigned long)mult);
> > -		if (!(freq % desired_clk)) {
> > -			ser_clk = freq;
> > -			break;
> > -		}
> > +		offset = div * abs_tol;
> > +		freq = clk_round_rate(clk, mult - offset);
> >   
> > -		if (!prev)
> > -			ser_clk = freq;
> > -		else if (prev == freq)
> > +		/* Can only get lower if we're done */
> > +		if (freq < mult - offset)
> >   			break;
> >   
> > -		prev = freq;
> > +		/*
> > +		 * Re-calculate div in case rounding skipped rates but we
> > +		 * ended up at a good one, then check for a match.
> > +		 */
> > +		div = DIV_ROUND_CLOSEST(freq, desired_clk);
> > +		achieved = DIV_ROUND_CLOSEST(freq, div);
> > +		if (achieved <= desired_clk + abs_tol &&
> > +			achieved >= desired_clk - abs_tol) {
> > +			*clk_div = div;
> > +			return freq;
> > +		}
> > +
> > +		div = DIV_ROUND_UP(freq, desired_clk);
> >   	}
> >   
> > -	if (!ser_clk) {
> > -		pr_err("%s: Can't find matching DFS entry for baud %d\n",
> > -								__func__, baud);
> > -		return ser_clk;
> > +	return 0;
> > +}
> > +
> > +static unsigned long get_clk_div_rate(struct clk *clk, unsigned int baud,
> > +			unsigned int sampling_rate, unsigned int *clk_div)
> > +{
> > +	unsigned long ser_clk;
> > +	unsigned long desired_clk;
> > +
> > +	desired_clk = baud * sampling_rate;
> > +	if (!desired_clk) {
> > +		pr_err("%s: Invalid frequency\n", __func__);
> Note, this is a driver, ALWAYS use dev_err() and friends instead.
>
> Also do not allow userspace to flood the kernel logs like this looks is
> possible, this should just be dev_dbg().
>
> And of course, never use __func__, it's not needed anymore for
> dev_dbg().

Ok.

> > +		return 0;
> Why if you have a error, are you returning 0?

Yes, and it has been so earlier too.

0 is an invalid clock rate and will be handled accordingly by caller.

> >   	}
> >   
> > -	*clk_div = ser_clk / desired_clk;
> > -	if (!(*clk_div))
> > -		*clk_div = 1;
> > +	/*
> > +	 * try to find a clock rate within 2% tolerance, then within
> > +	 */
> > +	ser_clk = find_clk_rate_in_tol(clk, desired_clk, clk_div, 2);
> > +	if (!ser_clk)
> > +		ser_clk = find_clk_rate_in_tol(clk, desired_clk, clk_div, 5);
> > +
> > +	if (!ser_clk)
> > +		pr_err("Couldn't find suitable clock rate for %lu\n", desired_clk);
> return an error?
>
> dev_err().

As mentioned, we didn't (and don't) return error from here but 0.

> > +	else
> > +		pr_debug("desired_clk-%lu, ser_clk-%lu, clk_div-%lu\n",
> > +			desired_clk, ser_clk, *clk_div);
> dev_dbg()?
Ok.
> Also, as the kernel test robot says, this does not build cleanly :(

change to dev_dbg should take care of these.

Will do.

Thank you.

Vijay/


> thanks,
>
> greg k-h