Re: [PATCH net-next v2 12/12] nfp: add VF and PF representors to flower app

[Simon Horman <simon.horman@netronome.com>]

On Thu, Jun 22, 2017 at 05:50:29PM +0300, Or Gerlitz wrote:
> On Wed, Jun 21, 2017 at 12:36 AM, Simon Horman
> <simon.horman@netronome.com> wrote:
> > Initialise VF and PF representors in flower app.
> >
> > Based in part on work by Benjamin LaHaise, Bert van Leeuwen and
> > Jakub Kicinski.
> >
> > Signed-off-by: Simon Horman <simon.horman@netronome.com>
> > Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> > ---
> >  drivers/net/ethernet/netronome/nfp/flower/main.c | 86 +++++++++++++++++++++++-
> >  1 file changed, 84 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/netronome/nfp/flower/main.c b/drivers/net/ethernet/netronome/nfp/flower/main.c
> > index d1d905727c54..582b7be3e219 100644
> > --- a/drivers/net/ethernet/netronome/nfp/flower/main.c
> > +++ b/drivers/net/ethernet/netronome/nfp/flower/main.c
> > @@ -149,15 +149,81 @@ static const struct net_device_ops nfp_flower_repr_netdev_ops = {
> >         .ndo_get_offload_stats  = nfp_repr_get_offload_stats,
> >  };
> >
> > +static void nfp_flower_sriov_disable(struct nfp_app *app)
> > +{
> > +       nfp_reprs_clean_and_free_by_type(app, NFP_REPR_TYPE_VF);
> > +}
> > +
> > +static int
> > +nfp_flower_spawn_vnic_reprs(struct nfp_app *app,
> > +                           enum nfp_flower_cmsg_port_vnic_type vnic_type,
> > +                           enum nfp_repr_type repr_type, unsigned int cnt)
> > +{
> > +       u8 nfp_pcie = nfp_cppcore_pcie_unit(app->pf->cpp);
> > +       struct nfp_flower_priv *priv = app->priv;
> > +       struct nfp_reprs *reprs, *old_reprs;
> > +       const u8 queue = 0;
> > +       int i, err;
> > +
> > +       reprs = nfp_reprs_alloc(cnt);
> > +       if (!reprs)
> > +               return -ENOMEM;
> > +
> > +       for (i = 0; i < cnt; i++) {
> > +               u32 port_id;
> > +
> > +               reprs->reprs[i] = nfp_repr_alloc(app);
> > +               if (!reprs->reprs[i]) {
> > +                       err = -ENOMEM;
> > +                       goto err_reprs_clean;
> > +               }
> > +
> > +               SET_NETDEV_DEV(reprs->reprs[i], &priv->nn->pdev->dev);
> 
> why? these are virtual devices, in the same manner that on para virt use case,
> tap devices are. Why we want them all to be linked to the PF PCI entry?
> 
> We had that on our code and removed it before upstreaming b/c it made
> provisioning
> systems (open-stack) to get crazy and didn't provide any benefit.
> 
> I would vote -1 for this line, suggest to remove it and see later
> if/why you need that.

Sure, I will drop this line. We can revisit this later.

> > +               eth_hw_addr_inherit(reprs->reprs[i], priv->nn->dp.netdev);
> 
> -1 vote here too.. having all your reps to carry the PF mac would
> create confusion for provisioning
> systems, DHCP daemons and such. We are following the para virt way and
> set random
> mac on the rep, which would be later changed by libvirt as done for tap device

Thanks, I will follow your example and use random mac addresses.