[PATCH 1/4] dt-bindings: soc: qcom: stats: add compatible for SM8150 platform

[PATCH 1/4] dt-bindings: soc: qcom: stats: add compatible for SM8150 platform

[Dmitry Baryshkov]

The RPMh stats on the SM8150 platform have 3 stat slots (unlike sdm845),
but still doesn't have DDR sleep stats (unlike other generic platforms).
Add SoC-specific compatible string.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 Documentation/devicetree/bindings/soc/qcom/qcom-stats.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom-stats.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom-stats.yaml
index 686a7ef2f48a..9cd75bedc7d2 100644
--- a/Documentation/devicetree/bindings/soc/qcom/qcom-stats.yaml
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom-stats.yaml
@@ -21,6 +21,7 @@ properties:
     enum:
       - qcom,rpmh-stats
       - qcom,sdm845-rpmh-stats
+      - qcom,sm8150-rpmh-stats
       - qcom,rpm-stats
       # For older RPM firmware versions with fixed offset for the sleep stats
       - qcom,apq8084-rpm-stats
-- 
2.39.2

[PATCH 2/4] soc: qcom: stats: don't crash if DDR offset contains invalid data

[Dmitry Baryshkov]

The stats ram on sm8150 platform contains invalid data at the
DDR_DYNAMIC_OFFSET. Most likely this is because the platform didn't
support DDR sleep stats. However this platform uses generic
"qcom,rpmh-stats" compatible, which implies presense of the DDR data.
Add safety net to prevent old DTB files from crashing the
qcom,rpmh-stats driver.

