linux-kernel - Re: [PATCH v2 3/5] clk: aspeed: Add platform driver and register PLLs

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, 5 Oct 2017 15:52:50 +0930
From:   Joel Stanley <joel@....id.au>
To:     Stephen Boyd <sboyd@...eaurora.org>
Cc:     Lee Jones <lee.jones@...aro.org>,
        Michael Turquette <mturquette@...libre.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-clk@...r.kernel.org,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Andrew Jeffery <andrew@...id.au>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Jeremy Kerr <jk@...abs.org>,
        Rick Altherr <raltherr@...gle.com>,
        Ryan Chen <ryan_chen@...eedtech.com>,
        Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH v2 3/5] clk: aspeed: Add platform driver and register PLLs

On Thu, Oct 5, 2017 at 6:48 AM, Stephen Boyd <sboyd@...eaurora.org> wrote:
> On 10/03, Joel Stanley wrote:
>> On Tue, Oct 3, 2017 at 6:54 AM, Stephen Boyd <sboyd@...eaurora.org> wrote:
>> > On 09/21, Joel Stanley wrote:
>> >> @@ -160,6 +191,104 @@ static struct clk_hw *aspeed_calc_pll(const char *name, u32 val)
>> >> +     /*
>> >> +      * Memory controller (M-PLL) PLL. This clock is configured by the
>> >> +      * bootloader, and is exposed to Linux as a read-only clock rate.
>> >> +      */
>> >> +     regmap_read(map, ASPEED_MPLL_PARAM, &val);
>> >> +     aspeed_clk_data->hws[ASPEED_CLK_MPLL] = aspeed_calc_pll("mpll", val);
>> >> +
>> >> +     /* SD/SDIO clock divider (TODO: There's a gate too) */
>> >> +     hw = clk_hw_register_divider_table(NULL, "sdio", "hpll", 0,
>> >
>> > Please pass your dev pointer here from the platform device.
>> >
>> >> +                     scu_base + ASPEED_CLK_SELECTION, 12, 3, 0,
>> >> +                     div_table,
>> >> +                     &aspeed_clk_lock);
>> >
>> > And check for errors? Perhaps use devm_clk_hw_regsiter() APIs and
>> > construct the dividers and muxes directly instead of using the
>> > basic type registration APIs.
>>
>> Do you think that devm_ is overkill, given we will never unload this driver?
>
> Is probe defer going to happen? Even if unload can't happen,
> probe defer is a concern unless that is also ruled out.

As I understand it, I will only get an EPROBE_DEFER if I'm requesting
something from another driver?

If that's the case then we can rule that out.

>>
>> Can you explain why you suggest to construct the structures directly
>> instead of using the APIs?
>
> There aren't devm APIs for some of these basic clk type
> registration functions.
>
>>
>> I had a read of the basic type registration functions, and the
>> relevant failure paths are memory allocation failures. If we're out of
>> memory that early in boot then things have gone pretty bad.
>>
>> I can add checks for null and bail out; I don't think there's value in
>> freeing the allocated memory: if a system can't load it's clock driver
>> then it's super hosed.
>
> If we can't proceed without this driver because it's super hosed,
> then perhaps we need to panic the system on errors here. Should
> be simple enough to add some error checks and goto panic("Things
> are super hosed").

Okay. I added checks in v4. Does that look ok you you?

Cheers,

Joel