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:
- First dev_pm_opp_find_bw_ceil() failed with error different than ERANGE,
- 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:
- There is a bug in the opp code
- 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