linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 00/10] backlight: qcom-wled: fix and solidify handling of enabled-strings
@ 2021-10-04 19:27 Marijn Suijten
  2021-10-04 19:27 ` [PATCH 01/10] backlight: qcom-wled: Pass number of elements to read to read_u32_array Marijn Suijten
                   ` (9 more replies)
  0 siblings, 10 replies; 34+ messages in thread
From: Marijn Suijten @ 2021-10-04 19:27 UTC (permalink / raw)
  To: phone-devel, Andy Gross, Bjorn Andersson, Lee Jones,
	Daniel Thompson, Jingoo Han
  Cc: ~postmarketos/upstreaming, AngeloGioacchino Del Regno,
	Konrad Dybcio, Martin Botka, Jami Kettunen, Pavel Dubrova,
	Marijn Suijten, Kiran Gunda, Courtney Cavin, Bryan Wu,
	linux-arm-msm, dri-devel, linux-fbdev, linux-kernel

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.

Marijn Suijten (10):
  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: Override num-strings when enabled-strings is set
  backlight: qcom-wled: Validate enabled string indices in DT
  backlight: qcom-wled: Fix off-by-one maximum with default num_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: Consistently use enabled-strings in
    set_brightness
  backlight: qcom-wled: Consider enabled_strings in autodetection

 drivers/video/backlight/qcom-wled.c | 88 ++++++++++++++++++-----------
 1 file changed, 55 insertions(+), 33 deletions(-)

-- 
2.33.0


^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH 01/10] backlight: qcom-wled: Pass number of elements to read to read_u32_array
  2021-10-04 19:27 [PATCH 00/10] backlight: qcom-wled: fix and solidify handling of enabled-strings Marijn Suijten
@ 2021-10-04 19:27 ` Marijn Suijten
  2021-10-05  9:05   ` Daniel Thompson
  2021-10-04 19:27 ` [PATCH 02/10] backlight: qcom-wled: Use cpu_to_le16 macro to perform conversion Marijn Suijten
                   ` (8 subsequent siblings)
  9 siblings, 1 reply; 34+ messages in thread
From: Marijn Suijten @ 2021-10-04 19:27 UTC (permalink / raw)
  To: phone-devel, Andy Gross, Bjorn Andersson, Lee Jones,
	Daniel Thompson, Jingoo Han
  Cc: ~postmarketos/upstreaming, AngeloGioacchino Del Regno,
	Konrad Dybcio, Martin Botka, Jami Kettunen, Pavel Dubrova,
	Marijn Suijten, Kiran Gunda, Courtney Cavin, Bryan Wu,
	linux-arm-msm, dri-devel, linux-fbdev, linux-kernel

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.

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

diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
index d094299c2a48..6af808af2328 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -1528,11 +1528,18 @@ 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)
-		of_property_read_u32_array(dev->of_node,
+	if (string_len > 0) {
+		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 -EINVAL;
+		}
+	}
 
 	return 0;
 }
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH 02/10] backlight: qcom-wled: Use cpu_to_le16 macro to perform conversion
  2021-10-04 19:27 [PATCH 00/10] backlight: qcom-wled: fix and solidify handling of enabled-strings Marijn Suijten
  2021-10-04 19:27 ` [PATCH 01/10] backlight: qcom-wled: Pass number of elements to read to read_u32_array Marijn Suijten
@ 2021-10-04 19:27 ` Marijn Suijten
  2021-10-05  9:06   ` Daniel Thompson
  2021-10-04 19:27 ` [PATCH 03/10] backlight: qcom-wled: Override num-strings when enabled-strings is set Marijn Suijten
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 34+ messages in thread
From: Marijn Suijten @ 2021-10-04 19:27 UTC (permalink / raw)
  To: phone-devel, Andy Gross, Bjorn Andersson, Lee Jones,
	Daniel Thompson, Jingoo Han
  Cc: ~postmarketos/upstreaming, AngeloGioacchino Del Regno,
	Konrad Dybcio, Martin Botka, Jami Kettunen, Pavel Dubrova,
	Marijn Suijten, Kiran Gunda, Courtney Cavin, Bryan Wu,
	linux-arm-msm, dri-devel, linux-fbdev, linux-kernel

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 <angelogioacchino.delregno@somainline.org>
---
 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 6af808af2328..9927ed98944a 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


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH 03/10] backlight: qcom-wled: Override num-strings when enabled-strings is set
  2021-10-04 19:27 [PATCH 00/10] backlight: qcom-wled: fix and solidify handling of enabled-strings Marijn Suijten
  2021-10-04 19:27 ` [PATCH 01/10] backlight: qcom-wled: Pass number of elements to read to read_u32_array Marijn Suijten
  2021-10-04 19:27 ` [PATCH 02/10] backlight: qcom-wled: Use cpu_to_le16 macro to perform conversion Marijn Suijten
@ 2021-10-04 19:27 ` Marijn Suijten
  2021-10-05  9:38   ` Daniel Thompson
  2021-10-04 19:27 ` [PATCH 04/10] backlight: qcom-wled: Validate enabled string indices in DT Marijn Suijten
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 34+ messages in thread
From: Marijn Suijten @ 2021-10-04 19:27 UTC (permalink / raw)
  To: phone-devel, Andy Gross, Bjorn Andersson, Lee Jones,
	Daniel Thompson, Jingoo Han
  Cc: ~postmarketos/upstreaming, AngeloGioacchino Del Regno,
	Konrad Dybcio, Martin Botka, Jami Kettunen, Pavel Dubrova,
	Marijn Suijten, Kiran Gunda, Courtney Cavin, Bryan Wu,
	linux-arm-msm, dri-devel, linux-fbdev, linux-kernel

DT-bindings do not specify num-strings as mandatory property, yet it is
required to be specified even if enabled-strings is used.  The length of
that property-array should already be enough to determine exactly which
and how many strings to enable.

Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3")
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
---
 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 9927ed98944a..29910e603c42 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -1536,6 +1536,8 @@ static int wled_configure(struct wled *wled)
 				string_len, rc);
 			return -EINVAL;
 		}
+
+		cfg->num_strings = string_len;
 	}
 
 	return 0;
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH 04/10] backlight: qcom-wled: Validate enabled string indices in DT
  2021-10-04 19:27 [PATCH 00/10] backlight: qcom-wled: fix and solidify handling of enabled-strings Marijn Suijten
                   ` (2 preceding siblings ...)
  2021-10-04 19:27 ` [PATCH 03/10] backlight: qcom-wled: Override num-strings when enabled-strings is set Marijn Suijten
@ 2021-10-04 19:27 ` Marijn Suijten
  2021-10-05  9:14   ` Daniel Thompson
  2021-10-04 19:27 ` [PATCH 05/10] backlight: qcom-wled: Fix off-by-one maximum with default num_strings Marijn Suijten
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 34+ messages in thread
From: Marijn Suijten @ 2021-10-04 19:27 UTC (permalink / raw)
  To: phone-devel, Andy Gross, Bjorn Andersson, Lee Jones,
	Daniel Thompson, Jingoo Han
  Cc: ~postmarketos/upstreaming, AngeloGioacchino Del Regno,
	Konrad Dybcio, Martin Botka, Jami Kettunen, Pavel Dubrova,
	Marijn Suijten, Kiran Gunda, Courtney Cavin, Bryan Wu,
	linux-arm-msm, dri-devel, linux-fbdev, linux-kernel

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 <angelogioacchino.delregno@somainline.org>
---
 drivers/video/backlight/qcom-wled.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
index 29910e603c42..27e8949c7922 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -1526,6 +1526,12 @@ static int wled_configure(struct wled *wled)
 						     "qcom,enabled-strings",
 						     sizeof(u32));
 	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;
+		}
+
 		rc = of_property_read_u32_array(dev->of_node,
 						"qcom,enabled-strings",
 						wled->cfg.enabled_strings,
@@ -1537,6 +1543,14 @@ static int wled_configure(struct wled *wled)
 			return -EINVAL;
 		}
 
+		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;
+			}
+		}
+
 		cfg->num_strings = string_len;
 	}
 
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH 05/10] backlight: qcom-wled: Fix off-by-one maximum with default num_strings
  2021-10-04 19:27 [PATCH 00/10] backlight: qcom-wled: fix and solidify handling of enabled-strings Marijn Suijten
                   ` (3 preceding siblings ...)
  2021-10-04 19:27 ` [PATCH 04/10] backlight: qcom-wled: Validate enabled string indices in DT Marijn Suijten
