diff mbox series

coresight: etm4x: Ensure valid drvdata and clock before clk_put()

Message ID 20230811062738.1066787-1-anshuman.khandual@arm.com
State New
Headers
Series coresight: etm4x: Ensure valid drvdata and clock before clk_put() |

Commit Message

Anshuman Khandual Aug. 11, 2023, 6:27 a.m. UTC 
  This validates 'drvdata' and 'drvdata->pclk' clock before calling clk_put()
in etm4_remove_platform_dev(). The problem was detected using Smatch static
checker as reported.

Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Mike Leach <mike.leach@linaro.org>
Cc: James Clark <james.clark@arm.com>
Cc: coresight@lists.linaro.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lists.linaro.org/archives/list/coresight@lists.linaro.org/thread/G4N6P4OXELPLLQSNU3GU2MR4LOLRXRMJ/
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
This applies on coresight-next

 drivers/hwtracing/coresight/coresight-etm4x-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

James Clark Aug. 11, 2023, 8:39 a.m. UTC | #1 
On 11/08/2023 07:27, Anshuman Khandual wrote:
> This validates 'drvdata' and 'drvdata->pclk' clock before calling clk_put()
> in etm4_remove_platform_dev(). The problem was detected using Smatch static
> checker as reported.
> 
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Mike Leach <mike.leach@linaro.org>
> Cc: James Clark <james.clark@arm.com>
> Cc: coresight@lists.linaro.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lists.linaro.org/archives/list/coresight@lists.linaro.org/thread/G4N6P4OXELPLLQSNU3GU2MR4LOLRXRMJ/
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
> This applies on coresight-next
> 
>  drivers/hwtracing/coresight/coresight-etm4x-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index 703b6fcbb6a5..eb412ce302cc 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -2269,7 +2269,7 @@ static int __exit etm4_remove_platform_dev(struct platform_device *pdev)
>  		etm4_remove_dev(drvdata);
>  	pm_runtime_disable(&pdev->dev);
>  
> -	if (drvdata->pclk)
> +	if (drvdata && drvdata->pclk && !IS_ERR(drvdata->pclk))
>  		clk_put(drvdata->pclk);
>  
>  	return 0;

It could be !IS_ERR_OR_NULL(drvdata->pclk), but I wouldn't bother
changing it at this point.

Reviewed-by: James Clark <james.clark@arm.com>
Mike Leach Aug. 11, 2023, 8:48 a.m. UTC | #2 
On Fri, 11 Aug 2023 at 07:27, Anshuman Khandual
<anshuman.khandual@arm.com> wrote:
>
> This validates 'drvdata' and 'drvdata->pclk' clock before calling clk_put()
> in etm4_remove_platform_dev(). The problem was detected using Smatch static
> checker as reported.
>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Mike Leach <mike.leach@linaro.org>
> Cc: James Clark <james.clark@arm.com>
> Cc: coresight@lists.linaro.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lists.linaro.org/archives/list/coresight@lists.linaro.org/thread/G4N6P4OXELPLLQSNU3GU2MR4LOLRXRMJ/
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
> This applies on coresight-next
>
>  drivers/hwtracing/coresight/coresight-etm4x-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index 703b6fcbb6a5..eb412ce302cc 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -2269,7 +2269,7 @@ static int __exit etm4_remove_platform_dev(struct platform_device *pdev)
>                 etm4_remove_dev(drvdata);
>         pm_runtime_disable(&pdev->dev);
>
> -       if (drvdata->pclk)
> +       if (drvdata && drvdata->pclk && !IS_ERR(drvdata->pclk))
>                 clk_put(drvdata->pclk);
>
>         return 0;
> --
> 2.25.1
>

