[RESEND PATCH v2 00/13] backlight: qcom-wled: fix and solidify handling of enabled-strings

This patchset fixes WLED's handling of enabled-strings: besides some
cleanup it is now actually possible to specify a non-contiguous array of
enabled strings (not necessarily starting at zero) and the values from
DT are now validated to prevent possible unexpected out-of-bounds
register and array element accesses.
Off-by-one mistakes in the maximum number of strings, also causing
out-of-bounds access, have been addressed as well.

Changes in v2:
- Reordered patch 4/10 (Validate enabled string indices in DT) to sit
  before patch 1/10 (Pass number of elements to read to read_u32_array);
- Pulled qcom,num-strings out of the DT enumeration parser, and moved it
  after qcom,enabled-strings parser to always have final sign-off over
  the number of strings;
- Extra validation for this number of strings against
  qcom,enabled-strings;
- Recombined patch 9 (Consistently use enabled-strings in
  set_brightness) and patch 10 (Consider enabled_strings in
  autodetection), which both solve the same problem in two different
  functions.  In addition the autodetection code uses set_brightness as
  helper already;
- Improved DT configurations for pmi8994 and pm660l, currently in 5.15
  rc's.

v1: https://lore.kernel.org/dri-devel/20211004192741.621870-1-marijn.suijten@somainline.org

Marijn Suijten (13):
  backlight: qcom-wled: Validate enabled string indices in DT
  backlight: qcom-wled: Pass number of elements to read to
    read_u32_array
  backlight: qcom-wled: Use cpu_to_le16 macro to perform conversion
  backlight: qcom-wled: Fix off-by-one maximum with default num_strings
  backlight: qcom-wled: Override default length with
    qcom,enabled-strings
  backlight: qcom-wled: Remove unnecessary 4th default string in WLED3
  backlight: qcom-wled: Provide enabled_strings default for WLED 4 and 5
  backlight: qcom-wled: Remove unnecessary double whitespace
  backlight: qcom-wled: Respect enabled-strings in set_brightness
  arm64: dts: qcom: pmi8994: Fix "eternal"->"external" typo in WLED node
  arm64: dts: qcom: pmi8994: Remove hardcoded linear WLED
    enabled-strings
  arm64: dts: qcom: Move WLED num-strings from pmi8994 to
    sony-xperia-tone
  arm64: dt: qcom: pm660l: Remove board-specific WLED configuration

 .../dts/qcom/msm8996-sony-xperia-tone.dtsi    |   1 +
 arch/arm64/boot/dts/qcom/pm660l.dtsi          |   7 -
 arch/arm64/boot/dts/qcom/pmi8994.dtsi         |   5 +-
 drivers/video/backlight/qcom-wled.c           | 131 ++++++++++--------
 4 files changed, 73 insertions(+), 71 deletions(-)

--
2.33.0

The strings passed in DT may possibly cause out-of-bounds register
accesses and should be validated before use.

Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3")
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-by: AngeloGioacchino Del Regno <redacted>
---
 drivers/video/backlight/qcom-wled.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
index d094299c2a48..8a42ed89c59c 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c

@@ -1528,12 +1528,28 @@ static int wled_configure(struct wled *wled)
 	string_len = of_property_count_elems_of_size(dev->of_node,
 						     "qcom,enabled-strings",
 						     sizeof(u32));
-	if (string_len > 0)
+	if (string_len > 0) {
+		if (string_len > wled->max_string_count) {
+			dev_err(dev, "Cannot have more than %d strings\n",
+				wled->max_string_count);
+			return -EINVAL;
+		}
+
 		of_property_read_u32_array(dev->of_node,
 						"qcom,enabled-strings",
 						wled->cfg.enabled_strings,
 						sizeof(u32));

+		for (i = 0; i < string_len; ++i) {
+			if (wled->cfg.enabled_strings[i] >= wled->max_string_count) {
+				dev_err(dev,
+					"qcom,enabled-strings index %d at %d is out of bounds\n",
+					wled->cfg.enabled_strings[i], i);
+				return -EINVAL;
+			}
+		}
+	}
+
 	return 0;
 }

--

2.33.0

The kernel already provides appropriate primitives to perform endianness
conversion which should be used in favour of manual bit-wrangling.

Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-by: AngeloGioacchino Del Regno <redacted>
Reviewed-by: Daniel Thompson <redacted>
---
 drivers/video/backlight/qcom-wled.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
index d413b913fef3..977cd75827d7 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c

@@ -231,14 +231,14 @@ struct wled {
 static int wled3_set_brightness(struct wled *wled, u16 brightness)
 {
 	int rc, i;
-	u8 v[2];
+	u16 v;

-	v[0] = brightness & 0xff;
-	v[1] = (brightness >> 8) & 0xf;
+	v = cpu_to_le16(brightness & WLED3_SINK_REG_BRIGHT_MAX);

 	for (i = 0;  i < wled->cfg.num_strings; ++i) {
 		rc = regmap_bulk_write(wled->regmap, wled->ctrl_addr +
-				       WLED3_SINK_REG_BRIGHT(i), v, 2);
+				       WLED3_SINK_REG_BRIGHT(i),
+				       &v, sizeof(v));
 		if (rc < 0)
 			return rc;
 	}

@@ -249,19 +249,18 @@ static int wled3_set_brightness(struct wled *wled, u16 brightness)
 static int wled4_set_brightness(struct wled *wled, u16 brightness)
 {
 	int rc, i;
-	u16 low_limit = wled->max_brightness * 4 / 1000;
-	u8 v[2];
+	u16 v, low_limit = wled->max_brightness * 4 / 1000;

 	/* WLED4's lower limit of operation is 0.4% */
 	if (brightness > 0 && brightness < low_limit)
 		brightness = low_limit;

-	v[0] = brightness & 0xff;
-	v[1] = (brightness >> 8) & 0xf;
+	v = cpu_to_le16(brightness & WLED3_SINK_REG_BRIGHT_MAX);

 	for (i = 0;  i < wled->cfg.num_strings; ++i) {
 		rc = regmap_bulk_write(wled->regmap, wled->sink_addr +
-				       WLED4_SINK_REG_BRIGHT(i), v, 2);
+				       WLED4_SINK_REG_BRIGHT(i),
+				       &v, sizeof(v));
 		if (rc < 0)
 			return rc;
 	}

@@ -272,22 +271,20 @@ static int wled4_set_brightness(struct wled *wled, u16 brightness)
 static int wled5_set_brightness(struct wled *wled, u16 brightness)
 {
 	int rc, offset;
-	u16 low_limit = wled->max_brightness * 1 / 1000;
-	u8 v[2];
+	u16 v, low_limit = wled->max_brightness * 1 / 1000;

 	/* WLED5's lower limit is 0.1% */
 	if (brightness < low_limit)
 		brightness = low_limit;

-	v[0] = brightness & 0xff;
-	v[1] = (brightness >> 8) & 0x7f;
+	v = cpu_to_le16(brightness & WLED5_SINK_REG_BRIGHT_MAX_15B);

 	offset = (wled->cfg.mod_sel == MOD_A) ?
 		  WLED5_SINK_REG_MOD_A_BRIGHTNESS_LSB :
 		  WLED5_SINK_REG_MOD_B_BRIGHTNESS_LSB;

 	rc = regmap_bulk_write(wled->regmap, wled->sink_addr + offset,
-			       v, 2);
+			       &v, sizeof(v));
 	return rc;
 }

