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

On Sat, Aug 20, 2022 at 08:16:18PM +0200, Andrew Lunn wrote:

On Fri, Aug 19, 2022 at 02:01:09PM +0200, Oleksij Rempel wrote:

quoted

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.

quoted

+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.

Yes. Sounds good. I'll make a new version.

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 |