Re: [PATCH] soc: qcom: icc-bwmon: Add tracepoints in bwmon_intr_thread

[Shivnandan Kumar <quic_kshivnan@quicinc.com>]

On 6/20/2024 12:46 AM, Konrad Dybcio wrote:
> 
> 
> On 6/19/24 15:51, Shivnandan Kumar wrote:
>> Add tracepoint for tracing the measured traffic in kbps,
>> up_kbps and down_kbps in bwmon. This information is valuable
>> for understanding what bwmon hw measures at the system cache
>> level and at the DDR level which is helpful in debugging
>> bwmon behavior.
>>
>> Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com>
>> ---
>>   MAINTAINERS                        |  1 +
>>   drivers/soc/qcom/icc-bwmon.c       |  7 +++--
>>   drivers/soc/qcom/trace_icc-bwmon.h | 49 ++++++++++++++++++++++++++++++
>>   3 files changed, 55 insertions(+), 2 deletions(-)
>>   create mode 100644 drivers/soc/qcom/trace_icc-bwmon.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 242fc612fbc5..1b410c0183bb 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -18573,6 +18573,7 @@ M:    Krzysztof Kozlowski 
>> <krzysztof.kozlowski@linaro.org>
>>   L:    linux-arm-msm@vger.kernel.org
>>   S:    Maintained
>>   F:    
>> Documentation/devicetree/bindings/interconnect/qcom,msm8998-bwmon.yaml
>> +F:    drivers/soc/qcom/trace_icc-bwmon.h
>>   F:    drivers/soc/qcom/icc-bwmon.c
>>
>>   QUALCOMM IOMMU
>> diff --git a/drivers/soc/qcom/icc-bwmon.c b/drivers/soc/qcom/icc-bwmon.c
>> index fb323b3364db..9b5ac1e62673 100644
>> --- a/drivers/soc/qcom/icc-bwmon.c
>> +++ b/drivers/soc/qcom/icc-bwmon.c
>> @@ -17,6 +17,8 @@
>>   #include <linux/pm_opp.h>
>>   #include <linux/regmap.h>
>>   #include <linux/sizes.h>
>> +#define CREATE_TRACE_POINTS
>> +#include "trace_icc-bwmon.h"
>>
>>   /*
>>    * The BWMON samples data throughput within 'sample_ms' time. With 
>> three
>> @@ -645,9 +647,9 @@ static irqreturn_t bwmon_intr_thread(int irq, void 
>> *dev_id)
>>       struct icc_bwmon *bwmon = dev_id;
>>       unsigned int irq_enable = 0;
>>       struct dev_pm_opp *opp, *target_opp;
>> -    unsigned int bw_kbps, up_kbps, down_kbps;
>> +    unsigned int bw_kbps, up_kbps, down_kbps, meas_kbps;
>>
>> -    bw_kbps = bwmon->target_kbps;
>> +    meas_kbps = bw_kbps = bwmon->target_kbps;
>>
>>       target_opp = dev_pm_opp_find_bw_ceil(bwmon->dev, &bw_kbps, 0);
> 
> This breaks bwmon, as dev_pm_opp_find_bw_ceil is now fed a random
> (uninitialized variable) value
> 

Thank you for reviewing the patch.
I didn't get it, still the variable "bw_kbps" is being initialized along 
with "meas_kbps". Which variable are you referring to as being fed to 
dev_pm_opp_find_bw_ceil  with an uninitialized value?

Thanks
Shivnandan

> Konrad