--

2.33.0

When not specifying num-strings in the DT the default is used, but +1 is
added to it which turns WLED3 into 4 and WLED4/5 into 5 strings instead
of 3 and 4 respectively, causing out-of-bounds reads and register
read/writes.  This +1 exists for a deficiency in the DT parsing code,
and is simply omitted entirely - solving this oob issue - by parsing the
property separately much like qcom,enabled-strings.

This also allows more stringent checks on the maximum value when
qcom,enabled-strings is provided in the DT.  Note that num-strings is
parsed after enabled-strings to give it final sign-off over the length,
which DT currently utilizes to get around an incorrect fixed read of
four elements from that array (has been addressed in a prior patch).

Fixes: 93c64f1ea1e8 ("leds: add Qualcomm PM8941 WLED driver")
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-By: AngeloGioacchino Del Regno <redacted>
---
 drivers/video/backlight/qcom-wled.c | 51 +++++++++++------------------
 1 file changed, 19 insertions(+), 32 deletions(-)

diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
index 977cd75827d7..c5232478a343 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c

@@ -1253,21 +1253,6 @@ static const struct wled_var_cfg wled5_ovp_cfg = {
 	.size = 16,
 };

-static u32 wled3_num_strings_values_fn(u32 idx)
-{
-	return idx + 1;
-}
-
-static const struct wled_var_cfg wled3_num_strings_cfg = {
-	.fn = wled3_num_strings_values_fn,
-	.size = 3,
-};
-
-static const struct wled_var_cfg wled4_num_strings_cfg = {
-	.fn = wled3_num_strings_values_fn,
-	.size = 4,
-};
-
 static u32 wled3_switch_freq_values_fn(u32 idx)
 {
 	return 19200 / (2 * (1 + idx));

@@ -1341,11 +1326,6 @@ static int wled_configure(struct wled *wled)
 			.val_ptr = &cfg->switch_freq,
 			.cfg = &wled3_switch_freq_cfg,
 		},
-		{
-			.name = "qcom,num-strings",
-			.val_ptr = &cfg->num_strings,
-			.cfg = &wled3_num_strings_cfg,
-		},
 	};

 	const struct wled_u32_opts wled4_opts[] = {

@@ -1369,11 +1349,6 @@ static int wled_configure(struct wled *wled)
 			.val_ptr = &cfg->switch_freq,
 			.cfg = &wled3_switch_freq_cfg,
 		},
-		{
-			.name = "qcom,num-strings",
-			.val_ptr = &cfg->num_strings,
-			.cfg = &wled4_num_strings_cfg,
-		},
 	};

 	const struct wled_u32_opts wled5_opts[] = {

@@ -1397,11 +1372,6 @@ static int wled_configure(struct wled *wled)
 			.val_ptr = &cfg->switch_freq,
 			.cfg = &wled3_switch_freq_cfg,
 		},
-		{
-			.name = "qcom,num-strings",
-			.val_ptr = &cfg->num_strings,
-			.cfg = &wled4_num_strings_cfg,
-		},
 		{
 			.name = "qcom,modulator-sel",
 			.val_ptr = &cfg->mod_sel,

@@ -1520,8 +1490,6 @@ static int wled_configure(struct wled *wled)
 			*bool_opts[i].val_ptr = true;
 	}

-	cfg->num_strings = cfg->num_strings + 1;
-
 	string_len = of_property_count_elems_of_size(dev->of_node,
 						     "qcom,enabled-strings",
 						     sizeof(u32));

@@ -1552,6 +1520,25 @@ static int wled_configure(struct wled *wled)
 		}
 	}

+	rc = of_property_read_u32(dev->of_node, "qcom,num-strings", &val);
+	if (!rc) {
+		if (val < 1 || val > wled->max_string_count) {
+			dev_err(dev, "qcom,num-strings must be between 1 and %d\n",
+				wled->max_string_count);
+			return -EINVAL;
+		}
+
+		if (string_len > 0) {
+			dev_warn(dev, "qcom,num-strings and qcom,enabled-strings are ambiguous\n");
+			if (val > string_len) {
+				dev_err(dev, "qcom,num-strings exceeds qcom,enabled-strings\n");
+				return -EINVAL;
+			}
+		}
+
+		cfg->num_strings = val;
+	}
+
 	return 0;
 }

--

2.33.0

of_property_read_u32_array takes the number of elements to read as last
argument. This does not always need to be 4 (sizeof(u32)) but should
instead be the size of the array in DT as read just above with
of_property_count_elems_of_size.

To not make such an error go unnoticed again the driver now bails
accordingly when of_property_read_u32_array returns an error.
Surprisingly the indentation of newlined arguments is lining up again
after prepending `rc = `.

Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3")
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-by: AngeloGioacchino Del Regno <redacted>
Reviewed-by: Daniel Thompson <redacted>
---
 drivers/video/backlight/qcom-wled.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
index 8a42ed89c59c..d413b913fef3 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c

@@ -1535,10 +1535,15 @@ static int wled_configure(struct wled *wled)
 			return -EINVAL;
 		}

