linux-kernel - Re: [Patch v9] driver/clk/clk-si5338: Add common clock framework driver for si5338

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

Date:   Thu, 06 Jun 2019 11:05:07 -0700
From:   Stephen Boyd <sboyd@...nel.org>
To:     Radu Nicolae Pirea <radu_nicolae.pirea@....ro>,
        York Sun <york.sun@....com>, linux-clk@...r.kernel.org
Cc:     York Sun <yorksun@...escale.com>,
        Mike Turquette <mturquette@...libre.com>,
        Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Andrey Filippov <andrey@...hel.com>,
        Paul Bolle <pebolle@...cali.nl>,
        Stephen Boyd <sboyd@...eaurora.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [Patch v9] driver/clk/clk-si5338: Add common clock framework driver for si5338

Quoting Radu Nicolae Pirea (2019-05-31 07:06:03)
> > +     remove_common_factor(&rate[1]);
> > +     dev_dbg(&drvdata->client->dev,
> > +             "PLL output frequency: %llu+%llu/%llu Hz\n",
> > +             rate[0], rate[1], rate[2]);
> > +
> > +     return rate[0];
> > +}
> > +
> > +static long si5338_pll_round_rate(struct clk_hw *hw, unsigned long
> > rate,
> > +                               unsigned long *parent_rate)
> > +{
> I think is a designs problem in clock subsystem.
> Description of the function round_rate is this.
> 
> @round_rate: Given a target rate as input, returns the closest rate
> actually supported by the clock. The parent rate is an input/output
> parameter.
> 
> If given rate is unsigned long and the return value is long, we have a
> problem. Return value can be an error value but can also be a 32 bit
> frequency value that can be huge enoguh to set MSB bit and if MSB bit
> is set, the return value is a negative value and will be considered
> error code.
> 
> In drivers/clk/clk.c, in function clk_core_determine_round_nolock, on
> line 1199 is this sequence of code that checks if the value returned by
> round_rate function is negative:
> 
>          } else if (core->ops->round_rate) {
>                  rate = core->ops->round_rate(core->hw, req->rate,
>                                               &req->best_parent_rate);
>                  if (rate < 0)
>                          return rate;
> 
> In drivers/clk/clk.c, in function clk_calc_new_rates, on line 1780, is
> the next sequence of code that checks if
> clk_core_determine_round_nolock returns a negative value:
> 
>                  ret = clk_core_determine_round_nolock(core, &req);
>                  if (ret < 0)
>                           return NULL;
> 
> and... if function clk_calc_new_rates returns NULL, in function
> clk_core_set_rate_nolock from drivers/clk/clk.c, on line 2023, is the
> next sequence of code that evaluates NULL, returned by
> clk_calc_new_rates as -EINVAL
> 
>          top = clk_calc_new_rates(core, req_rate);
>          if (!top)
>                  return -EINVAL;
> 
> So... si5338_pll_round_rate can return 32 bit values like 2500000000 Hz
> who have MSB bit set and are interpreted as error codes.
> 
> Have someone a suggestion to fix this issue?
> 

Can you use the determine_rate clk op? It won't work for rounding
outside of the framework though so things like clk_round_rate() still
fail.