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

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

[Fengguang Wu]

> 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

> 
> 
> 
> Begin forwarded message:
> 
> Date: Fri, 25 May 2012 15:07:15 +0530
> From: viresh kumar <viresh.linux@gmail.com>
> To: Paul Gortmaker <paul.gortmaker@windriver.com>
> Cc: linux-arm-kernel at lists.infradead.org, linux-next at vger.kernel.org, Andrew Morton <akpm@linux-foundation.org>, spear-devel at list.st.com
> Subject: Re: [linux-next] "clk: add non CONFIG_HAVE_CLK routines" commit
> 
> 
> On May 23, 2012 5:18 AM, "Paul Gortmaker" <paul.gortmaker@windriver.com>
> wrote:
> >
> > Hi Viresh,
> >
> > The above listed commit, in linux-next as:
> >
> > commit ebbf0cb5d39cc3e22ef4c425475e76b7f45027de
> >
> >    "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
> >  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
> >  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
> >  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
> >  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
> >  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

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

[Fengguang Wu]

[resend with corrected email addresses]

> 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

> 
> 
> 
> Begin forwarded message:
> 
> Date: Fri, 25 May 2012 15:07:15 +0530
> From: viresh kumar <viresh.linux@gmail.com>
> To: Paul Gortmaker <paul.gortmaker@windriver.com>
> Cc: linux-arm-kernel at lists.infradead.org, linux-next at vger.kernel.org, Andrew Morton <akpm@linux-foundation.org>, spear-devel at list.st.com
> Subject: Re: [linux-next] "clk: add non CONFIG_HAVE_CLK routines" commit
> 
> 
> On May 23, 2012 5:18 AM, "Paul Gortmaker" <paul.gortmaker@windriver.com>
> wrote:
> >
> > Hi Viresh,
> >
> > The above listed commit, in linux-next as:
> >
> > commit ebbf0cb5d39cc3e22ef4c425475e76b7f45027de
> >
> >    "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
> >  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
> >  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
> >  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
> >  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
> >  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

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

[Russell King - ARM Linux]

On Thu, Jun 07, 2012 at 07:42:25AM +0900, Fengguang Wu wrote:
> > 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?

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.

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

[Fengguang Wu]

On Wed, Jun 06, 2012 at 11:49:58PM +0100, Russell King - ARM Linux wrote:
> On Thu, Jun 07, 2012 at 07:42:25AM +0900, Fengguang Wu wrote:
> > > 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?
> 
> 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.

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.

--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -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

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

[Russell King - ARM Linux]

On Thu, Jun 07, 2012 at 08:03:40AM +0900, Fengguang Wu wrote:
> On Wed, Jun 06, 2012 at 11:49:58PM +0100, Russell King - ARM Linux wrote:
> > On Thu, Jun 07, 2012 at 07:42:25AM +0900, Fengguang Wu wrote:
> > > > 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?
> > 
> > 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.
> 
> 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.

[PATCH] clk: validate pointer in __clk_disable()

[Fengguang Wu]

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(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 687b00d..7bd795bf9 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -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;
 
-- 
1.7.10

[PATCH] clk: validate pointer in __clk_disable()

[Andrew Morton]

(cc viresh kumar <viresh.linux@gmail.com>!)

On Thu, 7 Jun 2012 13:52:59 +0900
Fengguang Wu <fengguang.wu@intel.com> 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(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 687b00d..7bd795bf9 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -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.