Fixes: e84e61bdb97c ("soc: qcom: stats: Add DDR sleep stats")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/soc/qcom/qcom_stats.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/qcom/qcom_stats.c b/drivers/soc/qcom/qcom_stats.c
index 4763d62a8cb0..813c9f3c6bec 100644
--- a/drivers/soc/qcom/qcom_stats.c
+++ b/drivers/soc/qcom/qcom_stats.c
@@ -319,6 +319,7 @@ static void qcom_create_subsystem_stat_files(struct dentry *root,
 static int qcom_create_ddr_stats_files(struct device *dev,
 				       struct dentry *root,
 				       void __iomem *reg,
+				       resource_size_t reg_size,
 				       const struct stats_config *config)
 {
 	struct ddr_stats_data *ddrd;
@@ -337,6 +338,8 @@ static int qcom_create_ddr_stats_files(struct device *dev,
 
 	/* Get the offset of DDR stats */
 	stats_offset = readl(reg + DDR_DYNAMIC_OFFSET) & DDR_OFFSET_MASK;
+	if (stats_offset >= reg_size || stats_offset % 4)
+		return -EINVAL;
 	ddrd->base = reg + stats_offset;
 
 	/* Check if DDR stats are present */
@@ -364,6 +367,7 @@ static int qcom_stats_probe(struct platform_device *pdev)
 	void __iomem *reg;
 	struct dentry *root;
 	const struct stats_config *config;
+	struct resource *res;
 	struct stats_data *d;
 	int i, ret;
 
@@ -371,7 +375,7 @@ static int qcom_stats_probe(struct platform_device *pdev)
 	if (!config)
 		return -ENODEV;
 
-	reg = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
+	reg = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
 	if (IS_ERR(reg))
 		return -ENOMEM;
 
@@ -387,7 +391,9 @@ static int qcom_stats_probe(struct platform_device *pdev)
 
 	qcom_create_subsystem_stat_files(root, config);
 	qcom_create_soc_sleep_stat_files(root, reg, d, config);
-	ret = qcom_create_ddr_stats_files(&pdev->dev, root, reg, config);
+	ret = qcom_create_ddr_stats_files(&pdev->dev, root, reg,
+					  resource_size(res),
+					  config);
 	if (ret) {
 		debugfs_remove_recursive(root);
 		return ret;
-- 
2.39.2

Re: [PATCH 2/4] soc: qcom: stats: don't crash if DDR offset contains invalid data

[Konrad Dybcio]

On 9.12.2023 22:55, Dmitry Baryshkov wrote:
> The stats ram on sm8150 platform contains invalid data at the
> DDR_DYNAMIC_OFFSET. Most likely this is because the platform didn't
> support DDR sleep stats.
Interesting. Can you read back DDR_DYNAMIC_OFFSET on 8350/8280 and
see if 8150 has correct data in there?

> However this platform uses generic
> "qcom,rpmh-stats" compatible, which implies presense of the DDR data.
> Add safety net to prevent old DTB files from crashing the
> qcom,rpmh-stats driver.
Yeah I'dve never thought there would be garbage in there..

I'd advocate for simply not doing anything wrt sleep stats if DDR
stats are unavailable though. The QMP handle can stay, as there
may (I don't know) be more data available that we want to export
through this driver.

Konrad

Re: [PATCH 2/4] soc: qcom: stats: don't crash if DDR offset contains invalid data

[Doug Anderson]

Hi,

On Mon, Dec 11, 2023 at 1:11 AM Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> On 9.12.2023 22:55, Dmitry Baryshkov wrote:
> > The stats ram on sm8150 platform contains invalid data at the
> > DDR_DYNAMIC_OFFSET. Most likely this is because the platform didn't
> > support DDR sleep stats.
> Interesting. Can you read back DDR_DYNAMIC_OFFSET on 8350/8280 and
> see if 8150 has correct data in there?
>
> > However this platform uses generic
> > "qcom,rpmh-stats" compatible, which implies presense of the DDR data.
> > Add safety net to prevent old DTB files from crashing the
> > qcom,rpmh-stats driver.
> Yeah I'dve never thought there would be garbage in there..
>
> I'd advocate for simply not doing anything wrt sleep stats if DDR
> stats are unavailable though. The QMP handle can stay, as there
> may (I don't know) be more data available that we want to export
> through this driver.

FWIW, I'm getting a crash on sc7180-trogdor like this too. In kgdb it
says I'm on line:

key = readl(ddrd->base);

...and

(gdb) print ddrd->base
$1 = (void *) 0xffffffc0833a3149
(gdb) print reg
$2 = (void *) 0xffffffc0833a3000

...so I guess my "stats_offset" must have been 0x149.

Can we get a fix landed or a revert? Thanks! :-)

-Doug

Re: [PATCH 2/4] soc: qcom: stats: don't crash if DDR offset contains invalid data

[Konrad Dybcio]

On 12/14/23 01:59, Doug Anderson wrote:
> Hi,
> 
> On Mon, Dec 11, 2023 at 1:11 AM Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>
>> On 9.12.2023 22:55, Dmitry Baryshkov wrote:
>>> The stats ram on sm8150 platform contains invalid data at the
>>> DDR_DYNAMIC_OFFSET. Most likely this is because the platform didn't
>>> support DDR sleep stats.
>> Interesting. Can you read back DDR_DYNAMIC_OFFSET on 8350/8280 and
>> see if 8150 has correct data in there?
>>
>>> However this platform uses generic
>>> "qcom,rpmh-stats" compatible, which implies presense of the DDR data.
>>> Add safety net to prevent old DTB files from crashing the
>>> qcom,rpmh-stats driver.
>> Yeah I'dve never thought there would be garbage in there..
>>
>> I'd advocate for simply not doing anything wrt sleep stats if DDR
>> stats are unavailable though. The QMP handle can stay, as there
>> may (I don't know) be more data available that we want to export
>> through this driver.
> 
> FWIW, I'm getting a crash on sc7180-trogdor like this too. In kgdb it
> says I'm on line:
> 
> key = readl(ddrd->base);
> 
> ...and
> 
> (gdb) print ddrd->base
> $1 = (void *) 0xffffffc0833a3149
> (gdb) print reg
> $2 = (void *) 0xffffffc0833a3000
> 
> ...so I guess my "stats_offset" must have been 0x149.
> 
> Can we get a fix landed or a revert? Thanks! :-)
Right, I guess we may want to revert it and I'll try to get more
info from Qualcomm folks..

Konrad

[PATCH 3/4] soc: qcom: stats: support SM8150 platform

[Dmitry Baryshkov]

On SM8150 the RPMh stats have 3 data records, but no DDR sleep stats,
which demands platform-specific compatible and data.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/soc/qcom/qcom_stats.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/soc/qcom/qcom_stats.c b/drivers/soc/qcom/qcom_stats.c
index 813c9f3c6bec..dd1b6fee7739 100644
--- a/drivers/soc/qcom/qcom_stats.c
+++ b/drivers/soc/qcom/qcom_stats.c
@@ -438,6 +438,14 @@ static const struct stats_config rpmh_data_sdm845 = {
 	.subsystem_stats_in_smem = true,
 };
 
+static const struct stats_config rpmh_data_sm8150 = {
+	.stats_offset = 0x48,
+	.num_records = 3,
+	.appended_stats_avail = false,
+	.dynamic_offset = false,
+	.subsystem_stats_in_smem = true,
+};
+
 static const struct stats_config rpmh_data = {
 	.stats_offset = 0x48,
 	.num_records = 3,
@@ -455,6 +463,7 @@ static const struct of_device_id qcom_stats_table[] = {
 	{ .compatible = "qcom,rpm-stats", .data = &rpm_data },
 	{ .compatible = "qcom,rpmh-stats", .data = &rpmh_data },
 	{ .compatible = "qcom,sdm845-rpmh-stats", .data = &rpmh_data_sdm845 },
+	{ .compatible = "qcom,sm8150-rpmh-stats", .data = &rpmh_data_sm8150 },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, qcom_stats_table);
-- 
2.39.2

Re: [PATCH 3/4] soc: qcom: stats: support SM8150 platform

[Konrad Dybcio]

On 9.12.2023 22:56, Dmitry Baryshkov wrote:
> On SM8150 the RPMh stats have 3 data records, but no DDR sleep stats,
> which demands platform-specific compatible and data.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
I don't think it makes sense considering the driver could detect the
presence (or possibility of presence) of DDR stats at runtime.

Konrad

Re: [PATCH 3/4] soc: qcom: stats: support SM8150 platform

[Dmitry Baryshkov]

On Mon, 11 Dec 2023 at 11:11, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> On 9.12.2023 22:56, Dmitry Baryshkov wrote:
> > On SM8150 the RPMh stats have 3 data records, but no DDR sleep stats,
> > which demands platform-specific compatible and data.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> I don't think it makes sense considering the driver could detect the
> presence (or possibility of presence) of DDR stats at runtime.

No, it can not really. We have safety nets for checking the offset
value and then checking the magic number. But I'd prefer to be
explicit here. It's not that the 'invalid' data at this offset is 0 or
~0.
So, I'd prefer to be explicit here.

Actually we probably should have used SoC compat entries from the
beginning, even if looks ridiculous for such small thing.

-- 
With best wishes
Dmitry

Re: [PATCH 3/4] soc: qcom: stats: support SM8150 platform

[Konrad Dybcio]

On 11.12.2023 10:43, Dmitry Baryshkov wrote:
> On Mon, 11 Dec 2023 at 11:11, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>
>> On 9.12.2023 22:56, Dmitry Baryshkov wrote:
>>> On SM8150 the RPMh stats have 3 data records, but no DDR sleep stats,
>>> which demands platform-specific compatible and data.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>> I don't think it makes sense considering the driver could detect the
>> presence (or possibility of presence) of DDR stats at runtime.
> 
> No, it can not really. We have safety nets for checking the offset
> value and then checking the magic number. But I'd prefer to be
> explicit here. It's not that the 'invalid' data at this offset is 0 or
> ~0.
> So, I'd prefer to be explicit here.
I'd say we're quite covered:

if (ddr_stats_offset)
	if (offset is within the range) // your latest patchset
		if (ddr_stats_magic)
			if (entries)
				"show stats"
			else
				"show nothing"
		else
			"no ddr stats"
	else
		"no ddr stats"
else
	"no ddr stats"

Konrad

Re: [PATCH 3/4] soc: qcom: stats: support SM8150 platform

[Dmitry Baryshkov]

On Mon, 11 Dec 2023 at 12:46, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> On 11.12.2023 10:43, Dmitry Baryshkov wrote:
> > On Mon, 11 Dec 2023 at 11:11, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> >>
> >> On 9.12.2023 22:56, Dmitry Baryshkov wrote:
> >>> On SM8150 the RPMh stats have 3 data records, but no DDR sleep stats,
> >>> which demands platform-specific compatible and data.
> >>>
> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>> ---
> >> I don't think it makes sense considering the driver could detect the
> >> presence (or possibility of presence) of DDR stats at runtime.
> >
> > No, it can not really. We have safety nets for checking the offset
> > value and then checking the magic number. But I'd prefer to be
> > explicit here. It's not that the 'invalid' data at this offset is 0 or
> > ~0.
> > So, I'd prefer to be explicit here.
> I'd say we're quite covered:
>
> if (ddr_stats_offset)
>         if (offset is within the range) // your latest patchset
>                 if (ddr_stats_magic)
>                         if (entries)
>                                 "show stats"
>                         else
>                                 "show nothing"
>                 else
>                         "no ddr stats"
>         else
>                 "no ddr stats"
> else
>         "no ddr stats"
>
> Konrad

I'd say, too many ifs. I'd prefer to have it disabled for this platform.
-- 
With best wishes
Dmitry

[PATCH 4/4] arm64: dts: qcom: sm8150: use SoC-specific compat for RPMh stats

[Dmitry Baryshkov]

The SM8150 platform doesn't support DDR sleep stats, so it needs
SoC-specific compat string for the RPMh stats data.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8150.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8150.dtsi b/arch/arm64/boot/dts/qcom/sm8150.dtsi
index 3cba87e00123..fb41f91cefc6 100644
--- a/arch/arm64/boot/dts/qcom/sm8150.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8150.dtsi
@@ -3950,7 +3950,7 @@ aoss_qmp: power-management@c300000 {
 		};
 
 		sram@c3f0000 {
-			compatible = "qcom,rpmh-stats";
+			compatible = "qcom,sm8150-rpmh-stats";
 			reg = <0 0x0c3f0000 0 0x400>;
 		};
 
-- 
2.39.2

Re: [PATCH 1/4] dt-bindings: soc: qcom: stats: add compatible for SM8150 platform

[Krzysztof Kozlowski]

On 09/12/2023 22:55, Dmitry Baryshkov wrote:
> The RPMh stats on the SM8150 platform have 3 stat slots (unlike sdm845),
> but still doesn't have DDR sleep stats (unlike other generic platforms).
> Add SoC-specific compatible string.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  Documentation/devicetree/bindings/soc/qcom/qcom-stats.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof