[PATCH] soc: qcom: icc-bwmon: Fix handling dev_pm_opp_find_bw_*() errors

[PATCH] soc: qcom: icc-bwmon: Fix handling dev_pm_opp_find_bw_*() errors

[Krzysztof Kozlowski]

The ISR calls dev_pm_opp_find_bw_ceil(), which can return EINVAL, ERANGE
or ENODEV, and if that one fails with ERANGE, then it tries again with
floor dev_pm_opp_find_bw_floor().

Code misses error checks for two cases:
1. First dev_pm_opp_find_bw_ceil() failed with error different than
   ERANGE,
2. Any error from second dev_pm_opp_find_bw_floor().

In an unlikely case these error happened, the code would further
dereference the ERR pointer.  Close that possibility and make the code
more obvious that all errors are correctly handled.

Reported by Smatch:
  icc-bwmon.c:693 bwmon_intr_thread() error: 'target_opp' dereferencing possible ERR_PTR()

Fixes: b9c2ae6cac40 ("soc: qcom: icc-bwmon: Add bandwidth monitoring driver")
Cc: <stable@vger.kernel.org>
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/r/aJTNEQsRFjrFknG9@stanley.mountain/
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

---

Some unreleased smatch, though, because I cannot reproduce the warning,
but I imagine Dan keeps the tastiests reports for later. :)
---
 drivers/soc/qcom/icc-bwmon.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/soc/qcom/icc-bwmon.c b/drivers/soc/qcom/icc-bwmon.c
index 3dfa448bf8cf..597f9025e422 100644
--- a/drivers/soc/qcom/icc-bwmon.c
+++ b/drivers/soc/qcom/icc-bwmon.c
@@ -656,6 +656,9 @@ static irqreturn_t bwmon_intr_thread(int irq, void *dev_id)
 	if (IS_ERR(target_opp) && PTR_ERR(target_opp) == -ERANGE)
 		target_opp = dev_pm_opp_find_bw_floor(bwmon->dev, &bw_kbps, 0);
 
+	if (IS_ERR(target_opp))
+		return IRQ_HANDLED;
+
 	bwmon->target_kbps = bw_kbps;
 
 	bw_kbps--;
-- 
2.48.1

Re: [PATCH] soc: qcom: icc-bwmon: Fix handling dev_pm_opp_find_bw_*() errors

[Konrad Dybcio]

On 8/14/25 8:32 AM, Krzysztof Kozlowski wrote:
> The ISR calls dev_pm_opp_find_bw_ceil(), which can return EINVAL, ERANGE
> or ENODEV, and if that one fails with ERANGE, then it tries again with
> floor dev_pm_opp_find_bw_floor().
> 
> Code misses error checks for two cases:
> 1. First dev_pm_opp_find_bw_ceil() failed with error different than
>    ERANGE,
> 2. Any error from second dev_pm_opp_find_bw_floor().
> 
> In an unlikely case these error happened, the code would further
> dereference the ERR pointer.  Close that possibility and make the code
> more obvious that all errors are correctly handled.
> 
> Reported by Smatch:
>   icc-bwmon.c:693 bwmon_intr_thread() error: 'target_opp' dereferencing possible ERR_PTR()
> 
> Fixes: b9c2ae6cac40 ("soc: qcom: icc-bwmon: Add bandwidth monitoring driver")
> Cc: <stable@vger.kernel.org>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/r/aJTNEQsRFjrFknG9@stanley.mountain/
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> ---
> 
> Some unreleased smatch, though, because I cannot reproduce the warning,
> but I imagine Dan keeps the tastiests reports for later. :)
> ---
>  drivers/soc/qcom/icc-bwmon.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/soc/qcom/icc-bwmon.c b/drivers/soc/qcom/icc-bwmon.c
> index 3dfa448bf8cf..597f9025e422 100644
> --- a/drivers/soc/qcom/icc-bwmon.c
> +++ b/drivers/soc/qcom/icc-bwmon.c
> @@ -656,6 +656,9 @@ static irqreturn_t bwmon_intr_thread(int irq, void *dev_id)
>  	if (IS_ERR(target_opp) && PTR_ERR(target_opp) == -ERANGE)
>  		target_opp = dev_pm_opp_find_bw_floor(bwmon->dev, &bw_kbps, 0);
>  
> +	if (IS_ERR(target_opp))
> +		return IRQ_HANDLED;

