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

Re: [PATCH net-next v14 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 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] net/dsa/devlink.c now installs `.flash_update =
  dsa_devlink_flash_update` unconditionally in the single shared…

--

net: dsa: add devlink flash_update callback to dsa_switch_ops

This adds a devlink_flash_update callback to dsa_switch_ops and a
trampoline in net/dsa/devlink.c that forwards to it, returning
-EOPNOTSUPP when the driver does not implement the callback.  The
trampoline is installed unconditionally in the shared dsa_devlink_ops.

(No Link: tags in the commit header.)

> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 7507d632e7c6d..9babab92a0fc6 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -1174,6 +1174,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,

[Severity: Medium]
Since dsa_devlink_ops is a single const structure shared by every DSA
switch, does installing .flash_update unconditionally make the devlink
core's capability probe always succeed for DSA?

The core uses the presence of the op as the gate, in
devlink_nl_flash_update_doit():

	if (!devlink->ops->flash_update)
		return -EOPNOTSUPP;

With the trampoline always present, that check can no longer fire for
any DSA switch, and the -EOPNOTSUPP from dsa_devlink_flash_update() is
only reached after the core has already done work:

net/devlink/dev.c:devlink_nl_flash_update_doit() {
	...
	ret = request_firmware_direct(&params.fw, file_name, devlink->dev);
	if (ret) {
		NL_SET_ERR_MSG_ATTR(info->extack, nla_file_name,
				    "failed to locate the requested firmware file");
		return ret;
	}

	devlink_flash_update_begin_notify(devlink);
	ret = devlink->ops->flash_update(devlink, &params, info->extack);
	devlink_flash_update_end_notify(devlink);
	...
}

The commit message covers the firmware fetch:

    the devlink core will then have fetched the firmware file from
    userspace before the request fails, which is acceptable for an
    operation as infrequent as a firmware update.

Are the other two effects of that ordering intended as well?  A
FLASH_UPDATE / FLASH_UPDATE_END notification pair is now broadcast to
monitors for a flash that never reached any hardware, and if the named
file does not exist the caller gets -ENOENT with the extack "failed to
locate the requested firmware file" rather than -EOPNOTSUPP, which hides
the fact that the switch cannot be flashed at all.

There is also a second entry point that the commit message does not
mention.  DSA user ports have no ethtool_ops->flash_device but do have a
devlink port, so ETHTOOL_FLASHDEV falls through:

net/ethtool/ioctl.c:ethtool_flash_device() {
	if (!dev->ethtool_ops->flash_device) {
		req->devlink = netdev_to_devlink_get(dev);
		return 0;
	}
	...
}

which lands in devlink_compat_flash_update(), where the same gate is now
always passed and the op is called with extack == NULL:

net/devlink/dev.c:devlink_compat_flash_update() {
	...
	if (!devlink->ops->flash_update) {
		ret = -EOPNOTSUPP;
		goto out_unlock;
	}

	ret = request_firmware_direct(&params.fw, file_name, devlink->dev);
	...
	ret = devlink->ops->flash_update(devlink, &params, NULL);
	...
}

Should any extack message a DSA driver sets be documented as dropped on
that path?

Would gating the assignment of the .flash_update pointer on
ds->ops->devlink_flash_update (for instance a second devlink_ops variant
selected at registration) keep the core's probe truthful?  The existing
trampolines this one follows (info_get, sb_*) are pure queries with no
core-side side effects before the op is called, so the -EOPNOTSUPP-from-
the-trampoline convention does not carry the same cost there.

For context on the series: at the end of the range mxl862xx does
implement devlink_flash_update, so that driver has a real
implementation, but all other DSA drivers (b53, mv88e6xxx, microchip,
ocelot, sja1105, ...) still advertise flash support without one.

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

[PATCH net-next v14 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 will then have fetched the firmware file from
userspace before the request fails, which is 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, whereas DSA serialises its switch and port ops
under rtnl_lock, so a driver has to serialise a flash against its own
ops itself.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
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 7507d632e7c6..9babab92a0fc 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -1174,6 +1174,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 ed342f345692..25311a87cbc5 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.55.0