| 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
| ||
|
Message-ID: <c91aab06-4263-8a96-3943-948cc64cdca6@quicinc.com>
Date: Mon, 11 Jul 2022 19:52:18 +0530
From: Vijaya Krishna Nivarthi <quic_vnivarth@...cinc.com>
To: Greg KH <gregkh@...uxfoundation.org>
CC: <agross@...nel.org>, <bjorn.andersson@...aro.org>,
<konrad.dybcio@...ainline.org>, <jirislaby@...nel.org>,
<linux-arm-msm@...r.kernel.org>, <linux-serial@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <quic_msavaliy@...cinc.com>,
<dianders@...omium.org>, <mka@...omium.org>, <swboyd@...omium.org>
Subject: Re: [V3] tty: serial: qcom-geni-serial: Fix get_clk_div_rate() which
otherwise could return a sub-optimal clock rate.
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@...cinc.com>
>> ---
>> 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
Powered by blists - more mailing lists