@ 2021-10-04 19:27 ` Marijn Suijten
  2021-10-05  9:19   ` Daniel Thompson
  2021-10-04 19:27 ` [PATCH 06/10] backlight: qcom-wled: Remove unnecessary 4th default string in wled3 Marijn Suijten
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 34+ messages in thread
From: Marijn Suijten @ 2021-10-04 19:27 UTC (permalink / raw)
  To: phone-devel, Andy Gross, Bjorn Andersson, Lee Jones,
	Daniel Thompson, Jingoo Han
  Cc: ~postmarketos/upstreaming, AngeloGioacchino Del Regno,
	Konrad Dybcio, Martin Botka, Jami Kettunen, Pavel Dubrova,
	Marijn Suijten, Kiran Gunda, Courtney Cavin, Bryan Wu,
	linux-arm-msm, dri-devel, linux-fbdev, linux-kernel

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 allowing
one extra iteration of the wled_var_cfg function parsing this particular
property.

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

diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
index 27e8949c7922..66ce77ee3099 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -1255,17 +1255,17 @@ static const struct wled_var_cfg wled5_ovp_cfg = {
 
 static u32 wled3_num_strings_values_fn(u32 idx)
 {
-	return idx + 1;
+	return idx;
 }
 
 static const struct wled_var_cfg wled3_num_strings_cfg = {
 	.fn = wled3_num_strings_values_fn,
-	.size = 3,
+	.size = 4, /* [0, 3] */
 };
 
 static const struct wled_var_cfg wled4_num_strings_cfg = {
 	.fn = wled3_num_strings_values_fn,
-	.size = 4,
+	.size = 5, /* [0, 4] */
 };
 
 static u32 wled3_switch_freq_values_fn(u32 idx)
@@ -1520,8 +1520,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));
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH 06/10] backlight: qcom-wled: Remove unnecessary 4th default string in wled3
  2021-10-04 19:27 [PATCH 00/10] backlight: qcom-wled: fix and solidify handling of enabled-strings Marijn Suijten
                   ` (4 preceding siblings ...)
  2021-10-04 19:27 ` [PATCH 05/10] backlight: qcom-wled: Fix off-by-one maximum with default num_strings Marijn Suijten
@ 2021-10-04 19:27 ` Marijn Suijten
  2021-10-05  9:20   ` Daniel Thompson
  2021-10-04 19:27 ` [PATCH 07/10] backlight: qcom-wled: Provide enabled_strings default for wled 4 and 5 Marijn Suijten
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 34+ messages in thread
From: Marijn Suijten @ 2021-10-04 19:27 UTC (permalink / raw)
  To: phone-devel, Andy Gross, Bjorn Andersson, Lee Jones,
	Daniel Thompson, Jingoo Han
  Cc: ~postmarketos/upstreaming, AngeloGioacchino Del Regno,
	Konrad Dybcio, Martin Botka, Jami Kettunen, Pavel Dubrova,
	Marijn Suijten, Kiran Gunda, Courtney Cavin, Bryan Wu,
	linux-arm-msm, dri-devel, linux-fbdev, linux-kernel

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 <angelogioacchino.delregno@somainline.org>
---
 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 66ce77ee3099..9ec1bdd374d2 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


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH 07/10] backlight: qcom-wled: Provide enabled_strings default for wled 4 and 5
  2021-10-04 19:27 [PATCH 00/10] backlight: qcom-wled: fix and solidify handling of enabled-strings Marijn Suijten
                   ` (5 preceding siblings ...)
  2021-10-04 19:27 ` [PATCH 06/10] backlight: qcom-wled: Remove unnecessary 4th default string in wled3 Marijn Suijten
@ 2021-10-04 19:27 ` Marijn Suijten
  2021-10-05  9:21   ` Daniel Thompson
  2021-10-04 19:27 ` [PATCH 08/10] backlight: qcom-wled: Remove unnecessary double whitespace Marijn Suijten
                   ` (2 subsequent siblings)
  9 siblings, 1 reply; 34+ messages in thread
From: Marijn Suijten @ 2021-10-04 19:27 UTC (permalink / raw)
  To: phone-devel, Andy Gross, Bjorn Andersson, Lee Jones,
	Daniel Thompson, Jingoo Han
  Cc: ~postmarketos/upstreaming, AngeloGioacchino Del Regno,
	Konrad Dybcio, Martin Botka, Jami Kettunen, Pavel Dubrova,
	Marijn Suijten, Kiran Gunda, Courtney Cavin, Bryan Wu,
	linux-arm-msm, dri-devel, linux-fbdev, linux-kernel

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 <angelogioacchino.delregno@somainline.org>
---
 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 9ec1bdd374d2..f143b2563bfe 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


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH 08/10] backlight: qcom-wled: Remove unnecessary double whitespace
  2021-10-04 19:27 [PATCH 00/10] backlight: qcom-wled: fix and solidify handling of enabled-strings Marijn Suijten
                   ` (6 preceding siblings ...)
  2021-10-04 19:27 ` [PATCH 07/10] backlight: qcom-wled: Provide enabled_strings default for wled 4 and 5 Marijn Suijten
@ 2021-10-04 19:27 ` Marijn Suijten
  2021-10-05  9:21   ` Daniel Thompson
  2021-10-04 19:27 ` [PATCH 09/10] backlight: qcom-wled: Consistently use enabled-strings in set_brightness Marijn Suijten
  2021-10-04 19:27 ` [PATCH 10/10] backlight: qcom-wled: Consider enabled_strings in autodetection Marijn Suijten
  9 siblings, 1 reply; 34+ messages in thread
From: Marijn Suijten @ 2021-10-04 19:27 UTC (permalink / raw)
  To: phone-devel, Andy Gross, Bjorn Andersson, Lee Jones,
	Daniel Thompson, Jingoo Han
  Cc: ~postmarketos/upstreaming, AngeloGioacchino Del Regno,
	Konrad Dybcio, Martin Botka, Jami Kettunen, Pavel Dubrova,
	Marijn Suijten, Kiran Gunda, Courtney Cavin, Bryan Wu,
	linux-arm-msm, dri-devel, linux-fbdev, linux-kernel

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 <angelogioacchino.delregno@somainline.org>
---
 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 f143b2563bfe..dbe503007ada 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


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH 09/10] backlight: qcom-wled: Consistently use enabled-strings in set_brightness
  2021-10-04 19:27 [PATCH 00/10] backlight: qcom-wled: fix and solidify handling of enabled-strings Marijn Suijten
                   ` (7 preceding siblings ...)
  2021-10-04 19:27 ` [PATCH 08/10] backlight: qcom-wled: Remove unnecessary double whitespace Marijn Suijten
@ 2021-10-04 19:27 ` Marijn Suijten
  2021-10-05  9:33   ` Daniel Thompson
  2021-10-04 19:27 ` [PATCH 10/10] backlight: qcom-wled: Consider enabled_strings in autodetection Marijn Suijten
  9 siblings, 1 reply; 34+ messages in thread
From: Marijn Suijten @ 2021-10-04 19:27 UTC (permalink / raw)
  To: phone-devel, Andy Gross, Bjorn Andersson, Lee Jones,
	Daniel Thompson, Jingoo Han
  Cc: ~postmarketos/upstreaming, AngeloGioacchino Del Regno,
	Konrad Dybcio, Martin Botka, Jami Kettunen, Pavel Dubrova,
	Marijn Suijten, Kiran Gunda, Courtney Cavin, Bryan Wu,
	linux-arm-msm, dri-devel, linux-fbdev, linux-kernel

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.

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 <angelogioacchino.delregno@somainline.org>
---
 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 dbe503007ada..c0b8d10c20a2 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;
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH 10/10] backlight: qcom-wled: Consider enabled_strings in autodetection
  2021-10-04 19:27 [PATCH 00/10] backlight: qcom-wled: fix and solidify handling of enabled-strings Marijn Suijten
                   ` (8 preceding siblings ...)
  2021-10-04 19:27 ` [PATCH 09/10] backlight: qcom-wled: Consistently use enabled-strings in set_brightness Marijn Suijten
@ 2021-10-04 19:27 ` Marijn Suijten
  9 siblings, 0 replies; 34+ messages in thread
From: Marijn Suijten @ 2021-10-04 19:27 UTC (permalink / raw)
  To: phone-devel, Andy Gross, Bjorn Andersson, Lee Jones,
	Daniel Thompson, Jingoo Han
  Cc: ~postmarketos/upstreaming, AngeloGioacchino Del Regno,
	Konrad Dybcio, Martin Botka, Jami Kettunen, Pavel Dubrova,
	Marijn Suijten, Kiran Gunda, Courtney Cavin, Bryan Wu,
	linux-arm-msm, dri-devel, linux-fbdev, linux-kernel

Following the previous commit using enabled_strings in set_brightness,
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.

Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
---
 drivers/video/backlight/qcom-wled.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
index c0b8d10c20a2..2c49f5d8dc26 100644
--- a/drivers/video/backlight/qcom-wled.c
+++ b/drivers/video/backlight/qcom-wled.c
@@ -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


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* Re: [PATCH 01/10] backlight: qcom-wled: Pass number of elements to read to read_u32_array
  2021-10-04 19:27 ` [PATCH 01/10] backlight: qcom-wled: Pass number of elements to read to read_u32_array Marijn Suijten
@ 2021-10-05  9:05   ` Daniel Thompson
  0 siblings, 0 replies; 34+ messages in thread
From: Daniel Thompson @ 2021-10-05  9:05 UTC (permalink / raw)
  To: Marijn Suijten
  Cc: phone-devel, Andy Gross, Bjorn Andersson, Lee Jones, Jingoo Han,
	~postmarketos/upstreaming, AngeloGioacchino Del Regno,
	Konrad Dybcio, Martin Botka, Jami Kettunen, Pavel Dubrova,
	Kiran Gunda, Courtney Cavin, Bryan Wu, linux-arm-msm, dri-devel,
	linux-fbdev, linux-kernel

On Mon, Oct 04, 2021 at 09:27:32PM +0200, Marijn Suijten wrote:
> 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.
> 
> Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3")
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>

Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>


> ---
>  drivers/video/backlight/qcom-wled.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
> index d094299c2a48..6af808af2328 100644
> --- a/drivers/video/backlight/qcom-wled.c
> +++ b/drivers/video/backlight/qcom-wled.c
> @@ -1528,11 +1528,18 @@ 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)
> -		of_property_read_u32_array(dev->of_node,
> +	if (string_len > 0) {
> +		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 -EINVAL;
> +		}
> +	}
>  
>  	return 0;
>  }
> -- 
> 2.33.0
> 

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 02/10] backlight: qcom-wled: Use cpu_to_le16 macro to perform conversion
  2021-10-04 19:27 ` [PATCH 02/10] backlight: qcom-wled: Use cpu_to_le16 macro to perform conversion Marijn Suijten
@ 2021-10-05  9:06   ` Daniel Thompson
  0 siblings, 0 replies; 34+ messages in thread
