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

[Jacob Keller <jacob.e.keller@intel.com>]

On 5/30/2023 11:33 PM, Jiri Pirko wrote:
> Tue, May 30, 2023 at 07:34:00PM CEST, kuba@kernel.org wrote:
>> 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?
> 
> Good question. Not possible atm. For SR-IOV VFs it probably does not make
> sense as there are PCI sysfs knobs to do that. Not sure about SIOV.
> 
> 

At least for ice, the current plan for Scalable IOV I had was creating
ports via port_new, so port_del makes sense there. I don't know how else
others were planning on doing this. We're a bit delayed on being able to
post actual patches just yet though :(

>>
>>> 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.
> 
> Okay, that is fair. Will make this change now. If the future brings
> different needs, easy to change.
> 
> 
>>
>>> 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.
> 

i like thinking about it in terms of destructor, but that sort of
implies its called whenever the port is removed (i.e. even if removed by
the driver via something like devlink_port_unregister or whatever its
called).

> Yeah :)
>