So the thunk above checks for a ceil freq relative to bw_kbps and then
if it doesn't exist, for a floor one

Meaning essentially if we fall into this branch, there's no OPPs in the
table, which would have been caught in probe

Konrad

Re: [PATCH] soc: qcom: icc-bwmon: Fix handling dev_pm_opp_find_bw_*() errors

[Krzysztof Kozlowski]

On 14/08/2025 11:15, Konrad Dybcio wrote:
> On 8/14/25 8:32 AM, Krzysztof Kozlowski wrote:
>> The ISR calls dev_pm_opp_find_bw_ceil(), which can return EINVAL, ERANGE
>> or ENODEV, and if that one fails with ERANGE, then it tries again with
>> floor dev_pm_opp_find_bw_floor().
>>
>> Code misses error checks for two cases:
>> 1. First dev_pm_opp_find_bw_ceil() failed with error different than
>>    ERANGE,
>> 2. Any error from second dev_pm_opp_find_bw_floor().
>>
>> In an unlikely case these error happened, the code would further
>> dereference the ERR pointer.  Close that possibility and make the code
>> more obvious that all errors are correctly handled.
>>
>> Reported by Smatch:
>>   icc-bwmon.c:693 bwmon_intr_thread() error: 'target_opp' dereferencing possible ERR_PTR()
>>
>> Fixes: b9c2ae6cac40 ("soc: qcom: icc-bwmon: Add bandwidth monitoring driver")
>> Cc: <stable@vger.kernel.org>
>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>> Closes: https://lore.kernel.org/r/aJTNEQsRFjrFknG9@stanley.mountain/
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>
>> ---
>>
>> Some unreleased smatch, though, because I cannot reproduce the warning,
>> but I imagine Dan keeps the tastiests reports for later. :)
>> ---
>>  drivers/soc/qcom/icc-bwmon.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/soc/qcom/icc-bwmon.c b/drivers/soc/qcom/icc-bwmon.c
>> index 3dfa448bf8cf..597f9025e422 100644
>> --- a/drivers/soc/qcom/icc-bwmon.c
>> +++ b/drivers/soc/qcom/icc-bwmon.c
>> @@ -656,6 +656,9 @@ static irqreturn_t bwmon_intr_thread(int irq, void *dev_id)
>>  	if (IS_ERR(target_opp) && PTR_ERR(target_opp) == -ERANGE)
>>  		target_opp = dev_pm_opp_find_bw_floor(bwmon->dev, &bw_kbps, 0);
>>  
>> +	if (IS_ERR(target_opp))
>> +		return IRQ_HANDLED;
> 
> So the thunk above checks for a ceil freq relative to bw_kbps and then
> if it doesn't exist, for a floor one
> 
> Meaning essentially if we fall into this branch, there's no OPPs in the
> table, which would have been caught in probe
Yes, unless:
1. There is a bug in the opp code
2. Probe code is anyhow changed in the future

I think the code should be readable and obviouswithin the function, not
depend on some pre-checks in the probe. But if you think that's
defensive coding I can also add a comment to silence future Smatch
complains.

Best regards,
Krzysztof

Re: [PATCH] soc: qcom: icc-bwmon: Fix handling dev_pm_opp_find_bw_*() errors

[Konrad Dybcio]

