+ clk-add-non-config_have_clk-routines.patch added to -mm tree

PATCH New Fengguang Wu 3 participants 7 messages Jun 6, 2012 – Jun 8, 2012 4 messages not in the archive Common clk

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): 20120606214621.GA8892@localhost

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): 20120606215951.GA9123@localhost

A message that is not in the archive (sent off-list or not yet received): [email protected]

Fengguang Wu

author

2012-06-06 22:42 UTC #4 lore raw

Quoted 2 lines

I didn't merge this patchset because it still has the build error reported by Paul, below.

I see. The arm's redefinitions are mostly empty function stubs that are identical to the ones provided by Viresh's patch. Except for this one, trying to act smarter:

arch/arm/mach-netx/fb.c:

        struct clk *clk_get(struct device *dev, const char *id)
        {       
                return dev && strcmp(dev_name(dev), "fb") == 0 ? NULL : ERR_PTR(-ENOENT);
        }

The return values are interesting. In this arm, clk_get() conditionally returns NULL or -ENOENT. While the clk_get() in clk.c always returns -ENOENT on error. Now Viresh comes and defines a clk_get() that always returns NULL on !CONFIG_HAVE_CLK.

What would be the difference between NULL and -ENOENT?

In my superficial view, -ENOENT is more error prone than plain NULL. If ever clk_get() returns -ENOENT which is passed straight forward to clk_disable() (eg. in the below code), we may get a kernel panic. The below particular clk_get() might always succeed and hence be safe, but such use scenarios look scary.

drivers/char/hw_random/mxc-rnga.c mxc_rnga_remove():

        struct clk *clk = clk_get(&pdev->dev, "rng");

        [...]

        clk_disable(clk);

where clk_disable() only checks NULL:

        if (!clk)
                return;

Thanks,
Fengguang

Quoted 55 lines

Begin forwarded message:
Date: Fri, 25 May 2012 15:07:15 +0530
From: viresh kumar <[email protected]>
To: Paul Gortmaker <[email protected]>
Cc: linux-arm-kernel at lists.infradead.org, linux-next at vger.kernel.org, Andrew Morton <[email protected]>, spear-devel at list.st.com
Subject: Re: [linux-next] "clk: add non CONFIG_HAVE_CLK routines" commit
Quoted 7 lines · Paul Gortmaker
Hi Viresh,
The above listed commit, in linux-next as:
commit ebbf0cb5d39cc3e22ef4c425475e76b7f45027de
Quoted 8 lines
"clk: add non CONFIG_HAVE_CLK routines"
shows up as failures in the netx_defconfig, since there are duplicate stub functions between your changes and the file below:
arch/arm/mach-netx/fb.c:72:6: error: redefinition of 'clk_disable' include/linux/clk.h:299:51: note: previous definition of 'clk_disable'
was here
Quoted 2 lines
arch/arm/mach-netx/fb.c:76:5: error: redefinition of 'clk_set_rate' include/linux/clk.h:306:50: note: previous definition of 'clk_set_rate'
was here
Quoted 2 lines
arch/arm/mach-netx/fb.c:81:5: error: redefinition of 'clk_enable' include/linux/clk.h:294:50: note: previous definition of 'clk_enable'
was here
Quoted 2 lines
arch/arm/mach-netx/fb.c:86:13: error: redefinition of 'clk_get' include/linux/clk.h:280:58: note: previous definition of 'clk_get' was
here
Quoted 2 lines
arch/arm/mach-netx/fb.c:91:6: error: redefinition of 'clk_put' include/linux/clk.h:290:51: note: previous definition of 'clk_put' was
here
Quoted 2 lines
make[2]: *** [arch/arm/mach-netx/fb.o] Error 1
Hi Paul,
I don't have access to any Linux machine for now as i am on leave.
Actually i have left ST and will join the next company in few weeks.
If you can provide a patch for this, i can review it.
--
Viresh

Russell King - ARM Linux

2012-06-06 22:49 UTC #5 lore raw

Quoted 14 lines · Fengguang Wu

Quoted 2 lines
I didn't merge this patchset because it still has the build error reported by Paul, below.
I see. The arm's redefinitions are mostly empty function stubs that are identical to the ones provided by Viresh's patch. Except for this one, trying to act smarter:
arch/arm/mach-netx/fb.c:
        struct clk *clk_get(struct device *dev, const char *id)
        {       
                return dev && strcmp(dev_name(dev), "fb") == 0 ? NULL : ERR_PTR(-ENOENT);
        }

Quoted 6 lines

The return values are interesting. In this arm, clk_get() conditionally returns NULL or -ENOENT. While the clk_get() in clk.c always returns -ENOENT on error. Now Viresh comes and defines a clk_get() that always returns NULL on !CONFIG_HAVE_CLK.
What would be the difference between NULL and -ENOENT?

