All of lore.kernel.org
 help / color / mirror / Atom feed
From: Stephen Boyd <sboyd@kernel.org>
To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
	mturquette@baylibre.com, robh+dt@kernel.org
Cc: linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	haitao.suo@bitmain.com, darren.tsao@bitmain.com,
	fisher.cheng@bitmain.com, alec.lin@bitmain.com,
	Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Subject: Re: [PATCH v5 2/8] clk: Warn if clk_init_data is not zero initialized
Date: Tue, 17 Sep 2019 13:38:53 -0700	[thread overview]
Message-ID: <20190917203854.8CF702054F@mail.kernel.org> (raw)
In-Reply-To: <20190916161447.32715-3-manivannan.sadhasivam@linaro.org>

Quoting Manivannan Sadhasivam (2019-09-16 09:14:41)
> The new implementation for determining parent map uses multiple ways
> to pass parent info. The order in which it gets processed depends on
> the first available member. Hence, it is necessary to zero init the
> clk_init_data struct so that the expected member gets processed correctly.
> So, add a warning if multiple clk_init_data members are available during
> clk registration.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/clk/clk.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index c0990703ce54..7d6d6984c979 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3497,6 +3497,14 @@ static int clk_core_populate_parent_map(struct clk_core *core)
>         if (!num_parents)
>                 return 0;
>  
> +       /*
> +        * Check for non-zero initialized clk_init_data struct. This is
> +        * required because, we only require one of the (parent_names/
> +        * parent_data/parent_hws) to be set at a time. Otherwise, the
> +        * current code would use first available member.
> +        */
> +       WARN_ON((parent_names && parent_data) || (parent_names && parent_hws));
> +

This will warn for many drivers because they set clk_init_data on the
stack and assign parent_names but let junk from the stack be assigned to
parent_data. The code uses parent_names first and then looks for
parent_data or parent_hws because of this fact of life that we've never
required clk_init_data to be initialized to all zero.

>         /*
>          * Avoid unnecessary string look-ups of clk_core's possible parents by
>          * having a cache of names/clk_hw pointers to clk_core pointers.

WARNING: multiple messages have this Message-ID (diff)
From: Stephen Boyd <sboyd@kernel.org>
To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
	mturquette@baylibre.com, robh+dt@kernel.org
Cc: devicetree@vger.kernel.org,
	Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
	darren.tsao@bitmain.com, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, fisher.cheng@bitmain.com,
	alec.lin@bitmain.com, linux-clk@vger.kernel.org,
	haitao.suo@bitmain.com
Subject: Re: [PATCH v5 2/8] clk: Warn if clk_init_data is not zero initialized
Date: Tue, 17 Sep 2019 13:38:53 -0700	[thread overview]
Message-ID: <20190917203854.8CF702054F@mail.kernel.org> (raw)
In-Reply-To: <20190916161447.32715-3-manivannan.sadhasivam@linaro.org>

Quoting Manivannan Sadhasivam (2019-09-16 09:14:41)
> The new implementation for determining parent map uses multiple ways
> to pass parent info. The order in which it gets processed depends on
> the first available member. Hence, it is necessary to zero init the
> clk_init_data struct so that the expected member gets processed correctly.
> So, add a warning if multiple clk_init_data members are available during
> clk registration.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/clk/clk.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index c0990703ce54..7d6d6984c979 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3497,6 +3497,14 @@ static int clk_core_populate_parent_map(struct clk_core *core)
>         if (!num_parents)
>                 return 0;
>  
> +       /*
> +        * Check for non-zero initialized clk_init_data struct. This is
> +        * required because, we only require one of the (parent_names/
> +        * parent_data/parent_hws) to be set at a time. Otherwise, the
> +        * current code would use first available member.
> +        */
> +       WARN_ON((parent_names && parent_data) || (parent_names && parent_hws));
> +

This will warn for many drivers because they set clk_init_data on the
stack and assign parent_names but let junk from the stack be assigned to
parent_data. The code uses parent_names first and then looks for
parent_data or parent_hws because of this fact of life that we've never
required clk_init_data to be initialized to all zero.

>         /*
>          * Avoid unnecessary string look-ups of clk_core's possible parents by
>          * having a cache of names/clk_hw pointers to clk_core pointers.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

WARNING: multiple messages have this Message-ID (diff)
From: Stephen Boyd <sboyd@kernel.org>
To: mturquette@baylibre.com, robh+dt@kernel.org
Cc: linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	haitao.suo@bitmain.com, darren.tsao@bitmain.com,
	fisher.cheng@bitmain.com, alec.lin@bitmain.com,
	Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Subject: Re: [PATCH v5 2/8] clk: Warn if clk_init_data is not zero initialized
Date: Tue, 17 Sep 2019 13:38:53 -0700	[thread overview]
Message-ID: <20190917203854.8CF702054F@mail.kernel.org> (raw)
In-Reply-To: <20190916161447.32715-3-manivannan.sadhasivam@linaro.org>

Quoting Manivannan Sadhasivam (2019-09-16 09:14:41)
> The new implementation for determining parent map uses multiple ways
> to pass parent info. The order in which it gets processed depends on
> the first available member. Hence, it is necessary to zero init the
> clk_init_data struct so that the expected member gets processed correctly.
> So, add a warning if multiple clk_init_data members are available during
> clk registration.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/clk/clk.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index c0990703ce54..7d6d6984c979 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3497,6 +3497,14 @@ static int clk_core_populate_parent_map(struct clk_core *core)
>         if (!num_parents)
>                 return 0;
>  
> +       /*
> +        * Check for non-zero initialized clk_init_data struct. This is
> +        * required because, we only require one of the (parent_names/
> +        * parent_data/parent_hws) to be set at a time. Otherwise, the
> +        * current code would use first available member.
> +        */
> +       WARN_ON((parent_names && parent_data) || (parent_names && parent_hws));
> +

