Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Date:   Wed, 13 Dec 2023 11:47:50 +0100
From:   Andrew Lunn <andrew@lunn.ch>
To:     Maxime Chevallier <maxime.chevallier@bootlin.com>
Cc:     Daniel Golle <daniel@makrotopia.org>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH net] net: phy: skip LED triggers on PHYs on SFP modules
Message-ID: <ac88a858-67fe-4932-a224-550d38454420@lunn.ch>
References: <102a9dce38bdf00215735d04cd4704458273ad9c.1702339354.git.daniel@makrotopia.org>
 <20231212153512.67a7a35b@device.home>
 <ec909d14-e571-4a50-926d-fbef4f4f9e0a@lunn.ch>
 <20231213110622.29f53391@device.home>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231213110622.29f53391@device.home>
Precedence: bulk

> > SFP LEDs are very unlikely to be on the front panel, since there is no
> > such pins on the SFP cage.
> > 
> > Russell, in your collection of SFPs do you have any with LEDs?
> 
> I mean, aren't the led triggers generic so that it can trigger any
> other LED to blink, and it's up to the user to decide ?

Correct. However, generic LEDs won't be registered via this code path,
so the deadlock is not an issue. Only LED controllers in a PHY within
an SFP, inside an SFP cage are the issue here. I don't have any Copper
SFP modules, so i've no idea if they are physically big enough to have
LEDs?

> I do however see one good thing with this patch is that it makes the
> behaviour coherent regarding triggers regardless if we have a
> media-converter or not.
> 
> If we have a SFP bus with phylink as its upstream (SFP bus directly
> connected to the MAC), then the phy is going to be attached through
> phy_attach_direct(), and before this patch, led triggers will be
> registered.
> 
> If we have an intermediate PHY acting as a media-converter and
> connected to the SFP bus, then the phy isn't attached to the net_device,
> and the triggers aren't registered.
> 
> So if in the end it doesn't make sense to register led triggers for
> PHY in modules, it might be more coherent not registering them at all
> as this patch does. What do you think ?

I think it is all messy. Say the media converter has its LEDs
connected to the front panel. You then get indications of activity on
the front panel. I've never seen a fibre SFP with LEDs, and its an
open question if Copper SFPs have LEDs. So you are limited to the MAC
LEDs or the media converter LEDs. Another scenario could be a PHY
which acts as a media switch, it can have an RJ-45 or an SFP cage,
first to get link wins. In such a situation, i would put the LEDs on
the front panel, since it would look odd for an empty RJ-45 socket
LEDs to blink for SFP activity.

So i think we should have the ability to describe all the LEDs in DT
and let it decided what gets registered.

	Andrew