From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>

Add devlink param attributes PD692X0_DEVLINK_PARAM_ID_SAVE_CONF and
PD692X0_DEVLINK_PARAM_ID_RESET_CONF to enable userspace management of the
PSE's permanent configuration stored in non-volatile memory.

The save_conf attribute with the '1' value allows saving the current
configuration to non-volatile memory. The reset_conf attribute restores
factory defaults configurations.

Skip hardware configuration initialization on probe when a saved
configuration is already present in non-volatile memory (detected via user
byte 42).

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---

Changes in v2:
- Move on from sysfs to devlink param for userspace management.
---
 Documentation/networking/devlink/index.rst   |   1 +
 Documentation/networking/devlink/pd692x0.rst |  32 +++++
 drivers/net/pse-pd/pd692x0.c                 | 205 ++++++++++++++++++++++++++-
 3 files changed, 235 insertions(+), 3 deletions(-)

diff --git a/Documentation/networking/devlink/index.rst b/Documentation/networking/devlink/index.rst
index 0c58e5c729d92..6db7d9b45f7aa 100644
--- a/Documentation/networking/devlink/index.rst
+++ b/Documentation/networking/devlink/index.rst
@@ -96,6 +96,7 @@ parameters, info versions, and other features it supports.
    netdevsim
    nfp
    octeontx2
+   pd692x0
    prestera
    qed
    sfc
diff --git a/Documentation/networking/devlink/pd692x0.rst b/Documentation/networking/devlink/pd692x0.rst
new file mode 100644
index 0000000000000..3f3edd0ac0361
--- /dev/null
+++ b/Documentation/networking/devlink/pd692x0.rst
@@ -0,0 +1,32 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+===========================
+PSE PD692x0 devlink support
+===========================
+
+This document describes the devlink features implemented by the PSE ``PD692x0``
+device drivers.
+
+Parameters
+==========
+
+The ``PD692x0`` drivers implement the following driver-specific parameters.
+
+.. list-table:: Driver-specific parameters implemented
+   :widths: 5 5 5 85
+
+   * - Name
+     - Type
+     - Mode
+     - Description
+   * - ``save_conf``
+     - bool
+     - runtime
+     - Save the current configuration to non-volatile memory using ``1``
+       attribute value.
+   * - ``reset_conf``
+     - bool
+     - runtime
+     - Reset the current and saved configuration using ``1`` attribute
+       value.
+
diff --git a/drivers/net/pse-pd/pd692x0.c b/drivers/net/pse-pd/pd692x0.c
index 782b1abf94cb1..eb4b911d438b3 100644
--- a/drivers/net/pse-pd/pd692x0.c
+++ b/drivers/net/pse-pd/pd692x0.c
@@ -14,6 +14,7 @@
 #include <linux/pse-pd/pse.h>
 #include <linux/regulator/driver.h>
 #include <linux/regulator/machine.h>
+#include <net/devlink.h>
 
 #define PD692X0_PSE_NAME "pd692x0_pse"
 
@@ -30,6 +31,8 @@
 #define PD692X0_FW_MIN_VER	5
 #define PD692X0_FW_PATCH_VER	5
 
+#define PD692X0_USER_BYTE	42
+
 enum pd692x0_fw_state {
 	PD692X0_FW_UNKNOWN,
 	PD692X0_FW_OK,
@@ -80,6 +83,9 @@ enum {
 	PD692X0_MSG_GET_PORT_PARAM,
 	PD692X0_MSG_GET_POWER_BANK,
 	PD692X0_MSG_SET_POWER_BANK,
+	PD692X0_MSG_SET_USER_BYTE,
+	PD692X0_MSG_SAVE_SYS_SETTINGS,
+	PD692X0_MSG_RESTORE_FACTORY,
 
 	/* add new message above here */
 	PD692X0_MSG_CNT
@@ -103,11 +109,13 @@ struct pd692x0_priv {
 	bool last_cmd_key;
 	unsigned long last_cmd_key_time;
 
+	bool cfg_saved;
 	enum ethtool_c33_pse_admin_state admin_state[PD692X0_MAX_PIS];
 	struct regulator_dev *manager_reg[PD692X0_MAX_MANAGERS];
 	int manager_pw_budget[PD692X0_MAX_MANAGERS];
 	int nmanagers;
 	struct pd692x0_matrix *port_matrix;
+	struct devlink *dl;
 };
 
 /* Template list of communication messages. The non-null bytes defined here
@@ -193,6 +201,24 @@ static const struct pd692x0_msg pd692x0_msg_template_list[PD692X0_MSG_CNT] = {
 		.key = PD692X0_KEY_CMD,
 		.sub = {0x07, 0x0b, 0x57},
 	},
+	[PD692X0_MSG_SET_USER_BYTE] = {
+		.key = PD692X0_KEY_PRG,
+		.sub = {0x41, PD692X0_USER_BYTE},
+		.data = {0x4e, 0x4e, 0x4e, 0x4e,
+			 0x4e, 0x4e, 0x4e, 0x4e},
+	},
+	[PD692X0_MSG_SAVE_SYS_SETTINGS] = {
+		.key = PD692X0_KEY_PRG,
+		.sub = {0x06, 0x0f},
+		.data = {0x4e, 0x4e, 0x4e, 0x4e,
+			 0x4e, 0x4e, 0x4e, 0x4e},
+	},
+	[PD692X0_MSG_RESTORE_FACTORY] = {
+		.key = PD692X0_KEY_PRG,
+		.sub = {0x2d, 0x4e},
+		.data = {0x4e, 0x4e, 0x4e, 0x4e,
+			 0x4e, 0x4e, 0x4e, 0x4e},
+	},
 };
 
 static u8 pd692x0_build_msg(struct pd692x0_msg *msg, u8 echo)
@@ -1268,9 +1294,12 @@ static int pd692x0_setup_pi_matrix(struct pse_controller_dev *pcdev)
 	if (ret)
 		goto err_managers_req_pw;
 
-	ret = pd692x0_hw_conf_init(priv);
-	if (ret)
-		goto err_managers_req_pw;
+	/* Do not init the conf if it is already saved */
+	if (!priv->cfg_saved) {
+		ret = pd692x0_hw_conf_init(priv);
+		if (ret)
+			goto err_managers_req_pw;
+	}
 
 	pd692x0_of_put_managers(priv, manager);
 	kfree(manager);
@@ -1727,14 +1756,148 @@ static const struct fw_upload_ops pd692x0_fw_ops = {
 	.cleanup = pd692x0_fw_cleanup,
 };
 
+/* Devlink Params APIs */
+enum pd692x0_devlink_param_id {
+	PD692X0_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX,
+	PD692X0_DEVLINK_PARAM_ID_SAVE_CONF,
+	PD692X0_DEVLINK_PARAM_ID_RESET_CONF,
+};
+
+struct pd692x0_devlink {
+	struct pd692x0_priv *priv;
+};
+
+static int pd692x0_dl_validate(struct devlink *devlink, u32 id,
+			       union devlink_param_value val,
+			       struct netlink_ext_ack *extack)
+{
+	if (!val.vbool) {
+		NL_SET_ERR_MSG_FMT(extack, "0 is not a valid value");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int pd692x0_dl_save_conf_set(struct devlink *devlink, u32 id,
+				    struct devlink_param_gset_ctx *ctx,
+				    struct netlink_ext_ack *extack)
+{
+	struct pd692x0_devlink *dl = devlink_priv(devlink);
+	struct pd692x0_priv *priv = dl->priv;
+	struct pd692x0_msg msg, buf = {0};
+	int ret;
+
+	mutex_lock(&priv->pcdev.lock);
+	ret = pd692x0_fw_unavailable(priv);
+	if (ret)
+		goto out;
+
+	msg = pd692x0_msg_template_list[PD692X0_MSG_SET_USER_BYTE];
+	ret = pd692x0_sendrecv_msg(priv, &msg, &buf);
+	if (ret)
+		goto out;
+
+	msg = pd692x0_msg_template_list[PD692X0_MSG_SAVE_SYS_SETTINGS];
+	ret = pd692x0_send_msg(priv, &msg);
+	if (ret) {
+		dev_err(&priv->client->dev,
+			"Failed to save the configuration (%pe)\n",
+			ERR_PTR(ret));
+		goto out;
+	}
+
+	msleep(50); /* Sleep 50ms as described in the datasheet */
+
+	ret = i2c_master_recv(priv->client, (u8 *)&buf, sizeof(buf));
+	if (ret != sizeof(buf)) {
+		if (ret >= 0)
+			ret = -EIO;
+		goto out;
+	}
+
+	ret = 0;
+
+out:
+	mutex_unlock(&priv->pcdev.lock);
+	return ret;
+}
+
+static int pd692x0_dl_reset_conf_set(struct devlink *devlink, u32 id,
+				     struct devlink_param_gset_ctx *ctx,
+				     struct netlink_ext_ack *extack)
+{
+	struct pd692x0_devlink *dl = devlink_priv(devlink);
+	struct pd692x0_priv *priv = dl->priv;
+	struct pd692x0_msg msg, buf = {0};
+	int ret;
+
+	mutex_lock(&priv->pcdev.lock);
+	ret = pd692x0_fw_unavailable(priv);
+	if (ret)
+		goto out;
+
+	msg = pd692x0_msg_template_list[PD692X0_MSG_RESTORE_FACTORY];
+	ret = pd692x0_send_msg(priv, &msg);
+	if (ret) {
+		dev_err(&priv->client->dev,
+			"Failed to reset the configuration (%pe)\n",
+			ERR_PTR(ret));
+		goto out;
+	}
+
+	msleep(100); /* Sleep 100ms as described in the datasheet */
+
+	ret = i2c_master_recv(priv->client, (u8 *)&buf, sizeof(buf));
+	if (ret != sizeof(buf)) {
+		if (ret >= 0)
+			ret = -EIO;
+		goto out;
+	}
+
+	ret = pd692x0_hw_conf_init(priv);
+	if (ret) {
+		dev_err(&priv->client->dev,
+			"Error configuring ports matrix (%pe)\n",
+			ERR_PTR(ret));
+	}
+
+out:
+	mutex_unlock(&priv->pcdev.lock);
+	return ret;
+}
+
+static int pd692x0_dl_dummy_get(struct devlink *devlink, u32 id,
+				struct devlink_param_gset_ctx *ctx)
+{
+	return 0;
+}
+
+static const struct devlink_param pd692x0_dl_params[] = {
+	DEVLINK_PARAM_DRIVER(PD692X0_DEVLINK_PARAM_ID_SAVE_CONF,
+			     "save_conf", DEVLINK_PARAM_TYPE_BOOL,
+			     BIT(DEVLINK_PARAM_CMODE_RUNTIME),
+			     pd692x0_dl_dummy_get, pd692x0_dl_save_conf_set,
+			     pd692x0_dl_validate),
+	DEVLINK_PARAM_DRIVER(PD692X0_DEVLINK_PARAM_ID_RESET_CONF,
+			     "reset_conf", DEVLINK_PARAM_TYPE_BOOL,
+			     BIT(DEVLINK_PARAM_CMODE_RUNTIME),
+			     pd692x0_dl_dummy_get, pd692x0_dl_reset_conf_set,
+			     pd692x0_dl_validate),
+};
+
+static const struct devlink_ops pd692x0_dl_ops = { };
+
 static int pd692x0_i2c_probe(struct i2c_client *client)
 {
 	static const char * const regulators[] = { "vdd", "vdda" };
 	struct pd692x0_msg msg, buf = {0}, zero = {0};
+	struct pd692x0_devlink *pd692x0_dl;
 	struct device *dev = &client->dev;
 	struct pd692x0_msg_ver ver;
 	struct pd692x0_priv *priv;
 	struct fw_upload *fwl;
+	struct devlink *dl;
 	int ret;
 
 	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulators),
@@ -1793,6 +1956,9 @@ static int pd692x0_i2c_probe(struct i2c_client *client)
 		}
 	}
 
+	if (buf.data[2] == PD692X0_USER_BYTE)
+		priv->cfg_saved = true;
+
 	priv->np = dev->of_node;
 	priv->pcdev.nr_lines = PD692X0_MAX_PIS;
 	priv->pcdev.owner = THIS_MODULE;
@@ -1813,14 +1979,47 @@ static int pd692x0_i2c_probe(struct i2c_client *client)
 				     "failed to register to the Firmware Upload API\n");
 	priv->fwl = fwl;
 
+	dl = devlink_alloc(&pd692x0_dl_ops,
+			   sizeof(struct pd692x0_devlink), dev);
+	if (!dl) {
+		dev_err(dev, "devlink_alloc failed\n");
+		ret = -ENOMEM;
+		goto err_unregister_fw;
+	}
+
+	pd692x0_dl = devlink_priv(dl);
+	pd692x0_dl->priv = priv;
+	priv->dl = dl;
+
+	ret = devlink_params_register(dl, pd692x0_dl_params,
+				      ARRAY_SIZE(pd692x0_dl_params));
+	if (ret) {
+		dev_err(dev,
+			"devlink params register failed with error %d",
+			ret);
+		goto err_free_dl;
+	}
+
+	devlink_register(dl);
 	return 0;
+
+err_free_dl:
+	devlink_free(dl);
+err_unregister_fw:
+	firmware_upload_unregister(priv->fwl);
+
+	return ret;
 }
 
 static void pd692x0_i2c_remove(struct i2c_client *client)
 {
 	struct pd692x0_priv *priv = i2c_get_clientdata(client);
+	struct devlink *dl = priv->dl;
 
 	pd692x0_managers_free_pw_budget(priv);
+	devlink_params_unregister(dl, pd692x0_dl_params,
+				  ARRAY_SIZE(pd692x0_dl_params));
+	devlink_unregister(dl);
 	firmware_upload_unregister(priv->fwl);
 }
 

