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: Wed, 31 May 2023 08:33:52 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Jakub Kicinski <kuba@...nel.org>
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

Tue, May 30, 2023 at 07:34:00PM CEST, kuba@...nel.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.


>
>> 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.

Yeah :)