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

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

Date:   Wed, 4 Oct 2017 14:18:20 -0700
From:   Stephen Boyd <sboyd@...eaurora.org>
To:     Joel Stanley <joel@....id.au>
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 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.

> 
> 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").

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project