[PATCH] soc: qcom: icc-bwmon: Don't ignore return values of regmap functions

[PATCH] soc: qcom: icc-bwmon: Don't ignore return values of regmap functions

[Konrad Dybcio]

As it turns out, not all regmap accesses succeed. Not knowing this is
particularly suboptimal when there's a breaking change to the regmap
APIs. Monitor the return values of regmap_ calls and propagate errors,
should any occur.

To keep any level of readability in bwmon_enable(), add some comments
to separate the logical blocks.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
Depends on: https://lore.kernel.org/linux-arm-msm/20230610-topic-bwmon_opp-v2-1-0d25c1ce7dca@linaro.org/
---
 drivers/soc/qcom/icc-bwmon.c | 211 ++++++++++++++++++++++++++++++-------------
 1 file changed, 150 insertions(+), 61 deletions(-)

diff --git a/drivers/soc/qcom/icc-bwmon.c b/drivers/soc/qcom/icc-bwmon.c
index 8daf0eb03279..306f911d2be0 100644
--- a/drivers/soc/qcom/icc-bwmon.c
+++ b/drivers/soc/qcom/icc-bwmon.c
@@ -449,9 +449,10 @@ static const struct regmap_config sdm845_llcc_bwmon_regmap_cfg = {
 	.cache_type		= REGCACHE_RBTREE,
 };
 
-static void bwmon_clear_counters(struct icc_bwmon *bwmon, bool clear_all)
+static int bwmon_clear_counters(struct icc_bwmon *bwmon, bool clear_all)
 {
 	unsigned int val = BWMON_CLEAR_CLEAR;
+	int ret;
 
 	if (clear_all)
 		val |= BWMON_CLEAR_CLEAR_ALL;
@@ -463,14 +464,20 @@ static void bwmon_clear_counters(struct icc_bwmon *bwmon, bool clear_all)
 	 * region. So, we need to make sure the counter clear is completed
 	 * before we try to clear the IRQ or do any other counter operations.
 	 */
-	regmap_field_force_write(bwmon->regs[F_CLEAR], val);
+	ret = regmap_field_force_write(bwmon->regs[F_CLEAR], val);
+	if (ret)
+		return ret;
+
 	if (bwmon->data->quirks & BWMON_NEEDS_FORCE_CLEAR)
-		regmap_field_force_write(bwmon->regs[F_CLEAR], 0);
+		ret = regmap_field_force_write(bwmon->regs[F_CLEAR], 0);
+
+	return ret;
 }
 
-static void bwmon_clear_irq(struct icc_bwmon *bwmon)
+static int bwmon_clear_irq(struct icc_bwmon *bwmon)
 {
 	struct regmap_field *global_irq_clr;
+	int ret;
 
 	if (bwmon->data->global_regmap_fields)
 		global_irq_clr = bwmon->global_regs[F_GLOBAL_IRQ_CLEAR];
@@ -493,17 +500,27 @@ static void bwmon_clear_irq(struct icc_bwmon *bwmon)
 	 * clearing here so that local writes don't happen before the
 	 * interrupt is cleared.
 	 */
-	regmap_field_force_write(bwmon->regs[F_IRQ_CLEAR], BWMON_IRQ_ENABLE_MASK);
-	if (bwmon->data->quirks & BWMON_NEEDS_FORCE_CLEAR)
-		regmap_field_force_write(bwmon->regs[F_IRQ_CLEAR], 0);
+	ret = regmap_field_force_write(bwmon->regs[F_IRQ_CLEAR], BWMON_IRQ_ENABLE_MASK);
+	if (ret)
+		return ret;
+
+	if (bwmon->data->quirks & BWMON_NEEDS_FORCE_CLEAR) {
+		ret = regmap_field_force_write(bwmon->regs[F_IRQ_CLEAR], 0);
+		if (ret)
+			return ret;
+	}
+
 	if (bwmon->data->quirks & BWMON_HAS_GLOBAL_IRQ)
-		regmap_field_force_write(global_irq_clr,
-					 BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE);
+		ret = regmap_field_force_write(global_irq_clr,
+					       BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE);
+
+	return ret;
 }
 
