Hi Jakub, > -----Original Message----- > From: Jakub Kicinski <jakub.kicin...@netronome.com> > Sent: Tuesday, July 9, 2019 11:10 AM > To: Parav Pandit <pa...@mellanox.com> > Cc: netdev@vger.kernel.org; Jiri Pirko <j...@mellanox.com>; Saeed Mahameed > <sae...@mellanox.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(). > Those two static helper functions doesn't need mlx5_ prefix. ndo ops are prefixed appropriately.
> 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.kicin...@netronome.com>