From: Daniel Thompson @ 2021-10-05  9:06 UTC (permalink / raw)
  To: Marijn Suijten
  Cc: phone-devel, Andy Gross, Bjorn Andersson, Lee Jones, Jingoo Han,
	~postmarketos/upstreaming, AngeloGioacchino Del Regno,
	Konrad Dybcio, Martin Botka, Jami Kettunen, Pavel Dubrova,
	Kiran Gunda, Courtney Cavin, Bryan Wu, linux-arm-msm, dri-devel,
	linux-fbdev, linux-kernel

On Mon, Oct 04, 2021 at 09:27:33PM +0200, Marijn Suijten wrote:
> 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 <angelogioacchino.delregno@somainline.org>

Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>


> ---
>  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 6af808af2328..9927ed98944a 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
> 

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 04/10] backlight: qcom-wled: Validate enabled string indices in DT
  2021-10-04 19:27 ` [PATCH 04/10] backlight: qcom-wled: Validate enabled string indices in DT Marijn Suijten
@ 2021-10-05  9:14   ` Daniel Thompson
  2021-10-05 10:03     ` Marijn Suijten
  0 siblings, 1 reply; 34+ messages in thread
From: Daniel Thompson @ 2021-10-05  9:14 UTC (permalink / raw)
  To: Marijn Suijten
  Cc: phone-devel, Andy Gross, Bjorn Andersson, Lee Jones, Jingoo Han,
	~postmarketos/upstreaming, AngeloGioacchino Del Regno,
	Konrad Dybcio, Martin Botka, Jami Kettunen, Pavel Dubrova,
	Kiran Gunda, Courtney Cavin, Bryan Wu, linux-arm-msm, dri-devel,
	linux-fbdev, linux-kernel

On Mon, Oct 04, 2021 at 09:27:35PM +0200, 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")

The first half of this patch actually fixes patch 1 from this patch set.
It would be better to move that code there.


Daniel.


> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> ---
>  drivers/video/backlight/qcom-wled.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
> index 29910e603c42..27e8949c7922 100644
> --- a/drivers/video/backlight/qcom-wled.c
> +++ b/drivers/video/backlight/qcom-wled.c
> @@ -1526,6 +1526,12 @@ static int wled_configure(struct wled *wled)
>  						     "qcom,enabled-strings",
>  						     sizeof(u32));
>  	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;
> +		}
> +
>  		rc = of_property_read_u32_array(dev->of_node,
>  						"qcom,enabled-strings",
>  						wled->cfg.enabled_strings,
> @@ -1537,6 +1543,14 @@ static int wled_configure(struct wled *wled)
>  			return -EINVAL;
>  		}
>  
> +		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;
> +			}
> +		}
> +
>  		cfg->num_strings = string_len;
>  	}
>  
> -- 
> 2.33.0
> 

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 05/10] backlight: qcom-wled: Fix off-by-one maximum with default num_strings
  2021-10-04 19:27 ` [PATCH 05/10] backlight: qcom-wled: Fix off-by-one maximum with default num_strings Marijn Suijten
@ 2021-10-05  9:19   ` Daniel Thompson
  2021-10-05 10:06     ` Marijn Suijten
  0 siblings, 1 reply; 34+ messages in thread
From: Daniel Thompson @ 2021-10-05  9:19 UTC (permalink / raw)
  To: Marijn Suijten
  Cc: phone-devel, Andy Gross, Bjorn Andersson, Lee Jones, Jingoo Han,
	~postmarketos/upstreaming, AngeloGioacchino Del Regno,
	Konrad Dybcio, Martin Botka, Jami Kettunen, Pavel Dubrova,
	Kiran Gunda, Courtney Cavin, Bryan Wu, linux-arm-msm, dri-devel,
	linux-fbdev, linux-kernel

On Mon, Oct 04, 2021 at 09:27:36PM +0200, Marijn Suijten wrote:
> 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 allowing
> one extra iteration of the wled_var_cfg function parsing this particular
> property.
> 
> Fixes: 93c64f1ea1e8 ("leds: add Qualcomm PM8941 WLED driver")
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> ---
>  drivers/video/backlight/qcom-wled.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
> index 27e8949c7922..66ce77ee3099 100644
> --- a/drivers/video/backlight/qcom-wled.c
> +++ b/drivers/video/backlight/qcom-wled.c
> @@ -1255,17 +1255,17 @@ static const struct wled_var_cfg wled5_ovp_cfg = {
>  
>  static u32 wled3_num_strings_values_fn(u32 idx)
>  {
> -	return idx + 1;
> +	return idx;
>  }
>  
>  static const struct wled_var_cfg wled3_num_strings_cfg = {
>  	.fn = wled3_num_strings_values_fn,
> -	.size = 3,
> +	.size = 4, /* [0, 3] */

0 is not a valid value for this property.


>  };
>  
>  static const struct wled_var_cfg wled4_num_strings_cfg = {
>  	.fn = wled3_num_strings_values_fn,
> -	.size = 4,
> +	.size = 5, /* [0, 4] */

Ditto.


>  };
>  
>  static u32 wled3_switch_freq_values_fn(u32 idx)
> @@ -1520,8 +1520,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));
> -- 
> 2.33.0
> 

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 06/10] backlight: qcom-wled: Remove unnecessary 4th default string in wled3
  2021-10-04 19:27 ` [PATCH 06/10] backlight: qcom-wled: Remove unnecessary 4th default string in wled3 Marijn Suijten
@ 2021-10-05  9:20   ` Daniel Thompson
  0 siblings, 0 replies; 34+ messages in thread
From: Daniel Thompson @ 2021-10-05  9:20 UTC (permalink / raw)
  To: Marijn Suijten
  Cc: phone-devel, Andy Gross, Bjorn Andersson, Lee Jones, Jingoo Han,
	~postmarketos/upstreaming, AngeloGioacchino Del Regno,
	Konrad Dybcio, Martin Botka, Jami Kettunen, Pavel Dubrova,
	Kiran Gunda, Courtney Cavin, Bryan Wu, linux-arm-msm, dri-devel,
	linux-fbdev, linux-kernel

On Mon, Oct 04, 2021 at 09:27:37PM +0200, Marijn Suijten wrote:
> 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 <angelogioacchino.delregno@somainline.org>

Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>


> ---
>  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 66ce77ee3099..9ec1bdd374d2 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
> 

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 07/10] backlight: qcom-wled: Provide enabled_strings default for wled 4 and 5
  2021-10-04 19:27 ` [PATCH 07/10] backlight: qcom-wled: Provide enabled_strings default for wled 4 and 5 Marijn Suijten
@ 2021-10-05  9:21   ` Daniel Thompson
  0 siblings, 0 replies; 34+ messages in thread
From: Daniel Thompson @ 2021-10-05  9:21 UTC (permalink / raw)
  To: Marijn Suijten
  Cc: phone-devel, Andy Gross, Bjorn Andersson, Lee Jones, Jingoo Han,
	~postmarketos/upstreaming, AngeloGioacchino Del Regno,
	Konrad Dybcio, Martin Botka, Jami Kettunen, Pavel Dubrova,
	Kiran Gunda, Courtney Cavin, Bryan Wu, linux-arm-msm, dri-devel,
	linux-fbdev, linux-kernel

On Mon, Oct 04, 2021 at 09:27:38PM +0200, Marijn Suijten wrote:
> 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 <angelogioacchino.delregno@somainline.org>

Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 08/10] backlight: qcom-wled: Remove unnecessary double whitespace
  2021-10-04 19:27 ` [PATCH 08/10] backlight: qcom-wled: Remove unnecessary double whitespace Marijn Suijten
@ 2021-10-05  9:21   ` Daniel Thompson
  0 siblings, 0 replies; 34+ messages in thread
From: Daniel Thompson @ 2021-10-05  9:21 UTC (permalink / raw)
  To: Marijn Suijten
  Cc: phone-devel, Andy Gross, Bjorn Andersson, Lee Jones, Jingoo Han,
	~postmarketos/upstreaming, AngeloGioacchino Del Regno,
	Konrad Dybcio, Martin Botka, Jami Kettunen, Pavel Dubrova,
	Kiran Gunda, Courtney Cavin, Bryan Wu, linux-arm-msm, dri-devel,
	linux-fbdev, linux-kernel

On Mon, Oct 04, 2021 at 09:27:39PM +0200, Marijn Suijten wrote:
> 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 <angelogioacchino.delregno@somainline.org>

Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 09/10] backlight: qcom-wled: Consistently use enabled-strings in set_brightness
  2021-10-04 19:27 ` [PATCH 09/10] backlight: qcom-wled: Consistently use enabled-strings in set_brightness Marijn Suijten
@ 2021-10-05  9:33   ` Daniel Thompson
  2021-10-05 10:12     ` Marijn Suijten
  0 siblings, 1 reply; 34+ messages in thread
From: Daniel Thompson @ 2021-10-05  9:33 UTC (permalink / raw)
  To: Marijn Suijten
  Cc: phone-devel, Andy Gross, Bjorn Andersson, Lee Jones, Jingoo Han,
	~postmarketos/upstreaming, AngeloGioacchino Del Regno,
	Konrad Dybcio, Martin Botka, Jami Kettunen, Pavel Dubrova,
	Kiran Gunda, Courtney Cavin, Bryan Wu, linux-arm-msm, dri-devel,
	linux-fbdev, linux-kernel

On Mon, Oct 04, 2021 at 09:27:40PM +0200, 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.

This isn't true until patch 10 is applied!

Given both patches fix the same issue in different functions I'd prefer
these to be squashed together (and doubly so because the autodetect code
uses set_brightness() as a helper function).


Daniel.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 03/10] backlight: qcom-wled: Override num-strings when enabled-strings is set
  2021-10-04 19:27 ` [PATCH 03/10] backlight: qcom-wled: Override num-strings when enabled-strings is set Marijn Suijten