-		of_property_read_u32_array(dev->of_node,
+		rc = of_property_read_u32_array(dev->of_node,
 						"qcom,enabled-strings",
 						wled->cfg.enabled_strings,
-						sizeof(u32));
+						string_len);
+		if (rc) {
+			dev_err(dev, "Failed to read %d elements from qcom,enabled-strings: %d\n",
+				string_len, rc);
+			return rc;
+		}

 		for (i = 0; i < string_len; ++i) {
 			if (wled->cfg.enabled_strings[i] >= wled->max_string_count) {
--

2.33.0

The length of qcom,enabled-strings as property array is enough to
determine the number of strings to be enabled, without needing to set
qcom,num-strings to override the default number of strings when less
than the default (which is also the maxium) is provided in DT.

Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3")
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-by: AngeloGioacchino Del Regno <redacted>
---
 drivers/video/backlight/qcom-wled.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
index c5232478a343..9bfbf601762a 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c

@@ -1518,6 +1518,8 @@ static int wled_configure(struct wled *wled)
 				return -EINVAL;
 			}
 		}
+
+		cfg->num_strings = string_len;
 	}

 	rc = of_property_read_u32(dev->of_node, "qcom,num-strings", &val);
--

2.33.0

The previous commit improves num_strings parsing to not go over the
maximum of 3 strings for WLED3 anymore.  Likewise this default index for
a hypothetical 4th string is invalid and could access registers that are
not mapped to the desired purpose.
Removing this value gets rid of undesired confusion and avoids the
possibility of accessing registers at this offset even if the 4th array
element is used by accident.

Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-by: AngeloGioacchino Del Regno <redacted>
Reviewed-by: Daniel Thompson <redacted>
---
 drivers/video/backlight/qcom-wled.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
index 9bfbf601762a..c342cd8440e1 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c

@@ -946,7 +946,7 @@ static const struct wled_config wled3_config_defaults = {
 	.cs_out_en = false,
 	.ext_gen = false,
 	.cabc = false,
-	.enabled_strings = {0, 1, 2, 3},
+	.enabled_strings = {0, 1, 2},
 };

 static int wled4_setup(struct wled *wled)
--

2.33.0

Remove redundant spaces inside for loop conditions.  No other double
spaces were found that are not part of indentation with `[^\s]  `.

Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-by: AngeloGioacchino Del Regno <redacted>
Reviewed-by: Daniel Thompson <redacted>
---
 drivers/video/backlight/qcom-wled.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
index a8fb8f19922d..4524e80591cd 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c

@@ -235,7 +235,7 @@ static int wled3_set_brightness(struct wled *wled, u16 brightness)

 	v = cpu_to_le16(brightness & WLED3_SINK_REG_BRIGHT_MAX);

