netdev - Re: [net-next,PATCH] net: phy: realtek: Add support for PHY LEDs on RTL8211F

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: <ZnjFTSmF3MGX7OuY@makrotopia.org>
Date: Mon, 24 Jun 2024 02:01:01 +0100
From: Daniel Golle <daniel@...rotopia.org>
To: Marek Vasut <marex@...x.de>
Cc: netdev@...r.kernel.org, Alexandre Torgue <alexandre.torgue@...s.st.com>,
	Andrew Lunn <andrew@...n.ch>,
	Christophe Roullier <christophe.roullier@...s.st.com>,
	"David S . Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Russell King <linux@...linux.org.uk>, kernel@...electronics.com,
	linux-kernel@...r.kernel.org
Subject: Re: [net-next,PATCH] net: phy: realtek: Add support for PHY LEDs on
 RTL8211F

On Mon, Jun 24, 2024 at 01:40:33AM +0200, Marek Vasut wrote:
> Realtek RTL8211F Ethernet PHY supports 3 LED pins which are used to
> indicate link status and activity. Add minimal LED controller driver
> supporting the most common uses with the 'netdev' trigger.
> 
> Signed-off-by: Marek Vasut <marex@...x.de>

> [...]
> +static int rtl8211f_led_hw_is_supported(struct phy_device *phydev, u8 index,
> +					unsigned long rules)
> +{
> +	const unsigned long mask = BIT(TRIGGER_NETDEV_LINK_10) |
> +				   BIT(TRIGGER_NETDEV_LINK_100) |
> +				   BIT(TRIGGER_NETDEV_LINK_1000) |
> +				   BIT(TRIGGER_NETDEV_RX) |
> +				   BIT(TRIGGER_NETDEV_TX);
> +
> +	/* The RTL8211F PHY supports these LED settings on up to three LEDs:
> +	 * - Link: Configurable subset of 10/100/1000 link rates
> +	 * - Active: Blink on activity, RX or TX is not differentiated
> +	 * The Active option has two modes, A and B:
> +	 * - A: Link and Active indication at configurable, but matching,
> +	 *      subset of 10/100/1000 link rates
> +	 * - B: Link indication at configurable subset of 10/100/1000 link
> +	 *      rates and Active indication always at all three 10+100+1000
> +	 *      link rates.
> +	 * This code currently uses mode B only.
> +	 */
> +
> +	if (index >= RTL8211F_LED_COUNT)
> +		return -EINVAL;
> +
> +	/* Filter out any other unsupported triggers. */
> +	if (rules & ~mask)
> +		return -EOPNOTSUPP;

It looks like it is not possible to let the hardware indicate only either
RX or TX, it will always have to go together.

Please express this in this function accordingly, so fallback to
software-driven trigger works as expected.

Example:
if (!(rules & BIT(TRIGGER_NETDEV_RX)) ^ !(rules & BIT(TRIGGER_NETDEV_TX)))
	return -EOPNOTSUPP;

> +
> +	return 0;
> +}