diff mbox series

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

Message ID 20190301180453.17778-4-jakub.kicinski@netronome.com
State Changes Requested
Delegated to: David Miller
Headers show
Series devlink: expose PF and VF representors as ports | expand

Commit Message

Jakub Kicinski March 1, 2019, 6:04 p.m. UTC
Register all representors as devlink ports.

The port_index is slightly tricky to figure out, we use a bit of
arbitrary math to create unique IDs for PCI ports.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 .../net/ethernet/netronome/nfp/nfp_devlink.c  | 40 ++++++++++++++++++-
 .../net/ethernet/netronome/nfp/nfp_net_repr.c | 16 +++++++-
 2 files changed, 53 insertions(+), 3 deletions(-)

Comments

Jiri Pirko March 2, 2019, 8:43 a.m. UTC | #1
Fri, Mar 01, 2019 at 07:04:49PM CET, jakub.kicinski@netronome.com wrote:
>Register all representors as devlink ports.
>
>The port_index is slightly tricky to figure out, we use a bit of
>arbitrary math to create unique IDs for PCI ports.
>
>Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>---
> .../net/ethernet/netronome/nfp/nfp_devlink.c  | 40 ++++++++++++++++++-
> .../net/ethernet/netronome/nfp/nfp_net_repr.c | 16 +++++++-
> 2 files changed, 53 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
>index 9af3cb1f2f17..bf7fd9614152 100644
>--- a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
>+++ b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
>@@ -350,7 +350,8 @@ const struct devlink_ops nfp_devlink_ops = {
> 	.flash_update		= nfp_devlink_flash_update,
> };
> 
>-int nfp_devlink_port_init(struct nfp_app *app, struct nfp_port *port)
>+static int
>+nfp_devlink_port_init_phys(struct devlink *devlink, struct nfp_port *port)
> {
> 	struct nfp_eth_table_port eth_port;
> 	int ret;
>@@ -368,6 +369,27 @@ int nfp_devlink_port_init(struct nfp_app *app, struct nfp_port *port)
> 	return 0;
> }
> 
>+int nfp_devlink_port_init(struct nfp_app *app, struct nfp_port *port)
>+{
>+	struct devlink *devlink = priv_to_devlink(app->pf);
>+
>+	switch (port->type) {
>+	case NFP_PORT_PHYS_PORT:
>+		return nfp_devlink_port_init_phys(devlink, port);
>+	case NFP_PORT_PF_PORT:
>+		devlink_port_type_eth_set(&port->dl_port, port->netdev);
>+		devlink_port_attrs_pci_pf_set(&port->dl_port, port->pf_id);
>+		return 0;
>+	case NFP_PORT_VF_PORT:
>+		devlink_port_type_eth_set(&port->dl_port, port->netdev);
>+		devlink_port_attrs_pci_vf_set(&port->dl_port, port->pf_id,
>+					      port->vf_id);

What is the reason to expose vf/pf id for switch port? Isn't it rather
an attribute of a peer?


>+		return 0;
>+	default:
>+		return -EINVAL;
>+	}
>+}
>+
> void nfp_devlink_port_clean(struct nfp_port *port)
> {
> }
>@@ -376,7 +398,21 @@ int nfp_devlink_port_register(struct nfp_app *app, struct nfp_port *port)
> {
> 	struct devlink *devlink = priv_to_devlink(app->pf);
> 
>-	return devlink_port_register(devlink, &port->dl_port, port->eth_id);
>+	switch (port->type) {
>+	case NFP_PORT_PHYS_PORT:
>+		return devlink_port_register(devlink, &port->dl_port,
>+					     port->eth_id);
>+	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?


>+	case NFP_PORT_VF_PORT:
>+		return devlink_port_register(devlink, &port->dl_port,
>+					     (port->pf_id + 1) * 10000 +
>+					     port->vf_id + 1);
>+	default:
>+		return -EINVAL;
>+	}
> }
> 
> 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
>@@ -292,7 +292,9 @@ nfp_repr_transfer_features(struct net_device *netdev, struct net_device *lower)
> 
> static void nfp_repr_clean(struct nfp_repr *repr)
> {
>+	nfp_devlink_port_unregister(repr->port);
> 	unregister_netdev(repr->netdev);
>+	nfp_devlink_port_clean(repr->port);
> 	nfp_app_repr_clean(repr->app, repr->netdev);
> 	dst_release((struct dst_entry *)repr->dst);
> 	nfp_port_free(repr->port);
>@@ -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.


>+	if (err)
>+		goto err_unreg_netdev;
>+
> 	return 0;
> 
>+err_unreg_netdev:
>+	unregister_netdev(repr->netdev);
>+err_port_clean:
>+	nfp_devlink_port_clean(repr->port);
> err_repr_clean:
> 	nfp_app_repr_clean(app, netdev);
> err_clean:
>-- 
>2.19.2
>
Jakub Kicinski March 2, 2019, 7:07 p.m. UTC | #2
On Sat, 2 Mar 2019 09:43:47 +0100, Jiri Pirko wrote:
> Fri, Mar 01, 2019 at 07:04:49PM CET, jakub.kicinski@netronome.com wrote:
> >Register all representors as devlink ports.
> >
> >The port_index is slightly tricky to figure out, we use a bit of
> >arbitrary math to create unique IDs for PCI ports.
> >
> >Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> >---
> > .../net/ethernet/netronome/nfp/nfp_devlink.c  | 40 ++++++++++++++++++-
> > .../net/ethernet/netronome/nfp/nfp_net_repr.c | 16 +++++++-
> > 2 files changed, 53 insertions(+), 3 deletions(-)
> >
> >diff --git a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
> >index 9af3cb1f2f17..bf7fd9614152 100644
> >--- a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
> >+++ b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
> >@@ -350,7 +350,8 @@ const struct devlink_ops nfp_devlink_ops = {
> > 	.flash_update		= nfp_devlink_flash_update,
> > };
> > 
> >-int nfp_devlink_port_init(struct nfp_app *app, struct nfp_port *port)
> >+static int
> >+nfp_devlink_port_init_phys(struct devlink *devlink, struct nfp_port *port)
> > {
> > 	struct nfp_eth_table_port eth_port;
> > 	int ret;
> >@@ -368,6 +369,27 @@ int nfp_devlink_port_init(struct nfp_app *app, struct nfp_port *port)
> > 	return 0;
> > }
> > 
> >+int nfp_devlink_port_init(struct nfp_app *app, struct nfp_port *port)
> >+{
> >+	struct devlink *devlink = priv_to_devlink(app->pf);
> >+
> >+	switch (port->type) {
> >+	case NFP_PORT_PHYS_PORT:
> >+		return nfp_devlink_port_init_phys(devlink, port);
> >+	case NFP_PORT_PF_PORT:
> >+		devlink_port_type_eth_set(&port->dl_port, port->netdev);
> >+		devlink_port_attrs_pci_pf_set(&port->dl_port, port->pf_id);
> >+		return 0;
> >+	case NFP_PORT_VF_PORT:
> >+		devlink_port_type_eth_set(&port->dl_port, port->netdev);
> >+		devlink_port_attrs_pci_vf_set(&port->dl_port, port->pf_id,
> >+					      port->vf_id);  
> 
> What is the reason to expose vf/pf id for switch port? Isn't it rather
> an attribute of a peer?

Naw, its an attribute of the port.  I leave the ASIC via PF n or VF m
of PF n.  Whatever is on the other side is isolated from the topology
of the ASIC.

Is the physical port ID an attribute of the other end of the cable?

> >+		return 0;
> >+	default:
> >+		return -EINVAL;
> >+	}
> >+}
> >+
> > void nfp_devlink_port_clean(struct nfp_port *port)
> > {
> > }
> >@@ -376,7 +398,21 @@ int nfp_devlink_port_register(struct nfp_app *app, struct nfp_port *port)
> > {
> > 	struct devlink *devlink = priv_to_devlink(app->pf);
> > 
> >-	return devlink_port_register(devlink, &port->dl_port, port->eth_id);
> >+	switch (port->type) {
> >+	case NFP_PORT_PHYS_PORT:
> >+		return devlink_port_register(devlink, &port->dl_port,
> >+					     port->eth_id);
> >+	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. 

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.

> >+	case NFP_PORT_VF_PORT:
> >+		return devlink_port_register(devlink, &port->dl_port,
> >+					     (port->pf_id + 1) * 10000 +
> >+					     port->vf_id + 1);
> >+	default:
> >+		return -EINVAL;
> >+	}
> > }
> > 
> > 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
> >@@ -292,7 +292,9 @@ nfp_repr_transfer_features(struct net_device *netdev, struct net_device *lower)
> > 
> > static void nfp_repr_clean(struct nfp_repr *repr)
> > {
> >+	nfp_devlink_port_unregister(repr->port);
> > 	unregister_netdev(repr->netdev);
> >+	nfp_devlink_port_clean(repr->port);
> > 	nfp_app_repr_clean(repr->app, repr->netdev);
> > 	dst_release((struct dst_entry *)repr->dst);
> > 	nfp_port_free(repr->port);
> >@@ -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? :)

> >+	if (err)
> >+		goto err_unreg_netdev;
> >+
> > 	return 0;
> > 
> >+err_unreg_netdev:
> >+	unregister_netdev(repr->netdev);
> >+err_port_clean:
> >+	nfp_devlink_port_clean(repr->port);
> > err_repr_clean:
> > 	nfp_app_repr_clean(app, netdev);
> > err_clean:
Jiri Pirko March 4, 2019, 7:36 a.m. UTC | #3
Sat, Mar 02, 2019 at 08:07:24PM CET, jakub.kicinski@netronome.com wrote:
>On Sat, 2 Mar 2019 09:43:47 +0100, Jiri Pirko wrote:
>> Fri, Mar 01, 2019 at 07:04:49PM CET, jakub.kicinski@netronome.com wrote:
>> >Register all representors as devlink ports.
>> >
>> >The port_index is slightly tricky to figure out, we use a bit of
>> >arbitrary math to create unique IDs for PCI ports.
>> >
>> >Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>> >---
>> > .../net/ethernet/netronome/nfp/nfp_devlink.c  | 40 ++++++++++++++++++-
>> > .../net/ethernet/netronome/nfp/nfp_net_repr.c | 16 +++++++-
>> > 2 files changed, 53 insertions(+), 3 deletions(-)
>> >
>> >diff --git a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
>> >index 9af3cb1f2f17..bf7fd9614152 100644
>> >--- a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
>> >+++ b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
>> >@@ -350,7 +350,8 @@ const struct devlink_ops nfp_devlink_ops = {
>> > 	.flash_update		= nfp_devlink_flash_update,
>> > };
>> > 
>> >-int nfp_devlink_port_init(struct nfp_app *app, struct nfp_port *port)
>> >+static int
>> >+nfp_devlink_port_init_phys(struct devlink *devlink, struct nfp_port *port)
>> > {
>> > 	struct nfp_eth_table_port eth_port;
>> > 	int ret;
>> >@@ -368,6 +369,27 @@ int nfp_devlink_port_init(struct nfp_app *app, struct nfp_port *port)
>> > 	return 0;
>> > }
>> > 
>> >+int nfp_devlink_port_init(struct nfp_app *app, struct nfp_port *port)
>> >+{
>> >+	struct devlink *devlink = priv_to_devlink(app->pf);
>> >+
>> >+	switch (port->type) {
>> >+	case NFP_PORT_PHYS_PORT:
>> >+		return nfp_devlink_port_init_phys(devlink, port);
>> >+	case NFP_PORT_PF_PORT:
>> >+		devlink_port_type_eth_set(&port->dl_port, port->netdev);
>> >+		devlink_port_attrs_pci_pf_set(&port->dl_port, port->pf_id);
>> >+		return 0;
>> >+	case NFP_PORT_VF_PORT:
>> >+		devlink_port_type_eth_set(&port->dl_port, port->netdev);
>> >+		devlink_port_attrs_pci_vf_set(&port->dl_port, port->pf_id,
>> >+					      port->vf_id);  
>> 
>> What is the reason to expose vf/pf id for switch port? Isn't it rather
>> an attribute of a peer?
>
>Naw, its an attribute of the port.  I leave the ASIC via PF n or VF m
>of PF n.  Whatever is on the other side is isolated from the topology
>of the ASIC.

Ok.


>
>Is the physical port ID an attribute of the other end of the cable?
>
>> >+		return 0;
>> >+	default:
>> >+		return -EINVAL;
>> >+	}
>> >+}
>> >+
>> > void nfp_devlink_port_clean(struct nfp_port *port)
>> > {
>> > }
>> >@@ -376,7 +398,21 @@ int nfp_devlink_port_register(struct nfp_app *app, struct nfp_port *port)
>> > {
>> > 	struct devlink *devlink = priv_to_devlink(app->pf);
>> > 
>> >-	return devlink_port_register(devlink, &port->dl_port, port->eth_id);
>> >+	switch (port->type) {
>> >+	case NFP_PORT_PHYS_PORT:
>> >+		return devlink_port_register(devlink, &port->dl_port,
>> >+					     port->eth_id);
>> >+	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.


>
>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.
>
>> >+	case NFP_PORT_VF_PORT:
>> >+		return devlink_port_register(devlink, &port->dl_port,
>> >+					     (port->pf_id + 1) * 10000 +
>> >+					     port->vf_id + 1);
>> >+	default:
>> >+		return -EINVAL;
>> >+	}
>> > }
>> > 
>> > 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
>> >@@ -292,7 +292,9 @@ nfp_repr_transfer_features(struct net_device *netdev, struct net_device *lower)
>> > 
>> > static void nfp_repr_clean(struct nfp_repr *repr)
>> > {
>> >+	nfp_devlink_port_unregister(repr->port);
>> > 	unregister_netdev(repr->netdev);
>> >+	nfp_devlink_port_clean(repr->port);
>> > 	nfp_app_repr_clean(repr->app, repr->netdev);
>> > 	dst_release((struct dst_entry *)repr->dst);
>> > 	nfp_port_free(repr->port);
>> >@@ -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.


>
>> >+	if (err)
>> >+		goto err_unreg_netdev;
>> >+
>> > 	return 0;
>> > 
>> >+err_unreg_netdev:
>> >+	unregister_netdev(repr->netdev);
>> >+err_port_clean:
>> >+	nfp_devlink_port_clean(repr->port);
>> > err_repr_clean:
>> > 	nfp_app_repr_clean(app, netdev);
>> > err_clean:
Jakub Kicinski March 4, 2019, 11:32 p.m. UTC | #4
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.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
index 9af3cb1f2f17..bf7fd9614152 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
@@ -350,7 +350,8 @@  const struct devlink_ops nfp_devlink_ops = {
 	.flash_update		= nfp_devlink_flash_update,
 };
 
-int nfp_devlink_port_init(struct nfp_app *app, struct nfp_port *port)
+static int
+nfp_devlink_port_init_phys(struct devlink *devlink, struct nfp_port *port)
 {
 	struct nfp_eth_table_port eth_port;
 	int ret;
@@ -368,6 +369,27 @@  int nfp_devlink_port_init(struct nfp_app *app, struct nfp_port *port)
 	return 0;
 }
 
+int nfp_devlink_port_init(struct nfp_app *app, struct nfp_port *port)
+{
+	struct devlink *devlink = priv_to_devlink(app->pf);
+
+	switch (port->type) {
+	case NFP_PORT_PHYS_PORT:
+		return nfp_devlink_port_init_phys(devlink, port);
+	case NFP_PORT_PF_PORT:
+		devlink_port_type_eth_set(&port->dl_port, port->netdev);
+		devlink_port_attrs_pci_pf_set(&port->dl_port, port->pf_id);
+		return 0;
+	case NFP_PORT_VF_PORT:
+		devlink_port_type_eth_set(&port->dl_port, port->netdev);
+		devlink_port_attrs_pci_vf_set(&port->dl_port, port->pf_id,
+					      port->vf_id);
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
 void nfp_devlink_port_clean(struct nfp_port *port)
 {
 }
@@ -376,7 +398,21 @@  int nfp_devlink_port_register(struct nfp_app *app, struct nfp_port *port)
 {
 	struct devlink *devlink = priv_to_devlink(app->pf);
 
-	return devlink_port_register(devlink, &port->dl_port, port->eth_id);
+	switch (port->type) {
+	case NFP_PORT_PHYS_PORT:
+		return devlink_port_register(devlink, &port->dl_port,
+					     port->eth_id);
+	case NFP_PORT_PF_PORT:
+		return devlink_port_register(devlink, &port->dl_port,
+					     (port->pf_id + 1) * 10000 +
+					     port->pf_split_id * 1000);
+	case NFP_PORT_VF_PORT:
+		return devlink_port_register(devlink, &port->dl_port,
+					     (port->pf_id + 1) * 10000 +
+					     port->vf_id + 1);
+	default:
+		return -EINVAL;
+	}
 }
 
 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
@@ -292,7 +292,9 @@  nfp_repr_transfer_features(struct net_device *netdev, struct net_device *lower)
 
 static void nfp_repr_clean(struct nfp_repr *repr)
 {
+	nfp_devlink_port_unregister(repr->port);
 	unregister_netdev(repr->netdev);
+	nfp_devlink_port_clean(repr->port);
 	nfp_app_repr_clean(repr->app, repr->netdev);
 	dst_release((struct dst_entry *)repr->dst);
 	nfp_port_free(repr->port);
@@ -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);
+	if (err)
+		goto err_unreg_netdev;
+
 	return 0;
 
+err_unreg_netdev:
+	unregister_netdev(repr->netdev);
+err_port_clean:
+	nfp_devlink_port_clean(repr->port);
 err_repr_clean:
 	nfp_app_repr_clean(app, netdev);
 err_clean: