| Message ID | 20211004192741.621870-6-marijn.suijten@somainline.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | backlight: qcom-wled: fix and solidify handling of enabled-strings | expand |
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 >
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 > >
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.
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()...
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
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
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
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.
[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
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
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.
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
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));
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(-)