Re: [PATCH net-next v1 7/7] ethtool: add interface to interact with Ethernet Power Equipment

From: Oleksij Rempel <o.rempel@pengutronix.de>
To: Andrew Lunn <andrew@lunn.ch>
Cc: devicetree@vger.kernel.org, kernel@pengutronix.de,
	Jonathan Corbet <corbet@lwn.net>,
	netdev@vger.kernel.org, linux-doc@vger.kernel.org,
	Russell King <linux@armlinux.org.uk>,
	linux-kernel@vger.kernel.org, Eric Dumazet <edumazet@google.com>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	David Jander <david@protonic.nl>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	"David S. Miller" <davem@davemloft.net>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	Luka Perkov <luka.perkov@sartura.hr>,
	Robert Marko <robert.marko@sartura.hr>
Subject: Re: [PATCH net-next v1 7/7] ethtool: add interface to interact with Ethernet Power Equipment
Date: Sat, 20 Aug 2022 14:31:48 +0200	[thread overview]
Message-ID: <20220820123148.GH10138@pengutronix.de> (raw)
In-Reply-To: <Yv/9XVjRaa5jwpBo@lunn.ch>

On Fri, Aug 19, 2022 at 11:15:09PM +0200, Andrew Lunn wrote:
> > $ ip l
> > ...
> > 5: t1l1@eth0: <BROADCAST,MULTICAST> ..
> > ...
> > 
> > $ ethtool --show-pse t1l1
> > PSE attributs for t1l1:
> > PoDL PSE Admin State: disabled
> > PoDL PSE Power Detection Status: disabled
> > 
> > $ ethtool --set-pse t1l1 podl-pse-admin-control enable
> > $ ethtool --show-pse t1l1
> > PSE attributs for t1l1:
> > PoDL PSE Admin State: enabled
> > PoDL PSE Power Detection Status: delivering power
> 
> Here you seem to indicate that delivering power is totally independent
> of the interface admin status, <BROADCAST,MULTICAST>. The interface is
> admin down, yet you can make it deliver power. I thought there might
> be a link between interface admin status and power? Do the standards
> say anything about this? Is there some sort of industrial norm?
> 
> I'm also wondering about the defaults. It seems like the defaults you
> are proposing is power is off by default, and you have to use ethtool
> to enable power. That does not seem like the most friendly
> settings. Why not an 'auto' mode where if the PHY has PoDL PSE
> capabilities, on ifup it is enabled, on ifdown it is disabled? And you
> can put it into a 'manual' mode where you control it independent of
> administrative status of the interface?

Hm. I would say, safe option is to enable PSE manually. Here are my
reasons:
- some system may require to have power be enabled on boot, before we
  start to care about administrative state of the interface.
- in some cases powered device should stay enabled, even if we do
  ifup/ifdown

I assume, safe defaults should be:
- keep PSE always off, except system was configured to enable it on
  boot.
- keep PSE on after it was enabled, even on if up/down
- bind PSE admin state to the interface state only if user explicitly
  requested it.

At this round is only default, manual mode is implemented. Automatic
mode can be added later if needed.

These are my points, but i'm open for discussion.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |