linux-kernel - RE: [PATCH] tty: serial: qcom-geni-serial: Fix get_clk_div_rate() which otherwise could return a sub-optimal clock rate.

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 29 Jun 2022 10:15:18 +0000
From:   "Vijaya Krishna Nivarthi (Temp) (QUIC)" <quic_vnivarth@...cinc.com>
To:     Doug Anderson <dianders@...omium.org>,
        "Vijaya Krishna Nivarthi (Temp) (QUIC)" <quic_vnivarth@...cinc.com>
CC:     Andy Gross <agross@...nel.org>,
        "bjorn.andersson@...aro.org" <bjorn.andersson@...aro.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        "linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        "Mukesh Savaliya (QUIC)" <quic_msavaliy@...cinc.com>,
        Matthias Kaehlcke <mka@...omium.org>,
        Stephen Boyd <swboyd@...omium.org>
Subject: RE: [PATCH] tty: serial: qcom-geni-serial: Fix get_clk_div_rate()
 which otherwise could return a sub-optimal clock rate.

Hi,


> > +
> > +               /*
> > +                * A dead-on freq is an insta-win, look for it only in 1st run
> > +                */
> > +               if (*exact_match) {
> > +                       if (!(freq % desired_clk)) {
> > +                               ser_clk = freq;
> > +                               *clk_div = freq / desired_clk;
> > +                               return ser_clk;
> > +                       }
> > +               }
> 
> The "*exact_match" if test isn't needed here. It's not saving you any
> significant amount of time. You're still doing an "if" test, right?
> ...so you're basically saving a mod operation by adding a pointer dereference
> and complexity? I don't think that's the right tradeoff.
> 
> 

Removed exact_match check from here.


> >
> > -       if (!ser_clk) {
> > -               pr_err("%s: Can't find matching DFS entry for baud %d\n",
> > -                                                               __func__, baud);
> > +       /* try to find exact clock rate or within 2% tolerance */
> > +       ser_clk = 0;
> > +       exact_match = true;
> > +       desired_tol = 2;
> 
> Don't need a "desired_tol" variable. Just pass 2 into the function.
> 
>

Done

 
> > +       ser_clk = find_clk_rate_in_tol(clk, desired_clk, clk_div, desired_tol,
> &exact_match);
> > +       if (ser_clk) {
> > +               if (!exact_match)
> > +                       pr_warn("Cannot find exact match clk_rate,
> > + using one within 2 percent tolerance\n");
> 
> IMO get rid of this printout. Just return what you found if it's not 0. It's
> perfectly fine. ...that means you can fully get rid of the "exact_match"
> variable.
> 
>

Done.
But retained exact_match as bool instead of pointer to help early out in 2nd call.
 
> >                 return ser_clk;
> >         }
> >
> > -       *clk_div = ser_clk / desired_clk;
> > -       if (!(*clk_div))
> > -               *clk_div = 1;
> > +       /* try within 5% tolerance now, no need to look for exact match */
> > +       exact_match = false;
> > +       desired_tol = 5;
> > +
> > +       ser_clk = find_clk_rate_in_tol(clk, desired_clk, clk_div, desired_tol,
> &exact_match);
> > +       if (ser_clk)
> > +               pr_warn("Cannot find exact match clk_rate, using one within 5
> percent tolerance\n");
> > +       else
> > +               pr_err("Cannot find suitable clk_rate, giving up\n");
> 
> Just keep the error message but not the warning. ...and ideally use "dev_err"
> and print out the clock you were trying to achieve.

Done. Retained pr_err since dev wasn’t readily available here.

Thank you.