-static void bwmon_disable(struct icc_bwmon *bwmon)
+static int bwmon_disable(struct icc_bwmon *bwmon)
 {
 	struct regmap_field *global_irq_en;
+	int ret;
 
 	if (bwmon->data->global_regmap_fields)
 		global_irq_en = bwmon->global_regs[F_GLOBAL_IRQ_ENABLE];
@@ -511,20 +528,29 @@ static void bwmon_disable(struct icc_bwmon *bwmon)
 		global_irq_en = bwmon->regs[F_GLOBAL_IRQ_ENABLE];
 
 	/* Disable interrupts. Strict ordering, see bwmon_clear_irq(). */
-	if (bwmon->data->quirks & BWMON_HAS_GLOBAL_IRQ)
-		regmap_field_write(global_irq_en, 0x0);
-	regmap_field_write(bwmon->regs[F_IRQ_ENABLE], 0x0);
+	if (bwmon->data->quirks & BWMON_HAS_GLOBAL_IRQ) {
+		ret = regmap_field_write(global_irq_en, 0x0);
+		if (ret)
+			return ret;
+	}
+
+	ret = regmap_field_write(bwmon->regs[F_IRQ_ENABLE], 0x0);
+	if (ret)
+		return ret;
 
 	/*
 	 * Disable bwmon. Must happen before bwmon_clear_irq() to avoid spurious
 	 * IRQ.
 	 */
-	regmap_field_write(bwmon->regs[F_ENABLE], 0x0);
+	ret = regmap_field_write(bwmon->regs[F_ENABLE], 0x0);
+
+	return ret;
 }
 
