Re: [PATCH net-next v15 1/6] net: dsa: add devlink flash_update callback to dsa_switch_ops
From: netdev-bot+sashiko@kernel.org
Date: 2026-09-14 18:34:53
Also in:
driver-core, lkml, netdev
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: Critical: 0 · High: 0 · Medium: 0 · Low: 1 - [Low] net/dsa/devlink.c 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 devlink dev flash requests to the driver. The trampoline is installed unconditionally in dsa_devlink_ops and returns -EOPNOTSUPP when the driver does not implement the callback.
quoted hunk
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); +} +
[ ... ]
quoted hunk
@@ -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: Low]
Since dsa_devlink_ops is shared by every DSA switch, does this make
devlink->ops->flash_update non-NULL even for the majority of drivers that
do not set ds->ops->devlink_flash_update?
The devlink core uses that pointer as the capability gate for the whole
feature, in devlink_nl_flash_update_doit():
if (!devlink->ops->flash_update)
return -EOPNOTSUPP;
...
ret = request_firmware_direct(¶ms.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, ¶ms, info->extack);
devlink_flash_update_end_notify(devlink);
With that early return now unreachable for DSA, two behaviour changes seem
to follow for switches that do not implement the callback:
- devlink dev flash with a missing or unreadable file returns -ENOENT with
the extack "failed to locate the requested firmware file" instead of the
previous -EOPNOTSUPP, which hides the fact that the device cannot be
flashed at all.
- a DEVLINK_CMD_FLASH_UPDATE / DEVLINK_CMD_FLASH_UPDATE_END notification
pair is multicast for a device where no flash takes place.
Does the same apply to the legacy ioctl path? DSA user ports have no
ethtool_ops->flash_device, so ethtool_flash_device() falls back to the
attached devlink instance:
if (!dev->ethtool_ops->flash_device) {
req->devlink = netdev_to_devlink_get(dev);
return 0;
}
and devlink_compat_flash_update() carries the same gate that is now dead
for DSA:
if (!devlink->ops->flash_update) {
ret = -EOPNOTSUPP;
goto out_unlock;
}
so ethtool -f swpN file would read the file and emit the begin/end
notifications before failing.
There is also devlink_flash_component_get(), which infers component-update
support from ops->info_get being non-NULL; DSA's info_get trampoline is
installed unconditionally too, so a component request on a switch without
either callback now gets past that check as well.
The commit message covers part of this:
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.
Could the errno change from -EOPNOTSUPP to -ENOENT and the emitted
FLASH_UPDATE/FLASH_UPDATE_END notifications be mentioned there as well, or
alternatively the ops table selected based on
ds->ops->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,
-- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1789175618.git.daniel%40makrotopia.org