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

Date:   Tue, 3 Oct 2017 15:18:00 +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 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?

Can you explain why you suggest to construct the structures directly
instead of using the APIs?

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.

Thanks for the review. I fixed all of the other things you mentioned.

Cheers,

Joel