-	for (i = 0;  i < wled->cfg.num_strings; ++i) {
+	for (i = 0; i < wled->cfg.num_strings; ++i) {
 		rc = regmap_bulk_write(wled->regmap, wled->ctrl_addr +
 				       WLED3_SINK_REG_BRIGHT(i),
 				       &v, sizeof(v));

@@ -257,7 +257,7 @@ static int wled4_set_brightness(struct wled *wled, u16 brightness)

 	v = cpu_to_le16(brightness & WLED3_SINK_REG_BRIGHT_MAX);

-	for (i = 0;  i < wled->cfg.num_strings; ++i) {
+	for (i = 0; i < wled->cfg.num_strings; ++i) {
 		rc = regmap_bulk_write(wled->regmap, wled->sink_addr +
 				       WLED4_SINK_REG_BRIGHT(i),
 				       &v, sizeof(v));
--

2.33.0

Only WLED 3 sets a sensible default that allows operating this driver
with just qcom,num-strings in the DT; WLED 4 and 5 require
qcom,enabled-strings to be provided otherwise enabled_strings remains
zero-initialized, resuling in every string-specific register write
(currently only the setup and config functions, brightness follows in a
future patch) to only configure the zero'th string multiple times.

Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-by: AngeloGioacchino Del Regno <redacted>
Reviewed-by: Daniel Thompson <redacted>
---
 drivers/video/backlight/qcom-wled.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
index c342cd8440e1..a8fb8f19922d 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c

@@ -1077,6 +1077,7 @@ static const struct wled_config wled4_config_defaults = {
 	.cabc = false,
 	.external_pfet = false,
 	.auto_detection_enabled = false,
+	.enabled_strings = {0, 1, 2, 3},
 };

 static int wled5_setup(struct wled *wled)

@@ -1190,6 +1191,7 @@ static const struct wled_config wled5_config_defaults = {
 	.cabc = false,
 	.external_pfet = false,
 	.auto_detection_enabled = false,
+	.enabled_strings = {0, 1, 2, 3},
 };

 static const u32 wled3_boost_i_limit_values[] = {
--

2.33.0

The hardware is capable of controlling any non-contiguous sequence of
LEDs specified in the DT using qcom,enabled-strings as u32
array, and this also follows from the DT-bindings documentation.  The
numbers specified in this array represent indices of the LED strings
that are to be enabled and disabled.

Its value is appropriately used to setup and enable string modules, but
completely disregarded in the set_brightness paths which only iterate
over the number of strings linearly.
Take an example where only string 2 is enabled with
qcom,enabled_strings=<2>: this string is appropriately enabled but
subsequent brightness changes would have only touched the zero'th
brightness register because num_strings is 1 here.  This is simply
addressed by looking up the string for this index in the enabled_strings
array just like the other codepaths that iterate over num_strings.

Likewise enabled_strings is now also used in the autodetection path for
consistent behaviour: when a list of strings is specified in DT only
those strings will be probed for autodetection, analogous to how the
number of strings that need to be probed is already bound by
qcom,num-strings.  After all autodetection uses the set_brightness
helpers to set an initial value, which could otherwise end up changing
brightness on a different set of strings.

Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3")
Fixes: 03b2b5e86986 ("backlight: qcom-wled: Add support for WLED4 peripheral")
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-by: AngeloGioacchino Del Regno <redacted>
---
 drivers/video/backlight/qcom-wled.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
index 4524e80591cd..bdda6b424113 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c

@@ -237,7 +237,7 @@ static int wled3_set_brightness(struct wled *wled, u16 brightness)

 	for (i = 0; i < wled->cfg.num_strings; ++i) {
 		rc = regmap_bulk_write(wled->regmap, wled->ctrl_addr +
-				       WLED3_SINK_REG_BRIGHT(i),
+				       WLED3_SINK_REG_BRIGHT(wled->cfg.enabled_strings[i]),
 				       &v, sizeof(v));
 		if (rc < 0)
 			return rc;

@@ -259,7 +259,7 @@ static int wled4_set_brightness(struct wled *wled, u16 brightness)

 	for (i = 0; i < wled->cfg.num_strings; ++i) {
 		rc = regmap_bulk_write(wled->regmap, wled->sink_addr +
-				       WLED4_SINK_REG_BRIGHT(i),
+				       WLED4_SINK_REG_BRIGHT(wled->cfg.enabled_strings[i]),
 				       &v, sizeof(v));
 		if (rc < 0)
 			return rc;

@@ -569,7 +569,7 @@ static irqreturn_t wled_short_irq_handler(int irq, void *_wled)

 static void wled_auto_string_detection(struct wled *wled)
 {
-	int rc = 0, i, delay_time_us;
+	int rc = 0, i, j, delay_time_us;
 	u32 sink_config = 0;
 	u8 sink_test = 0, sink_valid = 0, val;
 	bool fault_set;

@@ -616,14 +616,15 @@ static void wled_auto_string_detection(struct wled *wled)

 	/* Iterate through the strings one by one */
 	for (i = 0; i < wled->cfg.num_strings; i++) {
-		sink_test = BIT((WLED4_SINK_REG_CURR_SINK_SHFT + i));
+		j = wled->cfg.enabled_strings[i];
+		sink_test = BIT((WLED4_SINK_REG_CURR_SINK_SHFT + j));

 		/* Enable feedback control */
 		rc = regmap_write(wled->regmap, wled->ctrl_addr +
-				  WLED3_CTRL_REG_FEEDBACK_CONTROL, i + 1);
+				  WLED3_CTRL_REG_FEEDBACK_CONTROL, j + 1);
 		if (rc < 0) {
 			dev_err(wled->dev, "Failed to enable feedback for SINK %d rc = %d\n",
-				i + 1, rc);
+				j + 1, rc);
 			goto failed_detect;
 		}

@@ -632,7 +633,7 @@ static void wled_auto_string_detection(struct wled *wled)
 				  WLED4_SINK_REG_CURR_SINK, sink_test);
 		if (rc < 0) {
 			dev_err(wled->dev, "Failed to configure SINK %d rc=%d\n",
-				i + 1, rc);
+				j + 1, rc);
 			goto failed_detect;
 		}

@@ -659,7 +660,7 @@ static void wled_auto_string_detection(struct wled *wled)

 		if (fault_set)
 			dev_dbg(wled->dev, "WLED OVP fault detected with SINK %d\n",
-				i + 1);
+				j + 1);
 		else
 			sink_valid |= sink_test;

@@ -699,15 +700,16 @@ static void wled_auto_string_detection(struct wled *wled)
 	/* Enable valid sinks */
 	if (wled->version == 4) {
 		for (i = 0; i < wled->cfg.num_strings; i++) {
+			j = wled->cfg.enabled_strings[i];
 			if (sink_config &
-			    BIT(WLED4_SINK_REG_CURR_SINK_SHFT + i))
+			    BIT(WLED4_SINK_REG_CURR_SINK_SHFT + j))
 				val = WLED4_SINK_REG_STR_MOD_MASK;
 			else
 				/* Disable modulator_en for unused sink */
 				val = 0;

 			rc = regmap_write(wled->regmap, wled->sink_addr +
-					  WLED4_SINK_REG_STR_MOD_EN(i), val);
+					  WLED4_SINK_REG_STR_MOD_EN(j), val);
 			if (rc < 0) {
 				dev_err(wled->dev, "Failed to configure MODULATOR_EN rc=%d\n",
 					rc);
--

2.33.0

This string- and electrical configuration depend on the board and panel,
and should hence not be defined generically for every user of pm660l.
SoMainline will pick this configuration again when enabling WLED on the
Sony Nile platform.

Fixes: 7b56a804e58b ("arm64: dts: qcom: pm660l: Add WLED support")
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-By: AngeloGioacchino Del Regno <redacted>
---
 arch/arm64/boot/dts/qcom/pm660l.dtsi | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/pm660l.dtsi b/arch/arm64/boot/dts/qcom/pm660l.dtsi
index 05086cbe573b..cfef42353611 100644
--- a/arch/arm64/boot/dts/qcom/pm660l.dtsi
+++ b/arch/arm64/boot/dts/qcom/pm660l.dtsi

@@ -72,13 +72,6 @@ pm660l_wled: leds@d800 {
 			interrupt-names = "ovp";
 			label = "backlight";

-			qcom,switching-freq = <800>;
-			qcom,ovp-millivolt = <29600>;
-			qcom,current-boost-limit = <970>;
-			qcom,current-limit-microamp = <20000>;
-			qcom,num-strings = <2>;
-			qcom,enabled-strings = <0 1>;
-
 			status = "disabled";
 		};

--

2.33.0

The property is named "qcom,external-pfet", as found by
dt_binding_check:

    'qcom,eternal-pfet' does not match any of the regexes

Fixes: 37aa540cbd30 ("arm64: dts: qcom: pmi8994: Add WLED node")
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-By: AngeloGioacchino Del Regno <redacted>
---
 arch/arm64/boot/dts/qcom/pmi8994.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/pmi8994.dtsi b/arch/arm64/boot/dts/qcom/pmi8994.dtsi
index b4ac900ab115..a06ea9adae81 100644
--- a/arch/arm64/boot/dts/qcom/pmi8994.dtsi
+++ b/arch/arm64/boot/dts/qcom/pmi8994.dtsi

@@ -42,7 +42,7 @@ pmi8994_wled: wled@d800 {
 			/* Yes, all four strings *have to* be defined or things won't work. */
 			qcom,enabled-strings = <0 1 2 3>;
 			qcom,cabc;
-			qcom,eternal-pfet;
+			qcom,external-pfet;
 			status = "disabled";
 		};
 	};
--

2.33.0

The number of WLED strings used by a certain platform depend on the
panel connected to that board and may not be the same for every user of
pmi8994.

Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-By: AngeloGioacchino Del Regno <redacted>
---
 arch/arm64/boot/dts/qcom/msm8996-sony-xperia-tone.dtsi | 1 +
 arch/arm64/boot/dts/qcom/pmi8994.dtsi                  | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/msm8996-sony-xperia-tone.dtsi b/arch/arm64/boot/dts/qcom/msm8996-sony-xperia-tone.dtsi
index 507396c4d23b..ff7f39d29dd5 100644
--- a/arch/arm64/boot/dts/qcom/msm8996-sony-xperia-tone.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996-sony-xperia-tone.dtsi

@@ -620,6 +620,7 @@ pmi8994_s11: s11 {
 &pmi8994_wled {
 	status = "okay";
 	default-brightness = <512>;
+	qcom,num-strings = <3>;
 };

 &rpm_requests {

diff --git a/arch/arm64/boot/dts/qcom/pmi8994.dtsi b/arch/arm64/boot/dts/qcom/pmi8994.dtsi
index 89ba4146e747..6e7c252568e6 100644
--- a/arch/arm64/boot/dts/qcom/pmi8994.dtsi
+++ b/arch/arm64/boot/dts/qcom/pmi8994.dtsi

@@ -38,7 +38,6 @@ pmi8994_wled: wled@d800 {
 			reg = <0xd800 0xd900>;
 			interrupts = <3 0xd8 0x02 IRQ_TYPE_EDGE_RISING>;
 			interrupt-names = "short";
-			qcom,num-strings = <3>;
 			qcom,cabc;
 			qcom,external-pfet;
 			status = "disabled";
--

2.33.0

The driver now sets an appropriate default for WLED4 (and WLED5) just
like WLED3 making this linear array from 0-3 redundant.  In addition the
driver is now able to parse arrays of variable length solving the "all
four strings *have to* be defined" comment.

Besides the driver will now warn when both properties are specified to
prevent ambiguity: the length of the array is enough to imply a set
number of strings.

Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-By: AngeloGioacchino Del Regno <redacted>
---
 arch/arm64/boot/dts/qcom/pmi8994.dtsi | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/pmi8994.dtsi b/arch/arm64/boot/dts/qcom/pmi8994.dtsi
index a06ea9adae81..89ba4146e747 100644
--- a/arch/arm64/boot/dts/qcom/pmi8994.dtsi
+++ b/arch/arm64/boot/dts/qcom/pmi8994.dtsi

@@ -39,8 +39,6 @@ pmi8994_wled: wled@d800 {
 			interrupts = <3 0xd8 0x02 IRQ_TYPE_EDGE_RISING>;
 			interrupt-names = "short";
 			qcom,num-strings = <3>;
-			/* Yes, all four strings *have to* be defined or things won't work. */
-			qcom,enabled-strings = <0 1 2 3>;
 			qcom,cabc;
 			qcom,external-pfet;
 			status = "disabled";
--

2.33.0

On Fri, Nov 12, 2021 at 01:26:54AM +0100, Marijn Suijten wrote:

The strings passed in DT may possibly cause out-of-bounds register
accesses and should be validated before use.

Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3")
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-by: AngeloGioacchino Del Regno <redacted>

Reviewed-by: Daniel Thompson <redacted>


Daniel.

On Fri, Nov 12, 2021 at 01:26:57AM +0100, Marijn Suijten wrote:

This warning occurs even when there is no ambiguity.

This could be:

	if (string_len > 0 && val != string_len)

The warning should also be below the error message on the next if statement.
Combined these changes allows us to give a much more helpful and assertive
warning message:

qcom,num-strings mis-matches and will partially override
qcom,enabled-strings (remove qcom,num-strings?)

Daniel.

On Fri, Nov 12, 2021 at 01:26:58AM +0100, Marijn Suijten wrote:

I still don't really understand why this wants to be a separate patch.

The warning text emitted by the previous patch (whatever text we agree
on) will be nonsense until this patch is applied.

If this patch cannot appear before the warning is introduces then there
is no correct order for patches 4 and 5 (which implies they should be the
same patch).


Daniel.

On Fri, Nov 12, 2021 at 01:27:02AM +0100, Marijn Suijten wrote:

The hardware is capable of controlling any non-contiguous sequence of
LEDs specified in the DT using qcom,enabled-strings as u32
array, and this also follows from the DT-bindings documentation.  The
numbers specified in this array represent indices of the LED strings
that are to be enabled and disabled.

Its value is appropriately used to setup and enable string modules, but
completely disregarded in the set_brightness paths which only iterate
over the number of strings linearly.
Take an example where only string 2 is enabled with
qcom,enabled_strings=<2>: this string is appropriately enabled but
subsequent brightness changes would have only touched the zero'th
brightness register because num_strings is 1 here.  This is simply
addressed by looking up the string for this index in the enabled_strings
array just like the other codepaths that iterate over num_strings.

Likewise enabled_strings is now also used in the autodetection path for
consistent behaviour: when a list of strings is specified in DT only
those strings will be probed for autodetection, analogous to how the
number of strings that need to be probed is already bound by
qcom,num-strings.  After all autodetection uses the set_brightness
helpers to set an initial value, which could otherwise end up changing
brightness on a different set of strings.

Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3")
Fixes: 03b2b5e86986 ("backlight: qcom-wled: Add support for WLED4 peripheral")
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-by: AngeloGioacchino Del Regno <redacted>

Reviewed-by: Daniel Thompson <redacted>


Daniel.

On 2021-11-12 12:08:39, Daniel Thompson wrote:

On Fri, Nov 12, 2021 at 01:26:57AM +0100, Marijn Suijten wrote:

quoted

When not specifying num-strings in the DT the default is used, but +1 is
added to it which turns WLED3 into 4 and WLED4/5 into 5 strings instead
of 3 and 4 respectively, causing out-of-bounds reads and register
read/writes.  This +1 exists for a deficiency in the DT parsing code,
and is simply omitted entirely - solving this oob issue - by parsing the
property separately much like qcom,enabled-strings.

This also allows more stringent checks on the maximum value when
qcom,enabled-strings is provided in the DT.  Note that num-strings is
parsed after enabled-strings to give it final sign-off over the length,
which DT currently utilizes to get around an incorrect fixed read of
four elements from that array (has been addressed in a prior patch).

Fixes: 93c64f1ea1e8 ("leds: add Qualcomm PM8941 WLED driver")
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-By: AngeloGioacchino Del Regno <redacted>
---
 drivers/video/backlight/qcom-wled.c | 51 +++++++++++------------------
 1 file changed, 19 insertions(+), 32 deletions(-)

diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
index 977cd75827d7..c5232478a343 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c

@@ -1552,6 +1520,25 @@ static int wled_configure(struct wled *wled)
 		}
 	}

+	rc = of_property_read_u32(dev->of_node, "qcom,num-strings", &val);
+	if (!rc) {
+		if (val < 1 || val > wled->max_string_count) {
+			dev_err(dev, "qcom,num-strings must be between 1 and %d\n",
+				wled->max_string_count);
+			return -EINVAL;
+		}
+
+		if (string_len > 0) {
+			dev_warn(dev, "qcom,num-strings and qcom,enabled-strings are ambiguous\n");

The warning should also be below the error message on the next if statement.

Agreed.

This warning occurs even when there is no ambiguity.

This could be:

	if (string_len > 0 && val != string_len)

Combined these changes allows us to give a much more helpful and assertive
warning message:

qcom,num-strings mis-matches and will partially override
qcom,enabled-strings (remove qcom,num-strings?)

I want to let the user know it's set regardless of whether they're
equivalent; no need to set both.

How about:

    Only one of qcom,num-strings or qcom,enabled-strings should be set

That should be more descriptive?  Otherwise, let me know if you really
want to allow users to (unnecessarily) set both - or if it can / should
be caught in DT validation instead.

- Marijn

quoted

+			if (val > string_len) {
+				dev_err(dev, "qcom,num-strings exceeds qcom,enabled-strings\n");
+				return -EINVAL;
+			}
+		}

Daniel.

On 2021-11-12 12:12:38, Daniel Thompson wrote:

On Fri, Nov 12, 2021 at 01:26:58AM +0100, Marijn Suijten wrote:

quoted

The length of qcom,enabled-strings as property array is enough to
determine the number of strings to be enabled, without needing to set
qcom,num-strings to override the default number of strings when less
than the default (which is also the maxium) is provided in DT.

Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3")
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-by: AngeloGioacchino Del Regno <redacted>
---
 drivers/video/backlight/qcom-wled.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
index c5232478a343..9bfbf601762a 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c

@@ -1518,6 +1518,8 @@ static int wled_configure(struct wled *wled)
 				return -EINVAL;
 			}
 		}
+
+		cfg->num_strings = string_len;

I still don't really understand why this wants to be a separate patch.

I'm viewing this as a separate issue, and this makes it easier to
document the change in a loose commit.

The warning text emitted by the previous patch (whatever text we agree
on) will be nonsense until this patch is applied.

If this patch cannot appear before the warning is introduces then there
is no correct order for patches 4 and 5 (which implies they should be the
same patch).

Agreed, this is a weird way of doing things in v2 - the error message is
printed yet the length of qcom,enabled-strings is always ignored before
this patch.

If we were to reorder patch 5 before patch 4 that should also
temporarily move `cfg->num_strings = cfg->num_strings + 1;` right below
this `if` so that `qcom,num-strings` remains the definitive way to
set/override length.  That's doable, and makes it easier to read patch 4
as that bit of code will be replaced by of_property_read_u32 on that
exact line.  Let me know which method you prefer.

- Marijn

On Fri, Nov 12, 2021 at 01:35:01PM +0100, Marijn Suijten wrote:

On 2021-11-12 12:08:39, Daniel Thompson wrote:

quoted

On Fri, Nov 12, 2021 at 01:26:57AM +0100, Marijn Suijten wrote:

quoted

+		if (string_len > 0) {
+			dev_warn(dev, "qcom,num-strings and qcom,enabled-strings are ambiguous\n");

The warning should also be below the error message on the next if statement.

Agreed.

quoted

This warning occurs even when there is no ambiguity.

This could be:

	if (string_len > 0 && val != string_len)

Combined these changes allows us to give a much more helpful and assertive
warning message:

qcom,num-strings mis-matches and will partially override
qcom,enabled-strings (remove qcom,num-strings?)

I want to let the user know it's set regardless of whether they're
equivalent; no need to set both.

How about:

    Only one of qcom,num-strings or qcom,enabled-strings should be set

That should be more descriptive?  Otherwise, let me know if you really
want to allow users to (unnecessarily) set both - or if it can / should
be caught in DT validation instead.

Yes. I can live with that text. Let's use that.

Maybe I wouldn't if there gazilions of existing DTs with both
properties but IIRC the number is likely to be small or zero
(although we couldn't be 100% sure which).


Daniel.

On Fri, Nov 12, 2021 at 01:45:22PM +0100, Marijn Suijten wrote:

On 2021-11-12 12:12:38, Daniel Thompson wrote:

quoted

On Fri, Nov 12, 2021 at 01:26:58AM +0100, Marijn Suijten wrote:

quoted

The length of qcom,enabled-strings as property array is enough to
determine the number of strings to be enabled, without needing to set
qcom,num-strings to override the default number of strings when less
than the default (which is also the maxium) is provided in DT.

Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3")
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-by: AngeloGioacchino Del Regno <redacted>
---
 drivers/video/backlight/qcom-wled.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
index c5232478a343..9bfbf601762a 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c

@@ -1518,6 +1518,8 @@ static int wled_configure(struct wled *wled)
 				return -EINVAL;
 			}
 		}
+
+		cfg->num_strings = string_len;

I still don't really understand why this wants to be a separate patch.

I'm viewing this as a separate issue, and this makes it easier to
document the change in a loose commit.

quoted

The warning text emitted by the previous patch (whatever text we agree
on) will be nonsense until this patch is applied.

If this patch cannot appear before the warning is introduces then there
is no correct order for patches 4 and 5 (which implies they should be the
same patch).

Agreed, this is a weird way of doing things in v2 - the error message is
printed yet the length of qcom,enabled-strings is always ignored before
this patch.

If we were to reorder patch 5 before patch 4 that should also
temporarily move `cfg->num_strings = cfg->num_strings + 1;` right below
this `if` so that `qcom,num-strings` remains the definitive way to
set/override length.  That's doable, and makes it easier to read patch 4
as that bit of code will be replaced by of_property_read_u32 on that
exact line.  Let me know which method you prefer.

Personally I would just squash them together. There are no redundant
values in the DT that could be fixed until we can use the string_len
to set num_strings.

However I won't object to the other approach providing the result is
bisectable.


Daniel.

On 2021-11-12 13:23:36, Daniel Thompson wrote:

On Fri, Nov 12, 2021 at 01:45:22PM +0100, Marijn Suijten wrote:

quoted

On 2021-11-12 12:12:38, Daniel Thompson wrote:

quoted

On Fri, Nov 12, 2021 at 01:26:58AM +0100, Marijn Suijten wrote:

quoted

The length of qcom,enabled-strings as property array is enough to
determine the number of strings to be enabled, without needing to set
qcom,num-strings to override the default number of strings when less
than the default (which is also the maxium) is provided in DT.

Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3")
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-by: AngeloGioacchino Del Regno <redacted>
---
 drivers/video/backlight/qcom-wled.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
index c5232478a343..9bfbf601762a 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c

@@ -1518,6 +1518,8 @@ static int wled_configure(struct wled *wled)
 				return -EINVAL;
 			}
 		}
+
+		cfg->num_strings = string_len;

I still don't really understand why this wants to be a separate patch.

I'm viewing this as a separate issue, and this makes it easier to
document the change in a loose commit.

quoted

The warning text emitted by the previous patch (whatever text we agree
on) will be nonsense until this patch is applied.

If this patch cannot appear before the warning is introduces then there
is no correct order for patches 4 and 5 (which implies they should be the
same patch).

Agreed, this is a weird way of doing things in v2 - the error message is
printed yet the length of qcom,enabled-strings is always ignored before
this patch.

If we were to reorder patch 5 before patch 4 that should also
temporarily move `cfg->num_strings = cfg->num_strings + 1;` right below
this `if` so that `qcom,num-strings` remains the definitive way to
set/override length.  That's doable, and makes it easier to read patch 4
as that bit of code will be replaced by of_property_read_u32 on that
exact line.  Let me know which method you prefer.

Personally I would just squash them together. There are no redundant
values in the DT that could be fixed until we can use the string_len
to set num_strings.

Reordering this patch before patch 4 in the way described above should
allow just that, except that no warnings will be given for ambiguity
until patch 4 is applied after that - which is weird given that that
patch only intends the off-by-one error.  Perhaps we should keep the
order as it is, but add the ambiguity warning in this patch instead.

That means we have one patch to fix the off-by-one first, and another
that allows qcom,num-strings to provide a default for num_strings.  I
guess that's better to keep separated?

- Marijn

On Fri, Nov 12, 2021 at 03:19:17PM +0100, Marijn Suijten wrote:

On 2021-11-12 13:23:36, Daniel Thompson wrote:

quoted

On Fri, Nov 12, 2021 at 01:45:22PM +0100, Marijn Suijten wrote:

quoted

On 2021-11-12 12:12:38, Daniel Thompson wrote:

quoted

On Fri, Nov 12, 2021 at 01:26:58AM +0100, Marijn Suijten wrote:

quoted

The length of qcom,enabled-strings as property array is enough to
determine the number of strings to be enabled, without needing to set
qcom,num-strings to override the default number of strings when less
than the default (which is also the maxium) is provided in DT.

Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3")
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-by: AngeloGioacchino Del Regno <redacted>
---
 drivers/video/backlight/qcom-wled.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
index c5232478a343..9bfbf601762a 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c

@@ -1518,6 +1518,8 @@ static int wled_configure(struct wled *wled)
 				return -EINVAL;
 			}
 		}
+
+		cfg->num_strings = string_len;

I still don't really understand why this wants to be a separate patch.

I'm viewing this as a separate issue, and this makes it easier to
document the change in a loose commit.

quoted

The warning text emitted by the previous patch (whatever text we agree
on) will be nonsense until this patch is applied.

If this patch cannot appear before the warning is introduces then there
is no correct order for patches 4 and 5 (which implies they should be the
same patch).

Agreed, this is a weird way of doing things in v2 - the error message is
printed yet the length of qcom,enabled-strings is always ignored before
this patch.

If we were to reorder patch 5 before patch 4 that should also
temporarily move `cfg->num_strings = cfg->num_strings + 1;` right below
this `if` so that `qcom,num-strings` remains the definitive way to
set/override length.  That's doable, and makes it easier to read patch 4
as that bit of code will be replaced by of_property_read_u32 on that
exact line.  Let me know which method you prefer.

Personally I would just squash them together. There are no redundant
values in the DT that could be fixed until we can use the string_len
to set num_strings.

Reordering this patch before patch 4 in the way described above should
allow just that, except that no warnings will be given for ambiguity
until patch 4 is applied after that - which is weird given that that
patch only intends the off-by-one error.  Perhaps we should keep the
order as it is, but add the ambiguity warning in this patch instead.

That works for me. Sounds good.


Daniel.

On 2021-11-12 13:35:03, Marijn Suijten wrote:

On 2021-11-12 12:08:39, Daniel Thompson wrote:

quoted

On Fri, Nov 12, 2021 at 01:26:57AM +0100, Marijn Suijten wrote:

quoted

When not specifying num-strings in the DT the default is used, but +1 is
added to it which turns WLED3 into 4 and WLED4/5 into 5 strings instead
of 3 and 4 respectively, causing out-of-bounds reads and register
read/writes.  This +1 exists for a deficiency in the DT parsing code,
and is simply omitted entirely - solving this oob issue - by parsing the
property separately much like qcom,enabled-strings.

This also allows more stringent checks on the maximum value when
qcom,enabled-strings is provided in the DT.  Note that num-strings is
parsed after enabled-strings to give it final sign-off over the length,
which DT currently utilizes to get around an incorrect fixed read of
four elements from that array (has been addressed in a prior patch).

Fixes: 93c64f1ea1e8 ("leds: add Qualcomm PM8941 WLED driver")
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-By: AngeloGioacchino Del Regno <redacted>
---
 drivers/video/backlight/qcom-wled.c | 51 +++++++++++------------------
 1 file changed, 19 insertions(+), 32 deletions(-)

diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
index 977cd75827d7..c5232478a343 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c

@@ -1552,6 +1520,25 @@ static int wled_configure(struct wled *wled)
 		}
 	}