@ 2021-10-05  9:38   ` Daniel Thompson
  0 siblings, 0 replies; 34+ messages in thread
From: Daniel Thompson @ 2021-10-05  9:38 UTC (permalink / raw)
  To: Marijn Suijten
  Cc: phone-devel, Andy Gross, Bjorn Andersson, Lee Jones, Jingoo Han,
	~postmarketos/upstreaming, AngeloGioacchino Del Regno,
	Konrad Dybcio, Martin Botka, Jami Kettunen, Pavel Dubrova,
	Kiran Gunda, Courtney Cavin, Bryan Wu, linux-arm-msm, dri-devel,
	linux-fbdev, linux-kernel

On Mon, Oct 04, 2021 at 09:27:34PM +0200, Marijn Suijten wrote:
> DT-bindings do not specify num-strings as mandatory property, yet it is
> required to be specified even if enabled-strings is used.  The length of
> that property-array should already be enough to determine exactly which
> and how many strings to enable.
> 
> Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3")
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>

Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 04/10] backlight: qcom-wled: Validate enabled string indices in DT
  2021-10-05  9:14   ` Daniel Thompson
@ 2021-10-05 10:03     ` Marijn Suijten
  2021-10-05 10:42       ` Daniel Thompson
  0 siblings, 1 reply; 34+ messages in thread
From: Marijn Suijten @ 2021-10-05 10:03 UTC (permalink / raw)
  To: Daniel Thompson
  Cc: phone-devel, Andy Gross, Bjorn Andersson, Lee Jones, Jingoo Han,
	~postmarketos/upstreaming, AngeloGioacchino Del Regno,
	Konrad Dybcio, Martin Botka, Jami Kettunen, Pavel Dubrova,
	Kiran Gunda, Courtney Cavin, Bryan Wu, linux-arm-msm, dri-devel,
	linux-fbdev, linux-kernel

On 2021-10-05 10:14:52, Daniel Thompson wrote:
> On Mon, Oct 04, 2021 at 09:27:35PM +0200, 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")
> 
> The first half of this patch actually fixes patch 1 from this patch set.
> It would be better to move that code there.

It only helps guarding against a maximum of 3 leds for WLED3, while
using string_len instead of an unintentional sizeof(u32) (resulting in
a fixed size of 4) is a different issue requiring a separate patch to
fix.

Would it help to reorder this patch before 1/10, and mention in patch
1/10 (then 2/10) that, besides properly using string_len instead of
hardcoded 4 (which causes wrong reads from DT on top of this), it relies
on the previous patch to prevent against an array longer than 3 for
WLED3?

- Marijn

> Daniel.
> 
> 
> > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> > ---
> >  drivers/video/backlight/qcom-wled.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
> > index 29910e603c42..27e8949c7922 100644
> > --- a/drivers/video/backlight/qcom-wled.c
> > +++ b/drivers/video/backlight/qcom-wled.c
> > @@ -1526,6 +1526,12 @@ static int wled_configure(struct wled *wled)
> >  						     "qcom,enabled-strings",
> >  						     sizeof(u32));
> >  	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;
> > +		}
> > +
> >  		rc = of_property_read_u32_array(dev->of_node,
> >  						"qcom,enabled-strings",
> >  						wled->cfg.enabled_strings,
> > @@ -1537,6 +1543,14 @@ static int wled_configure(struct wled *wled)
> >  			return -EINVAL;
> >  		}
> >  
> > +		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;
> > +			}
> > +		}
> > +
> >  		cfg->num_strings = string_len;
> >  	}
> >  
> > -- 
> > 2.33.0
> > 

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 05/10] backlight: qcom-wled: Fix off-by-one maximum with default num_strings
  2021-10-05  9:19   ` Daniel Thompson
@ 2021-10-05 10:06     ` Marijn Suijten
  2021-10-05 10:38       ` Daniel Thompson
  0 siblings, 1 reply; 34+ messages in thread
From: Marijn Suijten @ 2021-10-05 10:06 UTC (permalink / raw)
  To: Daniel Thompson
  Cc: phone-devel, Andy Gross, Bjorn Andersson, Lee Jones, Jingoo Han,
	~postmarketos/upstreaming, AngeloGioacchino Del Regno,
	Konrad Dybcio, Martin Botka, Jami Kettunen, Pavel Dubrova,
	Kiran Gunda, Courtney Cavin, Bryan Wu, linux-arm-msm, dri-devel,
	linux-fbdev, linux-kernel

On 2021-10-05 10:19:47, Daniel Thompson wrote:
> On Mon, Oct 04, 2021 at 09:27:36PM +0200, Marijn Suijten wrote:
> > 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 allowing
> > one extra iteration of the wled_var_cfg function parsing this particular
> > property.
> > 
> > Fixes: 93c64f1ea1e8 ("leds: add Qualcomm PM8941 WLED driver")
> > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> > ---
> >  drivers/video/backlight/qcom-wled.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
> > index 27e8949c7922..66ce77ee3099 100644
> > --- a/drivers/video/backlight/qcom-wled.c
> > +++ b/drivers/video/backlight/qcom-wled.c
> > @@ -1255,17 +1255,17 @@ static const struct wled_var_cfg wled5_ovp_cfg = {
> >  
> >  static u32 wled3_num_strings_values_fn(u32 idx)
> >  {
> > -	return idx + 1;
> > +	return idx;
> >  }
> >  
> >  static const struct wled_var_cfg wled3_num_strings_cfg = {
> >  	.fn = wled3_num_strings_values_fn,
> > -	.size = 3,
> > +	.size = 4, /* [0, 3] */
> 
> 0 is not a valid value for this property.

These comments represent the possible loop iterations the DT "cfg
parser" runs through, starting at j=0 and running up until and including
j=3.  Should I make that more clear or omit these comments entirely?

- Marijn

> >  };
> >  
> >  static const struct wled_var_cfg wled4_num_strings_cfg = {
> >  	.fn = wled3_num_strings_values_fn,
> > -	.size = 4,
> > +	.size = 5, /* [0, 4] */
> 
> Ditto.
> 
> 
> >  };
> >  
> >  static u32 wled3_switch_freq_values_fn(u32 idx)
> > @@ -1520,8 +1520,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));
> > -- 
> > 2.33.0
> > 

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 09/10] backlight: qcom-wled: Consistently use enabled-strings in set_brightness
  2021-10-05  9:33   ` Daniel Thompson
@ 2021-10-05 10:12     ` Marijn Suijten
  0 siblings, 0 replies; 34+ messages in thread
From: Marijn Suijten @ 2021-10-05 10:12 UTC (permalink / raw)
  To: Daniel Thompson
  Cc: phone-devel, Andy Gross, Bjorn Andersson, Lee Jones, Jingoo Han,
	~postmarketos/upstreaming, AngeloGioacchino Del Regno,
	Konrad Dybcio, Martin Botka, Jami Kettunen, Pavel Dubrova,
	Kiran Gunda, Courtney Cavin, Bryan Wu, linux-arm-msm, dri-devel,
	linux-fbdev, linux-kernel

On 2021-10-05 10:33:31, Daniel Thompson wrote:
> On Mon, Oct 04, 2021 at 09:27:40PM +0200, 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.
> 
> This isn't true until patch 10 is applied!

Patch 9 and 10 were split up at a last resort to prevent a clash in the
title, apologies for that.

> Given both patches fix the same issue in different functions I'd prefer
> these to be squashed together (and doubly so because the autodetect code
> uses set_brightness() as a helper function).

That's a fair reason, and solution I agree on.  I'll figure out how to
generify the title and re-spin this patchset except if there are other
reviewers/maintainers I should wait for.

- Marijn

> Daniel.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 05/10] backlight: qcom-wled: Fix off-by-one maximum with default num_strings
  2021-10-05 10:06     ` Marijn Suijten
@ 2021-10-05 10:38       ` Daniel Thompson
  2021-10-05 10:53         ` Daniel Thompson
  0 siblings, 1 reply; 34+ messages in thread
From: Daniel Thompson @ 2021-10-05 10:38 UTC (permalink / raw)
  To: Marijn Suijten
  Cc: phone-devel, Andy Gross, Bjorn Andersson, Lee Jones, Jingoo Han,
	~postmarketos/upstreaming, AngeloGioacchino Del Regno,
	Konrad Dybcio, Martin Botka, Jami Kettunen, Pavel Dubrova,
	Kiran Gunda, Courtney Cavin, Bryan Wu, linux-arm-msm, dri-devel,
	linux-fbdev, linux-kernel

