linux-kernel - Re: [PATCH V2 4/9] clk: imx: Support building i.MX common clock driver as module

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: <159280954666.62212.15832087518734077634@swboyd.mtv.corp.google.com>
Date:   Mon, 22 Jun 2020 00:05:46 -0700
From:   Stephen Boyd <sboyd@...nel.org>
To:     Anson Huang <Anson.Huang@....com>, abel.vesa@....com,
        aisheng.dong@....com, allison@...utok.net, arnd@...db.de,
        daniel.baluta@....com, festevam@...il.com, fugang.duan@....com,
        gregkh@...uxfoundation.org, info@...ux.net, kernel@...gutronix.de,
        leonard.crestez@....com, linux-arm-kernel@...ts.infradead.org,
        linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux@...linux.org.uk, mturquette@...libre.com,
        oleksandr.suvorov@...adex.com, peng.fan@....com,
        s.hauer@...gutronix.de, sfr@...b.auug.org.au, shawnguo@...nel.org,
        stefan.agner@...adex.com, tglx@...utronix.de, yuehaibing@...wei.com
Cc:     Linux-imx@....com
Subject: Re: [PATCH V2 4/9] clk: imx: Support building i.MX common clock driver as module

Quoting Anson Huang (2020-06-09 00:32:08)
> diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile
> index 1af8cff..1291f9b 100644
> --- a/drivers/clk/imx/Makefile
> +++ b/drivers/clk/imx/Makefile
> @@ -1,25 +1,10 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
> -obj-$(CONFIG_MXC_CLK) += \
> -       clk.o \
> -       clk-busy.o \
> -       clk-composite-8m.o \
> -       clk-cpu.o \
> -       clk-composite-7ulp.o \
> -       clk-divider-gate.o \
> -       clk-fixup-div.o \
> -       clk-fixup-mux.o \
> -       clk-frac-pll.o \
> -       clk-gate-exclusive.o \
> -       clk-gate2.o \
> -       clk-pfd.o \
> -       clk-pfdv2.o \
> -       clk-pllv1.o \
> -       clk-pllv2.o \
> -       clk-pllv3.o \
> -       clk-pllv4.o \
> -       clk-sscg-pll.o \
> -       clk-pll14xx.o
> +mxc-clk-objs += clk.o clk-busy.o clk-composite-8m.o clk-cpu.o clk-composite-7ulp.o \
> +               clk-divider-gate.o clk-fixup-div.o clk-fixup-mux.o clk-frac-pll.o \
> +               clk-gate-exclusive.o clk-gate2.o clk-pfd.o clk-pfdv2.o clk-pllv1.o \
> +               clk-pllv2.o clk-pllv3.o clk-pllv4.o clk-sscg-pll.o clk-pll14xx.o
> +obj-$(CONFIG_MXC_CLK) += mxc-clk.o

I enjoyed it when the files were all on their own line. Can you keep it
like that and just add mxc-clk-objs += to all the lines and remove the
trailing slash?

>  
>  mxc-clk-scu-objs += clk-scu.o clk-lpcg-scu.o
>  obj-$(CONFIG_MXC_CLK_SCU) += mxc-clk-scu.o
> diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
> index d2b5af8..73e064b 100644
> --- a/drivers/clk/imx/clk-composite-8m.c
> +++ b/drivers/clk/imx/clk-composite-8m.c
> @@ -243,3 +243,4 @@ struct clk_hw *imx8m_clk_hw_composite_flags(const char *name,
>         kfree(mux);
>         return ERR_CAST(hw);
>  }
> +EXPORT_SYMBOL_GPL(imx8m_clk_hw_composite_flags);

Are all these files including <linux/export.h>? Because they should
unless they're using something from <linux/module.h> like
MODULE_LICENSE().

> diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c
> index 87ab8db..cc894b5 100644
> --- a/drivers/clk/imx/clk.c
> +++ b/drivers/clk/imx/clk.c
> @@ -143,16 +148,24 @@ void imx_cscmr1_fixup(u32 *val)
>  static int imx_keep_uart_clocks;
>  static struct clk ** const *imx_uart_clocks;
>  
> -static int __init imx_keep_uart_clocks_param(char *str)
> +static int __maybe_unused imx_keep_uart_clocks_param(char *str)
>  {
>         imx_keep_uart_clocks = 1;
>  
>         return 0;
>  }
> +
> +#ifdef MODULE
> +__setup_param("earlycon", imx_keep_uart_earlycon,
> +             imx_keep_uart_clocks_param);
> +__setup_param("earlyprintk", imx_keep_uart_earlyprintk,
> +             imx_keep_uart_clocks_param);

Why not fix __setup_param() to take the same number of arguments in
either case? That macro looks broken.

> +#else
>  __setup_param("earlycon", imx_keep_uart_earlycon,
>               imx_keep_uart_clocks_param, 0);
>  __setup_param("earlyprintk", imx_keep_uart_earlyprintk,
>               imx_keep_uart_clocks_param, 0);
> +#endif
>  
>  void imx_register_uart_clocks(struct clk ** const clks[])
>  {