+	rc = of_property_read_u32(dev->of_node, "qcom,num-strings", &val);
+	if (!rc) {
+		if (val < 1 || val > wled->max_string_count) {
+			dev_err(dev, "qcom,num-strings must be between 1 and %d\n",
+				wled->max_string_count);
+			return -EINVAL;
+		}
+
+		if (string_len > 0) {
+			dev_warn(dev, "qcom,num-strings and qcom,enabled-strings are ambiguous\n");

The warning should also be below the error message on the next if statement.

Agreed.

Thinking about this again while reworking the patches, I initially put
this above the error to make DT writers aware.  There's no point telling
them that their values are out of sync (num-strings >
len(enabled-strings)), when they "shouldn't even" (don't need to) set
both in the first place.  They might needlessly fix the discrepancy, see
the driver finally probe (working backlight) and carry on without
noticing this warning that now appears.

Sorry for bringing this back up, but I'm curious about your opinion.

- Marijn

On Fri, Nov 12, 2021 at 10:43:37PM +0100, Marijn Suijten wrote:

On 2021-11-12 13:35:03, Marijn Suijten wrote:

quoted

On 2021-11-12 12:08:39, Daniel Thompson wrote:

quoted

On Fri, Nov 12, 2021 at 01:26:57AM +0100, Marijn Suijten wrote:

quoted

When not specifying num-strings in the DT the default is used, but +1 is
added to it which turns WLED3 into 4 and WLED4/5 into 5 strings instead
of 3 and 4 respectively, causing out-of-bounds reads and register
read/writes.  This +1 exists for a deficiency in the DT parsing code,
and is simply omitted entirely - solving this oob issue - by parsing the
property separately much like qcom,enabled-strings.

This also allows more stringent checks on the maximum value when
qcom,enabled-strings is provided in the DT.  Note that num-strings is
parsed after enabled-strings to give it final sign-off over the length,
which DT currently utilizes to get around an incorrect fixed read of
four elements from that array (has been addressed in a prior patch).

Fixes: 93c64f1ea1e8 ("leds: add Qualcomm PM8941 WLED driver")
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-By: AngeloGioacchino Del Regno <redacted>
---
 drivers/video/backlight/qcom-wled.c | 51 +++++++++++------------------
 1 file changed, 19 insertions(+), 32 deletions(-)

diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
index 977cd75827d7..c5232478a343 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c

@@ -1552,6 +1520,25 @@ static int wled_configure(struct wled *wled)
 		}
 	}

+	rc = of_property_read_u32(dev->of_node, "qcom,num-strings", &val);
+	if (!rc) {
+		if (val < 1 || val > wled->max_string_count) {
+			dev_err(dev, "qcom,num-strings must be between 1 and %d\n",
+				wled->max_string_count);
+			return -EINVAL;
+		}
+
+		if (string_len > 0) {
+			dev_warn(dev, "qcom,num-strings and qcom,enabled-strings are ambiguous\n");

The warning should also be below the error message on the next if statement.

Agreed.

Thinking about this again while reworking the patches, I initially put
this above the error to make DT writers aware.  There's no point telling
them that their values are out of sync (num-strings >
len(enabled-strings)), when they "shouldn't even" (don't need to) set
both in the first place.  They might needlessly fix the discrepancy, see
the driver finally probe (working backlight) and carry on without
noticing this warning that now appears.

Sorry for bringing this back up, but I'm curious about your opinion.

With a more helpful warning about how to fix then I think it is OK to
have both the warning and the error.


Daniel.

On 2021-11-15 11:23:27, Daniel Thompson wrote:

On Fri, Nov 12, 2021 at 10:43:37PM +0100, Marijn Suijten wrote:

quoted

On 2021-11-12 13:35:03, Marijn Suijten wrote:

quoted

On 2021-11-12 12:08:39, Daniel Thompson wrote:

quoted

On Fri, Nov 12, 2021 at 01:26:57AM +0100, Marijn Suijten wrote:

quoted

When not specifying num-strings in the DT the default is used, but +1 is
added to it which turns WLED3 into 4 and WLED4/5 into 5 strings instead
of 3 and 4 respectively, causing out-of-bounds reads and register
read/writes.  This +1 exists for a deficiency in the DT parsing code,
and is simply omitted entirely - solving this oob issue - by parsing the
property separately much like qcom,enabled-strings.

This also allows more stringent checks on the maximum value when
qcom,enabled-strings is provided in the DT.  Note that num-strings is
parsed after enabled-strings to give it final sign-off over the length,
which DT currently utilizes to get around an incorrect fixed read of
four elements from that array (has been addressed in a prior patch).

Fixes: 93c64f1ea1e8 ("leds: add Qualcomm PM8941 WLED driver")
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-By: AngeloGioacchino Del Regno <redacted>
---
 drivers/video/backlight/qcom-wled.c | 51 +++++++++++------------------
 1 file changed, 19 insertions(+), 32 deletions(-)

diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
index 977cd75827d7..c5232478a343 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c

@@ -1552,6 +1520,25 @@ static int wled_configure(struct wled *wled)
 		}
 	}

+	rc = of_property_read_u32(dev->of_node, "qcom,num-strings", &val);
+	if (!rc) {
+		if (val < 1 || val > wled->max_string_count) {
+			dev_err(dev, "qcom,num-strings must be between 1 and %d\n",
+				wled->max_string_count);
+			return -EINVAL;
+		}
+
+		if (string_len > 0) {
+			dev_warn(dev, "qcom,num-strings and qcom,enabled-strings are ambiguous\n");

The warning should also be below the error message on the next if statement.

Agreed.

Thinking about this again while reworking the patches, I initially put
this above the error to make DT writers aware.  There's no point telling
them that their values are out of sync (num-strings >
len(enabled-strings)), when they "shouldn't even" (don't need to) set
both in the first place.  They might needlessly fix the discrepancy, see
the driver finally probe (working backlight) and carry on without
noticing this warning that now appears.

Sorry for bringing this back up, but I'm curious about your opinion.

With a more helpful warning about how to fix then I think it is OK to
have both the warning and the error.

Thanks - I presume the message we settled upon last time is helpful
enough:

    Only one of qcom,num-strings or qcom,enabled-strings should be set

I'll respin this, together with this warning reordered into the next
commit, and using __le16 for the cpu_to_le16 output.

- Marijn