On Tue, Oct 05, 2021 at 12:06:06PM +0200, Marijn Suijten wrote:
> On 2021-10-05 10:19:47, Daniel Thompson wrote:
> > On Mon, Oct 04, 2021 at 09:27:36PM +0200, Marijn Suijten wrote:
> > > 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 allowing
> > > one extra iteration of the wled_var_cfg function parsing this particular
> > > property.
> > > 
> > > Fixes: 93c64f1ea1e8 ("leds: add Qualcomm PM8941 WLED driver")
> > > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> > > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> > > ---
> > >  drivers/video/backlight/qcom-wled.c | 8 +++-----
> > >  1 file changed, 3 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
> > > index 27e8949c7922..66ce77ee3099 100644
> > > --- a/drivers/video/backlight/qcom-wled.c
> > > +++ b/drivers/video/backlight/qcom-wled.c
> > > @@ -1255,17 +1255,17 @@ static const struct wled_var_cfg wled5_ovp_cfg = {
> > >  
> > >  static u32 wled3_num_strings_values_fn(u32 idx)
> > >  {
> > > -	return idx + 1;
> > > +	return idx;


> > >  }
> > >  
> > >  static const struct wled_var_cfg wled3_num_strings_cfg = {
> > >  	.fn = wled3_num_strings_values_fn,
> > > -	.size = 3,
> > > +	.size = 4, /* [0, 3] */
> > 
> > 0 is not a valid value for this property.
> 
> These comments represent the possible loop iterations the DT "cfg
> parser" runs through, starting at j=0 and running up until and including
> j=3.  Should I make that more clear or omit these comments entirely?

The role of wled3_num_strings_values_fn() is to enumerate the list of
legal values for the property [ 1, 2, 3 ]. Your changes cause the
enumeration to include a non-legal value so that you can have an
identity mapping between the symbol and the enumerate value.

An alternative approach would be to leave the enumeration logic
alone but set the num_string default to UINT_MAX in all cases:

-	cfg->num_strings = cfg->num_strings + 1;
+	if (cfg->num_strings == UINT_MAX)
+		cfg->num_strings = 
+	else
+               /* Convert from enumerated to numeric form */
+		cfg->num_strings = wled3_num_strings_values_fn(
+						cfg->num_strings);


Daniel.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 04/10] backlight: qcom-wled: Validate enabled string indices in DT
  2021-10-05 10:03     ` Marijn Suijten
@ 2021-10-05 10:42       ` Daniel Thompson
  0 siblings, 0 replies; 34+ messages in thread
From: Daniel Thompson @ 2021-10-05 10:42 UTC (permalink / raw)
  To: phone-devel, Andy Gross, Bjorn Andersson, Lee Jones, Jingoo Han,
	~postmarketos/upstreaming, AngeloGioacchino Del Regno,
	Konrad Dybcio, Martin Botka, Jami Kettunen, Pavel Dubrova,
	Kiran Gunda, Courtney Cavin, Bryan Wu, linux-arm-msm, dri-devel,
	linux-fbdev, linux-kernel

On Tue, Oct 05, 2021 at 12:03:50PM +0200, Marijn Suijten wrote:
> On 2021-10-05 10:14:52, Daniel Thompson wrote:
> > On Mon, Oct 04, 2021 at 09:27:35PM +0200, 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")
> > 
> > The first half of this patch actually fixes patch 1 from this patch set.
> > It would be better to move that code there.
> 
> It only helps guarding against a maximum of 3 leds for WLED3, while
> using string_len instead of an unintentional sizeof(u32) (resulting in
> a fixed size of 4) is a different issue requiring a separate patch to
> fix.
> 
> Would it help to reorder this patch before 1/10, and mention in patch
> 1/10 (then 2/10) that, besides properly using string_len instead of
> hardcoded 4 (which causes wrong reads from DT on top of this), it relies
> on the previous patch to prevent against an array longer than 3 for
> WLED3?

Reordering is OK for me.


Daniel.


> > > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> > > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> > > ---
> > >  drivers/video/backlight/qcom-wled.c | 14 ++++++++++++++
> > >  1 file changed, 14 insertions(+)
> > > 
> > > diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
> > > index 29910e603c42..27e8949c7922 100644
> > > --- a/drivers/video/backlight/qcom-wled.c
> > > +++ b/drivers/video/backlight/qcom-wled.c
> > > @@ -1526,6 +1526,12 @@ static int wled_configure(struct wled *wled)
> > >  						     "qcom,enabled-strings",
> > >  						     sizeof(u32));
> > >  	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;
> > > +		}
> > > +
> > >  		rc = of_property_read_u32_array(dev->of_node,
> > >  						"qcom,enabled-strings",
> > >  						wled->cfg.enabled_strings,
> > > @@ -1537,6 +1543,14 @@ static int wled_configure(struct wled *wled)
> > >  			return -EINVAL;
> > >  		}
> > >  
> > > +		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;
> > > +			}
> > > +		}
> > > +
> > >  		cfg->num_strings = string_len;
> > >  	}
> > >  
> > > -- 
> > > 2.33.0
> > > 

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 05/10] backlight: qcom-wled: Fix off-by-one maximum with default num_strings
  2021-10-05 10:38       ` Daniel Thompson
@ 2021-10-05 10:53         ` Daniel Thompson
  2021-10-05 11:44           ` Marijn Suijten
  0 siblings, 1 reply; 34+ messages in thread
From: Daniel Thompson @ 2021-10-05 10:53 UTC (permalink / raw)
  To: Marijn Suijten
  Cc: phone-devel, Andy Gross, Bjorn Andersson, Lee Jones, Jingoo Han,
	~postmarketos/upstreaming, AngeloGioacchino Del Regno,
	Konrad Dybcio, Martin Botka, Jami Kettunen, Pavel Dubrova,
	Kiran Gunda, Courtney Cavin, Bryan Wu, linux-arm-msm, dri-devel,
	linux-fbdev, linux-kernel

On Tue, Oct 05, 2021 at 11:38:43AM +0100, Daniel Thompson wrote:
> On Tue, Oct 05, 2021 at 12:06:06PM +0200, Marijn Suijten wrote:
> > On 2021-10-05 10:19:47, Daniel Thompson wrote:
> > > On Mon, Oct 04, 2021 at 09:27:36PM +0200, Marijn Suijten wrote:
> > > > 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 allowing
> > > > one extra iteration of the wled_var_cfg function parsing this particular
> > > > property.
> > > > 
> > > > Fixes: 93c64f1ea1e8 ("leds: add Qualcomm PM8941 WLED driver")
> > > > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> > > > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> > > > ---
> > > >  drivers/video/backlight/qcom-wled.c | 8 +++-----
> > > >  1 file changed, 3 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
> > > > index 27e8949c7922..66ce77ee3099 100644
> > > > --- a/drivers/video/backlight/qcom-wled.c
> > > > +++ b/drivers/video/backlight/qcom-wled.c
> > > > @@ -1255,17 +1255,17 @@ static const struct wled_var_cfg wled5_ovp_cfg = {
> > > >  
> > > >  static u32 wled3_num_strings_values_fn(u32 idx)
> > > >  {
> > > > -	return idx + 1;
> > > > +	return idx;
> 
> 
> > > >  }
> > > >  
> > > >  static const struct wled_var_cfg wled3_num_strings_cfg = {
> > > >  	.fn = wled3_num_strings_values_fn,
> > > > -	.size = 3,
> > > > +	.size = 4, /* [0, 3] */
> > > 
> > > 0 is not a valid value for this property.
> > 
> > These comments represent the possible loop iterations the DT "cfg
> > parser" runs through, starting at j=0 and running up until and including
> > j=3.  Should I make that more clear or omit these comments entirely?
> 
> The role of wled3_num_strings_values_fn() is to enumerate the list of
> legal values for the property [ 1, 2, 3 ]. Your changes cause the
> enumeration to include a non-legal value so that you can have an
> identity mapping between the symbol and the enumerate value.
> 
> An alternative approach would be to leave the enumeration logic
> alone but set the num_string default to UINT_MAX in all cases:
> 
> -	cfg->num_strings = cfg->num_strings + 1;
> +	if (cfg->num_strings == UINT_MAX)
> +		cfg->num_strings = 

Oops... looks like I missed the cfg->max_string_count here.


> +	else
> +               /* Convert from enumerated to numeric form */
> +		cfg->num_strings = wled3_num_strings_values_fn(
> +						cfg->num_strings);


PS the alternative option is not to treat num-strings as an enumerated
   value at all and just read it directly without using wled_values()...

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 05/10] backlight: qcom-wled: Fix off-by-one maximum with default num_strings
  2021-10-05 10:53         ` Daniel Thompson
@ 2021-10-05 11:44           ` Marijn Suijten
  2021-10-05 14:03             ` Daniel Thompson
  0 siblings, 1 reply; 34+ messages in thread
From: Marijn Suijten @ 2021-10-05 11:44 UTC (permalink / raw)
  To: Daniel Thompson
  Cc: phone-devel, Andy Gross, Bjorn Andersson, Lee Jones, Jingoo Han,
	~postmarketos/upstreaming, AngeloGioacchino Del Regno,
	Konrad Dybcio, Martin Botka, Jami Kettunen, Pavel Dubrova,
	Kiran Gunda, Courtney Cavin, Bryan Wu, linux-arm-msm, dri-devel,
	linux-fbdev, linux-kernel

