lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <20230530103400.3d0be965@kernel.org> Date: Tue, 30 May 2023 10:34:00 -0700 From: Jakub Kicinski <kuba@...nel.org> To: Jiri Pirko <jiri@...nulli.us> Cc: netdev@...r.kernel.org, pabeni@...hat.com, davem@...emloft.net, edumazet@...gle.com, leon@...nel.org, saeedm@...dia.com, moshe@...dia.com, jesse.brandeburg@...el.com, anthony.l.nguyen@...el.com, tariqt@...dia.com, idosch@...dia.com, petrm@...dia.com, simon.horman@...igine.com, ecree.xilinx@...il.com, habetsm.xilinx@...il.com, michal.wilczynski@...el.com, jacob.e.keller@...el.com Subject: Re: [patch net-next v2 14/15] devlink: move port_del() to devlink_port_ops On Tue, 30 May 2023 08:58:47 +0200 Jiri Pirko wrote: > >> .port_fn_hw_addr_get = mlx5_devlink_port_fn_hw_addr_get, > >> .port_fn_hw_addr_set = mlx5_devlink_port_fn_hw_addr_set, > >> .port_fn_roce_get = mlx5_devlink_port_fn_roce_get, > > > >Is it okay if we deferred the port_del() patch until there's some > >clear benefit? > > Well actually, there is a clear benefit even in this patchset: > > We have 2 flavours of ports each with different ops in mlx5: > VF: > static const struct devlink_port_ops mlx5_esw_dl_port_ops = { > .port_fn_hw_addr_get = mlx5_devlink_port_fn_hw_addr_get, > .port_fn_hw_addr_set = mlx5_devlink_port_fn_hw_addr_set, > .port_fn_roce_get = mlx5_devlink_port_fn_roce_get, > .port_fn_roce_set = mlx5_devlink_port_fn_roce_set, > .port_fn_migratable_get = mlx5_devlink_port_fn_migratable_get, > .port_fn_migratable_set = mlx5_devlink_port_fn_migratable_set, > }; > > SF: > static const struct devlink_port_ops mlx5_esw_dl_sf_port_ops = { > .port_del = mlx5_devlink_sf_port_del, > .port_fn_hw_addr_get = mlx5_devlink_port_fn_hw_addr_get, > .port_fn_hw_addr_set = mlx5_devlink_port_fn_hw_addr_set, > .port_fn_roce_get = mlx5_devlink_port_fn_roce_get, > .port_fn_roce_set = mlx5_devlink_port_fn_roce_set, > .port_fn_state_get = mlx5_devlink_sf_port_fn_state_get, > .port_fn_state_set = mlx5_devlink_sf_port_fn_state_set, > }; > > You can see that the port_del() op is supported only on the SF flavour. > VF does not support it and therefore port_del() is not defined on it. This is what I started thinking as well yesterday. Is there any reason to delete a port which isn't an SF? Similarly - is there any reason to delete a port which wasn't allocated via port_new? > Without this patch, I would have to have a helper > mlx5_devlink_port_del() that would check if the port is SF and call > mlx5_devlink_sf_port_del() in that case. This patch reduces the > boilerplate. ... Because if port_del can only happen on port_new'd ports, we should try to move that check into the core. It'd prevent misuse of the API. > Btw if you look at the cmd line api, it also aligns: > $ devlink port add pci/0000:08:00.0 flavour pcisf pfnum 0 sfnum 101 > pci/0000:08:00.0/32768: type eth netdev eth4 flavour pcisf controller 0 pfnum 0 sfnum 101 splittable false > function: > hw_addr 00:00:00:00:00:00 state inactive opstate detached > $ devlink port del pci/0000:08:00.0/32768 > > You use pci/0000:08:00.0/32768 as a delete handle. > > port_del() is basically an object destructor. Would it perhaps help to > rename is to .port_destructor()? That would somehow ease the asymmetry > :) IDK. I would leave the name as it is a and move to port_ops. Meh.
Powered by blists - more mailing lists