Skip to contentSkip to conversation

clk: Correct handling of NULL clk in __clk_{get, put}

PATCH New Sylwester Nawrocki 6 participants 10 messages Jan 7, 2014 – Feb 25, 2014

Subsystems (2)
Common clk Arm Samsung Exynos
lore
Share & tools
lore

Sylwester Nawrocki

author
#0 lore raw

PATCH 1/1 clk: Correct handling of NULL clk in __clk_{get, put}

Ensure clk->kref is dereferenced only when clk is not NULL.

Signed-off-by Sylwester Nawrocki <[email protected]>

---
Hi Sachin,

please try if this patch fixes the exyno5420 boot crash.

Thanks,
Sylwester

 drivers/clk/clk.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)
drivers/clk/clk.c +7 -6
index e3e0327..a1fe86f 100644
@@ -2179,24 +2179,25 @@ EXPORT_SYMBOL_GPL(devm_clk_unregister);
  */
 int __clk_get(struct clk *clk)
 {
-	if (clk && !try_module_get(clk->owner))
-		return 0;
+	if (clk) {
+		if (!try_module_get(clk->owner))
+			return 0;
 
-	kref_get(&clk->ref);
+		kref_get(&clk->ref);
+	}
 	return 1;
 }
 
 void __clk_put(struct clk *clk)
 {
-	if (WARN_ON_ONCE(IS_ERR(clk)))
+	if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
 		return;
 
 	clk_prepare_lock();
 	kref_put(&clk->ref, __clk_release);
 	clk_prepare_unlock();
 
-	if (clk)
-		module_put(clk->owner);
+	module_put(clk->owner);
 }
 
 /***        clk rate change notifiers        ***/

--
1.7.9.5

#1 lore raw

PATCH 1/1 clk: Correct handling of NULL clk in __clk_{get, put}

Hi,

Quoted 19 lines · Sylwester Nawrocki

Ensure clk->kref is dereferenced only when clk is not NULL.

Signed-off-by Sylwester Nawrocki <[email protected]>

---
Hi Sachin,

please try if this patch fixes the exyno5420 boot crash.

Thanks,
Sylwester

 drivers/clk/clk.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)
drivers/clk/clk.c +0 -0
index e3e0327..a1fe86f 100644

drivers/clk/clk.c EXPORT_SYMBOL_GPL(devm_clk_unregister); · line 2185