On 2021-10-05 11:53:12, Daniel Thompson wrote:
> On Tue, Oct 05, 2021 at 11:38:43AM +0100, Daniel Thompson wrote:
> > On Tue, Oct 05, 2021 at 12:06:06PM +0200, Marijn Suijten wrote:
> > > On 2021-10-05 10:19:47, Daniel Thompson wrote:
> > > > On Mon, Oct 04, 2021 at 09:27:36PM +0200, Marijn Suijten wrote:
> > > > > 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 allowing
> > > > > one extra iteration of the wled_var_cfg function parsing this particular
> > > > > property.
> > > > > 
> > > > > Fixes: 93c64f1ea1e8 ("leds: add Qualcomm PM8941 WLED driver")
> > > > > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> > > > > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> > > > > ---
> > > > >  drivers/video/backlight/qcom-wled.c | 8 +++-----
> > > > >  1 file changed, 3 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
> > > > > index 27e8949c7922..66ce77ee3099 100644
> > > > > --- a/drivers/video/backlight/qcom-wled.c
> > > > > +++ b/drivers/video/backlight/qcom-wled.c
> > > > > @@ -1255,17 +1255,17 @@ static const struct wled_var_cfg wled5_ovp_cfg = {
> > > > >  
> > > > >  static u32 wled3_num_strings_values_fn(u32 idx)
> > > > >  {
> > > > > -	return idx + 1;
> > > > > +	return idx;
> > > > >  }
> > > > >  
> > > > >  static const struct wled_var_cfg wled3_num_strings_cfg = {
> > > > >  	.fn = wled3_num_strings_values_fn,
> > > > > -	.size = 3,
> > > > > +	.size = 4, /* [0, 3] */
> > > > 
> > > > 0 is not a valid value for this property.
> > > 
> > > These comments represent the possible loop iterations the DT "cfg
> > > parser" runs through, starting at j=0 and running up until and including
> > > j=3.  Should I make that more clear or omit these comments entirely?
> > 
> > The role of wled3_num_strings_values_fn() is to enumerate the list of
> > legal values for the property [ 1, 2, 3 ]. Your changes cause the
> > enumeration to include a non-legal value so that you can have an
> > identity mapping between the symbol and the enumerate value.
> > 
> > An alternative approach would be to leave the enumeration logic
> > alone but set the num_string default to UINT_MAX in all cases:
> > 
> > -	cfg->num_strings = cfg->num_strings + 1;
> > +	if (cfg->num_strings == UINT_MAX)
> > +		cfg->num_strings = 
> 
> Oops... looks like I missed the cfg->max_string_count here.
> 
> 
> > +	else
> > +               /* Convert from enumerated to numeric form */
> > +		cfg->num_strings = wled3_num_strings_values_fn(
> > +						cfg->num_strings);
> 
> 
> PS the alternative option is not to treat num-strings as an enumerated
>    value at all and just read it directly without using wled_values()...

I much prefer doing that instead of trying to wrangle enumeration
parsing around integer values that are supposed to be used as-is.  After
all this variable is already named to set the `+ 1` override currently,
and `qcom,enabled_strings` has "custom" handling as well.  I'll extend
the validation to ensure num_strings>=1 too.

In addition, and this needs some investigation on the dt-bindings side
too, it might be beneficial to make both properties mutually exclusive.
When specifying qcom,enabled_strings it makes little sense to also
provide qcom,num_strings and we want the former to take precedence.  At
that point one might ask why qcom,num_strings remains at all when DT can
use qcom,enabled_strings instead.
We will supposedly have to keep backwards compatibility with DTs in mind
so none of this can be removed or made mutually exclusive from a driver
standpoint, that all has to be done in dt-bindings yaml to be enforced
on checked-in DTs.

- Marijn

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 05/10] backlight: qcom-wled: Fix off-by-one maximum with default num_strings
  2021-10-05 11:44           ` Marijn Suijten
@ 2021-10-05 14:03             ` Daniel Thompson
  2021-10-05 15:23               ` Marijn Suijten
  0 siblings, 1 reply; 34+ messages in thread
From: Daniel Thompson @ 2021-10-05 14:03 UTC (permalink / raw)
  To: Marijn Suijten, phone-devel, Andy Gross, Bjorn Andersson,
	Lee Jones, Jingoo Han, ~postmarketos/upstreaming,
	AngeloGioacchino Del Regno, Konrad Dybcio, Martin Botka,
	Jami Kettunen, Pavel Dubrova, Kiran Gunda, Courtney Cavin,
	Bryan Wu, linux-arm-msm, dri-devel, linux-fbdev, linux-kernel

On Tue, Oct 05, 2021 at 01:44:35PM +0200, Marijn Suijten wrote:
> On 2021-10-05 11:53:12, Daniel Thompson wrote:
> > On Tue, Oct 05, 2021 at 11:38:43AM +0100, Daniel Thompson wrote:
> > > On Tue, Oct 05, 2021 at 12:06:06PM +0200, Marijn Suijten wrote:
> > > > On 2021-10-05 10:19:47, Daniel Thompson wrote:
> > > > > On Mon, Oct 04, 2021 at 09:27:36PM +0200, Marijn Suijten wrote:
> > > > > > 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 allowing
> > > > > > one extra iteration of the wled_var_cfg function parsing this particular
> > > > > > property.
> > > > > > 
> > > > > > Fixes: 93c64f1ea1e8 ("leds: add Qualcomm PM8941 WLED driver")
> > > > > > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> > > > > > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> > > > > > ---
> > > > > >  drivers/video/backlight/qcom-wled.c | 8 +++-----
> > > > > >  1 file changed, 3 insertions(+), 5 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c
> > > > > > index 27e8949c7922..66ce77ee3099 100644
> > > > > > --- a/drivers/video/backlight/qcom-wled.c
> > > > > > +++ b/drivers/video/backlight/qcom-wled.c
> > > > > > @@ -1255,17 +1255,17 @@ static const struct wled_var_cfg wled5_ovp_cfg = {
> > > > > >  
> > > > > >  static u32 wled3_num_strings_values_fn(u32 idx)
> > > > > >  {
> > > > > > -	return idx + 1;
> > > > > > +	return idx;
> > > > > >  }
> > > > > >  
> > > > > >  static const struct wled_var_cfg wled3_num_strings_cfg = {
> > > > > >  	.fn = wled3_num_strings_values_fn,
> > > > > > -	.size = 3,
> > > > > > +	.size = 4, /* [0, 3] */
> > > > > 
> > > > > 0 is not a valid value for this property.
> > > > 
> > > > These comments represent the possible loop iterations the DT "cfg
> > > > parser" runs through, starting at j=0 and running up until and including
> > > > j=3.  Should I make that more clear or omit these comments entirely?
> > > 
> > > The role of wled3_num_strings_values_fn() is to enumerate the list of
> > > legal values for the property [ 1, 2, 3 ]. Your changes cause the
> > > enumeration to include a non-legal value so that you can have an
> > > identity mapping between the symbol and the enumerate value.
> > > 
> > > An alternative approach would be to leave the enumeration logic
> > > alone but set the num_string default to UINT_MAX in all cases:
> > > 
> > > -	cfg->num_strings = cfg->num_strings + 1;
> > > +	if (cfg->num_strings == UINT_MAX)
> > > +		cfg->num_strings = 
> > 
> > Oops... looks like I missed the cfg->max_string_count here.
> > 
> > 
> > > +	else
> > > +               /* Convert from enumerated to numeric form */
> > > +		cfg->num_strings = wled3_num_strings_values_fn(
> > > +						cfg->num_strings);
> > 
> > 
> > PS the alternative option is not to treat num-strings as an enumerated
> >    value at all and just read it directly without using wled_values()...
> 
> I much prefer doing that instead of trying to wrangle enumeration
> parsing around integer values that are supposed to be used as-is.  After
> all this variable is already named to set the `+ 1` override currently,
> and `qcom,enabled_strings` has "custom" handling as well.  I'll extend
> the validation to ensure num_strings>=1 too.

Great.


> In addition, and this needs some investigation on the dt-bindings side
> too, it might be beneficial to make both properties mutually exclusive.
> When specifying qcom,enabled_strings it makes little sense to also
> provide qcom,num_strings and we want the former to take precedence.

If we are designing a "fix" for that then my view is that if both are
passed then num-strings should take precedence because it is an
explicit statement about the number of strings where enabled_strings
is implicit. In other words, if num-strings <= len(enabled_strings) then
we should do what we are told, otherwise report error.


> At that point one might ask why qcom,num_strings remains at all when
> DT can use qcom,enabled_strings instead.  We will supposedly have to
> keep backwards compatibility with DTs in mind so none of this can be
> removed or made mutually exclusive from a driver standpoint, that all
> has to be done in dt-bindings yaml to be enforced on checked-in DTs.

So... perhaps I made a make offering a Reviewed-by: to a patch
that allows len(enabled-strings) to have precedence. If anything
currently uses enabled-strings then it *will* be 4 cells long and
is relying on num-strings to ensure the right things happens ;-) .

We'd like that case to keep working so we must allow num-strings to have
precedence. In other words, when you add the new code, please put it at
the end of the function!


Daniel.


> 
> - Marijn

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 05/10] backlight: qcom-wled: Fix off-by-one maximum with default num_strings
  2021-10-05 14:03             ` Daniel Thompson
@ 2021-10-05 15:23               ` Marijn Suijten
  2021-10-05 16:24                 ` Daniel Thompson
  0 siblings, 1 reply; 34+ messages in thread
From: Marijn Suijten @ 2021-10-05 15:23 UTC (permalink / raw)
  To: Daniel Thompson
  Cc: phone-devel, Andy Gross, Bjorn Andersson, Lee Jones, Jingoo Han,
	~postmarketos/upstreaming, AngeloGioacchino Del Regno,
	Konrad Dybcio, Martin Botka, Jami Kettunen, Pavel Dubrova,
	Kiran Gunda, Courtney Cavin, Bryan Wu, linux-arm-msm, dri-devel,
	linux-fbdev, linux-kernel

On 2021-10-05 15:03:49, Daniel Thompson wrote:
[..]
> > I much prefer doing that instead of trying to wrangle enumeration
> > parsing around integer values that are supposed to be used as-is.  After
> > all this variable is already named to set the `+ 1` override currently,
> > and `qcom,enabled_strings` has "custom" handling as well.  I'll extend
> > the validation to ensure num_strings>=1 too.
> 
> Great.
> 
> 
> > In addition, and this needs some investigation on the dt-bindings side
> > too, it might be beneficial to make both properties mutually exclusive.
> > When specifying qcom,enabled_strings it makes little sense to also
> > provide qcom,num_strings and we want the former to take precedence.
> 
> If we are designing a "fix" for that then my view is that if both are
> passed then num-strings should take precedence because it is an
> explicit statement about the number of strings where enabled_strings
> is implicit. In other words, if num-strings <= len(enabled_strings) then
> we should do what we are told, otherwise report error.

IMO both should be identical (num-strings == len(enabled-strings)) to
avoid ambiguity, but do read on.

> > At that point one might ask why qcom,num_strings remains at all when
> > DT can use qcom,enabled_strings instead.  We will supposedly have to
> > keep backwards compatibility with DTs in mind so none of this can be
> > removed or made mutually exclusive from a driver standpoint, that all
> > has to be done in dt-bindings yaml to be enforced on checked-in DTs.
> 
> So... perhaps I made a make offering a Reviewed-by: to a patch
> that allows len(enabled-strings) to have precedence. If anything
> currently uses enabled-strings then it *will* be 4 cells long and
> is relying on num-strings to ensure the right things happens ;-) .