On 8/14/25 1:25 PM, Krzysztof Kozlowski wrote:
> On 14/08/2025 11:15, Konrad Dybcio wrote:
>> On 8/14/25 8:32 AM, Krzysztof Kozlowski wrote:
>>> The ISR calls dev_pm_opp_find_bw_ceil(), which can return EINVAL, ERANGE
>>> or ENODEV, and if that one fails with ERANGE, then it tries again with
>>> floor dev_pm_opp_find_bw_floor().
>>>
>>> Code misses error checks for two cases:
>>> 1. First dev_pm_opp_find_bw_ceil() failed with error different than
>>>    ERANGE,
>>> 2. Any error from second dev_pm_opp_find_bw_floor().
>>>
>>> In an unlikely case these error happened, the code would further
>>> dereference the ERR pointer.  Close that possibility and make the code
>>> more obvious that all errors are correctly handled.
>>>
>>> Reported by Smatch:
>>>   icc-bwmon.c:693 bwmon_intr_thread() error: 'target_opp' dereferencing possible ERR_PTR()
>>>
>>> Fixes: b9c2ae6cac40 ("soc: qcom: icc-bwmon: Add bandwidth monitoring driver")
>>> Cc: <stable@vger.kernel.org>
>>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>>> Closes: https://lore.kernel.org/r/aJTNEQsRFjrFknG9@stanley.mountain/
>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>
>>> ---
>>>
>>> Some unreleased smatch, though, because I cannot reproduce the warning,
>>> but I imagine Dan keeps the tastiests reports for later. :)
>>> ---
>>>  drivers/soc/qcom/icc-bwmon.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/soc/qcom/icc-bwmon.c b/drivers/soc/qcom/icc-bwmon.c
>>> index 3dfa448bf8cf..597f9025e422 100644
>>> --- a/drivers/soc/qcom/icc-bwmon.c
>>> +++ b/drivers/soc/qcom/icc-bwmon.c
>>> @@ -656,6 +656,9 @@ static irqreturn_t bwmon_intr_thread(int irq, void *dev_id)
>>>  	if (IS_ERR(target_opp) && PTR_ERR(target_opp) == -ERANGE)
>>>  		target_opp = dev_pm_opp_find_bw_floor(bwmon->dev, &bw_kbps, 0);
>>>  
>>> +	if (IS_ERR(target_opp))
>>> +		return IRQ_HANDLED;
>>
>> So the thunk above checks for a ceil freq relative to bw_kbps and then
>> if it doesn't exist, for a floor one
>>
>> Meaning essentially if we fall into this branch, there's no OPPs in the
>> table, which would have been caught in probe
> Yes, unless:
> 1. There is a bug in the opp code
> 2. Probe code is anyhow changed in the future
> 
> I think the code should be readable and obviouswithin the function, not
> depend on some pre-checks in the probe. But if you think that's
> defensive coding I can also add a comment to silence future Smatch
> complains.

I ultimately don't *really* mind either, just wanted to point out that
currently it's effectively a false positive

Konrad

Re: [PATCH] soc: qcom: icc-bwmon: Fix handling dev_pm_opp_find_bw_*() errors

[Dan Carpenter]

On Thu, Aug 14, 2025 at 11:15:08AM +0200, Konrad Dybcio wrote:
> On 8/14/25 8:32 AM, Krzysztof Kozlowski wrote:
> > The ISR calls dev_pm_opp_find_bw_ceil(), which can return EINVAL, ERANGE
> > or ENODEV, and if that one fails with ERANGE, then it tries again with
> > floor dev_pm_opp_find_bw_floor().
> > 
> > Code misses error checks for two cases:
> > 1. First dev_pm_opp_find_bw_ceil() failed with error different than
> >    ERANGE,
> > 2. Any error from second dev_pm_opp_find_bw_floor().
> > 
> > In an unlikely case these error happened, the code would further
> > dereference the ERR pointer.  Close that possibility and make the code
> > more obvious that all errors are correctly handled.
> > 
> > Reported by Smatch:
> >   icc-bwmon.c:693 bwmon_intr_thread() error: 'target_opp' dereferencing possible ERR_PTR()
> > 
> > Fixes: b9c2ae6cac40 ("soc: qcom: icc-bwmon: Add bandwidth monitoring driver")
> > Cc: <stable@vger.kernel.org>
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Closes: https://lore.kernel.org/r/aJTNEQsRFjrFknG9@stanley.mountain/
> > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > 
> > ---
> > 
> > Some unreleased smatch, though, because I cannot reproduce the warning,
> > but I imagine Dan keeps the tastiests reports for later. :)
> > ---
> >  drivers/soc/qcom/icc-bwmon.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/soc/qcom/icc-bwmon.c b/drivers/soc/qcom/icc-bwmon.c
> > index 3dfa448bf8cf..597f9025e422 100644
> > --- a/drivers/soc/qcom/icc-bwmon.c
> > +++ b/drivers/soc/qcom/icc-bwmon.c
> > @@ -656,6 +656,9 @@ static irqreturn_t bwmon_intr_thread(int irq, void *dev_id)
> >  	if (IS_ERR(target_opp) && PTR_ERR(target_opp) == -ERANGE)
> >  		target_opp = dev_pm_opp_find_bw_floor(bwmon->dev, &bw_kbps, 0);
> >  
> > +	if (IS_ERR(target_opp))
> > +		return IRQ_HANDLED;
> 
> So the thunk above checks for a ceil freq relative to bw_kbps and then
> if it doesn't exist, for a floor one
> 
> Meaning essentially if we fall into this branch, there's no OPPs in the
> table, which would have been caught in probe

