linux-kernel - Re: [RESEND PATCH v7 2/5] leds: sun50i-a100: New driver for the A100 LED controller

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: <f5468eac-96ec-e8c9-bcc1-9ec219bf3352@sholland.org>
Date:   Sun, 29 Oct 2023 13:37:25 -0500
From:   Samuel Holland <samuel@...lland.org>
To:     Lee Jones <lee@...nel.org>
Cc:     Pavel Machek <pavel@....cz>, linux-leds@...r.kernel.org,
        Chen-Yu Tsai <wens@...e.org>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        linux-kernel@...r.kernel.org, linux-sunxi@...ts.linux.dev
Subject: Re: [RESEND PATCH v7 2/5] leds: sun50i-a100: New driver for the A100
 LED controller

Hi Lee,

On 10/17/23 19:38, Samuel Holland wrote:
> On 3/16/23 08:34, Lee Jones wrote:
>> On Sat, 31 Dec 2022, Samuel Holland wrote:
>>> +	for_each_available_child_of_node(np, child) {
>>> +		struct sun50i_a100_ledc_led *led;
>>> +		struct led_classdev *cdev;
>>> +		u32 addr, color;
>>> +
>>> +		ret = of_property_read_u32(child, "reg", &addr);
>>> +		if (ret || addr >= count) {
>>> +			dev_err(dev, "LED 'reg' values must be from 0 to %d\n",
>>
>> Doesn't sounds like an address.
> 
> The one-wire protocol involves the first LED responding to the first 24
> bits of the transfer, then forwarding the rest to the next LED. The
> address is the ordinal position in the chain. So I don't think there is
> any reason to have gaps in the addresses--the LEDs would still have to
> physically be there, but you would not be able to control them. That
> said, the driver doesn't need to enforce this, so I can remove the check.

There's actually a reason for the driver enforcing that LED addresses
are contiguous. Removing this check decouples the largest address from
the number of LED class devices. Unfortunately, there's a register field
(LEDC_RESET_TIMING_CTRL_REG_LED_NUM) that must be set to the largest
address for transfers to work correctly.

This means the driver would need to iterate through the child nodes in
two passes inside the probe function: first to find the largest reg
value, and second to actually register the LED class devices.

Since I don't think having gaps in the addresses is a realistic use
case, I'd like to keep this restriction for now (with a comment). We
could always pay the complexity cost later if someone really does want
to relax this check.

Regards,
Samuel