Unfortunately Konrad (one of my team members) landed such a patch at the
beginning of this year because I failed to submit this patchset in time
while it has been sitting in my queue since 2019 after being used in a
downstream project.  This is in pmi8994 which doesn't have anything
widely used / production ready yet, so I'd prefer to fix the DT instead
and remove / fix his comment:

    /* Yes, all four strings *have to* be defined or things won't work. */

But this is mostly because, prior to this patchset, no default was set
for WLED4 so the 0'th string would get enabled num-strings (3 in
pmi8994's case) times.

Aside that there's only one more PMIC (also being worked on by
SoMainline) that sets qcom,enabled-strings: this is pm660l, pulled from
our local tree, and it actually has enabled-strings of length 2 which is
broken in its current form, exactly because of relying on this patchset.

Finally, we already discussed this inside SoMainline and the
number/enabled leds should most likely be moved out of the PMIC dtsi's
as they're probably panel, hence board or even device dependent.

> We'd like that case to keep working so we must allow num-strings to have
> precedence. In other words, when you add the new code, please put it at
> the end of the function!

Since there don't seem to be any substantial platforms/PMICs using this
functionality in a working manner, can I talk you into agreeing with
fixing the DT instead?

PS. In -next pmi8994_wled is only enabled for sony-xperia-tone, and
pm660l_wled has yet to be enabled by anything.

- Marijn

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 05/10] backlight: qcom-wled: Fix off-by-one maximum with default num_strings
  2021-10-05 15:23               ` Marijn Suijten
@ 2021-10-05 16:24                 ` Daniel Thompson
  2021-10-05 16:50                   ` Konrad Dybcio
  2021-10-05 17:34                   ` Marijn Suijten
  0 siblings, 2 replies; 34+ messages in thread
From: Daniel Thompson @ 2021-10-05 16:24 UTC (permalink / raw)
  To: Marijn Suijten, phone-devel, Andy Gross, Bjorn Andersson,
	Lee Jones, Jingoo Han, ~postmarketos/upstreaming,
	AngeloGioacchino Del Regno, Konrad Dybcio, Martin Botka,
	Jami Kettunen, Pavel Dubrova, Kiran Gunda, Courtney Cavin,
	Bryan Wu, linux-arm-msm, dri-devel, linux-fbdev, linux-kernel

On Tue, Oct 05, 2021 at 05:23:26PM +0200, Marijn Suijten wrote:
> On 2021-10-05 15:03:49, Daniel Thompson wrote:
> [..]
> > > At that point one might ask why qcom,num_strings remains at all when
> > > DT can use qcom,enabled_strings instead.  We will supposedly have to
> > > keep backwards compatibility with DTs in mind so none of this can be
> > > removed or made mutually exclusive from a driver standpoint, that all
> > > has to be done in dt-bindings yaml to be enforced on checked-in DTs.
> > 
> > So... perhaps I made a make offering a Reviewed-by: to a patch
> > that allows len(enabled-strings) to have precedence. If anything
> > currently uses enabled-strings then it *will* be 4 cells long and
> > is relying on num-strings to ensure the right things happens ;-) .
> 
> Unfortunately Konrad (one of my team members) landed such a patch at the
> beginning of this year because I failed to submit this patchset in time
> while it has been sitting in my queue since 2019 after being used in a
> downstream project.  This is in pmi8994 which doesn't have anything
> widely used / production ready yet, so I'd prefer to fix the DT instead
> and remove / fix his comment:
> 
>     /* Yes, all four strings *have to* be defined or things won't work. */
> 
> But this is mostly because, prior to this patchset, no default was set
> for WLED4 so the 0'th string would get enabled num-strings (3 in
> pmi8994's case) times.
> 
> Aside that there's only one more PMIC (also being worked on by
> SoMainline) that sets qcom,enabled-strings: this is pm660l, pulled from
> our local tree, and it actually has enabled-strings of length 2 which is
> broken in its current form, exactly because of relying on this patchset.
> 
> Finally, we already discussed this inside SoMainline and the
> number/enabled leds should most likely be moved out of the PMIC dtsi's
> as they're probably panel, hence board or even device dependent.
> 
> > We'd like that case to keep working so we must allow num-strings to have
> > precedence. In other words, when you add the new code, please put it at
> > the end of the function!
> 
> Since there don't seem to be any substantial platforms/PMICs using this
> functionality in a working manner, can I talk you into agreeing with
> fixing the DT instead?

I've no objections to seeing the DT updated. However I don't really see
what benefit we get from breaking existing DTs in order to do so.

"Cleaning up annoying legacy" is seldom a good reason to break existing
DTs since, if we could break DTs whenever we choose, there would never
be any annoying legacy to worry about. When conflicting properties
result in uninterpretable DTs then a break may be justified but that is
not the case here.


Daniel.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 05/10] backlight: qcom-wled: Fix off-by-one maximum with default num_strings
  2021-10-05 16:24                 ` Daniel Thompson
@ 2021-10-05 16:50                   ` Konrad Dybcio
  2021-10-05 17:34                   ` Marijn Suijten
  1 sibling, 0 replies; 34+ messages in thread
From: Konrad Dybcio @ 2021-10-05 16:50 UTC (permalink / raw)
  To: Daniel Thompson, Marijn Suijten, phone-devel, Andy Gross,
	Bjorn Andersson, Lee Jones, Jingoo Han,
	~postmarketos/upstreaming, AngeloGioacchino Del Regno,
	Martin Botka, Jami Kettunen, Pavel Dubrova, Kiran Gunda,
	Courtney Cavin, Bryan Wu, linux-arm-msm, dri-devel, linux-fbdev,
	linux-kernel

[snipping to not have the entire thread here]
> I've no objections to seeing the DT updated. However I don't really see
> what benefit we get from breaking existing DTs in order to do so.
>
> "Cleaning up annoying legacy" is seldom a good reason to break existing
> DTs since, if we could break DTs whenever we choose, there would never
> be any annoying legacy to worry about. When conflicting properties
> result in uninterpretable DTs then a break may be justified but that is
> not the case here.
>
>
> Daniel.

The only true user of wled as of right now is Xperia Tone platform, 
which does not yet

have display support upstream, so unless one classifies lighting up an 
otherwise black display

a dealbreaker, I think it'd be fine to bend the rules this time.


Konrad


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 05/10] backlight: qcom-wled: Fix off-by-one maximum with default num_strings
  2021-10-05 16:24                 ` Daniel Thompson
  2021-10-05 16:50                   ` Konrad Dybcio
@ 2021-10-05 17:34                   ` Marijn Suijten
  2021-10-06 14:44                     ` Daniel Thompson
  1 sibling, 1 reply; 34+ messages in thread
From: Marijn Suijten @ 2021-10-05 17:34 UTC (permalink / raw)
  To: Daniel Thompson
  Cc: phone-devel, Andy Gross, Bjorn Andersson, Lee Jones, Jingoo Han,
	~postmarketos/upstreaming, AngeloGioacchino Del Regno,
	Konrad Dybcio, Martin Botka, Jami Kettunen, Pavel Dubrova,
	Kiran Gunda, Courtney Cavin, Bryan Wu, linux-arm-msm, dri-devel,
	linux-fbdev, linux-kernel

On 2021-10-05 17:24:53, Daniel Thompson wrote:
> On Tue, Oct 05, 2021 at 05:23:26PM +0200, Marijn Suijten wrote:
> > On 2021-10-05 15:03:49, Daniel Thompson wrote:
> > [..]
> > > > At that point one might ask why qcom,num_strings remains at all when
> > > > DT can use qcom,enabled_strings instead.  We will supposedly have to
> > > > keep backwards compatibility with DTs in mind so none of this can be
> > > > removed or made mutually exclusive from a driver standpoint, that all
> > > > has to be done in dt-bindings yaml to be enforced on checked-in DTs.
> > > 
> > > So... perhaps I made a make offering a Reviewed-by: to a patch
> > > that allows len(enabled-strings) to have precedence. If anything
> > > currently uses enabled-strings then it *will* be 4 cells long and
> > > is relying on num-strings to ensure the right things happens ;-) .
> > 
> > Unfortunately Konrad (one of my team members) landed such a patch at the
> > beginning of this year because I failed to submit this patchset in time
> > while it has been sitting in my queue since 2019 after being used in a
> > downstream project.  This is in pmi8994 which doesn't have anything
> > widely used / production ready yet, so I'd prefer to fix the DT instead
> > and remove / fix his comment:
> > 
> >     /* Yes, all four strings *have to* be defined or things won't work. */
> > 
> > But this is mostly because, prior to this patchset, no default was set
> > for WLED4 so the 0'th string would get enabled num-strings (3 in
> > pmi8994's case) times.
> > 
> > Aside that there's only one more PMIC (also being worked on by
> > SoMainline) that sets qcom,enabled-strings: this is pm660l, pulled from
> > our local tree, and it actually has enabled-strings of length 2 which is
> > broken in its current form, exactly because of relying on this patchset.
> > 
> > Finally, we already discussed this inside SoMainline and the
> > number/enabled leds should most likely be moved out of the PMIC dtsi's
> > as they're probably panel, hence board or even device dependent.
> > 
> > > We'd like that case to keep working so we must allow num-strings to have
> > > precedence. In other words, when you add the new code, please put it at
> > > the end of the function!
> > 
> > Since there don't seem to be any substantial platforms/PMICs using this
> > functionality in a working manner, can I talk you into agreeing with
> > fixing the DT instead?
> 
> I've no objections to seeing the DT updated. However I don't really see
> what benefit we get from breaking existing DTs in order to do so.
> 
> "Cleaning up annoying legacy" is seldom a good reason to break existing
> DTs since, if we could break DTs whenever we choose, there would never
> be any annoying legacy to worry about. When conflicting properties
> result in uninterpretable DTs then a break may be justified but that is
> not the case here.

As mentioned in my message and repeated by Konrad, the only "existing
DT" that could possibly be broken is a platform that's brought up by us
(SoMainline) and we're more than happy to improve the driver and leave
legacy DT behind us, unless there's more DT in circulation that hasn't
landed in Linux mainline but should be taken into account?

Anyway the plan is to leave qcom,num-strings in place so that the
default enabled_strings list in this driver actually serves a purpose.
Then, if num-strings and enabled-strings is provided the former has
precedence (assuming it doesn't exceed the size of the latter) but we'll
print a warning about this (now unnecessary) ambiguity, and if possible
at all - haven't found an example yet - make the properties mutually
exclusive in dt-bindings.

Disallowing both cases would only simplify the code in the end but we
can spend a few lines to support the desired legacy.

- Marijn

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 05/10] backlight: qcom-wled: Fix off-by-one maximum with default num_strings
  2021-10-05 17:34                   ` Marijn Suijten
