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

From: Andrew Lunn <andrew@lunn.ch>
To: Oleksij Rempel <o.rempel@pengutronix.de>
Cc: 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>,
	Russell King <linux@armlinux.org.uk>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Jonathan Corbet <corbet@lwn.net>,
	kernel@pengutronix.de, linux-kernel@vger.kernel.org,
	netdev@vger.kernel.org, devicetree@vger.kernel.org,
	linux-doc@vger.kernel.org, David Jander <david@protonic.nl>
Subject: Re: [PATCH net-next v1 7/7] ethtool: add interface to interact with Ethernet Power Equipment
Date: Sat, 20 Aug 2022 20:16:18 +0200	[thread overview]
Message-ID: <YwEk8h9C9XhT6Yyc@lunn.ch> (raw)
In-Reply-To: <20220819120109.3857571-8-o.rempel@pengutronix.de>

On Fri, Aug 19, 2022 at 02:01:09PM +0200, Oleksij Rempel wrote:
> Add interface to support Power Sourcing Equipment. At current step it
> provides generic way to address all variants of PSE devices as defined
> in IEEE 802.3-2018 but support only objects specified for IEEE 802.3-2018 104.4
> PoDL Power Sourcing Equipment (PSE).
> 
> Currently supported and mandatory objects are:
> IEEE 802.3-2018 30.15.1.1.3 aPoDLPSEPowerDetectionStatus
> IEEE 802.3-2018 30.15.1.1.2 aPoDLPSEAdminState
> IEEE 802.3-2018 30.15.1.2.1 acPoDLPSEAdminControl
> 
> This is minimal interface needed to control PSE on each separate
> ethernet port but it provides not all mandatory objects specified in
> IEEE 802.3-2018.

> +static int pse_get_pse_attributs(struct net_device *dev,
> +				 struct pse_reply_data *data)
> +{
> +	struct phy_device *phydev = dev->phydev;
> +	int ret;
> +
> +	if (!phydev)
> +		return -EOPNOTSUPP;
> +
> +	mutex_lock(&phydev->lock);
> +	if (!phydev->psec) {
> +		ret = -EOPNOTSUPP;
> +		goto error_unlock;
> +	}
> +
> +	ret = pse_podl_get_admin_sate(phydev->psec);
> +	if (ret < 0)
> +		goto error_unlock;
> +
> +	data->podl_pse_admin_state = ret;
> +
> +	ret = pse_podl_get_pw_d_status(phydev->psec);
> +	if (ret < 0)
> +		goto error_unlock;
> +
> +	data->podl_pse_pw_d_status = ret;

I'm wondering how this is going to scale. At some point, i expect
there will be an implementation that follows C45.2.9. I see 14 values
which could be returned. I don't think 14 ops in the driver structure
makes sense. Plus c30.15.1 defines other values.

The nice thing about netlink is you can have as many or are little
attributes in the message as you want. For cable testing, i made use
of this. There is no standardisation, different PHYs offer different
sorts of results. So i made the API flexible. The PHY puts whatever
results it has into the message, and ethtool(1) just walks the message
and prints what is in it.

I'm wondering if we want a similar sort of API here?
net/ethtool/pse-pd.c allocates the netlink messages, adds the header,
and then passes it to the driver. The driver then uses helpers from
ethtool to add whatever attributes it wants to the message. pse-pd
then completes the message, and returns it to user space? This seems
like it will scale better.

     Andrew