-- 
2.43.0

On Fri, 29 Aug 2025 18:28:46 +0200 Kory Maincent wrote:
> +The ``PD692x0`` drivers implement the following driver-specific parameters.
> +
> +.. list-table:: Driver-specific parameters implemented
> +   :widths: 5 5 5 85
> +
> +   * - Name
> +     - Type
> +     - Mode
> +     - Description
> +   * - ``save_conf``
> +     - bool
> +     - runtime
> +     - Save the current configuration to non-volatile memory using ``1``
> +       attribute value.
> +   * - ``reset_conf``
> +     - bool
> +     - runtime
> +     - Reset the current and saved configuration using ``1`` attribute
> +       value.

Sorry for not offering a clear alternative, but I'm not aware of any
precedent for treating devlink params as action triggers. devlink params
should be values that can be set and read, which is clearly not
the case here:

> +static int pd692x0_dl_dummy_get(struct devlink *devlink, u32 id,
> +				struct devlink_param_gset_ctx *ctx)
> +{
> +	return 0;
> +}
-- 
pw-bot: cr

On Mon, 1 Sep 2025 13:31:00 -0700
Jakub Kicinski <kuba@kernel.org> wrote:

> On Fri, 29 Aug 2025 18:28:46 +0200 Kory Maincent wrote:
> > +The ``PD692x0`` drivers implement the following driver-specific parameters.
> > +
> > +.. list-table:: Driver-specific parameters implemented
> > +   :widths: 5 5 5 85
> > +
> > +   * - Name
> > +     - Type
> > +     - Mode
> > +     - Description
> > +   * - ``save_conf``
> > +     - bool
> > +     - runtime
> > +     - Save the current configuration to non-volatile memory using ``1``
> > +       attribute value.
> > +   * - ``reset_conf``
> > +     - bool
> > +     - runtime
> > +     - Reset the current and saved configuration using ``1`` attribute
> > +       value.  
> 
> Sorry for not offering a clear alternative, but I'm not aware of any
> precedent for treating devlink params as action triggers. devlink params
> should be values that can be set and read, which is clearly not
> the case here:

Ok.
We could save the configuration for every config change and add a reset-conf
action to devlink reload uAPI? The drawback it that it will bring a bit of
latency (about 110ms) for every config change.

Or adding a new devlink uAPI like a devlink conf but maybe we don't have enough
cases to add such generic new uAPI.
Or get back to the first proposition to use sysfs. 

What do you think?

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

On Tue, 2 Sep 2025 16:43:14 +0200 Kory Maincent wrote:
> > On Fri, 29 Aug 2025 18:28:46 +0200 Kory Maincent wrote:  
> > > +The ``PD692x0`` drivers implement the following driver-specific parameters.
> > > +
> > > +.. list-table:: Driver-specific parameters implemented
> > > +   :widths: 5 5 5 85
> > > +
> > > +   * - Name
> > > +     - Type
> > > +     - Mode
> > > +     - Description
> > > +   * - ``save_conf``
> > > +     - bool
> > > +     - runtime
> > > +     - Save the current configuration to non-volatile memory using ``1``
> > > +       attribute value.
> > > +   * - ``reset_conf``
> > > +     - bool
> > > +     - runtime
> > > +     - Reset the current and saved configuration using ``1`` attribute
> > > +       value.    
> > 
> > Sorry for not offering a clear alternative, but I'm not aware of any
> > precedent for treating devlink params as action triggers. devlink params
> > should be values that can be set and read, which is clearly not
> > the case here:  
> 
> Ok.
> We could save the configuration for every config change and add a reset-conf
> action to devlink reload uAPI? The drawback it that it will bring a bit of
> latency (about 110ms) for every config change.
> 
> Or adding a new devlink uAPI like a devlink conf but maybe we don't have enough
> cases to add such generic new uAPI.
> Or get back to the first proposition to use sysfs. 
> 
> What do you think?