Quoted 9 lines
drivers/clk/clk.c +3 -2
@@ -2179,24 +2179,25 @@ EXPORT_SYMBOL_GPL(devm_clk_unregister);
  */
 int __clk_get(struct clk *clk)
 {
-	if (clk && !try_module_get(clk->owner))
-		return 0;
+	if (clk) {
+		if (!try_module_get(clk->owner))
+			return 0;

This change is unnecessary! The part after the && is only evaluated when clk is not NULL.

Lothar Wa?mann

Signature · 9 lines
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________
author
#2 lore raw

PATCH 1/1 clk: Correct handling of NULL clk in __clk_{get, put}

Hi,

Quoted 15 lines · Lothar Wa?mann
Quoted 14 lines · Sylwester Nawrocki

Ensure clk->kref is dereferenced only when clk is not NULL.

Signed-off-by Sylwester Nawrocki <[email protected]>

---
Hi Sachin,

please try if this patch fixes the exyno5420 boot crash.

Thanks,
Sylwester

 drivers/clk/clk.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)
Quoted 16 lines
Quoted 5 lines
drivers/clk/clk.c +0 -0
index e3e0327..a1fe86f 100644

drivers/clk/clk.c EXPORT_SYMBOL_GPL(devm_clk_unregister); · line 2185

Quoted 9 lines
drivers/clk/clk.c +3 -2
@@ -2179,24 +2179,25 @@ EXPORT_SYMBOL_GPL(devm_clk_unregister);
  */
 int __clk_get(struct clk *clk)
 {
-	if (clk && !try_module_get(clk->owner))
-		return 0;
+	if (clk) {
+		if (!try_module_get(clk->owner))
+			return 0;

This change is unnecessary! The part after the && is only evaluated when clk is not NULL.

It is, but to ensure there is no NULL clk passed at the kref_get(&clk->ref) line everything got moved under a common "if (clk) { }".

The code will look like this:

-----------

	if (clk) {
		if (!try_module_get(clk->owner))
			return 0;

		kref_get(&clk->ref);
	}

------------

If preferred I could make it:

------------
if (clk && !try_module_get(clk->owner))

	return 0;

if (clk)

	kref_get(&clk->ref);

------------

--
Regards,
Sylwester

#3 lore raw

PATCH 1/1 clk: Correct handling of NULL clk in __clk_{get, put}

In reply to Sylwester Nawrocki

Hi Sylwester,

Quoted 7 lines · Sylwester Nawrocki

Ensure clk->kref is dereferenced only when clk is not NULL.

Signed-off-by Sylwester Nawrocki <[email protected]>

---
Hi Sachin,

please try if this patch fixes the exyno5420 boot crash.

Confirmed that this patch works fine on 5420 as well as already working 4210 and 5250 boards.
Thanks for the quick fix.

Tested-by Sachin Kamat <[email protected]>
Signature · 2 lines
With warm regards,
Sachin
author
#4 lore raw

PATCH 1/1 clk: Correct handling of NULL clk in __clk_{get, put}

Quoted 9 lines · Sachin Kamat

Hi Sylwester,

Quoted 6 lines · Sylwester Nawrocki

Ensure clk->kref is dereferenced only when clk is not NULL.

Signed-off-by Sylwester Nawrocki <[email protected]>

---
Hi Sachin,

Quoted 7 lines
Quoted 1 line

please try if this patch fixes the exyno5420 boot crash.

Confirmed that this patch works fine on 5420 as well as already working 4210 and 5250 boards.
Thanks for the quick fix.

Tested-by Sachin Kamat <[email protected]>

Thanks Sachin. Mike, it seems we need this patch on top of my clk-unregister branch. Sorry for overlooking this issue. Could you add the $subject patch to your clk-next tree ?

Regards,
Sylwester

#5 lore raw

PATCH 1/1 clk: Correct handling of NULL clk in __clk_{get, put}

Quoted 16 lines · Sylwester Nawrocki
Quoted 15 lines · Sachin Kamat

Hi Sylwester,

Quoted 7 lines · Sylwester Nawrocki

Ensure clk->kref is dereferenced only when clk is not NULL.

Signed-off-by Sylwester Nawrocki <[email protected]>

---
Hi Sachin,

please try if this patch fixes the exyno5420 boot crash.

Confirmed that this patch works fine on 5420 as well as already working 4210 and 5250 boards.
Thanks for the quick fix.

Quoted 5 lines
Quoted 1 line
Tested-by Sachin Kamat <[email protected]>

Thanks Sachin. Mike, it seems we need this patch on top of my clk-unregister branch. Sorry for overlooking this issue. Could you add the $subject patch to your clk-next tree ?

Gentle ping Mike. Hope we can have this patch in rc-2.

Signature · 2 lines
With warm regards,
Sachin
author
#6 lore raw

PATCH 1/1 clk: Correct handling of NULL clk in __clk_{get, put}

Quoted 17 lines · Sachin Kamat
Quoted 16 lines · Sylwester Nawrocki
Quoted 16 lines · Sachin Kamat
Quoted 15 lines
Quoted 15 lines

Hi Sylwester,

Quoted 7 lines · Sylwester Nawrocki
Quoted 7 lines

Ensure clk->kref is dereferenced only when clk is not NULL.

Signed-off-by Sylwester Nawrocki <[email protected]>

---
Hi Sachin,

please try if this patch fixes the exyno5420 boot crash.

Confirmed that this patch works fine on 5420 as well as already working 4210 and 5250 boards.
Thanks for the quick fix.

Quoted 7 lines
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
Quoted 1 line
Tested-by Sachin Kamat <[email protected]>

Thanks Sachin. Mike, it seems we need this patch on top of my clk-unregister branch. Sorry for overlooking this issue. Could you add the $subject patch to your clk-next tree ?

Gentle ping Mike. Hope we can have this patch in rc-2.

A gentle reminder, this patch is still not in current -rc and is required to fix booting of some exynos platforms.

Thanks,
Sylwester

#7 lore raw

PATCH 1/1 clk: Correct handling of NULL clk in __clk_{get, put}

Mike,

Quoted 20 lines · Sylwester Nawrocki
Quoted 19 lines · Sachin Kamat
Quoted 18 lines · Sylwester Nawrocki
Quoted 18 lines · Sachin Kamat
Quoted 16 lines
Quoted 16 lines

Hi Sylwester,

Quoted 7 lines · Sylwester Nawrocki
Quoted 7 lines

Ensure clk->kref is dereferenced only when clk is not NULL.

Signed-off-by Sylwester Nawrocki <[email protected]>

---
Hi Sachin,

please try if this patch fixes the exyno5420 boot crash.

Confirmed that this patch works fine on 5420 as well as already working 4210 and 5250 boards.
Thanks for the quick fix.

Tested-by Sachin Kamat <[email protected]>
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Quoted 3 lines

Thanks Sachin. Mike, it seems we need this patch on top of my clk-unregister branch. Sorry for overlooking this issue. Could you add the $subject patch to your clk-next tree ?

Gentle ping Mike. Hope we can have this patch in rc-2.

A gentle reminder, this patch is still not in current -rc and is required to fix booting of some exynos platforms.

It's been one and a half of a month since Sylwester posted this patch. Sorry for nagging, but for this whole period we've been observing complete breakage of several platforms, including Exynos4412-Trats2 board, due to NULL pointer dereference at boot-up time, so I'd consider this patch a _critical_ fix.

Can we expect this patch to be applied to your tree and sent for next rc release or I should apply it to my tree and send a pull request to Linus directly?

Best regards,
Tomasz

#8 lore raw

PATCH 1/1 clk: Correct handling of NULL clk in __clk_{get, put}

In reply to Sylwester Nawrocki

Quoting Sylwester Nawrocki (2014-01-07 04:03:43)

Quoted 3 lines

Ensure clk->kref is dereferenced only when clk is not NULL.

Signed-off-by Sylwester Nawrocki <[email protected]>

Taken into clk-fixes toward -rc5.

Thanks!
Mike

Quoted 50 lines

---
Hi Sachin,

please try if this patch fixes the exyno5420 boot crash.

Thanks,
Sylwester

 drivers/clk/clk.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)
drivers/clk/clk.c +7 -7
index e3e0327..a1fe86f 100644
@@ -2179,24 +2179,25 @@ EXPORT_SYMBOL_GPL(devm_clk_unregister);
  */
 int __clk_get(struct clk *clk)
 {
-       if (clk && !try_module_get(clk->owner))
-               return 0;
+       if (clk) {
+               if (!try_module_get(clk->owner))
+                       return 0;
 
-       kref_get(&clk->ref);
+               kref_get(&clk->ref);
+       }
        return 1;
 }
 
 void __clk_put(struct clk *clk)
 {
-       if (WARN_ON_ONCE(IS_ERR(clk)))
+       if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
                return;
 
        clk_prepare_lock();
        kref_put(&clk->ref, __clk_release);
        clk_prepare_unlock();
 
-       if (clk)
-               module_put(clk->owner);
+       module_put(clk->owner);
 }
 
 /***        clk rate change notifiers        ***/
--

1.7.9.5

#9 lore raw

PATCH 1/1 clk: Correct handling of NULL clk in __clk_{get, put}

Hi Mike,

Quoted 6 lines · Mike Turquette

Quoting Sylwester Nawrocki (2014-01-07 04:03:43)

Quoted 3 lines

Ensure clk->kref is dereferenced only when clk is not NULL.

Signed-off-by Sylwester Nawrocki <[email protected]>

Taken into clk-fixes toward -rc5.

Thanks! \o/

Best regards,
Tomasz

Quoted 54 lines

Thanks!
Mike

Quoted 50 lines

---
Hi Sachin,

please try if this patch fixes the exyno5420 boot crash.

Thanks,
Sylwester

  drivers/clk/clk.c |   13 +++++++------
  1 file changed, 7 insertions(+), 6 deletions(-)
drivers/clk/clk.c +7 -7
index e3e0327..a1fe86f 100644
@@ -2179,24 +2179,25 @@ EXPORT_SYMBOL_GPL(devm_clk_unregister);
   */
  int __clk_get(struct clk *clk)
  {
-       if (clk && !try_module_get(clk->owner))
-               return 0;
+       if (clk) {
+               if (!try_module_get(clk->owner))
+                       return 0;
 
-       kref_get(&clk->ref);
+               kref_get(&clk->ref);
+       }
         return 1;
  }
 
  void __clk_put(struct clk *clk)
  {
-       if (WARN_ON_ONCE(IS_ERR(clk)))
+       if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
                 return;
 
         clk_prepare_lock();
         kref_put(&clk->ref, __clk_release);
         clk_prepare_unlock();
 
-       if (clk)
-               module_put(clk->owner);
+       module_put(clk->owner);
  }
 
  /***        clk rate change notifiers        ***/
--

1.7.9.5