Reviewed-by: Mike Leach <mike.leach@lnaro.org>
Suzuki K Poulose Aug. 11, 2023, 9:09 a.m. UTC | #3 
On 11/08/2023 09:39, James Clark wrote:
> 
> 
> On 11/08/2023 07:27, Anshuman Khandual wrote:
>> This validates 'drvdata' and 'drvdata->pclk' clock before calling clk_put()
>> in etm4_remove_platform_dev(). The problem was detected using Smatch static
>> checker as reported.
>>
>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>> Cc: Mike Leach <mike.leach@linaro.org>
>> Cc: James Clark <james.clark@arm.com>
>> Cc: coresight@lists.linaro.org
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>> Closes: https://lists.linaro.org/archives/list/coresight@lists.linaro.org/thread/G4N6P4OXELPLLQSNU3GU2MR4LOLRXRMJ/
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>> This applies on coresight-next
>>
>>   drivers/hwtracing/coresight/coresight-etm4x-core.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> index 703b6fcbb6a5..eb412ce302cc 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> @@ -2269,7 +2269,7 @@ static int __exit etm4_remove_platform_dev(struct platform_device *pdev)
>>   		etm4_remove_dev(drvdata);
>>   	pm_runtime_disable(&pdev->dev);
>>   
>> -	if (drvdata->pclk)
>> +	if (drvdata && drvdata->pclk && !IS_ERR(drvdata->pclk))
>>   		clk_put(drvdata->pclk);
>>   
>>   	return 0;
> 
> It could be !IS_ERR_OR_NULL(drvdata->pclk), but I wouldn't bother
> changing it at this point.

+1, please could we have that. Someone else will run a code scanner and
send a patch later. Given this is straight and easy change, lets do it
in the first place.

Cheers
Suzuki

> 
> Reviewed-by: James Clark <james.clark@arm.com>
Anshuman Khandual Aug. 11, 2023, 9:22 a.m. UTC | #4 
On 8/11/23 14:39, Suzuki K Poulose wrote:
> On 11/08/2023 09:39, James Clark wrote:
>>
>>
>> On 11/08/2023 07:27, Anshuman Khandual wrote:
>>> This validates 'drvdata' and 'drvdata->pclk' clock before calling clk_put()
>>> in etm4_remove_platform_dev(). The problem was detected using Smatch static
>>> checker as reported.
>>>
>>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>>> Cc: Mike Leach <mike.leach@linaro.org>
>>> Cc: James Clark <james.clark@arm.com>
>>> Cc: coresight@lists.linaro.org
>>> Cc: linux-arm-kernel@lists.infradead.org
>>> Cc: linux-kernel@vger.kernel.org
>>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>>> Closes: https://lists.linaro.org/archives/list/coresight@lists.linaro.org/thread/G4N6P4OXELPLLQSNU3GU2MR4LOLRXRMJ/
>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>>> ---
>>> This applies on coresight-next
>>>
>>>   drivers/hwtracing/coresight/coresight-etm4x-core.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>> index 703b6fcbb6a5..eb412ce302cc 100644
>>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>> @@ -2269,7 +2269,7 @@ static int __exit etm4_remove_platform_dev(struct platform_device *pdev)
>>>           etm4_remove_dev(drvdata);
>>>       pm_runtime_disable(&pdev->dev);
>>>   -    if (drvdata->pclk)
>>> +    if (drvdata && drvdata->pclk && !IS_ERR(drvdata->pclk))
>>>           clk_put(drvdata->pclk);
>>>         return 0;
>>
>> It could be !IS_ERR_OR_NULL(drvdata->pclk), but I wouldn't bother
>> changing it at this point.
> 
> +1, please could we have that. Someone else will run a code scanner and
> send a patch later. Given this is straight and easy change, lets do it
> in the first place.