-static void bwmon_enable(struct icc_bwmon *bwmon, unsigned int irq_enable)
+static int bwmon_enable(struct icc_bwmon *bwmon, unsigned int irq_enable)
 {
 	struct regmap_field *global_irq_en;
+	int ret;
 
 	if (bwmon->data->global_regmap_fields)
 		global_irq_en = bwmon->global_regs[F_GLOBAL_IRQ_ENABLE];
@@ -532,14 +558,21 @@ static void bwmon_enable(struct icc_bwmon *bwmon, unsigned int irq_enable)
 		global_irq_en = bwmon->regs[F_GLOBAL_IRQ_ENABLE];
 
 	/* Enable interrupts */
-	if (bwmon->data->quirks & BWMON_HAS_GLOBAL_IRQ)
-		regmap_field_write(global_irq_en,
-				   BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE);
+	if (bwmon->data->quirks & BWMON_HAS_GLOBAL_IRQ) {
+		ret = regmap_field_write(global_irq_en,
+					 BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE);
+		if (ret)
+			return ret;
+	}
 
-	regmap_field_write(bwmon->regs[F_IRQ_ENABLE], irq_enable);
+	ret = regmap_field_write(bwmon->regs[F_IRQ_ENABLE], irq_enable);
+	if (ret)
+		return ret;
 
 	/* Enable bwmon */
-	regmap_field_write(bwmon->regs[F_ENABLE], BWMON_ENABLE_ENABLE);
+	ret = regmap_field_write(bwmon->regs[F_ENABLE], BWMON_ENABLE_ENABLE);
+
+	return ret;
 }
 
 static unsigned int bwmon_kbps_to_count(struct icc_bwmon *bwmon,
@@ -548,55 +581,97 @@ static unsigned int bwmon_kbps_to_count(struct icc_bwmon *bwmon,
 	return kbps / bwmon->data->count_unit_kb;
 }
 
-static void bwmon_set_threshold(struct icc_bwmon *bwmon,
+static int bwmon_set_threshold(struct icc_bwmon *bwmon,
 				struct regmap_field *reg, unsigned int kbps)
 {
 	unsigned int thres;
 
 	thres = mult_frac(bwmon_kbps_to_count(bwmon, kbps),
 			  bwmon->data->sample_ms, MSEC_PER_SEC);
-	regmap_field_write(reg, thres);
+	return regmap_field_write(reg, thres);
 }
 
-static void bwmon_start(struct icc_bwmon *bwmon)
+static int bwmon_start(struct icc_bwmon *bwmon)
 {
 	const struct icc_bwmon_data *data = bwmon->data;
+	int ret, window;
 	u32 bw_low = 0;
-	int window;
 
 	/* No need to check for errors, as this must have succeeded before. */
 	dev_pm_opp_find_bw_ceil(bwmon->dev, &bw_low, 0);
 
-	bwmon_clear_counters(bwmon, true);
+	ret = bwmon_clear_counters(bwmon, true);
+	if (ret)
+		return ret;
 
 	window = mult_frac(bwmon->data->sample_ms, HW_TIMER_HZ, MSEC_PER_SEC);
 	/* Maximum sampling window: 0xffffff for v4 and 0xfffff for v5 */
-	regmap_field_write(bwmon->regs[F_SAMPLE_WINDOW], window);
-
-	bwmon_set_threshold(bwmon, bwmon->regs[F_THRESHOLD_HIGH], bw_low);
-	bwmon_set_threshold(bwmon, bwmon->regs[F_THRESHOLD_MED], bw_low);
-	bwmon_set_threshold(bwmon, bwmon->regs[F_THRESHOLD_LOW], 0);
-
-	regmap_field_write(bwmon->regs[F_THRESHOLD_COUNT_ZONE0],
-			   BWMON_THRESHOLD_COUNT_ZONE0_DEFAULT);
-	regmap_field_write(bwmon->regs[F_THRESHOLD_COUNT_ZONE1],
-			   data->zone1_thres_count);
-	regmap_field_write(bwmon->regs[F_THRESHOLD_COUNT_ZONE2],
-			   BWMON_THRESHOLD_COUNT_ZONE2_DEFAULT);
-	regmap_field_write(bwmon->regs[F_THRESHOLD_COUNT_ZONE3],
-			   data->zone3_thres_count);
-
-	regmap_field_write(bwmon->regs[F_ZONE_ACTIONS_ZONE0],
-			   BWMON_ZONE_ACTIONS_ZONE0);
-	regmap_field_write(bwmon->regs[F_ZONE_ACTIONS_ZONE1],
-			   BWMON_ZONE_ACTIONS_ZONE1);
-	regmap_field_write(bwmon->regs[F_ZONE_ACTIONS_ZONE2],
-			   BWMON_ZONE_ACTIONS_ZONE2);
-	regmap_field_write(bwmon->regs[F_ZONE_ACTIONS_ZONE3],
-			   BWMON_ZONE_ACTIONS_ZONE3);
-
-	bwmon_clear_irq(bwmon);
-	bwmon_enable(bwmon, BWMON_IRQ_ENABLE_MASK);
+	ret = regmap_field_write(bwmon->regs[F_SAMPLE_WINDOW], window);
+	if (ret)
+		return ret;
+
+	/* Set up bandwidth thresholds */
+	ret = bwmon_set_threshold(bwmon, bwmon->regs[F_THRESHOLD_HIGH], bw_low);
+	if (ret)
+		return ret;
+
+	ret = bwmon_set_threshold(bwmon, bwmon->regs[F_THRESHOLD_MED], bw_low);
+	if (ret)
+		return ret;
+
+	ret = bwmon_set_threshold(bwmon, bwmon->regs[F_THRESHOLD_LOW], 0);
+	if (ret)
+		return ret;
+
+	/* Set up threshold counts */
+	ret = regmap_field_write(bwmon->regs[F_THRESHOLD_COUNT_ZONE0],
+				 BWMON_THRESHOLD_COUNT_ZONE0_DEFAULT);
+	if (ret)
+		return ret;
+
+	ret = regmap_field_write(bwmon->regs[F_THRESHOLD_COUNT_ZONE1],
+				 data->zone1_thres_count);
+	if (ret)
+		return ret;
+
+	ret = regmap_field_write(bwmon->regs[F_THRESHOLD_COUNT_ZONE2],
+				 BWMON_THRESHOLD_COUNT_ZONE2_DEFAULT);
+	if (ret)
+		return ret;
+
+	ret = regmap_field_write(bwmon->regs[F_THRESHOLD_COUNT_ZONE3],
+				 data->zone3_thres_count);
+	if (ret)
+		return ret;
+
+	/* Set up zone actions */
+	ret = regmap_field_write(bwmon->regs[F_ZONE_ACTIONS_ZONE0],
+				 BWMON_ZONE_ACTIONS_ZONE0);
+	if (ret)
+		return ret;
+
+	ret = regmap_field_write(bwmon->regs[F_ZONE_ACTIONS_ZONE1],
+				 BWMON_ZONE_ACTIONS_ZONE1);
+	if (ret)
+		return ret;
+
+	ret = regmap_field_write(bwmon->regs[F_ZONE_ACTIONS_ZONE2],
+				 BWMON_ZONE_ACTIONS_ZONE2);
+	if (ret)
+		return ret;
+
+	ret = regmap_field_write(bwmon->regs[F_ZONE_ACTIONS_ZONE3],
+				 BWMON_ZONE_ACTIONS_ZONE3);
+	if (ret)
+		return ret;
+
+	/* Clear any previous interrupts and get the stone rolling! */
+	ret = bwmon_clear_irq(bwmon);
+	if (ret)
+		return ret;
+
+
+	return bwmon_enable(bwmon, BWMON_IRQ_ENABLE_MASK);
 }
 
 static irqreturn_t bwmon_intr(int irq, void *dev_id)
@@ -622,7 +697,8 @@ static irqreturn_t bwmon_intr(int irq, void *dev_id)
 		return IRQ_NONE;
 	}
 
-	bwmon_disable(bwmon);
+	if (bwmon_disable(bwmon))
+		return IRQ_NONE;
 
 	zone = get_bitmask_order(status) - 1;
 	/*
@@ -671,13 +747,20 @@ static irqreturn_t bwmon_intr_thread(int irq, void *dev_id)
 	else
 		irq_enable = BWMON_IRQ_ENABLE_MASK;
 
-	bwmon_set_threshold(bwmon, bwmon->regs[F_THRESHOLD_HIGH],
-			    up_kbps);
-	bwmon_set_threshold(bwmon, bwmon->regs[F_THRESHOLD_MED],
-			    down_kbps);
-	bwmon_clear_counters(bwmon, false);
-	bwmon_clear_irq(bwmon);
-	bwmon_enable(bwmon, irq_enable);
+	if (bwmon_set_threshold(bwmon, bwmon->regs[F_THRESHOLD_HIGH], up_kbps))
+		return IRQ_NONE;
+
+	if (bwmon_set_threshold(bwmon, bwmon->regs[F_THRESHOLD_MED], down_kbps))
+		return IRQ_NONE;
+
+	if (bwmon_clear_counters(bwmon, false))
+		return IRQ_NONE;
+
+	if (bwmon_clear_irq(bwmon))
+		return IRQ_NONE;
+
+	if (bwmon_enable(bwmon, irq_enable))
+		return IRQ_NONE;
 
 	if (bwmon->target_kbps == bwmon->current_kbps)
 		goto out;
@@ -780,7 +863,10 @@ static int bwmon_probe(struct platform_device *pdev)
 
 	bwmon->dev = dev;
 
-	bwmon_disable(bwmon);
+	ret = bwmon_disable(bwmon);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to disable BWMON\n");
+
 	ret = devm_request_threaded_irq(dev, bwmon->irq, bwmon_intr,
 					bwmon_intr_thread,
 					IRQF_ONESHOT, dev_name(dev), bwmon);
@@ -788,7 +874,10 @@ static int bwmon_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, ret, "failed to request IRQ\n");
 
 	platform_set_drvdata(pdev, bwmon);
-	bwmon_start(bwmon);
+
+	ret = bwmon_start(bwmon);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to start BWMON\n");
 
 	return 0;
 }

---
base-commit: 772c02db794651e8d006813f545b8bfd6906b718
change-id: 20230615-topic-bwmonretval-3f260e1284d8

Best regards,
-- 
Konrad Dybcio <konrad.dybcio@linaro.org>

Re: [PATCH] soc: qcom: icc-bwmon: Don't ignore return values of regmap functions

[Krzysztof Kozlowski]

On 15/06/2023 23:12, Konrad Dybcio wrote:
> As it turns out, not all regmap accesses succeed. Not knowing this is
> particularly suboptimal when there's a breaking change to the regmap
> APIs. Monitor the return values of regmap_ calls and propagate errors,
> should any occur.
> 
> To keep any level of readability in bwmon_enable(), add some comments
> to separate the logical blocks.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Nice coincidence, I just had some talks with a friend about uselessness
(IMHO) of regmap MMIO return status checks.

Sorry, for me most of this makes the code difficult to read for no gain.
Errors are not real. This is some artificial problem. Solving it makes
code less maintainable.

If we used here readl/writel, you would not add any checks, right? Then
don't add for regmap mmio.

Best regards,
Krzysztof

Re: [PATCH] soc: qcom: icc-bwmon: Don't ignore return values of regmap functions

[Bjorn Andersson]

On Thu, Jun 15, 2023 at 11:26:13PM +0200, Krzysztof Kozlowski wrote:
> On 15/06/2023 23:12, Konrad Dybcio wrote:
> > As it turns out, not all regmap accesses succeed. Not knowing this is
> > particularly suboptimal when there's a breaking change to the regmap
> > APIs. Monitor the return values of regmap_ calls and propagate errors,
> > should any occur.
> > 
> > To keep any level of readability in bwmon_enable(), add some comments
> > to separate the logical blocks.
> > 
> > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> 
> Nice coincidence, I just had some talks with a friend about uselessness
> (IMHO) of regmap MMIO return status checks.
> 
> Sorry, for me most of this makes the code difficult to read for no gain.
> Errors are not real. This is some artificial problem. Solving it makes
> code less maintainable.
> 
> If we used here readl/writel, you would not add any checks, right? Then
> don't add for regmap mmio.
> 

I agree, the mmio regmap interface should only fail because of bugs or
things are misconfigured. Would be nice to capture that in a WARN_ON()
or something...

Regards,
Bjorn

Re: [PATCH] soc: qcom: icc-bwmon: Don't ignore return values of regmap functions

[Krzysztof Kozlowski]

On 20/06/2023 20:14, Bjorn Andersson wrote:
> On Thu, Jun 15, 2023 at 11:26:13PM +0200, Krzysztof Kozlowski wrote:
>> On 15/06/2023 23:12, Konrad Dybcio wrote:
>>> As it turns out, not all regmap accesses succeed. Not knowing this is
>>> particularly suboptimal when there's a breaking change to the regmap
>>> APIs. Monitor the return values of regmap_ calls and propagate errors,
>>> should any occur.
>>>
>>> To keep any level of readability in bwmon_enable(), add some comments
>>> to separate the logical blocks.
>>>
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>
>> Nice coincidence, I just had some talks with a friend about uselessness
>> (IMHO) of regmap MMIO return status checks.
>>
>> Sorry, for me most of this makes the code difficult to read for no gain.
>> Errors are not real. This is some artificial problem. Solving it makes
>> code less maintainable.
>>
>> If we used here readl/writel, you would not add any checks, right? Then
>> don't add for regmap mmio.
>>
> 
> I agree, the mmio regmap interface should only fail because of bugs or
> things are misconfigured. Would be nice to capture that in a WARN_ON()
> or something...
> 

One choice could be to have for entire functions doing reads/writes:

	ret = 0;
	ret != regmap_write();
	ret != regmap_write();
	ret != regmap_write();
	return ret;

and handle this in the caller somehow. I don't think that aborting such
chain early, just because regmap mmio failures, makes sense.

Best regards,
Krzysztof

Re: [PATCH] soc: qcom: icc-bwmon: Don't ignore return values of regmap functions

[Konrad Dybcio]

On 20.06.2023 20:15, Krzysztof Kozlowski wrote:
> On 20/06/2023 20:14, Bjorn Andersson wrote:
>> On Thu, Jun 15, 2023 at 11:26:13PM +0200, Krzysztof Kozlowski wrote:
>>> On 15/06/2023 23:12, Konrad Dybcio wrote:
>>>> As it turns out, not all regmap accesses succeed. Not knowing this is
>>>> particularly suboptimal when there's a breaking change to the regmap
>>>> APIs. Monitor the return values of regmap_ calls and propagate errors,
>>>> should any occur.
>>>>
>>>> To keep any level of readability in bwmon_enable(), add some comments
>>>> to separate the logical blocks.
>>>>
>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>
>>> Nice coincidence, I just had some talks with a friend about uselessness
>>> (IMHO) of regmap MMIO return status checks.
>>>
>>> Sorry, for me most of this makes the code difficult to read for no gain.
>>> Errors are not real. This is some artificial problem. Solving it makes
>>> code less maintainable.
>>>
>>> If we used here readl/writel, you would not add any checks, right? Then
>>> don't add for regmap mmio.
>>>
>>
>> I agree, the mmio regmap interface should only fail because of bugs or
>> things are misconfigured. Would be nice to capture that in a WARN_ON()
>> or something...
>>
> 
> One choice could be to have for entire functions doing reads/writes:
> 
> 	ret = 0;
> 	ret != regmap_write();
> 	ret != regmap_write();
> 	ret != regmap_write();
> 	return ret;
> 
> and handle this in the caller somehow. I don't think that aborting such
> chain early, just because regmap mmio failures, makes sense.
Meh, perhaps let's just forget about this patch.

Konrad
> 
> Best regards,
> Krzysztof
>