linux-kernel - Re: [PATCH v12 04/10] clk: use struct clk only for external API

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]

Message-ID: <20140922180807.GN5182@n2100.arm.linux.org.uk>
Date:	Mon, 22 Sep 2014 19:08:07 +0100
From:	Russell King - ARM Linux <linux@....linux.org.uk>
To:	Tomeu Vizoso <tomeu.vizoso@...labora.com>
Cc:	Mike Turquette <mturquette@...aro.org>,
	Stephen Warren <swarren@...dotorg.org>,
	Peter De Schrijver <pdeschrijver@...dia.com>,
	linux-kernel@...r.kernel.org, tomasz.figa@...il.com, rabin@....in,
	Thierry Reding <thierry.reding@...il.com>,
	Javier Martinez Canillas <javier.martinez@...labora.co.uk>,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v12 04/10] clk: use struct clk only for external API

On Mon, Sep 22, 2014 at 04:15:47PM +0200, Tomeu Vizoso wrote:
> diff --git a/drivers/clk/clk.h b/drivers/clk/clk.h
> index d278572..3b3068b 100644
> --- a/drivers/clk/clk.h
> +++ b/drivers/clk/clk.h
> @@ -9,9 +9,15 @@
>   * published by the Free Software Foundation.
>   */
>  
> +#include <linux/clk-private.h>
> +
>  #if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
>  struct clk_core *of_clk_get_by_clkspec(struct of_phandle_args *clkspec);
>  struct clk_core *__of_clk_get_from_provider(struct of_phandle_args *clkspec);
>  void of_clk_lock(void);
>  void of_clk_unlock(void);
>  #endif
> +
> +#if defined(CONFIG_COMMON_CLK)
> +struct clk *__clk_create_clk(struct clk_core *clk_core);
> +#endif

Why does any of the above need ifdefs?

> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
> index c751d0c..060041b 100644
> --- a/drivers/clk/clkdev.c
> +++ b/drivers/clk/clkdev.c
> @@ -18,6 +18,7 @@
>  #include <linux/string.h>
>  #include <linux/mutex.h>
>  #include <linux/clk.h>
> +#include <linux/clk-private.h>
>  #include <linux/clk-provider.h>
>  #include <linux/clkdev.h>
>  #include <linux/of.h>
> @@ -33,13 +34,13 @@ static DEFINE_MUTEX(clocks_mutex);

I'm not happy with any of these changes, because it introduces a huge
number of ifdefs.

Instead of doing this, please introduce a local typedef in this .c file
which is conditional on whether we're using your new struct clk_core
or not, eg, clkdev_ret_t.

Then, rename all the functions here to be _provider based, and arrange
for these to return clkdev_ret_t.

Then provide the original functions, with your __clk_create_clk() call
to allocate your struct clk.  A dummy version which merely returns its
argument can be provided when we're not using clk_core.

This should result in substantially fewer ifdefs - probably just one
ifdef.

Another concern here is what happens when __clk_create_clk() fails.
Your __clk_create_clk() function does nothing in that regard, so any
references which the parent function took are never released.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/