netdev - Re: [PATCH net-next 3/4] net: phy: marvell10g: Add LED support for 88X3310

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: <ZXxs7JhgEMzp/cli@shell.armlinux.org.uk>
Date: Fri, 15 Dec 2023 15:12:44 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Andrew Lunn <andrew@...n.ch>
Cc: Tobias Waldekranz <tobias@...dekranz.com>, davem@...emloft.net,
	kuba@...nel.org, kabel@...nel.org, hkallweit1@...il.com,
	robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
	conor+dt@...nel.org, netdev@...r.kernel.org,
	devicetree@...r.kernel.org
Subject: Re: [PATCH net-next 3/4] net: phy: marvell10g: Add LED support for
 88X3310

On Fri, Dec 15, 2023 at 03:44:07PM +0100, Andrew Lunn wrote:
> > +static int mv3310_led_funcs_from_flags(struct mv3310_led *led,
> > +				       unsigned long flags,
> > +				       enum mv3310_led_func *solid,
> > +				       enum mv3310_led_func *blink)
> > +{
> > +	unsigned long activity, duplex, link;
> > +
> > +	if (flags & ~(BIT(TRIGGER_NETDEV_LINK) |
> > +		      BIT(TRIGGER_NETDEV_HALF_DUPLEX) |
> > +		      BIT(TRIGGER_NETDEV_FULL_DUPLEX) |
> > +		      BIT(TRIGGER_NETDEV_TX) |
> > +		      BIT(TRIGGER_NETDEV_RX)))
> > +		return -EINVAL;
> 
> This probably should be -EOPNOTSUPP. The trigger will then do the
> blinking in software.
> 
> > +
> > +	link = flags & BIT(TRIGGER_NETDEV_LINK);
> > +
> > +	duplex = flags & (BIT(TRIGGER_NETDEV_HALF_DUPLEX) |
> > +			  BIT(TRIGGER_NETDEV_FULL_DUPLEX));
> > +
> > +	activity = flags & (BIT(TRIGGER_NETDEV_TX) |
> > +			    BIT(TRIGGER_NETDEV_RX));
> > +
> > +	if (link && duplex)
> > +		return -EINVAL;
> 
> It is an odd combination, but again, if the hardware cannot do it,
> return -EOPNOTSUPP and leave it to the software.

I don't recall how the LED triggers work (whether they logically OR
or AND). The hardware supports indicating whether it has a half or
full duplex link, and if the LED is programmed for that, then it will
illuminate when it has link _and_ the duplex is of the specified type.
If the link is down, or the duplex is of the other type, then it won't.

I'm guessing that if link is set and duplex is set, then what userspace
is asking for is the LED to be illuminated whenever the link is up _or_
the duplex is of the specified type, which basically logically resolves
to _only_ "link is up" (because a link that is down has no duplex.)

So, I'm not sure "leaving it to software" is good, that combination is
effectively just "has link".

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!