If you are asking for my real preference, abstracting away whether it's
doable and justifiable amount of effort for you -- I'd explore using
flags in the ethtool header to control whether setting is written to
the flash.

On Tue, 2 Sep 2025 13:42:12 -0700 Jakub Kicinski wrote:
> On Tue, 2 Sep 2025 16:43:14 +0200 Kory Maincent wrote:
> > > Sorry for not offering a clear alternative, but I'm not aware of any
> > > precedent for treating devlink params as action triggers. devlink params
> > > should be values that can be set and read, which is clearly not
> > > the case here:    
> > 
> > Ok.
> > We could save the configuration for every config change and add a reset-conf
> > action to devlink reload uAPI? The drawback it that it will bring a bit of
> > latency (about 110ms) for every config change.
> > 
> > Or adding a new devlink uAPI like a devlink conf but maybe we don't have enough
> > cases to add such generic new uAPI.
> > Or get back to the first proposition to use sysfs. 
> > 
> > What do you think?  
> 
> If you are asking for my real preference, abstracting away whether it's
> doable and justifiable amount of effort for you -- I'd explore using
> flags in the ethtool header to control whether setting is written to
> the flash.

PS. failing that the less uAPI the better. Tho, given that the whole
point here is giving user the ability to write the flash -- asking for
uAPI-light approach feels contradictory.

Taking a step back -- the "save to flash" is something that OEM FW
often supports. But for Linux-based control the "save to flash" should
really be equivalent to updating some user space config. When user
configures interfaces in OpenWRT we're not flashing them into the
device tree... Could you perhaps explain what makes updating the
in-flash config a high-priority requirement for PoE?

On Tue, Sep 02, 2025 at 01:48:44PM -0700, Jakub Kicinski wrote:
> On Tue, 2 Sep 2025 13:42:12 -0700 Jakub Kicinski wrote:
> > On Tue, 2 Sep 2025 16:43:14 +0200 Kory Maincent wrote:
> > > > Sorry for not offering a clear alternative, but I'm not aware of any
> > > > precedent for treating devlink params as action triggers. devlink params
> > > > should be values that can be set and read, which is clearly not
> > > > the case here:    
> > > 
> > > Ok.
> > > We could save the configuration for every config change and add a reset-conf
> > > action to devlink reload uAPI? The drawback it that it will bring a bit of
> > > latency (about 110ms) for every config change.
> > > 
> > > Or adding a new devlink uAPI like a devlink conf but maybe we don't have enough
> > > cases to add such generic new uAPI.
> > > Or get back to the first proposition to use sysfs. 
> > > 
> > > What do you think?  
> > 
> > If you are asking for my real preference, abstracting away whether it's
> > doable and justifiable amount of effort for you -- I'd explore using
> > flags in the ethtool header to control whether setting is written to
> > the flash.
> 
> PS. failing that the less uAPI the better. Tho, given that the whole
> point here is giving user the ability to write the flash -- asking for
> uAPI-light approach feels contradictory.
> 
> Taking a step back -- the "save to flash" is something that OEM FW
> often supports. But for Linux-based control the "save to flash" should
> really be equivalent to updating some user space config. When user
> configures interfaces in OpenWRT we're not flashing them into the
> device tree... Could you perhaps explain what makes updating the
> in-flash config a high-priority requirement for PoE?
> 

