2023-08-11 07:18:00

by Anshuman Khandual

Subject: [PATCH] coresight: etm4x: Ensure valid drvdata and clock before clk_put()

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 <[email protected]>
Cc: Mike Leach <[email protected]>
Cc: James Clark <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Reported-by: Dan Carpenter <[email protected]>
Closes: https://lists.linaro.org/archives/list/[email protected]/thread/G4N6P4OXELPLLQSNU3GU2MR4LOLRXRMJ/
Signed-off-by: Anshuman Khandual <[email protected]>
---
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

2023-08-11 08:59:27

by James Clark

Subject: Re: [PATCH] coresight: etm4x: Ensure valid drvdata and clock before clk_put()

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 <[email protected]>
> Cc: Mike Leach <[email protected]>
> Cc: James Clark <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Reported-by: Dan Carpenter <[email protected]>
> Closes: https://lists.linaro.org/archives/list/[email protected]/thread/G4N6P4OXELPLLQSNU3GU2MR4LOLRXRMJ/
> Signed-off-by: Anshuman Khandual <[email protected]>
> ---
> 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 <[email protected]>

2023-08-11 09:31:04

by Anshuman Khandual

Subject: Re: [PATCH] coresight: etm4x: Ensure valid drvdata and clock before clk_put()

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 <[email protected]>
>>> Cc: Mike Leach <[email protected]>
>>> Cc: James Clark <[email protected]>
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> Reported-by: Dan Carpenter <[email protected]>
>>> Closes: https://lists.linaro.org/archives/list/[email protected]/thread/G4N6P4OXELPLLQSNU3GU2MR4LOLRXRMJ/
>>> Signed-off-by: Anshuman Khandual <[email protected]>
>>> ---
>>> 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 ?

2023-08-11 09:32:16

by Suzuki K Poulose

Subject: Re: [PATCH] coresight: etm4x: Ensure valid drvdata and clock before clk_put()

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 <[email protected]>
>> Cc: Mike Leach <[email protected]>
>> Cc: James Clark <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> Reported-by: Dan Carpenter <[email protected]>
>> Closes: https://lists.linaro.org/archives/list/[email protected]/thread/G4N6P4OXELPLLQSNU3GU2MR4LOLRXRMJ/
>> Signed-off-by: Anshuman Khandual <[email protected]>
>> ---
>> 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 <[email protected]>

2023-08-11 10:02:23

by Dan Carpenter

Subject: Re: [PATCH] coresight: etm4x: Ensure valid drvdata and clock before clk_put()

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 <[email protected]>
> > > Cc: Mike Leach <[email protected]>
> > > Cc: James Clark <[email protected]>
> > > Cc: [email protected]
> > > Cc: [email protected]
> > > Cc: [email protected]
> > > Reported-by: Dan Carpenter <[email protected]>
> > > Closes: https://lists.linaro.org/archives/list/[email protected]/thread/G4N6P4OXELPLLQSNU3GU2MR4LOLRXRMJ/
> > > Signed-off-by: Anshuman Khandual <[email protected]>
> > > ---
> > > 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

2023-08-11 10:09:06

by Mike Leach

Subject: Re: [PATCH] coresight: etm4x: Ensure valid drvdata and clock before clk_put()

On Fri, 11 Aug 2023 at 07:27, Anshuman Khandual
<[email protected]> 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 <[email protected]>
> Cc: Mike Leach <[email protected]>
> Cc: James Clark <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Reported-by: Dan Carpenter <[email protected]>
> Closes: https://lists.linaro.org/archives/list/[email protected]/thread/G4N6P4OXELPLLQSNU3GU2MR4LOLRXRMJ/
> Signed-off-by: Anshuman Khandual <[email protected]>
> ---
> 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 <[email protected]>
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

2023-08-11 10:59:55

by James Clark

Subject: Re: [PATCH] coresight: etm4x: Ensure valid drvdata and clock before clk_put()

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 <[email protected]>
>>>> Cc: Mike Leach <[email protected]>
>>>> Cc: James Clark <[email protected]>
>>>> Cc: [email protected]
>>>> Cc: [email protected]
>>>> Cc: [email protected]
>>>> Reported-by: Dan Carpenter <[email protected]>
>>>> Closes: https://lists.linaro.org/archives/list/[email protected]/thread/G4N6P4OXELPLLQSNU3GU2MR4LOLRXRMJ/
>>>> Signed-off-by: Anshuman Khandual <[email protected]>
>>>> ---
>>>> 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.

2023-08-16 11:02:49

by Anshuman Khandual

Subject: Re: [PATCH] coresight: etm4x: Ensure valid drvdata and clock before clk_put()

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 <[email protected]>
>>>>> Cc: Mike Leach <[email protected]>
>>>>> Cc: James Clark <[email protected]>
>>>>> Cc: [email protected]
>>>>> Cc: [email protected]
>>>>> Cc: [email protected]
>>>>> Reported-by: Dan Carpenter <[email protected]>
>>>>> Closes: https://lists.linaro.org/archives/list/[email protected]/thread/G4N6P4OXELPLLQSNU3GU2MR4LOLRXRMJ/
>>>>> Signed-off-by: Anshuman Khandual <[email protected]>
>>>>> ---
>>>>> 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);