netdev - 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: <20240310113738.GA1623@kernel.org>
Date: Sun, 10 Mar 2024 11:37:38 +0000
From: Simon Horman <horms@...nel.org>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: 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>,
	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 Sun, Mar 10, 2024 at 09:01:46AM +0100, Krzysztof Kozlowski wrote:
> 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?

FWIIW, I think this relates to review of an RFC of this patch-set.

https://lore.kernel.org/netdev/CACRpkda8tQ2u4+jCrpOQXbBd84oW98vmiDgU+GgdYCHuZfn49A@mail.gmail.com/