I think the main use case question is: what happens if the application
CPU reboots?
Do we go back to “safe defaults”? But what are safe defaults - that can
vary a lot between systems.

In many setups, if the CPU reboots it also means the bridge is down, so
there is no packet forwarding. In that case, does it even make sense to
keep providing PoE power if the networking part is non-functional?

Another angle: does it make sense to overwrite the hardware power-on
defaults each time the system starts? Or should we rather be able to
read back the stored defaults from the hardware into the driver and work
with them?

Does anyone here have field experience with similar devices? How are
these topics usually handled outside of my bubble?

Best 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 |

On Wed, 3 Sep 2025 09:10:28 +0200
Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> On Tue, Sep 02, 2025 at 01:48:44PM -0700, Jakub Kicinski wrote:
> > On Tue, 2 Sep 2025 13:42:12 -0700 Jakub Kicinski wrote:  
> > > On Tue, 2 Sep 2025 16:43:14 +0200 Kory Maincent wrote:  
> > > > > Sorry for not offering a clear alternative, but I'm not aware of any
> > > > > precedent for treating devlink params as action triggers. devlink
> > > > > params should be values that can be set and read, which is clearly not
> > > > > the case here:      
> > > > 
> > > > Ok.
> > > > We could save the configuration for every config change and add a
> > > > reset-conf action to devlink reload uAPI? The drawback it that it will
> > > > bring a bit of latency (about 110ms) for every config change.
> > > > 
> > > > Or adding a new devlink uAPI like a devlink conf but maybe we don't
> > > > have enough cases to add such generic new uAPI.
> > > > Or get back to the first proposition to use sysfs. 
> > > > 
> > > > What do you think?    
> > > 
> > > If you are asking for my real preference, abstracting away whether it's
> > > doable and justifiable amount of effort for you -- I'd explore using
> > > flags in the ethtool header to control whether setting is written to
> > > the flash.  
> > 
> > PS. failing that the less uAPI the better. Tho, given that the whole
> > point here is giving user the ability to write the flash -- asking for
> > uAPI-light approach feels contradictory.
> > 
> > Taking a step back -- the "save to flash" is something that OEM FW
> > often supports. But for Linux-based control the "save to flash" should
> > really be equivalent to updating some user space config. When user
> > configures interfaces in OpenWRT we're not flashing them into the
> > device tree... Could you perhaps explain what makes updating the
> > in-flash config a high-priority requirement for PoE?
> >   
> 
> I think the main use case question is: what happens if the application
> CPU reboots?
> Do we go back to “safe defaults”? But what are safe defaults - that can
> vary a lot between systems.

In case of CPU reboot, the port matrix will be flashed, which means the
controller is restarted and the ports get disconnected.
Therefore indeed we will go back to default settings.
 
> In many setups, if the CPU reboots it also means the bridge is down, so
> there is no packet forwarding. In that case, does it even make sense to
> keep providing PoE power if the networking part is non-functional?

It depends, we might not want to reboot the Powered Devices if the switch
reboot. I don't currently have specific case in mind which could need this
behavior.
Mainly, the Dent Project final aim was to support mainline all the features
supported in their poed tool.
https://github.com/dentproject/poed/blob/main/dentos-poe-agent/opt/poeagent/docs/Userguide

> Another angle: does it make sense to overwrite the hardware power-on
> defaults each time the system starts? Or should we rather be able to
> read back the stored defaults from the hardware into the driver and work
> with them?

Yes that is one of the design proposition, but we will still need a way to
reset the conf as said before.

> Does anyone here have field experience with similar devices? How are
> these topics usually handled outside of my bubble?

Kyle any field experience on this?

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