Look, it's all very very very very simple.

The clock API. clk_get(). If IS_ERR() is true, then the pointer is _not_ valid, it is an error.

If IS_ERR() is false, then *all* *drivers* must assume that the cookie is valid as far as the driver is concerned. It is up to the clk API to interpret these cookies in whatever way the clk API implementation sees fit.

Fengguang Wu

author

2012-06-06 23:03 UTC #6 lore raw

Quoted 24 lines · Russell King - ARM Linux

Quoted 14 lines · Fengguang Wu
Quoted 2 lines
I didn't merge this patchset because it still has the build error reported by Paul, below.
I see. The arm's redefinitions are mostly empty function stubs that are identical to the ones provided by Viresh's patch. Except for this one, trying to act smarter:
arch/arm/mach-netx/fb.c:
        struct clk *clk_get(struct device *dev, const char *id)
        {       
                return dev && strcmp(dev_name(dev), "fb") == 0 ? NULL : ERR_PTR(-ENOENT);
        }
Quoted 6 lines
The return values are interesting. In this arm, clk_get() conditionally returns NULL or -ENOENT. While the clk_get() in clk.c always returns -ENOENT on error. Now Viresh comes and defines a clk_get() that always returns NULL on !CONFIG_HAVE_CLK.
What would be the difference between NULL and -ENOENT?
Look, it's all very very very very simple.

Quoted 7 lines

The clock API. clk_get(). If IS_ERR() is true, then the pointer is _not_ valid, it is an error.
If IS_ERR() is false, then *all* *drivers* must assume that the cookie is valid as far as the driver is concerned. It is up to the clk API to interpret these cookies in whatever way the clk API implementation sees fit.

That's understandable. Russell, do you think it good to add a check in clk_disable()? This should gracefully avoid the kernel oops and still catch buggy driver code.

drivers/clk/clk.c +3 -0

@@ -464,6 +464,9 @@ static void __clk_disable(struct clk *clk)
        if (!clk)
                return;
 
+       if (WARN_ON(clk == ERR_PTR(-ENOENT)))
+               return;
+
        if (WARN_ON(clk->enable_count == 0))
                return;

Thanks,
Fengguang

Russell King - ARM Linux

2012-06-06 23:54 UTC #7 lore raw

Quoted 28 lines · Fengguang Wu

Quoted 27 lines · Russell King - ARM Linux
Quoted 14 lines · Fengguang Wu
Quoted 2 lines
I didn't merge this patchset because it still has the build error reported by Paul, below.
I see. The arm's redefinitions are mostly empty function stubs that are identical to the ones provided by Viresh's patch. Except for this one, trying to act smarter:
arch/arm/mach-netx/fb.c:
        struct clk *clk_get(struct device *dev, const char *id)
        {       
                return dev && strcmp(dev_name(dev), "fb") == 0 ? NULL : ERR_PTR(-ENOENT);
        }
Quoted 6 lines
The return values are interesting. In this arm, clk_get() conditionally returns NULL or -ENOENT. While the clk_get() in clk.c always returns -ENOENT on error. Now Viresh comes and defines a clk_get() that always returns NULL on !CONFIG_HAVE_CLK.
What would be the difference between NULL and -ENOENT?
Look, it's all very very very very simple.
The clock API. clk_get(). If IS_ERR() is true, then the pointer is _not_ valid, it is an error.

Quoted 8 lines

Quoted 4 lines
If IS_ERR() is false, then *all* *drivers* must assume that the cookie is valid as far as the driver is concerned. It is up to the clk API to interpret these cookies in whatever way the clk API implementation sees fit.
That's understandable. Russell, do you think it good to add a check in clk_disable()? This should gracefully avoid the kernel oops and still catch buggy driver code.

We could - it'd be better to make it IS_ERR(clk) though, let's not be specific about the error code here.

Fengguang Wu

author

2012-06-07 04:52 UTC #8 lore raw

PATCH 1/1 clk: validate pointer in __clk_disable()

clk_get() returns -ENOENT on error and some careless caller might dereference it without error checking:

In mxc_rnga_remove():

        struct clk *clk = clk_get(&pdev->dev, "rng");

	// ...

        clk_disable(clk);

Since it's insane to audit the lots of existing and future clk users, let's add a check in the callee to avoid kernel panic and warn about any buggy user.

---
 drivers/clk/clk.c |    3 +++
 1 file changed, 3 insertions(+)

drivers/clk/clk.c +3 -0

index 687b00d..7bd795bf9 100644
@@ -464,6 +464,9 @@ static void __clk_disable(struct clk *clk)
 	if (!clk)
 		return;
 
