diff mbox

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

Message ID 1389096223-23934-1-git-send-email-s.nawrocki@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

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

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
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(-)

--
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Lothar Waßmann Jan. 7, 2014, 1 p.m. UTC | #1
Hi,

Sylwester Nawrocki wrote:
> Ensure clk->kref is dereferenced only when clk is not NULL.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
> 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(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index e3e0327..a1fe86f 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -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
Hi,

On 07/01/14 14:00, Lothar Waßmann wrote:
> Sylwester Nawrocki wrote:
>> Ensure clk->kref is dereferenced only when clk is not NULL.
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> ---
>> 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(-)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index e3e0327..a1fe86f 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -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
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sachin Kamat Jan. 8, 2014, 4:44 a.m. UTC | #3
Hi Sylwester,

On 7 January 2014 17:33, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote:
> Ensure clk->kref is dereferenced only when clk is not NULL.
>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
> 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 <sachin.kamat@linaro.org>
On 08/01/14 05:44, Sachin Kamat wrote:
> Hi Sylwester,
> 
> On 7 January 2014 17:33, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote:
>> Ensure clk->kref is dereferenced only when clk is not NULL.
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> ---
>> 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 <sachin.kamat@linaro.org>

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
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sachin Kamat Jan. 30, 2014, 4:50 a.m. UTC | #5
On 10 January 2014 15:34, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote:
> On 08/01/14 05:44, Sachin Kamat wrote:
>> Hi Sylwester,
>>
>> On 7 January 2014 17:33, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote:
>>> Ensure clk->kref is dereferenced only when clk is not NULL.
>>>
>>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>>> ---
>>> 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 <sachin.kamat@linaro.org>
>
> 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.
On 30/01/14 05:50, Sachin Kamat wrote:
> On 10 January 2014 15:34, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote:
>> > On 08/01/14 05:44, Sachin Kamat wrote:
>>> >> Hi Sylwester,
>>> >>
>>> >> On 7 January 2014 17:33, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote:
>>>> >>> Ensure clk->kref is dereferenced only when clk is not NULL.
>>>> >>>
>>>> >>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>>>> >>> ---
>>>> >>> 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 <sachin.kamat@linaro.org>
>> >
>> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomasz Figa Feb. 24, 2014, 12:59 p.m. UTC | #7
Mike,

On 10.02.2014 16:02, Sylwester Nawrocki wrote:
> On 30/01/14 05:50, Sachin Kamat wrote:
>> On 10 January 2014 15:34, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote:
>>>> On 08/01/14 05:44, Sachin Kamat wrote:
>>>>>> Hi Sylwester,
>>>>>>
>>>>>> On 7 January 2014 17:33, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote:
>>>>>>>> Ensure clk->kref is dereferenced only when clk is not NULL.
>>>>>>>>
>>>>>>>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>>>>>>>> ---
>>>>>>>> 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 <sachin.kamat@linaro.org>
>>>>
>>>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mike Turquette Feb. 25, 2014, 6:16 a.m. UTC | #8
Quoting Sylwester Nawrocki (2014-01-07 04:03:43)
> Ensure clk->kref is dereferenced only when clk is not NULL.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

Taken into clk-fixes toward -rc5.

Thanks!
Mike

> ---
> 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(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index e3e0327..a1fe86f 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -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
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomasz Figa Feb. 25, 2014, 5:34 p.m. UTC | #9
Hi Mike,

On 25.02.2014 07:16, Mike Turquette wrote:
> Quoting Sylwester Nawrocki (2014-01-07 04:03:43)
>> Ensure clk->kref is dereferenced only when clk is not NULL.
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>
> Taken into clk-fixes toward -rc5.

Thanks! \o/

Best regards,
Tomasz

>
> Thanks!
> Mike
>
>> ---
>> 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(-)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index e3e0327..a1fe86f 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -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
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index e3e0327..a1fe86f 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -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        ***/