This will warn for many drivers because they set clk_init_data on the
stack and assign parent_names but let junk from the stack be assigned to
parent_data. The code uses parent_names first and then looks for
parent_data or parent_hws because of this fact of life that we've never
required clk_init_data to be initialized to all zero.

>         /*
>          * Avoid unnecessary string look-ups of clk_core's possible parents by
>          * having a cache of names/clk_hw pointers to clk_core pointers.

  reply	other threads:[~2019-09-17 20:38 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-16 16:14 [PATCH v5 0/8] Add Bitmain BM1880 clock driver Manivannan Sadhasivam
2019-09-16 16:14 ` Manivannan Sadhasivam
2019-09-16 16:14 ` [PATCH v5 1/8] clk: Zero init clk_init_data in helpers Manivannan Sadhasivam
2019-09-16 16:14   ` Manivannan Sadhasivam
2019-09-17 20:39   ` Stephen Boyd
2019-09-17 20:39     ` Stephen Boyd
2019-09-17 20:39     ` Stephen Boyd
2019-10-20 15:26     ` Manivannan Sadhasivam
2019-10-20 15:26       ` Manivannan Sadhasivam
2019-09-16 16:14 ` [PATCH v5 2/8] clk: Warn if clk_init_data is not zero initialized Manivannan Sadhasivam
2019-09-16 16:14   ` Manivannan Sadhasivam
2019-09-17 20:38   ` Stephen Boyd [this message]
2019-09-17 20:38     ` Stephen Boyd
2019-09-17 20:38     ` Stephen Boyd
2019-10-20 15:25     ` Manivannan Sadhasivam
2019-10-20 15:25       ` Manivannan Sadhasivam
2019-09-16 16:14 ` [PATCH v5 3/8] clk: Add clk_hw_unregister_composite helper function definition Manivannan Sadhasivam
2019-09-16 16:14   ` Manivannan Sadhasivam
2019-09-17 20:40   ` Stephen Boyd
2019-09-17 20:40     ` Stephen Boyd
2019-09-17 20:40     ` Stephen Boyd
2019-10-20 15:26     ` Manivannan Sadhasivam
2019-10-20 15:26       ` Manivannan Sadhasivam
2019-09-16 16:14 ` [PATCH v5 4/8] dt-bindings: clock: Add devicetree binding for BM1880 SoC Manivannan Sadhasivam
2019-09-16 16:14   ` Manivannan Sadhasivam
2019-09-16 18:02   ` Rob Herring
2019-09-16 18:02     ` Rob Herring
2019-09-16 16:14 ` [PATCH v5 5/8] arm64: dts: bitmain: Add clock controller support " Manivannan Sadhasivam
2019-09-16 16:14   ` Manivannan Sadhasivam
2019-09-16 16:14 ` [PATCH v5 6/8] arm64: dts: bitmain: Source common clock for UART controllers Manivannan Sadhasivam
2019-09-16 16:14   ` Manivannan Sadhasivam
2019-09-16 16:14 ` [PATCH v5 7/8] clk: Add common clock driver for BM1880 SoC Manivannan Sadhasivam
2019-09-16 16:14   ` Manivannan Sadhasivam
2019-09-18  4:47   ` Stephen Boyd
2019-09-18  4:47     ` Stephen Boyd
2019-09-18  4:47     ` Stephen Boyd
2019-10-20 15:40     ` Manivannan Sadhasivam
2019-10-20 15:40       ` Manivannan Sadhasivam
2019-09-16 16:14 ` [PATCH v5 8/8] MAINTAINERS: Add entry for BM1880 SoC clock driver Manivannan Sadhasivam
2019-09-16 16:14   ` Manivannan Sadhasivam

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190917203854.8CF702054F@mail.kernel.org \
    --to=sboyd@kernel.org \
    --cc=alec.lin@bitmain.com \
    --cc=darren.tsao@bitmain.com \
    --cc=devicetree@vger.kernel.org \
    --cc=fisher.cheng@bitmain.com \
    --cc=haitao.suo@bitmain.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=manivannan.sadhasivam@linaro.org \
    --cc=mturquette@baylibre.com \
    --cc=robh+dt@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.