@ 2021-10-06 14:44                     ` Daniel Thompson
  2021-10-07 21:28                       ` Marijn Suijten
  0 siblings, 1 reply; 34+ messages in thread
From: Daniel Thompson @ 2021-10-06 14:44 UTC (permalink / raw)
  To: Marijn Suijten
  Cc: phone-devel, Andy Gross, Bjorn Andersson, Lee Jones, Jingoo Han,
	~postmarketos/upstreaming, AngeloGioacchino Del Regno,
	Konrad Dybcio, Martin Botka, Jami Kettunen, Pavel Dubrova,
	Kiran Gunda, Courtney Cavin, Bryan Wu, linux-arm-msm, dri-devel,
	linux-fbdev, linux-kernel

On Tue, Oct 05, 2021 at 07:34:00PM +0200, Marijn Suijten wrote:
> On 2021-10-05 17:24:53, Daniel Thompson wrote:
> > On Tue, Oct 05, 2021 at 05:23:26PM +0200, Marijn Suijten wrote:
> > > Since there don't seem to be any substantial platforms/PMICs using this
> > > functionality in a working manner, can I talk you into agreeing with
> > > fixing the DT instead?
> > 
> > I've no objections to seeing the DT updated. However I don't really see
> > what benefit we get from breaking existing DTs in order to do so.
> > 
> > "Cleaning up annoying legacy" is seldom a good reason to break existing
> > DTs since, if we could break DTs whenever we choose, there would never
> > be any annoying legacy to worry about. When conflicting properties
> > result in uninterpretable DTs then a break may be justified but that is
> > not the case here.
> 
> As mentioned in my message and repeated by Konrad, the only "existing
> DT" that could possibly be broken is a platform that's brought up by us
> (SoMainline) and we're more than happy to improve the driver and leave
> legacy DT behind us, unless there's more DT in circulation that hasn't
> landed in Linux mainline but should be taken into account?

Devicetrees are supposed to be the domain of firmware (e.g. not part of
the kernel).

I'm therefore reluctant to adopt an "it only exists if it is upstream"
approach for documented DT bindings. Doubly so when it is our bugs that
causes DTs to be written in a manner which we then retrospectively
declare to be wrong.


> Anyway the plan is to leave qcom,num-strings in place so that the
> default enabled_strings list in this driver actually serves a purpose.
> Then, if num-strings and enabled-strings is provided the former has
> precedence (assuming it doesn't exceed the size of the latter) but
> we'll print a warning about this (now unnecessary) ambiguity, and if
> possible at all - haven't found an example yet - make the properties
> mutually exclusive in dt-bindings.
> 
> Disallowing both cases would only simplify the code in the end but we
> can spend a few lines to support the desired legacy.

Yes, warning is OK for me.


Daniel.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 05/10] backlight: qcom-wled: Fix off-by-one maximum with default num_strings
  2021-10-06 14:44                     ` Daniel Thompson
@ 2021-10-07 21:28                       ` Marijn Suijten
  0 siblings, 0 replies; 34+ messages in thread
From: Marijn Suijten @ 2021-10-07 21:28 UTC (permalink / raw)
  To: Daniel Thompson
  Cc: phone-devel, Andy Gross, Bjorn Andersson, Lee Jones, Jingoo Han,
	~postmarketos/upstreaming, AngeloGioacchino Del Regno,
	Konrad Dybcio, Martin Botka, Jami Kettunen, Pavel Dubrova,
	Kiran Gunda, Courtney Cavin, Bryan Wu, linux-arm-msm, dri-devel,
	linux-fbdev, linux-kernel

On 2021-10-06 15:44:44, Daniel Thompson wrote:
> On Tue, Oct 05, 2021 at 07:34:00PM +0200, Marijn Suijten wrote:
> > On 2021-10-05 17:24:53, Daniel Thompson wrote:
> > > On Tue, Oct 05, 2021 at 05:23:26PM +0200, Marijn Suijten wrote:
> > > > Since there don't seem to be any substantial platforms/PMICs using this
> > > > functionality in a working manner, can I talk you into agreeing with
> > > > fixing the DT instead?
> > > 
> > > I've no objections to seeing the DT updated. However I don't really see
> > > what benefit we get from breaking existing DTs in order to do so.
> > > 
> > > "Cleaning up annoying legacy" is seldom a good reason to break existing
> > > DTs since, if we could break DTs whenever we choose, there would never
> > > be any annoying legacy to worry about. When conflicting properties
> > > result in uninterpretable DTs then a break may be justified but that is
> > > not the case here.
> > 
> > As mentioned in my message and repeated by Konrad, the only "existing
> > DT" that could possibly be broken is a platform that's brought up by us
> > (SoMainline) and we're more than happy to improve the driver and leave
> > legacy DT behind us, unless there's more DT in circulation that hasn't
> > landed in Linux mainline but should be taken into account?
> 
> Devicetrees are supposed to be the domain of firmware (e.g. not part of
> the kernel).
> 
> I'm therefore reluctant to adopt an "it only exists if it is upstream"
> approach for documented DT bindings. Doubly so when it is our bugs that
> causes DTs to be written in a manner which we then retrospectively
> declare to be wrong.

I'm aware that DT is considered firmware and is ""intended"" to be
shipped separately (and probably only once out of the factory) but it
seems so far there's an advantage in updating DT in parallel with the
kernel.  However this is the first time hearing that having dt-bindings
documentation available contributes to considering the DT contract
(more) stable.  Either way I'd expect these bindings to have been fixed
much sooner if it was really actively used.

> > Anyway the plan is to leave qcom,num-strings in place so that the
> > default enabled_strings list in this driver actually serves a purpose.
> > Then, if num-strings and enabled-strings is provided the former has
> > precedence (assuming it doesn't exceed the size of the latter) but
> > we'll print a warning about this (now unnecessary) ambiguity, and if
> > possible at all - haven't found an example yet - make the properties
> > mutually exclusive in dt-bindings.
> > 
> > Disallowing both cases would only simplify the code in the end but we
> > can spend a few lines to support the desired legacy.
> 
> Yes, warning is OK for me.

Great, sending v2 shortly.

- Marijn

^ permalink raw reply	[flat|nested] 34+ messages in thread

end of thread, other threads:[~2021-10-07 21:28 UTC | newest]

Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-04 19:27 [PATCH 00/10] backlight: qcom-wled: fix and solidify handling of enabled-strings Marijn Suijten
2021-10-04 19:27 ` [PATCH 01/10] backlight: qcom-wled: Pass number of elements to read to read_u32_array Marijn Suijten
2021-10-05  9:05   ` Daniel Thompson
2021-10-04 19:27 ` [PATCH 02/10] backlight: qcom-wled: Use cpu_to_le16 macro to perform conversion Marijn Suijten
2021-10-05  9:06   ` Daniel Thompson
2021-10-04 19:27 ` [PATCH 03/10] backlight: qcom-wled: Override num-strings when enabled-strings is set Marijn Suijten
2021-10-05  9:38   ` Daniel Thompson
2021-10-04 19:27 ` [PATCH 04/10] backlight: qcom-wled: Validate enabled string indices in DT Marijn Suijten
2021-10-05  9:14   ` Daniel Thompson
2021-10-05 10:03     ` Marijn Suijten
2021-10-05 10:42       ` Daniel Thompson
2021-10-04 19:27 ` [PATCH 05/10] backlight: qcom-wled: Fix off-by-one maximum with default num_strings Marijn Suijten
2021-10-05  9:19   ` Daniel Thompson
2021-10-05 10:06     ` Marijn Suijten
2021-10-05 10:38       ` Daniel Thompson
2021-10-05 10:53         ` Daniel Thompson
2021-10-05 11:44           ` Marijn Suijten
2021-10-05 14:03             ` Daniel Thompson
2021-10-05 15:23               ` Marijn Suijten
2021-10-05 16:24                 ` Daniel Thompson
2021-10-05 16:50                   ` Konrad Dybcio
2021-10-05 17:34                   ` Marijn Suijten
2021-10-06 14:44                     ` Daniel Thompson
2021-10-07 21:28                       ` Marijn Suijten
2021-10-04 19:27 ` [PATCH 06/10] backlight: qcom-wled: Remove unnecessary 4th default string in wled3 Marijn Suijten
2021-10-05  9:20   ` Daniel Thompson
2021-10-04 19:27 ` [PATCH 07/10] backlight: qcom-wled: Provide enabled_strings default for wled 4 and 5 Marijn Suijten
2021-10-05  9:21   ` Daniel Thompson
2021-10-04 19:27 ` [PATCH 08/10] backlight: qcom-wled: Remove unnecessary double whitespace Marijn Suijten
2021-10-05  9:21   ` Daniel Thompson
2021-10-04 19:27 ` [PATCH 09/10] backlight: qcom-wled: Consistently use enabled-strings in set_brightness Marijn Suijten
2021-10-05  9:33   ` Daniel Thompson
2021-10-05 10:12     ` Marijn Suijten
2021-10-04 19:27 ` [PATCH 10/10] backlight: qcom-wled: Consider enabled_strings in autodetection Marijn Suijten

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).