Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Date:   Wed, 13 Dec 2023 20:23:01 +0000
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Daniel Golle <daniel@makrotopia.org>
Cc:     Andrew Lunn <andrew@lunn.ch>,
        Maxime Chevallier <maxime.chevallier@bootlin.com>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        "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: <ZXoSpcp5i8C2C/EJ@shell.armlinux.org.uk>
References: <102a9dce38bdf00215735d04cd4704458273ad9c.1702339354.git.daniel@makrotopia.org>
 <20231212153512.67a7a35b@device.home>
 <ec909d14-e571-4a50-926d-fbef4f4f9e0a@lunn.ch>
 <ZXnNYJer0JrJxOsl@shell.armlinux.org.uk>
 <ZXn_id6-XWYr2Seo@makrotopia.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZXn_id6-XWYr2Seo@makrotopia.org>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

On Wed, Dec 13, 2023 at 07:01:29PM +0000, Daniel Golle wrote:
> On Wed, Dec 13, 2023 at 03:27:28PM +0000, Russell King (Oracle) wrote:
> > No, and we should _not_ mess around with the "LED" configuration on
> > PHYs on SFPs. It's possible that the LED output is wired to the LOS
> > pin on the module, and messing around with the configuration of that
> > would be asking for trouble.
> > 
> > In any case, I thought we didn't drive the LED configuration on PHYs
> > where the LED configuration isn't described by firmware - and as the
> > PHY on SFP modules would never be described by firmware, hooking
> > such a PHY up to the LED framework sounds like a waste of resources
> > to me.
> 
> This was exactly my line of thought when posting the patch, however,
> Maxime correctly pointed out that the issue with locking and also
> what the patch prevents is registration of LED *triggers* rather than
> the PHY-controlled LEDs themselves. And having the triggers available
> is desirable even beyond the hardware offloaded case (which is probably
> the aspect we both were dealing with the most recently and hence had in
> mind). It is common to control another system SoC GPIO driven LED(s)
> representing the link status and rx/tx traffic, for example.
> 
> So better we get to the core of it and fix the locking issue
> (for example by registering LED trigger asynchronously using
> delayed_work)...

I don't think a delayed work solves anything. Well, it solves the
registration problem, but when the PHY is removed, you have to
_synchronously_ cancel the delayed work, which could result in it
waiting on the called work function to complete. If that called work
function is waiting for the lock which we're holding on the remove
path, then we're still in a deadlock-prone situation.

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