diff mbox series

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

Message ID 20230615-topic-bwmonretval-v1-1-223bd048ebf7@linaro.org
State New
Headers
Series soc: qcom: icc-bwmon: Don't ignore return values of regmap functions |

Commit Message

Konrad Dybcio June 15, 2023, 9:12 p.m. UTC 
  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(-)


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

Best regards,

Comments

Bjorn Andersson June 20, 2023, 6:14 p.m. UTC | #1 
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
Krzysztof Kozlowski June 20, 2023, 6:15 p.m. UTC | #2 
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
Konrad Dybcio June 20, 2023, 6:59 p.m. UTC | #3 
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
>
diff mbox series

Patch

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;
 }