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

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, 4 Mar 2019 15:32:10 -0800
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     davem@...emloft.net, netdev@...r.kernel.org,
        oss-drivers@...ronome.com
Subject: Re: [PATCH net-next v2 3/7] nfp: register devlink ports of all
 reprs

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.