netdev - Re: [PATCH net-next v6 0/5] devlink: Introduce PCI PF, VF ports and attributes

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:   Mon, 8 Jul 2019 22:40:12 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Parav Pandit <parav@...lanox.com>
Cc:     netdev@...r.kernel.org, jiri@...lanox.com, saeedm@...lanox.com
Subject: Re: [PATCH net-next v6 0/5] devlink: Introduce PCI PF, VF ports and
 attributes

On Mon,  8 Jul 2019 23:17:34 -0500, Parav Pandit wrote:
> This patchset carry forwards the work initiated in [1] and discussion
> futher concluded at [2].
> 
> To improve visibility of representor netdevice, its association with
> PF or VF, physical port, two new devlink port flavours are added as
> PCI PF and PCI VF ports.
> 
> A sample eswitch view can be seen below, which will be futher extended to
> mdev subdevices of a PCI function in future.
> 
> Patch-1 moves physical port's attribute to new structure
> Patch-2 enhances netlink response to consider port flavour
> Patch-3,4 extends devlink port attributes and port flavour
> Patch-5 extends mlx5 driver to register devlink ports for PF, VF and
> physical link.

The coding leaves something to be desired:

1) flavour handling in devlink_nl_port_attrs_put() really calls for a
   switch statement,
2) devlink_port_attrs_.*set() can take a pointer to flavour specific
   structure instead of attr structure for setting the parameters,
3) the "ret" variable there is unnecessary,
4) there is inconsistency in whether there is an empty line between
   if (ret) return; after __devlink_port_attrs_set() and attr setting,
5) /* Associated PCI VF for of the PCI PF for this port. */ doesn't
   read great;
6) mlx5 functions should preferably have an appropriate prefix - f.e.
   register_devlink_port() or is_devlink_port_supported().

But I'll leave it to Jiri and Dave to decide if its worth a respin :)
Functionally I think this is okay.

Reviewed-by: Jakub Kicinski <jakub.kicinski@...ronome.com>