linux-kernel - Re: [PATCH net-next 2/4] net: dsa: realtek: keep default LED state in rtl8366rb

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: <388b435f-13c5-446f-b265-6da98ccfd313@kernel.org>
Date: Sun, 10 Mar 2024 09:01:46 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Luiz Angelo Daros de Luca <luizluca@...il.com>,
 Linus Walleij <linus.walleij@...aro.org>,
 Alvin Šipraga <alsi@...g-olufsen.dk>,
 Andrew Lunn <andrew@...n.ch>, Florian Fainelli <f.fainelli@...il.com>,
 Vladimir Oltean <olteanv@...il.com>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 2/4] net: dsa: realtek: keep default LED state in
 rtl8366rb

On 10/03/2024 05:51, Luiz Angelo Daros de Luca wrote:
> This switch family supports four LEDs for each of its six ports. Each
> LED group is composed of one of these four LEDs from all six ports. LED
> groups can be configured to display hardware information, such as link
> activity, or manually controlled through a bitmap in registers
> RTL8366RB_LED_0_1_CTRL_REG and RTL8366RB_LED_2_3_CTRL_REG.
> 
> After a reset, the default LED group configuration for groups 0 to 3
> indicates, respectively, link activity, link at 1000M, 100M, and 10M, or
> RTL8366RB_LED_CTRL_REG as 0x5432. These configurations are commonly used
> for LED indications. However, the driver was replacing that
> configuration to use manually controlled LEDs (RTL8366RB_LED_FORCE)
> without providing a way for the OS to control them. The default
> configuration is deemed more useful than fixed, uncontrollable turned-on
> LEDs.
> 
> The driver was enabling/disabling LEDs during port_enable/disable.
> However, these events occur when the port is administratively controlled
> (up or down) and are not related to link presence. Additionally, when a
> port N was disabled, the driver was turning off all LEDs for group N,
> not only the corresponding LED for port N in any of those 4 groups. In
> such cases, if port 0 was brought down, the LEDs for all ports in LED
> group 0 would be turned off. As another side effect, the driver was
> wrongly warning that port 5 didn't have an LED ("no LED for port 5").
> Since showing the administrative state of ports is not an orthodox way
> to use LEDs, it was not worth it to fix it and all this code was
> dropped.
> 
> The code to disable LEDs was simplified only changing each LED group to
> the RTL8366RB_LED_OFF state. Registers RTL8366RB_LED_0_1_CTRL_REG and
> RTL8366RB_LED_2_3_CTRL_REG are only used when the corresponding LED
> group is configured with RTL8366RB_LED_FORCE and they don't need to be
> cleaned. The code still references an LED controlled by
> RTL8366RB_INTERRUPT_CONTROL_REG, but as of now, no test device has
> actually used it. Also, some magic numbers were replaced by macros.
> 
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@...il.com>
> Reviewed-by: Linus Walleij <linus.walleij@...aro.org>

This is the first version, so where did review happen?

Best regards,
Krzysztof