But we already have a drvdata->pclk validation check before IS_ERR().
Would not _OR_NULL be redundant ?
Dan Carpenter Aug. 11, 2023, 9:26 a.m. UTC | #5 
On Fri, Aug 11, 2023 at 10:09:43AM +0100, Suzuki K Poulose wrote:
> On 11/08/2023 09:39, James Clark wrote:
> > 
> > 
> > On 11/08/2023 07:27, Anshuman Khandual wrote:
> > > This validates 'drvdata' and 'drvdata->pclk' clock before calling clk_put()
> > > in etm4_remove_platform_dev(). The problem was detected using Smatch static
> > > checker as reported.
> > > 
> > > Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> > > Cc: Mike Leach <mike.leach@linaro.org>
> > > Cc: James Clark <james.clark@arm.com>
> > > Cc: coresight@lists.linaro.org
> > > Cc: linux-arm-kernel@lists.infradead.org
> > > Cc: linux-kernel@vger.kernel.org
> > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > > Closes: https://lists.linaro.org/archives/list/coresight@lists.linaro.org/thread/G4N6P4OXELPLLQSNU3GU2MR4LOLRXRMJ/
> > > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> > > ---
> > > This applies on coresight-next
> > > 
> > >   drivers/hwtracing/coresight/coresight-etm4x-core.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > > index 703b6fcbb6a5..eb412ce302cc 100644
> > > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > > @@ -2269,7 +2269,7 @@ static int __exit etm4_remove_platform_dev(struct platform_device *pdev)
> > >   		etm4_remove_dev(drvdata);
> > >   	pm_runtime_disable(&pdev->dev);
> > > -	if (drvdata->pclk)
> > > +	if (drvdata && drvdata->pclk && !IS_ERR(drvdata->pclk))
> > >   		clk_put(drvdata->pclk);
> > >   	return 0;
> > 
> > It could be !IS_ERR_OR_NULL(drvdata->pclk), but I wouldn't bother
> > changing it at this point.
> 
> +1, please could we have that. Someone else will run a code scanner and
> send a patch later. Given this is straight and easy change, lets do it
> in the first place.

drvdata->pclk can't actually be an error pointer.  probe() will
correctly not allow that.  All the IS_ERR(drvdata->pclk) checks should
be removed except for the first check in probe().

There is also no need to check "drvdata->pclk" for NULL because
clk_put() accepts NULL pointers.  (Returning NULL means the clk
subsystem has been disabled deliberately).

Also drvdata can't actually be NULL either.

regards,
dan carpenter
James Clark Aug. 11, 2023, 10:14 a.m. UTC | #6 
On 11/08/2023 10:22, Anshuman Khandual wrote:
> 
> 
> On 8/11/23 14:39, Suzuki K Poulose wrote:
>> On 11/08/2023 09:39, James Clark wrote:
>>>
>>>
>>> On 11/08/2023 07:27, Anshuman Khandual wrote:
>>>> This validates 'drvdata' and 'drvdata->pclk' clock before calling clk_put()
>>>> in etm4_remove_platform_dev(). The problem was detected using Smatch static
>>>> checker as reported.
>>>>
>>>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>> Cc: Mike Leach <mike.leach@linaro.org>
>>>> Cc: James Clark <james.clark@arm.com>
>>>> Cc: coresight@lists.linaro.org
>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>> Cc: linux-kernel@vger.kernel.org
>>>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>>>> Closes: https://lists.linaro.org/archives/list/coresight@lists.linaro.org/thread/G4N6P4OXELPLLQSNU3GU2MR4LOLRXRMJ/
>>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>>>> ---
>>>> This applies on coresight-next
>>>>
>>>>   drivers/hwtracing/coresight/coresight-etm4x-core.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>> index 703b6fcbb6a5..eb412ce302cc 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>> @@ -2269,7 +2269,7 @@ static int __exit etm4_remove_platform_dev(struct platform_device *pdev)
>>>>           etm4_remove_dev(drvdata);
>>>>       pm_runtime_disable(&pdev->dev);
>>>>   -    if (drvdata->pclk)
>>>> +    if (drvdata && drvdata->pclk && !IS_ERR(drvdata->pclk))
>>>>           clk_put(drvdata->pclk);
>>>>         return 0;
>>>
>>> It could be !IS_ERR_OR_NULL(drvdata->pclk), but I wouldn't bother
>>> changing it at this point.
>>
>> +1, please could we have that. Someone else will run a code scanner and
>> send a patch later. Given this is straight and easy change, lets do it
>> in the first place.
> 
> But we already have a drvdata->pclk validation check before IS_ERR().
> Would not _OR_NULL be redundant ?

