linux-kernel - Re: [PATCH v5 1/9] clkdev: add managed clkdev lookup registration

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:   Mon, 3 Dec 2018 18:06:12 +0000
From:   Russell King - ARM Linux <linux@...linux.org.uk>
To:     Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
Cc:     mazziesaccount@...il.com, mturquette@...libre.com,
        sboyd@...nel.org, cw00.choi@...sung.com, krzk@...nel.org,
        b.zolnierkie@...sung.com, andy.gross@...aro.org,
        david.brown@...aro.org, pavel@....cz, andrew.smirnov@...il.com,
        lee.jones@...aro.org, pombredanne@...b.com, sjhuang@...vatar.ai,
        akshu.agrawal@....com, djkurtz@...omium.org,
        rafael.j.wysocki@...el.com, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v5 1/9] clkdev: add managed clkdev lookup registration

Hi,

On Mon, Dec 03, 2018 at 02:17:56PM +0200, Matti Vaittinen wrote:
> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
> index 9ab3db8b3988..68a1e55a60d2 100644
> --- a/drivers/clk/clkdev.c
> +++ b/drivers/clk/clkdev.c
> @@ -401,6 +401,25 @@ static struct clk_lookup *__clk_register_clkdev(struct clk_hw *hw,
>  	return cl;
>  }
>  
> +static int do_clk_register_clkdev(struct clk_hw *hw,
> +	struct clk_lookup **cl, const char *con_id, const char *dev_id)
> +{
> +

No need for this blank line.

> +	if (IS_ERR(hw))
> +		return PTR_ERR(hw);
> +	/*
> +	 * Since dev_id can be NULL, and NULL is handled specially, we must
> +	 * pass it as either a NULL format string, or with "%s".
> +	 */
> +	if (dev_id)
> +		*cl = __clk_register_clkdev(hw, con_id, "%s",
> +					   dev_id);

Please move "dev_id);" onto the line above.

> @@ -420,20 +439,10 @@ int clk_register_clkdev(struct clk *clk, const char *con_id,
>  {
>  	struct clk_lookup *cl;
>  
> -	if (IS_ERR(clk))
> -		return PTR_ERR(clk);
> -
> -	/*
> -	 * Since dev_id can be NULL, and NULL is handled specially, we must
> -	 * pass it as either a NULL format string, or with "%s".
> -	 */
> -	if (dev_id)
> -		cl = __clk_register_clkdev(__clk_get_hw(clk), con_id, "%s",
> -					   dev_id);
> -	else
> -		cl = __clk_register_clkdev(__clk_get_hw(clk), con_id, NULL);
> -
> -	return cl ? 0 : -ENOMEM;
> +	if (!IS_ERR(clk))
> +		return do_clk_register_clkdev(__clk_get_hw(clk), &cl, con_id,
> +					      dev_id);
> +	return PTR_ERR(clk);

Please keep this as:

	if (IS_ERR(clk))
		return PTR_ERR(clk);

	return do_clk_register_clkdev(__clk_get_hw(clk), &cl, con_id,
				      dev_id);

rather than inverting it - it makes it more consistent with code elsewhere.

> +static int devm_clk_match_clkdev(struct device *dev, void *res, void *data)
> +{
> +	struct clk_lookup **l = res;
> +
> +	if (!l || !*l) {
> +		WARN_ON(!l || !*l);

How can "l" be NULL here?  How can *l be NULL?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up