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

[Jakub Kicinski <kuba@kernel.org>]

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.