[PATCH net-next v19 1/6] net: dsa: add devlink flash_update callback to dsa_switch_ops

[PATCH net-next v19 1/6] net: dsa: add devlink flash_update callback to dsa_switch_ops

[Daniel Golle]

Add a devlink_flash_update callback to dsa_switch_ops so that DSA
drivers can support devlink dev flash without open-coding the devlink
plumbing. Like the other trampolines in net/dsa/devlink.c, the op
returns -EOPNOTSUPP when the driver does not implement the callback.
The devlink core takes a non-NULL ops->flash_update as the capability
gate, so on a switch without the callback a flash request now fetches
the firmware file, unless it names a component, which the core refuses
after consulting the driver's info versions, or an overwrite mask,
which it refuses outright: a missing file fails with the firmware
loader's errno, -ENOENT, instead of -EOPNOTSUPP, and a file that is
found reaches the trampoline inside a FLASH_UPDATE/FLASH_UPDATE_END
notification pair. The ETHTOOL_FLASHDEV ioctl on a user port reaches
the same trampoline through devlink's compat path, without extack.
Both are acceptable for an operation as infrequent as a firmware
update.

The devlink core calls the op with the devlink instance lock held and
without rtnl_lock. DSA calls its port ops from paths of its own, some
without rtnl_lock either, and registers the devlink instance before
the user ports exist, so a driver serialises a flash against its own
ops and its own setup itself.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
v19: commit message: DSA calls some port ops without rtnl and registers
     devlink before the user ports exist, so the driver serialises its
     flash against its setup as well; a request with a component or an
     overwrite mask is refused before the file is fetched, the errno of
     a missing file is the firmware loader's, and the ETHTOOL_FLASHDEV
     ioctl on a user port reaches the trampoline too, without extack
     (found by Sashiko AI review and a local review)
v18: no changes
v17: no changes
v16:
 - commit message: name the errno and the notifications the shared ops
   table changes for switches without the callback (found by Sashiko
   AI review)
 - commit message: a missing firmware file fails before the
   notification pair, which wraps only the call into the trampoline
v15: no changes
v14: no changes, picked up Andrew's v13 Reviewed-by
v13: no changes
v12: no changes
v11: no changes
v10: no changes
v9: install the flash_update op unconditionally and return -EOPNOTSUPP
    from the trampoline like the other DSA devlink trampolines,
    instead of a second devlink_ops permutation (Andrew Lunn)
v8:
 - retitled: this patch adds the callback, its first user is patch 3
 - describe the op's calling context in the commit message
v7: no changes
v6: no changes
v5: no changes
v4: only install the flash_update op for drivers implementing the
    callback so the devlink core keeps rejecting unsupported flash
    requests before fetching the firmware file
v3: no changes
v2: align continuation lines with the open parenthesis

 include/net/dsa.h |  3 +++
 net/dsa/devlink.c | 13 +++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 5d12191b6f6f5..1fcf4af6c5064 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -1176,6 +1176,9 @@ struct dsa_switch_ops {
 	int	(*devlink_info_get)(struct dsa_switch *ds,
 				    struct devlink_info_req *req,
 				    struct netlink_ext_ack *extack);
+	int	(*devlink_flash_update)(struct dsa_switch *ds,
+					struct devlink_flash_update_params *params,
+					struct netlink_ext_ack *extack);
 	int	(*devlink_sb_pool_get)(struct dsa_switch *ds,
 				       unsigned int sb_index, u16 pool_index,
 				       struct devlink_sb_pool_info *pool_info);
diff --git a/net/dsa/devlink.c b/net/dsa/devlink.c
index ed342f345692b..25311a87cbc56 100644
--- a/net/dsa/devlink.c
+++ b/net/dsa/devlink.c
@@ -20,6 +20,18 @@ static int dsa_devlink_info_get(struct devlink *dl,
 	return -EOPNOTSUPP;
 }
 
+static int dsa_devlink_flash_update(struct devlink *dl,
+				    struct devlink_flash_update_params *params,
+				    struct netlink_ext_ack *extack)
+{
+	struct dsa_switch *ds = dsa_devlink_to_ds(dl);
+
+	if (!ds->ops->devlink_flash_update)
+		return -EOPNOTSUPP;
+
+	return ds->ops->devlink_flash_update(ds, params, extack);
+}
+
 static int dsa_devlink_sb_pool_get(struct devlink *dl,
 				   unsigned int sb_index, u16 pool_index,
 				   struct devlink_sb_pool_info *pool_info)
@@ -169,6 +181,7 @@ dsa_devlink_sb_occ_tc_port_bind_get(struct devlink_port *dlp,
 
 static const struct devlink_ops dsa_devlink_ops = {
 	.info_get			= dsa_devlink_info_get,
+	.flash_update			= dsa_devlink_flash_update,
 	.sb_pool_get			= dsa_devlink_sb_pool_get,
 	.sb_pool_set			= dsa_devlink_sb_pool_set,
 	.sb_port_pool_get		= dsa_devlink_sb_port_pool_get,
-- 
2.56.0

Re: [PATCH net-next v19 1/6] net: dsa: add devlink flash_update callback to dsa_switch_ops

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] This patch gives every DSA switch a new devlink capability:
  firmware flash through the new `dsa_switch_ops::devlink_flash_update`…

--

net: dsa: add devlink flash_update callback to dsa_switch_ops

This adds a devlink_flash_update callback to dsa_switch_ops. It also
installs a dsa_devlink_flash_update() trampoline in dsa_devlink_ops,
which returns -EOPNOTSUPP when the driver does not implement the
callback. The commit message describes how flash requests behave on
switches without the callback, and the locking context the op is
called in.

> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 5d12191b6f6f5..1fcf4af6c5064 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -1176,6 +1176,9 @@ struct dsa_switch_ops {
>  	int	(*devlink_info_get)(struct dsa_switch *ds,
>  				    struct devlink_info_req *req,
>  				    struct netlink_ext_ack *extack);
> +	int	(*devlink_flash_update)(struct dsa_switch *ds,
> +					struct devlink_flash_update_params *params,
> +					struct netlink_ext_ack *extack);

[Severity: Low]
This isn't a bug, but should the Devlink section of
Documentation/networking/dsa/dsa.rst mention the new op?

That section currently says:

    DSA drivers can make use of the following devlink features:

and lists only Regions, Params, Resources and Shared buffers. It ends
with a pointer to Documentation/networking/devlink/.

The later patches in the series add only the driver-specific
Documentation/networking/devlink/mxl862xx.rst. They don't touch dsa.rst.

The calling context in the commit message is not written down anywhere
in the DSA docs either:

  - the devlink instance lock is held
  - rtnl_lock is not held
  - the devlink instance is registered in dsa_switch_setup(), before
    dsa_tree_setup_ports() creates the user ports

Would a short entry for flash update in that list be useful to driver
authors? It could also cover these constraints.

The list already left out devlink_info_get before this patch.

>  	int	(*devlink_sb_pool_get)(struct dsa_switch *ds,
>  				       unsigned int sb_index, u16 pool_index,
>  				       struct devlink_sb_pool_info *pool_info);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1791035885.git.daniel%40makrotopia.org