This patchset fixes WLED's handling of enabled-strings: besides some
cleanup it is now actually possible to specify a non-contiguous array of
enabled strings (not necessarily starting at zero) and the values from
DT are now validated to prevent possible unexpected out-of-bounds
register and array element accesses.
Off-by-one mistakes in the maximum number of strings, also causing
out-of-bounds access, have been addressed as well.
Changes in v2:
- Reordered patch 4/10 (Validate enabled string indices in DT) to sit
before patch 1/10 (Pass number of elements to read to read_u32_array);
- Pulled qcom,num-strings out of the DT enumeration parser, and moved it
after qcom,enabled-strings parser to always have final sign-off over
the number of strings;
- Extra validation for this number of strings against
qcom,enabled-strings;
- Recombined patch 9 (Consistently use enabled-strings in
set_brightness) and patch 10 (Consider enabled_strings in
autodetection), which both solve the same problem in two different
functions. In addition the autodetection code uses set_brightness as
helper already;
- Improved DT configurations for pmi8994 and pm660l, currently in 5.15
rc's.
v1: https://lore.kernel.org/dri-devel/20211004192741.621870-1-marijn.suijten@somainline.org
Marijn Suijten (13):
backlight: qcom-wled: Validate enabled string indices in DT
backlight: qcom-wled: Pass number of elements to read to
read_u32_array
backlight: qcom-wled: Use cpu_to_le16 macro to perform conversion
backlight: qcom-wled: Fix off-by-one maximum with default num_strings
backlight: qcom-wled: Override default length with
qcom,enabled-strings
backlight: qcom-wled: Remove unnecessary 4th default string in WLED3
backlight: qcom-wled: Provide enabled_strings default for WLED 4 and 5
backlight: qcom-wled: Remove unnecessary double whitespace
backlight: qcom-wled: Respect enabled-strings in set_brightness
arm64: dts: qcom: pmi8994: Fix "eternal"->"external" typo in WLED node
arm64: dts: qcom: pmi8994: Remove hardcoded linear WLED
enabled-strings
arm64: dts: qcom: Move WLED num-strings from pmi8994 to
sony-xperia-tone
arm64: dt: qcom: pm660l: Remove board-specific WLED configuration
.../dts/qcom/msm8996-sony-xperia-tone.dtsi | 1 +
arch/arm64/boot/dts/qcom/pm660l.dtsi | 7 -
arch/arm64/boot/dts/qcom/pmi8994.dtsi | 5 +-
drivers/video/backlight/qcom-wled.c | 131 ++++++++++--------
4 files changed, 73 insertions(+), 71 deletions(-)
--
2.33.0
The strings passed in DT may possibly cause out-of-bounds register
accesses and should be validated before use.
Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3")
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-by: AngeloGioacchino Del Regno <redacted>
---
drivers/video/backlight/qcom-wled.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
@@ -1528,12 +1528,28 @@ static int wled_configure(struct wled *wled)string_len=of_property_count_elems_of_size(dev->of_node,"qcom,enabled-strings",sizeof(u32));-if(string_len>0)+if(string_len>0){+if(string_len>wled->max_string_count){+dev_err(dev,"Cannot have more than %d strings\n",+wled->max_string_count);+return-EINVAL;+}+of_property_read_u32_array(dev->of_node,"qcom,enabled-strings",wled->cfg.enabled_strings,sizeof(u32));+for(i=0;i<string_len;++i){+if(wled->cfg.enabled_strings[i]>=wled->max_string_count){+dev_err(dev,+"qcom,enabled-strings index %d at %d is out of bounds\n",+wled->cfg.enabled_strings[i],i);+return-EINVAL;+}+}+}+return0;}
of_property_read_u32_array takes the number of elements to read as last
argument. This does not always need to be 4 (sizeof(u32)) but should
instead be the size of the array in DT as read just above with
of_property_count_elems_of_size.
To not make such an error go unnoticed again the driver now bails
accordingly when of_property_read_u32_array returns an error.
Surprisingly the indentation of newlined arguments is lining up again
after prepending `rc = `.
Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3")
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-by: AngeloGioacchino Del Regno <redacted>
Reviewed-by: Daniel Thompson <redacted>
---
drivers/video/backlight/qcom-wled.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
@@ -1535,10 +1535,15 @@ static int wled_configure(struct wled *wled)return-EINVAL;}-of_property_read_u32_array(dev->of_node,+rc=of_property_read_u32_array(dev->of_node,"qcom,enabled-strings",wled->cfg.enabled_strings,-sizeof(u32));+string_len);+if(rc){+dev_err(dev,"Failed to read %d elements from qcom,enabled-strings: %d\n",+string_len,rc);+returnrc;+}for(i=0;i<string_len;++i){if(wled->cfg.enabled_strings[i]>=wled->max_string_count){
The kernel already provides appropriate primitives to perform endianness
conversion which should be used in favour of manual bit-wrangling.
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-by: AngeloGioacchino Del Regno <redacted>
Reviewed-by: Daniel Thompson <redacted>
---
drivers/video/backlight/qcom-wled.c | 25 +++++++++++--------------
1 file changed, 11 insertions(+), 14 deletions(-)
When not specifying num-strings in the DT the default is used, but +1 is
added to it which turns WLED3 into 4 and WLED4/5 into 5 strings instead
of 3 and 4 respectively, causing out-of-bounds reads and register
read/writes. This +1 exists for a deficiency in the DT parsing code,
and is simply omitted entirely - solving this oob issue - by parsing the
property separately much like qcom,enabled-strings.
This also allows more stringent checks on the maximum value when
qcom,enabled-strings is provided in the DT. Note that num-strings is
parsed after enabled-strings to give it final sign-off over the length,
which DT currently utilizes to get around an incorrect fixed read of
four elements from that array (has been addressed in a prior patch).
Fixes: 93c64f1ea1e8 ("leds: add Qualcomm PM8941 WLED driver")
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-By: AngeloGioacchino Del Regno <redacted>
---
drivers/video/backlight/qcom-wled.c | 51 +++++++++++------------------
1 file changed, 19 insertions(+), 32 deletions(-)
@@ -1341,11 +1326,6 @@ static int wled_configure(struct wled *wled).val_ptr=&cfg->switch_freq,.cfg=&wled3_switch_freq_cfg,},-{-.name="qcom,num-strings",-.val_ptr=&cfg->num_strings,-.cfg=&wled3_num_strings_cfg,-},};conststructwled_u32_optswled4_opts[]={
@@ -1369,11 +1349,6 @@ static int wled_configure(struct wled *wled).val_ptr=&cfg->switch_freq,.cfg=&wled3_switch_freq_cfg,},-{-.name="qcom,num-strings",-.val_ptr=&cfg->num_strings,-.cfg=&wled4_num_strings_cfg,-},};conststructwled_u32_optswled5_opts[]={
@@ -1397,11 +1372,6 @@ static int wled_configure(struct wled *wled).val_ptr=&cfg->switch_freq,.cfg=&wled3_switch_freq_cfg,},-{-.name="qcom,num-strings",-.val_ptr=&cfg->num_strings,-.cfg=&wled4_num_strings_cfg,-},{.name="qcom,modulator-sel",.val_ptr=&cfg->mod_sel,
@@ -1520,8 +1490,6 @@ static int wled_configure(struct wled *wled)*bool_opts[i].val_ptr=true;}-cfg->num_strings=cfg->num_strings+1;-string_len=of_property_count_elems_of_size(dev->of_node,"qcom,enabled-strings",sizeof(u32));
@@ -1552,6 +1520,25 @@ static int wled_configure(struct wled *wled)}}+rc=of_property_read_u32(dev->of_node,"qcom,num-strings",&val);+if(!rc){+if(val<1||val>wled->max_string_count){+dev_err(dev,"qcom,num-strings must be between 1 and %d\n",+wled->max_string_count);+return-EINVAL;+}++if(string_len>0){+dev_warn(dev,"qcom,num-strings and qcom,enabled-strings are ambiguous\n");+if(val>string_len){+dev_err(dev,"qcom,num-strings exceeds qcom,enabled-strings\n");+return-EINVAL;+}+}++cfg->num_strings=val;+}+return0;}
The length of qcom,enabled-strings as property array is enough to
determine the number of strings to be enabled, without needing to set
qcom,num-strings to override the default number of strings when less
than the default (which is also the maxium) is provided in DT.
Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3")
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-by: AngeloGioacchino Del Regno <redacted>
---
drivers/video/backlight/qcom-wled.c | 2 ++
1 file changed, 2 insertions(+)
The previous commit improves num_strings parsing to not go over the
maximum of 3 strings for WLED3 anymore. Likewise this default index for
a hypothetical 4th string is invalid and could access registers that are
not mapped to the desired purpose.
Removing this value gets rid of undesired confusion and avoids the
possibility of accessing registers at this offset even if the 4th array
element is used by accident.
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-by: AngeloGioacchino Del Regno <redacted>
Reviewed-by: Daniel Thompson <redacted>
---
drivers/video/backlight/qcom-wled.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Only WLED 3 sets a sensible default that allows operating this driver
with just qcom,num-strings in the DT; WLED 4 and 5 require
qcom,enabled-strings to be provided otherwise enabled_strings remains
zero-initialized, resuling in every string-specific register write
(currently only the setup and config functions, brightness follows in a
future patch) to only configure the zero'th string multiple times.
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-by: AngeloGioacchino Del Regno <redacted>
Reviewed-by: Daniel Thompson <redacted>
---
drivers/video/backlight/qcom-wled.c | 2 ++
1 file changed, 2 insertions(+)
The hardware is capable of controlling any non-contiguous sequence of
LEDs specified in the DT using qcom,enabled-strings as u32
array, and this also follows from the DT-bindings documentation. The
numbers specified in this array represent indices of the LED strings
that are to be enabled and disabled.
Its value is appropriately used to setup and enable string modules, but
completely disregarded in the set_brightness paths which only iterate
over the number of strings linearly.
Take an example where only string 2 is enabled with
qcom,enabled_strings=<2>: this string is appropriately enabled but
subsequent brightness changes would have only touched the zero'th
brightness register because num_strings is 1 here. This is simply
addressed by looking up the string for this index in the enabled_strings
array just like the other codepaths that iterate over num_strings.
Likewise enabled_strings is now also used in the autodetection path for
consistent behaviour: when a list of strings is specified in DT only
those strings will be probed for autodetection, analogous to how the
number of strings that need to be probed is already bound by
qcom,num-strings. After all autodetection uses the set_brightness
helpers to set an initial value, which could otherwise end up changing
brightness on a different set of strings.
Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3")
Fixes: 03b2b5e86986 ("backlight: qcom-wled: Add support for WLED4 peripheral")
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-by: AngeloGioacchino Del Regno <redacted>
---
drivers/video/backlight/qcom-wled.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
@@ -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);gotofailed_detect;}
Remove redundant spaces inside for loop conditions. No other double
spaces were found that are not part of indentation with `[^\s] `.
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
Reviewed-by: AngeloGioacchino Del Regno <redacted>
Reviewed-by: Daniel Thompson <redacted>
---
drivers/video/backlight/qcom-wled.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)