linux-kernel - Re: [PATCH net-next + leds v2 6/7] net: phy: marvell: add support for LEDs controlled by Marvell PHYs

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: <20200910202345.GA18431@ucw.cz>
Date:   Thu, 10 Sep 2020 22:23:45 +0200
From:   Pavel Machek <pavel@....cz>
To:     Marek Behún <marek.behun@....cz>
Cc:     Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org,
        linux-leds@...r.kernel.org, Dan Murphy <dmurphy@...com>,
        Ondřej Jirman <megous@...ous.com>,
        Russell King <linux@...linux.org.uk>,
        linux-kernel@...r.kernel.org,
        Matthias Schiffer <matthias.schiffer@...tq-group.com>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH net-next + leds v2 6/7] net: phy: marvell: add support
 for LEDs controlled by Marvell PHYs

Hi!

> Okay, so the netdev trigger offers modes `link`, `rx`, `tx`.
> You can enable/disable either of these (via separate sysfs files). `rx`
> and `tx` blink the LED, `link` turns the LED on if the interface is
> linked.

I wonder if people really need separate rx and tx, but... this sounds
reasonable.

> The phy_led_trigger subsystem works differently. Instead of registering
> one trigger (like netdev) it registers one trigger per PHY device and
> per speed. So for a PHY with name XYZ and supported speeds 1Gbps,
> 100Mbps, 10Mbps it registers 3 triggers:
>   XYZ:1Gbps XYZ:100Mbps XYZ:10Mbps

That is not reasonable.

> I propose that at least these HW modes should be available (and
> documented) for ethernet PHY controlled LEDs:

Ok, and which of these will you actually use?

>   mode to determine link on:
>     - `link`
>   mode for activity (these should blink):
>     - `activity` (both rx and tx), `rx`, `tx`
>   mode for link (on) and activity (blink)
>     - `link/activity`, maybe `link/rx` and `link/tx`
>   mode for every supported speed:
>     - `1Gbps`, `100Mbps`, `10Mbps`, ...
>   mode for every supported cable type:
>     - `copper`, `fiber`, ... (are there others?)

That's ... way too many options.

Can we do it like netdev trigger? link? yes/no. rx? yes/no. tx? yes/no.

If displaying link only for certain speeds is useful, have link_min
and link_max, specifying values in Mbps? Default would be link_min ==
0, and link_max = 25000, so it would react on any link speed.

Is mode for cable type really useful? Then we should have link_fiber?
yes/no. link_copper? yes/no.

>   mode that allows the user to determine link speed
>     - `speed` (or maybe `linkspeed` ?)
>     - on some Marvell PHYs the speed can be determined by how fast
>       the LED is blinking (ie. 1Gbps blinks with default blinking
>       frequency, 100Mbps with half blinking frequeny of 1Gbps, 10Mbps
>       of half blinking frequency of 100Mbps)
>     - on other Marvell PHYs this is instead:
>       1Gpbs blinks 3 times, pause, 3 times, pause, ...
>       100Mpbs blinks 2 times, pause, 2 times, pause, ...
>       10Mpbs blinks 1 time, pause, 1 time, pause, ...
>     - we don't need to differentiate these modes with different names,
>       because the important thing is just that this mode allows the
>       user to determine the speed from how the LED blinks

I'd be very careful. Userspace should know what they are asking
for. I'd propose simply ignoring this feature.

>   mode to just force blinking - `blink`

We already have different support for blinking in LED subsystem. Lets use that.

Best regards,
									Pavel