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] [day] [month] [year] [list]

Message-ID: <851e68e1-f358-49c6-8147-912d582c6a00@gmail.com>
Date: Sat, 14 Jun 2025 15:38:36 +0200
From: Jacek Anaszewski <jacek.anaszewski@...il.com>
To: Lee Jones <lee@...nel.org>
Cc: Johan Adolfsson <johan.adolfsson@...s.com>,
 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>,
 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

On 6/12/25 12:53, Lee Jones wrote:
> On Mon, 26 May 2025, Jacek Anaszewski wrote:
> 
>> 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.
> 
> We cannot assume that a patch won't end up in LTS just by omitting the
> Fixes: tag.  Sasha's AUTOSEL tooling it still likely to pick it up if we
> describe the commit as a fix, which we do and is correct.
> 
> I see no reason not to apply it.  If users are relying on broken
> semantics, then those should be fixed also.
> 
> Is everyone happy with this patch as-is?

Nope, we should require presence of proper 'reg' value then.

>>> 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) {

Here we should fail if 'reg' is not present too.

>>> +				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
>>
> 

-- 
Best regards,
Jacek Anaszewski