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

[netdev-bot+sashiko@kernel.org]

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