I meant that it could be replaced with the single check:

  if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
      clk_put(drvdata->pclk);

As Dan mentions it can't be an error pointer anyway, but leaving it like
this could just be considered defensive coding.
Anshuman Khandual Aug. 16, 2023, 8:06 a.m. UTC | #7 
On 8/11/23 15:44, James Clark wrote:
> 
> 
> On 11/08/2023 10:22, Anshuman Khandual wrote:
>>
>>
>> On 8/11/23 14:39, Suzuki K Poulose wrote:
>>> On 11/08/2023 09:39, James Clark wrote:
>>>>
>>>>
>>>> On 11/08/2023 07:27, Anshuman Khandual wrote:
>>>>> This validates 'drvdata' and 'drvdata->pclk' clock before calling clk_put()
>>>>> in etm4_remove_platform_dev(). The problem was detected using Smatch static
>>>>> checker as reported.
>>>>>
>>>>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>>> Cc: Mike Leach <mike.leach@linaro.org>
>>>>> Cc: James Clark <james.clark@arm.com>
>>>>> Cc: coresight@lists.linaro.org
>>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>>> Cc: linux-kernel@vger.kernel.org
>>>>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>>>>> Closes: https://lists.linaro.org/archives/list/coresight@lists.linaro.org/thread/G4N6P4OXELPLLQSNU3GU2MR4LOLRXRMJ/
>>>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>>>>> ---
>>>>> This applies on coresight-next
>>>>>
>>>>>   drivers/hwtracing/coresight/coresight-etm4x-core.c | 2 +-
>>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>>> index 703b6fcbb6a5..eb412ce302cc 100644
>>>>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>>> @@ -2269,7 +2269,7 @@ static int __exit etm4_remove_platform_dev(struct platform_device *pdev)
>>>>>           etm4_remove_dev(drvdata);
>>>>>       pm_runtime_disable(&pdev->dev);
>>>>>   -    if (drvdata->pclk)
>>>>> +    if (drvdata && drvdata->pclk && !IS_ERR(drvdata->pclk))
>>>>>           clk_put(drvdata->pclk);
>>>>>         return 0;
>>>>
>>>> It could be !IS_ERR_OR_NULL(drvdata->pclk), but I wouldn't bother
>>>> changing it at this point.
>>>
>>> +1, please could we have that. Someone else will run a code scanner and
>>> send a patch later. Given this is straight and easy change, lets do it
>>> in the first place.
>>
>> But we already have a drvdata->pclk validation check before IS_ERR().
>> Would not _OR_NULL be redundant ?
> 
> I meant that it could be replaced with the single check:
> 
>   if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
>       clk_put(drvdata->pclk);
> 
> As Dan mentions it can't be an error pointer anyway, but leaving it like
> this could just be considered defensive coding.

Let's just go with the above change as you had suggested unless there is any
particular objection.

if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
	clk_put(drvdata->pclk);
diff mbox series

Patch

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index 703b6fcbb6a5..eb412ce302cc 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -2269,7 +2269,7 @@  static int __exit etm4_remove_platform_dev(struct platform_device *pdev)
 		etm4_remove_dev(drvdata);
 	pm_runtime_disable(&pdev->dev);
 
-	if (drvdata->pclk)
+	if (drvdata && drvdata->pclk && !IS_ERR(drvdata->pclk))
 		clk_put(drvdata->pclk);
 
 	return 0;