remoteproc: qcom_wcnss: Fix handling the lack of PD regulators in v3

The changes introduced to handle single power domain platforms have swapped the info pointer increment from num_pd_vregs to num_pds, which would shift the info pointer past the end of the array for pronto-v3, which does not list power domain regulators in vregs.

This showed up as a difference between GCC- and LLVM-compiled kernels on SDM632 devices, where only with LLVM one would get the "regulator request with no identifier" error, because the out-of-bounds memory ended up being zeroed. Fix by skipping the increment when there are more power domains than regulators.

Signed-off-by Val Packett <[email protected]>

---

The changes introduced to handle single power domain platforms have swapped the info pointer increment from num_pd_vregs to num_pds, which would shift the info pointer past the end of the array for pronto-v3, which does not list power domain regulators in vregs.

This showed up as a difference between GCC- and LLVM-compiled kernels on SDM632 devices, where only with LLVM one would get the "regulator request with no identifier" error, because the out-of-bounds memory ended up being zeroed. Fix by skipping the increment when there are more power domains than regulators.

Signed-off-by Val Packett <[email protected]>

---
v2: changed to detect the >= condition suggested by Konrad
v1: https://lore.kernel.org/all/[email protected]/

"possible_pds" is the best name I could come up with (as "num" is already taken by the number of *successfully attached* PDs and "max" is the constant for the array length) for the count we're checking against. Maybe the "num" could be changed to "attached" but that feels like too much diff.

~val
---

 drivers/remoteproc/qcom_wcnss.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

drivers/remoteproc/qcom_wcnss.c +14 -9

index ee18bf2e8054..60f629b5bbed 100644
@@ -441,25 +441,31 @@ static void wcnss_release_pds(struct qcom_wcnss *wcnss)
 }
 
 static int wcnss_init_regulators(struct qcom_wcnss *wcnss,
-				 const struct wcnss_vreg_info *info,
-				 int num_vregs, int num_pd_vregs)
+				 const struct wcnss_data *data)
 {
+	const struct wcnss_vreg_info *info = data->vregs;
 	struct regulator_bulk_data *bulk;
+	size_t i, possible_pds = 0, num_vregs = data->num_vregs;
 	int ret;
-	int i;
+
+	for (i = 0; i < WCNSS_MAX_PDS; i++)
+		if (data->pd_names[i])
+			possible_pds++;
 
 	/*
 	 * If attaching the power domains suceeded we can skip requesting
 	 * the regulators for the power domains. For old device trees we need to
 	 * reserve extra space to manage them through the regulator interface.
 	 */
-	if (wcnss->num_pds) {
+	if (possible_pds >= num_vregs) {
+		/* Do nothing if vregs do not include PD regulators (pronto-v3) */
+	} else if (wcnss->num_pds) {
 		info += wcnss->num_pds;
 		/* Handle single power domain case */
-		if (wcnss->num_pds < num_pd_vregs)
-			num_vregs += num_pd_vregs - wcnss->num_pds;
+		if (wcnss->num_pds < data->num_pd_vregs)
+			num_vregs += data->num_pd_vregs - wcnss->num_pds;
 	} else {
-		num_vregs += num_pd_vregs;
+		num_vregs += data->num_pd_vregs;
 	}
 
 	bulk = devm_kcalloc(wcnss->dev,

@@ -607,8 +613,7 @@ static int wcnss_probe(struct platform_device *pdev)
 	if (ret && (ret != -ENODATA || !data->num_pd_vregs))
 		return ret;
 
-	ret = wcnss_init_regulators(wcnss, data->vregs, data->num_vregs,
-				    data->num_pd_vregs);
+	ret = wcnss_init_regulators(wcnss, data);
 	if (ret)
 		goto detach_pds;
 

Signature · 1 line

2.52.0

Quoted 5 lines · Val Packett

The changes introduced to handle single power domain platforms have swapped the info pointer increment from num_pd_vregs to num_pds, which would shift the info pointer past the end of the array for pronto-v3, which does not list power domain regulators in vregs.

Quoted 6 lines

This showed up as a difference between GCC- and LLVM-compiled kernels on SDM632 devices, where only with LLVM one would get the "regulator request with no identifier" error, because the out-of-bounds memory ended up being zeroed. Fix by skipping the increment when there are more power domains than regulators.

Is the error only an error print, or did the thing stop working as well?

Quoted 12 lines · Bjorn Andersson

Quoted 11 lines · Val Packett

The changes introduced to handle single power domain platforms have swapped the info pointer increment from num_pd_vregs to num_pds, which would shift the info pointer past the end of the array for pronto-v3, which does not list power domain regulators in vregs.

This showed up as a difference between GCC- and LLVM-compiled kernels on SDM632 devices, where only with LLVM one would get the "regulator request with no identifier" error, because the out-of-bounds memory ended up being zeroed. Fix by skipping the increment when there are more power domains than regulators.

Quoted 1 line

Is the error only an error print, or did the thing stop working as well?

It's a real error. Likely no one would've bothered debugging it if it weren't breaking everything :)

It was a blocker for allowing LLVM/clang builds of the msm8953 kernel in postmarketOS, the whole reason to dig into this was "switching to clang breaks the modem, WTF?!"

~val

Quoted 14 lines · Val Packett

Quoted 13 lines · Bjorn Andersson

Quoted 11 lines · Val Packett

The changes introduced to handle single power domain platforms have swapped the info pointer increment from num_pd_vregs to num_pds, which would shift the info pointer past the end of the array for pronto-v3, which does not list power domain regulators in vregs.

This showed up as a difference between GCC- and LLVM-compiled kernels on SDM632 devices, where only with LLVM one would get the "regulator request with no identifier" error, because the out-of-bounds memory ended up being zeroed. Fix by skipping the increment when there are more power domains than regulators.

Is the error only an error print, or did the thing stop working as well?

Quoted 7 lines

It's a real error. Likely no one would've bothered debugging it if it weren't breaking everything :)

It was a blocker for allowing LLVM/clang builds of the msm8953 kernel in postmarketOS, the whole reason to dig into this was "switching to clang breaks the modem, WTF?!"

Can you provide me with a Fixes: line please?

The changes introduced to handle single power domain platforms have swapped the info pointer increment from num_pd_vregs to num_pds, which would shift the info pointer past the end of the array for pronto-v3, which does not list power domain regulators in vregs.

This showed up as a difference between GCC- and LLVM-compiled kernels on SDM632 devices, where only with LLVM one would get the "regulator request with no identifier" error, because the out-of-bounds memory ended up being zeroed. Fix by skipping the increment when there are more power domains than regulators.

[...]