linux-kernel - Re: [PATCH net v2] net: dsa: tag_rtl4

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: <CAJq09z6xA7roZJYuQFn6B1ivOUOyhrE4-wf4Kj3o6az8hArNqg@mail.gmail.com>
Date: Sat, 30 Dec 2023 20:56:10 -0300
From: Luiz Angelo Daros de Luca <luizluca@...il.com>
To: Linus Walleij <linus.walleij@...aro.org>
Cc: Ansuel Smith <ansuelsmth@...il.com>, Andrew Lunn <andrew@...n.ch>, 
	Vladimir Oltean <olteanv@...il.com>, Florian Fainelli <f.fainelli@...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 v2] net: dsa: tag_rtl4_a: Bump min packet size

> > I took a look at the LED code. It looks like you got it a little bit wrong.
>
> You are right...
>
> > If LEDs are not disabled, it will use the RTL8366RB_LED_FORCE for all
> > 4 LED groups. That RTL8366RB_LED_FORCE keeps the LEDs on. I would use
> > RTL8366RB_LED_LINK_ACT by default to make it blink on link activity
> > (or make it configurable as the comment suggests) but it is not wrong.
> > I cannot evaluate the RTL8366RB_INTERRUPT_CONTROL_REG usage when you
> > disable the LEDs but it seems to be odd.
>
> The problem is that since I don't have a device with LEDs connected
> to tHE RTL8366RB it is all just dry coding.
>
> I would suggest if you can test it just make a basic patch that will
> at least turn on the LEDs to some default setting that works for
> you?

Sure. I believe using link act will be much more useful than just
turning all leds on, independently from the port state. It is an easy
one-line fix.
I can do that after the other series gets merged.

I think that we should also remove rb8366rb_set_port_led(). Even if I
fix it, it will just turn the LED off when the port is
administratively down. RTL8366RB_LED_0_1_CTRL_REG and
RTL8366RB_LED_2_3_CTRL_REG are only used to control leds when you
force them to be on (RTL8366RB_LED_FORCE). In that scenario, the OS
might be in charge of triggering them, even for uses not related to
the switch.

> > I though that maybe we could setup a LED driver to expose the LEDs
> > status in sysfs. However, I'm not sure it is worth it. If you change a
> > LED behavior, it would break the HW triggering rule for all the group.
> > I'm not sure the LED API is ready to expose LEDs with related fate. It
> > would, indeed, be useful as a readonly source or just to
> > enable/disable a LED.
>
> The LED subsystem supports hardware triggering etc thanks to the
> elaborate work by Christian (ansuel). You can see an example of how
> this is done in:
> drivers/net/dsa/qca/qca8k-leds.c
>
> Christian also extended the LEDs subsystem with the necessary
> callbacks to support HW-backed LED control.
>
> This can be used already to achieve HW triggers for the LEDs
> from sysfs. (See callbacks .hw_control_is_supported,
> .hw_control_set etc etc).
>
> I was working to implement this for the Marvell switches but Andrew
> wanted to do some more structured approach with a LED library
> for DSA switches.

Yes, I took a look at it. It would be my inspiration if I go down that
road. I can use the HW offload only if all LEDs in the group share the
same trigger and timer. Otherwise, I'll need to fall back to software
control. I'll think about it if that is a viable solution.

Regards,

Luiz