+	if (WARN_ON(IS_ERR(clk)))
+		return;
+
 	if (WARN_ON(clk->enable_count == 0))
 		return;

Signature · 1 line

1.7.10

Andrew Morton

2012-06-08 22:37 UTC #9 lore raw

PATCH 1/1 clk: validate pointer in __clk_disable()

(cc viresh kumar <[email protected]>!)

Quoted 23 lines · On Thu, 7 Jun 2012 13:52:59 +0900 Fengguang Wu <[email protected]> wrote:

clk_get() returns -ENOENT on error and some careless caller might dereference it without error checking:
In mxc_rnga_remove():
        struct clk *clk = clk_get(&pdev->dev, "rng");

	// ...

        clk_disable(clk);
Since it's insane to audit the lots of existing and future clk users, let's add a check in the callee to avoid kernel panic and warn about any buggy user.
---
 drivers/clk/clk.c |    3 +++
 1 file changed, 3 insertions(+)
drivers/clk/clk.c +0 -0
index 687b00d..7bd795bf9 100644

drivers/clk/clk.c static void __clk_disable(struct clk *clk) · line 471

Quoted 8 lines

drivers/clk/clk.c +3 -0

@@ -464,6 +464,9 @@ static void __clk_disable(struct clk *clk)
 	if (!clk)
 		return;
 
+	if (WARN_ON(IS_ERR(clk)))
+		return;
+
 	if (WARN_ON(clk->enable_count == 0))
 		return;

Fair enough.

But the build breakage reported by Paul remains, and the patchset remains unmerged.

Fengguang Wu

author

2012-06-06 22:47 UTC #10 lore raw

In reply to a message not in the archive

[resend with corrected email addresses]

Quoted 2 lines

I didn't merge this patchset because it still has the build error reported by Paul, below.

I see. The arm's redefinitions are mostly empty function stubs that are identical to the ones provided by Viresh's patch. Except for this one, trying to act smarter:

arch/arm/mach-netx/fb.c:

        struct clk *clk_get(struct device *dev, const char *id)
        {       
                return dev && strcmp(dev_name(dev), "fb") == 0 ? NULL : ERR_PTR(-ENOENT);
        }

What would be the difference between NULL and -ENOENT?

drivers/char/hw_random/mxc-rnga.c mxc_rnga_remove():

        struct clk *clk = clk_get(&pdev->dev, "rng");

        [...]

        clk_disable(clk);

where clk_disable() only checks NULL:

        if (!clk)
                return;

Thanks,
Fengguang

Quoted 55 lines

Begin forwarded message:
Date: Fri, 25 May 2012 15:07:15 +0530
From: viresh kumar <[email protected]>
To: Paul Gortmaker <[email protected]>
Cc: linux-arm-kernel at lists.infradead.org, linux-next at vger.kernel.org, Andrew Morton <[email protected]>, spear-devel at list.st.com
Subject: Re: [linux-next] "clk: add non CONFIG_HAVE_CLK routines" commit
Quoted 7 lines · Paul Gortmaker
Hi Viresh,
The above listed commit, in linux-next as:
commit ebbf0cb5d39cc3e22ef4c425475e76b7f45027de
Quoted 8 lines
"clk: add non CONFIG_HAVE_CLK routines"
shows up as failures in the netx_defconfig, since there are duplicate stub functions between your changes and the file below:
arch/arm/mach-netx/fb.c:72:6: error: redefinition of 'clk_disable' include/linux/clk.h:299:51: note: previous definition of 'clk_disable'
was here
Quoted 2 lines
arch/arm/mach-netx/fb.c:76:5: error: redefinition of 'clk_set_rate' include/linux/clk.h:306:50: note: previous definition of 'clk_set_rate'
was here
Quoted 2 lines
arch/arm/mach-netx/fb.c:81:5: error: redefinition of 'clk_enable' include/linux/clk.h:294:50: note: previous definition of 'clk_enable'
was here
Quoted 2 lines
arch/arm/mach-netx/fb.c:86:13: error: redefinition of 'clk_get' include/linux/clk.h:280:58: note: previous definition of 'clk_get' was
here
Quoted 2 lines
arch/arm/mach-netx/fb.c:91:6: error: redefinition of 'clk_put' include/linux/clk.h:290:51: note: previous definition of 'clk_put' was
here
Quoted 2 lines
make[2]: *** [arch/arm/mach-netx/fb.o] Error 1
Hi Paul,
I don't have access to any Linux machine for now as i am on leave.
Actually i have left ST and will join the next company in few weeks.
If you can provide a patch for this, i can review it.
--
Viresh