On Thu, Jun 20, 2024 at 12:48:04PM GMT, Konrad Dybcio wrote:
>
>
> On 6/20/24 10:55, Shivnandan Kumar wrote:
> >
> >
> > 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@xxxxxxxxxxx>
> > > > ---
> > > > 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@xxxxxxxxxx>
> > > > L: linux-arm-msm@xxxxxxxxxxxxxxx
> > > > 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?
>
> Oh this one's on me, I skipped over the middle assignment.. Sorry!
Still:
CHECK: multiple assignments should be avoided
#57: FILE: drivers/soc/qcom/icc-bwmon.c:652:
+ meas_kbps = bw_kbps = bwmon->target_kbps;
While we are at it:
CHECK: Alignment should match open parenthesis
#88: FILE: drivers/soc/qcom/trace_icc-bwmon.h:14:
+TRACE_EVENT(qcom_bwmon_update,
+
CHECK: Alignment should match open parenthesis
#109: FILE: drivers/soc/qcom/trace_icc-bwmon.h:35:
+ TP_printk("name=%s meas_kbps=%u up_kbps=%u down_kbps=%u",
+ __get_str(name),
--
With best wishes
Dmitry