On Wed, Sep 03, 2025 at 11:10:25AM +0200, Kory Maincent wrote:
> On Wed, 3 Sep 2025 09:10:28 +0200
> Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> 
> > On Tue, Sep 02, 2025 at 01:48:44PM -0700, Jakub Kicinski wrote:
> > > On Tue, 2 Sep 2025 13:42:12 -0700 Jakub Kicinski wrote:  
> > > > On Tue, 2 Sep 2025 16:43:14 +0200 Kory Maincent wrote:  
> > > > > > Sorry for not offering a clear alternative, but I'm not aware of any
> > > > > > precedent for treating devlink params as action triggers. devlink
> > > > > > params should be values that can be set and read, which is clearly not
> > > > > > the case here:      
> > > > > 
> > > > > Ok.
> > > > > We could save the configuration for every config change and add a
> > > > > reset-conf action to devlink reload uAPI? The drawback it that it will
> > > > > bring a bit of latency (about 110ms) for every config change.
> > > > > 
> > > > > Or adding a new devlink uAPI like a devlink conf but maybe we don't
> > > > > have enough cases to add such generic new uAPI.
> > > > > Or get back to the first proposition to use sysfs. 
> > > > > 
> > > > > What do you think?    
> > > > 
> > > > If you are asking for my real preference, abstracting away whether it's
> > > > doable and justifiable amount of effort for you -- I'd explore using
> > > > flags in the ethtool header to control whether setting is written to
> > > > the flash.  
> > > 
> > > PS. failing that the less uAPI the better. Tho, given that the whole
> > > point here is giving user the ability to write the flash -- asking for
> > > uAPI-light approach feels contradictory.
> > > 
> > > Taking a step back -- the "save to flash" is something that OEM FW
> > > often supports. But for Linux-based control the "save to flash" should
> > > really be equivalent to updating some user space config. When user
> > > configures interfaces in OpenWRT we're not flashing them into the
> > > device tree... Could you perhaps explain what makes updating the
> > > in-flash config a high-priority requirement for PoE?
> > >   
> > 
> > I think the main use case question is: what happens if the application
> > CPU reboots?
> > Do we go back to “safe defaults”? But what are safe defaults - that can
> > vary a lot between systems.
> 
> In case of CPU reboot, the port matrix will be flashed, which means the
> controller is restarted and the ports get disconnected.
> Therefore indeed we will go back to default settings.
>  
> > In many setups, if the CPU reboots it also means the bridge is down, so
> > there is no packet forwarding. In that case, does it even make sense to
> > keep providing PoE power if the networking part is non-functional?
> 
> It depends, we might not want to reboot the Powered Devices if the switch
> reboot. I don't currently have specific case in mind which could need this
> behavior.
> Mainly, the Dent Project final aim was to support mainline all the features
> supported in their poed tool.
> https://github.com/dentproject/poed/blob/main/dentos-poe-agent/opt/poeagent/docs/Userguide
> 
> > Another angle: does it make sense to overwrite the hardware power-on
> > defaults each time the system starts? Or should we rather be able to
> > read back the stored defaults from the hardware into the driver and work
> > with them?
> 
> Yes that is one of the design proposition, but we will still need a way to
> reset the conf as said before.
> 
> > Does anyone here have field experience with similar devices? How are
> > these topics usually handled outside of my bubble?
> 
> Kyle any field experience on this?

I can confirm a field case from industrial/medical gear. Closed system,
several modules on SPE, PoDL for power. Requirement: power the PDs as
early as possible, even before Linux. The box boots faster if power-up
and Linux init run in parallel. In this setup the power-on state is
pre-designed by the product team and should not be changed by Linux at
runtime.

So the question is how to communicate and control this:

Option A - Vendor tool writes a fixed config to NVM (EEPROM)
Pro: matches "pre-designed, don't touch" model; PDs come up early
without Linux.
Con: needs extra vendor tooling; hard to keep in sync with what
userspace shows; Linux may not know what is in NVM unless we
read/reflect it.

Option B - Do all changes in RAM, then one explicit "commit to NVM"
Pro: one write; predictable latency hit only on commit; maps to a
"transaction/commit" model.
Con: what if the controller discarded some changes during the session?
We would need a clear commit status and a way to report which settings
actually stuck.

Option C - Write-through: every change also goes to NVM
Pro: if the system resets, config is always up to date.
Con: adds about 50-110 ms per change on this hardware; may be too slow
for interactive tools or batch reconfig.

From API side, if write-through is possible on this hardware, we can
likely make this per-port and per-setting:

ethtool per-port setters can take a "persist=1" flag. Driver applies the
change and also writes it to NVM for that port.

