Re: [PATCH net-next v2 3/7] nfp: register devlink ports of all reprs

From: Jakub Kicinski <jakub.kicinski@netronome.com>
To: Jiri Pirko <jiri@resnulli.us>
Cc: davem@davemloft.net, netdev@vger.kernel.org, oss-drivers@netronome.com
Subject: Re: [PATCH net-next v2 3/7] nfp: register devlink ports of all reprs
Date: Mon, 4 Mar 2019 15:32:10 -0800	[thread overview]
Message-ID: <20190304153210.5fc4f9af@cakuba.netronome.com> (raw)
In-Reply-To: <20190304073631.GU2314@nanopsycho>

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.