On Mon, 4 Mar 2019 08:36:31 +0100, Jiri Pirko wrote:
> >> >+ case NFP_PORT_PF_PORT:
> >> >+ return devlink_port_register(devlink, &port->dl_port,
> >> >+ (port->pf_id + 1) * 10000 +
> >> >+ port->pf_split_id * 1000);
> >>
> >> Wait. What this 10000/1000 magic about?
> >
> >port_index has to be unique, I need some unique number here, as I
> >stated both in the commit message and the cover letter, this is
> >arbitrary.
>
> You can at least use some defines for that.
Ok.
> >I can put the datapath port identifier in there but its (a)
> >meaningless, (b) a bitfield, so it will look like 8972367083. And it
> >may change depending on the FW load, so its not stable either.
> >> > void nfp_devlink_port_unregister(struct nfp_port *port)
> >> >diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
> >> >b/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
> >> >index d2c803bb4e56..869d22760a6e 100644
> >> >--- a/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
> >> >+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_repr.c
> >> >@@ -395,12 +397,24 @@ int nfp_repr_init(struct nfp_app *app, struct
> >> >net_device *netdev,
> >> > if (err)
> >> > goto err_clean;
> >> >
> >> >- err = register_netdev(netdev);
> >> >+ err = nfp_devlink_port_init(app, repr->port);
> >> > if (err)
> >> > goto err_repr_clean;
> >> >
> >> >+ err = register_netdev(netdev);
> >> >+ if (err)
> >> >+ goto err_port_clean;
> >> >+
> >> >+ err = nfp_devlink_port_register(app, repr->port);
> >>
> >> Don't you want to take my patch ("nfp: register devlink port before
> >> netdev") to change order of register_netdev and devlink_port_register,
> >> include it to this patchset before this patch and change the order in
> >> this patch too? I think it would be clearer to do it from the beginning.
> >
> >This way both netdev and devlink_port can get registered fully
> >initialized. Otherwise we'd get two notifications. Are we trying to
> >establish some ordering rules to get around the rtnl locking? :)
>
> The order of devlink_port_register and register_netdev is given by
> layering. For example, for port change, the devlink_port is still there
> and registered, only the netdev is unregistered and ib_dev registered
> instead of vice versa. It has really no relation to rtnl locking.
Ok, I shouldn't worry about the notifications too much, I agree the
order you suggests makes sense in principal.
