| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20230615-topic-bwmonretval-v1-1-223bd048ebf7@linaro.org>
Date: Thu, 15 Jun 2023 23:12:48 +0200
From: Konrad Dybcio <konrad.dybcio@...aro.org>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
Andy Gross <agross@...nel.org>,
Bjorn Andersson <andersson@...nel.org>
Cc: Marijn Suijten <marijn.suijten@...ainline.org>,
linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
Konrad Dybcio <konrad.dybcio@...aro.org>
Subject: [PATCH] soc: qcom: icc-bwmon: Don't ignore return values of regmap
functions
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@...aro.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@...aro.org>
Powered by blists - more mailing lists