diff mbox

clk: tegra: ensure all provided clock values are valid cookies

Message ID 1358187400-6824-1-git-send-email-swarren@wwwdotorg.org (mailing list archive)
State New, archived
Headers show

Commit Message

Stephen Warren Jan. 14, 2013, 6:16 p.m. UTC
From: Stephen Warren <swarren@nvidia.com>

Tegra's clock implementation uses pointers as the clock cookies, and
hence chooses to make NULL an invalid clock cookie. However, there are
gaps in the assigned device tree clock IDs, and hence the array mapping
DT clock ID contains entries with NULL values (uninitialized BSS) unless
explicit action is taken. This patch enhances the Tegra clock code to
detect this case and explicitly initialize those lookup table entries to
an error value. This prevents clk_get() from ever returning NULL. Hence,
Tegra's clock APIs don't have to check the clock cookie they're passed
for NULL.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
Mike, this also will need to go through the Tegra tree; just looking for
any review/ack from you. Thanks.

 drivers/clk/tegra/clk-tegra20.c |    5 ++++-
 drivers/clk/tegra/clk-tegra30.c |    5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

Comments

Mike Turquette Jan. 22, 2013, 6:29 p.m. UTC | #1
Quoting Stephen Warren (2013-01-14 10:16:40)
> From: Stephen Warren <swarren@nvidia.com>
> 
> Tegra's clock implementation uses pointers as the clock cookies, and
> hence chooses to make NULL an invalid clock cookie. However, there are
> gaps in the assigned device tree clock IDs, and hence the array mapping
> DT clock ID contains entries with NULL values (uninitialized BSS) unless
> explicit action is taken. This patch enhances the Tegra clock code to
> detect this case and explicitly initialize those lookup table entries to
> an error value. This prevents clk_get() from ever returning NULL. Hence,
> Tegra's clock APIs don't have to check the clock cookie they're passed
> for NULL.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> Mike, this also will need to go through the Tegra tree; just looking for
> any review/ack from you. Thanks.
> 

Stephen,

I think this is a bit strange.  Will you have gaps in the DT clock id's
forever or is this a temporary issue?

Also it is important than any driver using the clock api does not
dereference the struct clk pointer and assume that a NULL clk is
invalid.  I know that this series doesn't imply any such thing but I
wanted to state that anyways.

Also are you planning to roll this into the existing tegra ccf series?

Regards,
Mike

>  drivers/clk/tegra/clk-tegra20.c |    5 ++++-
>  drivers/clk/tegra/clk-tegra30.c |    5 ++++-
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
> index 5847b5e..e59ac14 100644
> --- a/drivers/clk/tegra/clk-tegra20.c
> +++ b/drivers/clk/tegra/clk-tegra20.c
> @@ -1243,12 +1243,15 @@ void __init tegra20_clock_init(struct device_node *np)
>         tegra20_audio_clk_init();
>  
>  
> -       for (i = 0; i < ARRAY_SIZE(clks); i++)
> +       for (i = 0; i < ARRAY_SIZE(clks); i++) {
>                 if (IS_ERR(clks[i])) {
>                         pr_err("Tegra20 clk %d: register failed with %ld\n",
>                                i, PTR_ERR(clks[i]));
>                         BUG();
>                 }
> +               if (!clks[i])
> +                       clks[i] = ERR_PTR(-EINVAL);
> +       }
>  
>         tegra_init_dup_clks(tegra_clk_duplicates, clks, clk_max);
>  
> diff --git a/drivers/clk/tegra/clk-tegra30.c b/drivers/clk/tegra/clk-tegra30.c
> index 987312c..9c0b2ee 100644
> --- a/drivers/clk/tegra/clk-tegra30.c
> +++ b/drivers/clk/tegra/clk-tegra30.c
> @@ -2022,12 +2022,15 @@ void __init tegra30_clock_init(struct device_node *np)
>         tegra30_audio_clk_init();
>         tegra30_pmc_clk_init();
>  
> -       for (i = 0; i < ARRAY_SIZE(clks); i++)
> +       for (i = 0; i < ARRAY_SIZE(clks); i++) {
>                 if (IS_ERR(clks[i])) {
>                         pr_err("Tegra30 clk %d: register failed with %ld\n",
>                                i, PTR_ERR(clks[i]));
>                         BUG();
>                 }
> +               if (!clks[i])
> +                       clks[i] = ERR_PTR(-EINVAL);
> +       }
>  
>         tegra_init_dup_clks(tegra_clk_duplicates, clks, clk_max);
>  
> -- 
> 1.7.10.4
Stephen Warren Jan. 22, 2013, 6:38 p.m. UTC | #2
On 01/22/2013 11:29 AM, Mike Turquette wrote:
> Quoting Stephen Warren (2013-01-14 10:16:40)
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> Tegra's clock implementation uses pointers as the clock cookies, and
>> hence chooses to make NULL an invalid clock cookie. However, there are
>> gaps in the assigned device tree clock IDs, and hence the array mapping
>> DT clock ID contains entries with NULL values (uninitialized BSS) unless
>> explicit action is taken. This patch enhances the Tegra clock code to
>> detect this case and explicitly initialize those lookup table entries to
>> an error value. This prevents clk_get() from ever returning NULL. Hence,
>> Tegra's clock APIs don't have to check the clock cookie they're passed
>> for NULL.
>>
>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>> ---
>> Mike, this also will need to go through the Tegra tree; just looking for
>> any review/ack from you. Thanks.
> 
> Stephen,
> 
> I think this is a bit strange.  Will you have gaps in the DT clock id's
> forever or is this a temporary issue?

