From: Jakub Kicinski <kuba@kernel.org>
To: Leon Romanovsky <leon@kernel.org>
Cc: "David S . Miller" <davem@davemloft.net>,
Leon Romanovsky <leonro@nvidia.com>,
Ido Schimmel <idosch@nvidia.com>, Ingo Molnar <mingo@redhat.com>,
Jiri Pirko <jiri@nvidia.com>,
linux-kernel@vger.kernel.org, linux-rdma@vger.kernel.org,
mlxsw@nvidia.com, Moshe Shemesh <moshe@nvidia.com>,
netdev@vger.kernel.org, Saeed Mahameed <saeedm@nvidia.com>,
Salil Mehta <salil.mehta@huawei.com>,
Shay Drory <shayd@nvidia.com>,
Steven Rostedt <rostedt@goodmis.org>,
Tariq Toukan <tariqt@nvidia.com>,
Yisen Zhuang <yisen.zhuang@huawei.com>
Subject: Re: [PATCH net-next v2 3/5] devlink: Allow set specific ops callbacks dynamically
Date: Mon, 4 Oct 2021 16:44:13 -0700 [thread overview]
Message-ID: <20211004164413.60e9ce80@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> (raw)
In-Reply-To: <92971648bcad41d095d12f5296246fc44ab8f5c7.1633284302.git.leonro@nvidia.com>
On Sun, 3 Oct 2021 21:12:04 +0300 Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
>
> Introduce new devlink call to set specific ops callback during
> device initialization phase after devlink_alloc() is already
> called.
>
> This allows us to set specific ops based on device property which
> is not known at the beginning of driver initialization.
>
> For the sake of simplicity, this API lacks any type of locking and
> needs to be called before devlink_register() to make sure that no
> parallel access to the ops is possible at this stage.
The fact that it's not registered does not mean that the callbacks
won't be invoked. Look at uses of devlink_compat_flash_update().
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index 4e484afeadea..25c2aa2b35cd 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -53,7 +53,7 @@ struct devlink {
> struct list_head trap_list;
> struct list_head trap_group_list;
> struct list_head trap_policer_list;
> - const struct devlink_ops *ops;
> + struct devlink_ops ops;
Security people like ops to live in read-only memory. You're making
them r/w for every devlink instance now.
> struct xarray snapshot_ids;
> struct devlink_dev_stats stats;
> struct device *dev;
> +/**
> + * devlink_set_ops - Set devlink ops dynamically
> + *
> + * @devlink: devlink
> + * @ops: devlink ops to set
> + *
> + * This interface allows us to set ops based on device property
> + * which is known after devlink_alloc() was already called.
> + *
> + * This call sets fields that are not initialized yet and ignores
> + * already set fields.
> + *
> + * It should be called before devlink_register(), so doesn't have any
> + * protection from concurent access.
> + */
> +void devlink_set_ops(struct devlink *devlink, const struct devlink_ops *ops)
> +{
> + struct devlink_ops *dev_ops = &devlink->ops;
> +
> + WARN_ON(!devlink_reload_actions_valid(ops));
> + ASSERT_DEVLINK_NOT_REGISTERED(devlink);
> +
> +#define SET_DEVICE_OP(ptr, op, name) \
> + do { \
> + if ((op)->name) \
> + if (!((ptr)->name)) \
> + (ptr)->name = (op)->name; \
> + } while (0)
> +
> + /* Keep sorte alphabetically for readability */
> + SET_DEVICE_OP(dev_ops, ops, eswitch_encap_mode_get);
> + SET_DEVICE_OP(dev_ops, ops, eswitch_encap_mode_set);
> + SET_DEVICE_OP(dev_ops, ops, eswitch_inline_mode_get);
> + SET_DEVICE_OP(dev_ops, ops, eswitch_inline_mode_set);
> + SET_DEVICE_OP(dev_ops, ops, eswitch_mode_get);
> + SET_DEVICE_OP(dev_ops, ops, eswitch_mode_set);
> + SET_DEVICE_OP(dev_ops, ops, flash_update);
> + SET_DEVICE_OP(dev_ops, ops, info_get);
> + SET_DEVICE_OP(dev_ops, ops, port_del);
> + SET_DEVICE_OP(dev_ops, ops, port_fn_state_get);
> + SET_DEVICE_OP(dev_ops, ops, port_fn_state_set);
> + SET_DEVICE_OP(dev_ops, ops, port_function_hw_addr_get);
> + SET_DEVICE_OP(dev_ops, ops, port_function_hw_addr_set);
> + SET_DEVICE_OP(dev_ops, ops, port_new);
> + SET_DEVICE_OP(dev_ops, ops, port_split);
> + SET_DEVICE_OP(dev_ops, ops, port_type_set);
> + SET_DEVICE_OP(dev_ops, ops, port_unsplit);
> + SET_DEVICE_OP(dev_ops, ops, rate_leaf_parent_set);
> + SET_DEVICE_OP(dev_ops, ops, rate_leaf_tx_max_set);
> + SET_DEVICE_OP(dev_ops, ops, rate_leaf_tx_share_set);
> + SET_DEVICE_OP(dev_ops, ops, rate_node_del);
> + SET_DEVICE_OP(dev_ops, ops, rate_node_new);
> + SET_DEVICE_OP(dev_ops, ops, rate_node_parent_set);
> + SET_DEVICE_OP(dev_ops, ops, rate_node_tx_max_set);
> + SET_DEVICE_OP(dev_ops, ops, rate_node_tx_share_set);
> + SET_DEVICE_OP(dev_ops, ops, reload_actions);
> + SET_DEVICE_OP(dev_ops, ops, reload_down);
> + SET_DEVICE_OP(dev_ops, ops, reload_limits);
> + SET_DEVICE_OP(dev_ops, ops, reload_up);
> + SET_DEVICE_OP(dev_ops, ops, sb_occ_max_clear);
> + SET_DEVICE_OP(dev_ops, ops, sb_occ_port_pool_get);
> + SET_DEVICE_OP(dev_ops, ops, sb_occ_snapshot);
> + SET_DEVICE_OP(dev_ops, ops, sb_occ_tc_port_bind_get);
> + SET_DEVICE_OP(dev_ops, ops, sb_pool_get);
> + SET_DEVICE_OP(dev_ops, ops, sb_pool_set);
> + SET_DEVICE_OP(dev_ops, ops, sb_port_pool_get);
> + SET_DEVICE_OP(dev_ops, ops, sb_port_pool_set);
> + SET_DEVICE_OP(dev_ops, ops, sb_tc_pool_bind_get);
> + SET_DEVICE_OP(dev_ops, ops, sb_tc_pool_bind_set);
> + SET_DEVICE_OP(dev_ops, ops, supported_flash_update_params);
> + SET_DEVICE_OP(dev_ops, ops, trap_action_set);
> + SET_DEVICE_OP(dev_ops, ops, trap_drop_counter_get);
> + SET_DEVICE_OP(dev_ops, ops, trap_fini);
> + SET_DEVICE_OP(dev_ops, ops, trap_group_action_set);
> + SET_DEVICE_OP(dev_ops, ops, trap_group_init);
> + SET_DEVICE_OP(dev_ops, ops, trap_group_set);
> + SET_DEVICE_OP(dev_ops, ops, trap_init);
> + SET_DEVICE_OP(dev_ops, ops, trap_policer_counter_get);
> + SET_DEVICE_OP(dev_ops, ops, trap_policer_fini);
> + SET_DEVICE_OP(dev_ops, ops, trap_policer_init);
> + SET_DEVICE_OP(dev_ops, ops, trap_policer_set);
> +
> +#undef SET_DEVICE_OP
> +}
> +EXPORT_SYMBOL_GPL(devlink_set_ops);
I still don't like this. IMO using feature bits to dynamically mask-off
capabilities has much better properties. We already have static caps
in devlink_ops (first 3 members), we should build on top of that.
next prev parent reply other threads:[~2021-10-04 23:44 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-03 18:12 [PATCH net-next v2 0/5] devlink reload simplification Leon Romanovsky
2021-10-03 18:12 ` [PATCH net-next v2 1/5] devlink: Reduce struct devlink exposure Leon Romanovsky
2021-10-04 23:38 ` Jakub Kicinski
2021-10-05 6:13 ` Leon Romanovsky
2021-10-03 18:12 ` [PATCH net-next v2 2/5] devlink: Annotate devlink API calls Leon Romanovsky
2021-10-03 18:12 ` [PATCH net-next v2 3/5] devlink: Allow set specific ops callbacks dynamically Leon Romanovsky
2021-10-04 23:44 ` Jakub Kicinski [this message]
2021-10-05 7:32 ` Leon Romanovsky
2021-10-05 18:32 ` Jakub Kicinski
2021-10-05 19:15 ` Leon Romanovsky
2021-10-06 0:39 ` Jakub Kicinski
2021-10-06 3:37 ` Leon Romanovsky
2021-10-06 13:35 ` Jakub Kicinski
2021-10-06 14:48 ` Leon Romanovsky
2021-10-03 18:12 ` [PATCH net-next v2 4/5] net/mlx5: Register separate reload devlink ops for multiport device Leon Romanovsky
2021-10-03 18:12 ` [PATCH net-next v2 5/5] devlink: Delete reload enable/disable interface Leon Romanovsky
2021-10-04 14:19 ` Ido Schimmel
2021-10-04 15:45 ` Leon Romanovsky
2021-10-04 16:54 ` Ido Schimmel
2021-10-04 19:02 ` Leon Romanovsky
2021-10-05 6:10 ` Ido Schimmel
2021-10-05 7:40 ` Leon Romanovsky
2021-10-05 8:18 ` Ido Schimmel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20211004164413.60e9ce80@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com \
--to=kuba@kernel.org \
--cc=davem@davemloft.net \
--cc=idosch@nvidia.com \
--cc=jiri@nvidia.com \
--cc=leon@kernel.org \
--cc=leonro@nvidia.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rdma@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=mlxsw@nvidia.com \
--cc=moshe@nvidia.com \
--cc=netdev@vger.kernel.org \
--cc=rostedt@goodmis.org \
--cc=saeedm@nvidia.com \
--cc=salil.mehta@huawei.com \
--cc=shayd@nvidia.com \
--cc=tariqt@nvidia.com \
--cc=yisen.zhuang@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).