It would be really hard to silence a false positive like this...

regards,
dan carpenter

Re: [PATCH] soc: qcom: icc-bwmon: Fix handling dev_pm_opp_find_bw_*() errors

[Krzysztof Kozlowski]

On 14/08/2025 13:27, Konrad Dybcio wrote:
> On 8/14/25 1:25 PM, Krzysztof Kozlowski wrote:
>> On 14/08/2025 11:15, Konrad Dybcio wrote:
>>> On 8/14/25 8:32 AM, Krzysztof Kozlowski wrote:
>>>> The ISR calls dev_pm_opp_find_bw_ceil(), which can return EINVAL, ERANGE
>>>> or ENODEV, and if that one fails with ERANGE, then it tries again with
>>>> floor dev_pm_opp_find_bw_floor().
>>>>
>>>> Code misses error checks for two cases:
>>>> 1. First dev_pm_opp_find_bw_ceil() failed with error different than
>>>>    ERANGE,
>>>> 2. Any error from second dev_pm_opp_find_bw_floor().
>>>>
>>>> In an unlikely case these error happened, the code would further
>>>> dereference the ERR pointer.  Close that possibility and make the code
>>>> more obvious that all errors are correctly handled.
>>>>
>>>> Reported by Smatch:
>>>>   icc-bwmon.c:693 bwmon_intr_thread() error: 'target_opp' dereferencing possible ERR_PTR()
>>>>
>>>> Fixes: b9c2ae6cac40 ("soc: qcom: icc-bwmon: Add bandwidth monitoring driver")
>>>> Cc: <stable@vger.kernel.org>
>>>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>>>> Closes: https://lore.kernel.org/r/aJTNEQsRFjrFknG9@stanley.mountain/
>>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>>
>>>> ---
>>>>
>>>> Some unreleased smatch, though, because I cannot reproduce the warning,
>>>> but I imagine Dan keeps the tastiests reports for later. :)
>>>> ---
>>>>  drivers/soc/qcom/icc-bwmon.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/soc/qcom/icc-bwmon.c b/drivers/soc/qcom/icc-bwmon.c
>>>> index 3dfa448bf8cf..597f9025e422 100644
>>>> --- a/drivers/soc/qcom/icc-bwmon.c
>>>> +++ b/drivers/soc/qcom/icc-bwmon.c
>>>> @@ -656,6 +656,9 @@ static irqreturn_t bwmon_intr_thread(int irq, void *dev_id)
>>>>  	if (IS_ERR(target_opp) && PTR_ERR(target_opp) == -ERANGE)
>>>>  		target_opp = dev_pm_opp_find_bw_floor(bwmon->dev, &bw_kbps, 0);
>>>>  
>>>> +	if (IS_ERR(target_opp))
>>>> +		return IRQ_HANDLED;
>>>
>>> So the thunk above checks for a ceil freq relative to bw_kbps and then
>>> if it doesn't exist, for a floor one
>>>
>>> Meaning essentially if we fall into this branch, there's no OPPs in the
>>> table, which would have been caught in probe
>> Yes, unless:
>> 1. There is a bug in the opp code
>> 2. Probe code is anyhow changed in the future
>>
>> I think the code should be readable and obviouswithin the function, not
>> depend on some pre-checks in the probe. But if you think that's
>> defensive coding I can also add a comment to silence future Smatch
>> complains.
> 
> I ultimately don't *really* mind either, just wanted to point out that
> currently it's effectively a false positive
I will adjust the commit msg to point out that it is actually impossible
condition now.

Best regards,
Krzysztof