Forever as far as I know. Since the clock IDs are just arbitrary tokens,
I don't see any particular need for them to be strictly packed.

> Also it is important than any driver using the clock api does not
> dereference the struct clk pointer and assume that a NULL clk is
> invalid.  I know that this series doesn't imply any such thing but I
> wanted to state that anyways.

Right. This ensures that clk_get() returns an IS_ERR() pointer, or a
valid cookie that the clock driver is prepared to handle (which doesn't
include NULL).

> Also are you planning to roll this into the existing tegra ccf series?

It's already rolled into the latest version of the series which I posted.
diff mbox

Patch

diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
index 5847b5e..e59ac14 100644
--- a/drivers/clk/tegra/clk-tegra20.c
+++ b/drivers/clk/tegra/clk-tegra20.c
@@ -1243,12 +1243,15 @@  void __init tegra20_clock_init(struct device_node *np)
 	tegra20_audio_clk_init();
 
 
-	for (i = 0; i < ARRAY_SIZE(clks); i++)
+	for (i = 0; i < ARRAY_SIZE(clks); i++) {
 		if (IS_ERR(clks[i])) {
 			pr_err("Tegra20 clk %d: register failed with %ld\n",
 			       i, PTR_ERR(clks[i]));
 			BUG();
 		}
+		if (!clks[i])
+			clks[i] = ERR_PTR(-EINVAL);
+	}
 
 	tegra_init_dup_clks(tegra_clk_duplicates, clks, clk_max);
 
diff --git a/drivers/clk/tegra/clk-tegra30.c b/drivers/clk/tegra/clk-tegra30.c
index 987312c..9c0b2ee 100644
--- a/drivers/clk/tegra/clk-tegra30.c
+++ b/drivers/clk/tegra/clk-tegra30.c
@@ -2022,12 +2022,15 @@  void __init tegra30_clock_init(struct device_node *np)
 	tegra30_audio_clk_init();
 	tegra30_pmc_clk_init();
 
-	for (i = 0; i < ARRAY_SIZE(clks); i++)
+	for (i = 0; i < ARRAY_SIZE(clks); i++) {
 		if (IS_ERR(clks[i])) {
 			pr_err("Tegra30 clk %d: register failed with %ld\n",
 			       i, PTR_ERR(clks[i]));
 			BUG();
 		}
+		if (!clks[i])
+			clks[i] = ERR_PTR(-EINVAL);
+	}
 
 	tegra_init_dup_clks(tegra_clk_duplicates, clks, clk_max);