If a particular setting (bit/field) cannot be persisted by the
controller/NVM, the driver returns an error for the whole request. Userspace
then knows persistence is not supported for that item.

Factory reset:
If hardware supports per-port defaults in NVM, provide a per-port
factory_reset op.

Do PD692x0 supports per-port save/restore functionality?

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 |

On Wed, 3 Sep 2025 12:59:42 +0200
Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> On Wed, Sep 03, 2025 at 11:10:25AM +0200, Kory Maincent wrote:
> > On Wed, 3 Sep 2025 09:10:28 +0200
> > Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> >   
> > > On Tue, Sep 02, 2025 at 01:48:44PM -0700, Jakub Kicinski wrote:  
>  [...]  
>  [...]  
>  [...]  
>  [...]  
>  [...]  
>  [...]  
> > > 
> > > I think the main use case question is: what happens if the application
> > > CPU reboots?
> > > Do we go back to “safe defaults”? But what are safe defaults - that can
> > > vary a lot between systems.  
> > 
> > In case of CPU reboot, the port matrix will be flashed, which means the
> > controller is restarted and the ports get disconnected.
> > Therefore indeed we will go back to default settings.
> >    
> > > In many setups, if the CPU reboots it also means the bridge is down, so
> > > there is no packet forwarding. In that case, does it even make sense to
> > > keep providing PoE power if the networking part is non-functional?  
> > 
> > It depends, we might not want to reboot the Powered Devices if the switch
> > reboot. I don't currently have specific case in mind which could need this
> > behavior.
> > Mainly, the Dent Project final aim was to support mainline all the features
> > supported in their poed tool.
> > https://github.com/dentproject/poed/blob/main/dentos-poe-agent/opt/poeagent/docs/Userguide
> >   
> > > Another angle: does it make sense to overwrite the hardware power-on
> > > defaults each time the system starts? Or should we rather be able to
> > > read back the stored defaults from the hardware into the driver and work
> > > with them?  
> > 
> > Yes that is one of the design proposition, but we will still need a way to
> > reset the conf as said before.
> >   
> > > Does anyone here have field experience with similar devices? How are
> > > these topics usually handled outside of my bubble?  
> > 
> > Kyle any field experience on this?  
> 
> I can confirm a field case from industrial/medical gear. Closed system,
> several modules on SPE, PoDL for power. Requirement: power the PDs as
> early as possible, even before Linux. The box boots faster if power-up
> and Linux init run in parallel. In this setup the power-on state is
> pre-designed by the product team and should not be changed by Linux at
> runtime.
> 
> So the question is how to communicate and control this:
> 
> Option A - Vendor tool writes a fixed config to NVM (EEPROM)
> Pro: matches "pre-designed, don't touch" model; PDs come up early
> without Linux.
> Con: needs extra vendor tooling; hard to keep in sync with what
> userspace shows; Linux may not know what is in NVM unless we
> read/reflect it.
> 
> Option B - Do all changes in RAM, then one explicit "commit to NVM"
> Pro: one write; predictable latency hit only on commit; maps to a
> "transaction/commit" model.
> Con: what if the controller discarded some changes during the session?
> We would need a clear commit status and a way to report which settings
> actually stuck.
> 
> Option C - Write-through: every change also goes to NVM
> Pro: if the system resets, config is always up to date.
> Con: adds about 50-110 ms per change on this hardware; may be too slow
> for interactive tools or batch reconfig.
> 
> From API side, if write-through is possible on this hardware, we can
> likely make this per-port and per-setting:
> 
> ethtool per-port setters can take a "persist=1" flag. Driver applies the
> change and also writes it to NVM for that port.

The thing is, as it is not per port save if we use one time this ethtool flag
all the other ports configurations will also be saved. This could mislead users
by letting them believe they can save configuration for one specific port
without saving all the other ports config.
I think saving the config to NVM globally on each config change is a better
idea.

> If a particular setting (bit/field) cannot be persisted by the
> controller/NVM, the driver returns an error for the whole request. Userspace
> then knows persistence is not supported for that item.
>
> Factory reset:
> If hardware supports per-port defaults in NVM, provide a per-port
> factory_reset op.
>
> Do PD692x0 supports per-port save/restore functionality?

No it is not per port but at the PSE controller level for both.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com