netdev - Re: [patch net-next v2 14/15] devlink: move port_del() to devlink_port

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

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.