linux-kernel - Re: [PATCH RFC v4 1/2] leds: leds-lp50xx: Handle reg to get correct multi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2bb62450-df35-4063-ac5e-60bc1ef8f5e7@gmail.com>
Date: Mon, 26 May 2025 23:54:21 +0200
From: Jacek Anaszewski <jacek.anaszewski@...il.com>
To: Johan Adolfsson <johan.adolfsson@...s.com>, Lee Jones <lee@...nel.org>,
 Pavel Machek <pavel@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Andrew Davis <afd@...com>
Cc: linux-leds@...r.kernel.org, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, kernel@...s.com
Subject: Re: [PATCH RFC v4 1/2] leds: leds-lp50xx: Handle reg to get correct
 multi_index

Hi Johan,

On 5/26/25 16:54, Johan Adolfsson wrote:
> mc_subled used for multi_index needs well defined array indexes,
> to guarantee the desired result, optionally use reg for that.
> 
> If devicetree child nodes is processed in random or reverse order
> you may end up with multi_index "blue green red" instead of the expected
> "red green blue".
> If user space apps uses multi_index to deduce how to control the leds
> they would most likely be broken without this patch if devicetree
> processing is reversed (which it appears to be).
> 
> arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-fuji.dts has reg set
> but I don't see how it can have worked without this change.
> 
> If reg is not set, the previous behavior is kept, index will be in
> the order nodes are processed.

This is a bug and I don't see any value in keeping buggy code.
Just expect reg to be present and make sure that all in-tree
dts files using these bindings use them in a proper way.

To not break existing users of stable releases, if any of them
implement DT subnodes without 'reg' property, we can just not mark this
commit with "Fixed" tag, so that it wasn't applied to stable releases.
Although I am not sure if we should not fix it there as well.
I'm leaving it to Lee.

> If reg is out of range, an error is returned.
> reg within led child nodes starts with 0, to map to the iout in each bank.
> 
> Signed-off-by: Johan Adolfsson <johan.adolfsson@...s.com>
> ---
>   drivers/leds/leds-lp50xx.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/leds/leds-lp50xx.c b/drivers/leds/leds-lp50xx.c
> index 02cb1565a9fb..8067aaa916bf 100644
> --- a/drivers/leds/leds-lp50xx.c
> +++ b/drivers/leds/leds-lp50xx.c
> @@ -476,6 +476,7 @@ static int lp50xx_probe_dt(struct lp50xx *priv)
>   			return -ENOMEM;
>   
>   		fwnode_for_each_child_node(child, led_node) {
> +			int multi_index = num_colors;
>   			ret = fwnode_property_read_u32(led_node, "color",
>   						       &color_id);
>   			if (ret) {
> @@ -483,8 +484,13 @@ static int lp50xx_probe_dt(struct lp50xx *priv)
>   				dev_err(priv->dev, "Cannot read color\n");
>   				return ret;
>   			}
> +			ret = fwnode_property_read_u32(led_node, "reg", &multi_index);
> +			if (ret == 0 && multi_index >= LP50XX_LEDS_PER_MODULE) {
> +				dev_err(priv->dev, "reg %i out of range\n", multi_index);
> +				return -EINVAL;
> +			}
>   
> -			mc_led_info[num_colors].color_index = color_id;
> +			mc_led_info[multi_index].color_index = color_id;
>   			num_colors++;
>   		}
>   
> 

-- 
Best regards,
Jacek Anaszewski