diff mbox series

[v3,net-next,08/10] net: mscc: ocelot: register devlink ports

Message ID 20210108175950.484854-9-olteanv@gmail.com
State New
Headers show
Series Configuring congestion watermarks on ocelot switch using devlink-sb | expand

Commit Message

Vladimir Oltean Jan. 8, 2021, 5:59 p.m. UTC
From: Vladimir Oltean <vladimir.oltean@nxp.com>

Add devlink integration into the mscc_ocelot switchdev driver. Only the
probed interfaces are registered with devlink, because for convenience,
struct devlink_port was included into struct ocelot_port_private, which
is only initialized for the ports that are used.

Since we use devlink_port_type_eth_set to link the devlink port to the
net_device, we can as well remove the .ndo_get_phys_port_name and
.ndo_get_port_parent_id implementations, since devlink takes care of
retrieving the port name and number automatically, once
.ndo_get_devlink_port is implemented.

Note that the felix DSA driver is already integrated with devlink by
default, since that is a thing that the DSA core takes care of. This is
the reason why these devlink stubs were put in ocelot_net.c and not in
the common library.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Changes in v3:
None.

Changes in v2:
Using devlink_port_type_eth_set as per Jiri's suggestion.

 drivers/net/ethernet/mscc/ocelot.h         |   4 +
 drivers/net/ethernet/mscc/ocelot_net.c     | 139 ++++++++++++++++-----
 drivers/net/ethernet/mscc/ocelot_vsc7514.c |   7 ++
 include/soc/mscc/ocelot.h                  |   1 +
 4 files changed, 123 insertions(+), 28 deletions(-)

Comments

Jakub Kicinski Jan. 10, 2021, 1:44 a.m. UTC | #1
On Fri,  8 Jan 2021 19:59:48 +0200 Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>

> 

> Add devlink integration into the mscc_ocelot switchdev driver. Only the

> probed interfaces are registered with devlink, because for convenience,

> struct devlink_port was included into struct ocelot_port_private, which

> is only initialized for the ports that are used.

> 

> Since we use devlink_port_type_eth_set to link the devlink port to the

> net_device, we can as well remove the .ndo_get_phys_port_name and

> .ndo_get_port_parent_id implementations, since devlink takes care of

> retrieving the port name and number automatically, once

> .ndo_get_devlink_port is implemented.

> 

> Note that the felix DSA driver is already integrated with devlink by

> default, since that is a thing that the DSA core takes care of. This is

> the reason why these devlink stubs were put in ocelot_net.c and not in

> the common library.

> 

> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>


> diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c

> index 2bd2840d88bd..d0d98c6adea8 100644

> --- a/drivers/net/ethernet/mscc/ocelot_net.c

> +++ b/drivers/net/ethernet/mscc/ocelot_net.c

> @@ -8,6 +8,116 @@

>  #include "ocelot.h"

>  #include "ocelot_vcap.h"

>  

> +struct ocelot_devlink_private {

> +	struct ocelot *ocelot;

> +};


Why not make struct ocelot part of devlink_priv?

> +static const struct devlink_ops ocelot_devlink_ops = {

> +};

> +

> +static int ocelot_port_devlink_init(struct ocelot *ocelot, int port)

> +{

> +	struct ocelot_port *ocelot_port = ocelot->ports[port];

> +	int id_len = sizeof(ocelot->base_mac);

> +	struct devlink *dl = ocelot->devlink;

> +	struct devlink_port_attrs attrs = {};

> +	struct ocelot_port_private *priv;

> +	struct devlink_port *dlp;

> +	int err;

> +

> +	if (!ocelot_port)

> +		return 0;

> +

> +	priv = container_of(ocelot_port, struct ocelot_port_private, port);

> +	dlp = &priv->devlink_port;

> +

> +	memcpy(attrs.switch_id.id, &ocelot->base_mac, id_len);

> +	attrs.switch_id.id_len = id_len;

> +	attrs.phys.port_number = port;

> +

> +	if (priv->dev)

> +		attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL;

> +	else

> +		attrs.flavour = DEVLINK_PORT_FLAVOUR_UNUSED;

> +

> +	devlink_port_attrs_set(dlp, &attrs);

> +

> +	err = devlink_port_register(dl, dlp, port);

> +	if (err)

> +		return err;

> +

> +	if (priv->dev)

> +		devlink_port_type_eth_set(dlp, priv->dev);


devlink_port_attrs_set() should be called before netdev is registered,
and devlink_port_type_eth_set() after. So this sequence makes me a tad
suspicious.

In particular IIRC devlink's .ndo_get_phys_port_name depends on it,
because udev event needs to carry the right info for interface renaming
to work reliably. No?

> +	return 0;

> +}

> +

> +static void ocelot_port_devlink_teardown(struct ocelot *ocelot, int port)

> +{

> +	struct ocelot_port *ocelot_port = ocelot->ports[port];

> +	struct ocelot_port_private *priv;

> +	struct devlink_port *dlp;

> +

> +	if (!ocelot_port)

> +		return;

> +

> +	priv = container_of(ocelot_port, struct ocelot_port_private, port);

> +	dlp = &priv->devlink_port;

> +

> +	devlink_port_unregister(dlp);

> +}

> +

> +int ocelot_devlink_init(struct ocelot *ocelot)

> +{

> +	struct ocelot_devlink_private *dl_priv;

> +	int port, err;

> +

> +	ocelot->devlink = devlink_alloc(&ocelot_devlink_ops, sizeof(*dl_priv));

> +	if (!ocelot->devlink)

> +		return -ENOMEM;

> +	dl_priv = devlink_priv(ocelot->devlink);

> +	dl_priv->ocelot = ocelot;

> +

> +	err = devlink_register(ocelot->devlink, ocelot->dev);

> +	if (err)

> +		goto free_devlink;

> +

> +	for (port = 0; port < ocelot->num_phys_ports; port++) {

> +		err = ocelot_port_devlink_init(ocelot, port);

> +		if (err) {

> +			while (port-- > 0)

> +				ocelot_port_devlink_teardown(ocelot, port);

> +			goto unregister_devlink;


nit: should this also be on the error path below?

> +		}

> +	}

> +

> +	return 0;

> +

> +unregister_devlink:

> +	devlink_unregister(ocelot->devlink);

> +free_devlink:

> +	devlink_free(ocelot->devlink);

> +	return err;

> +}


> --- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c

> +++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c

> @@ -1293,6 +1293,12 @@ static int mscc_ocelot_probe(struct platform_device *pdev)

>  		}

>  	}

>  

> +	err = ocelot_devlink_init(ocelot);

> +	if (err) {

> +		mscc_ocelot_release_ports(ocelot);

> +		goto out_ocelot_deinit;


No need to add ocelot_deinit_timestamp(ocelot); to the error path?

> +	}

> +

>  	register_netdevice_notifier(&ocelot_netdevice_nb);

>  	register_switchdev_notifier(&ocelot_switchdev_nb);

>  	register_switchdev_blocking_notifier(&ocelot_switchdev_blocking_nb);
Jakub Kicinski Jan. 10, 2021, 2:01 a.m. UTC | #2
On Fri,  8 Jan 2021 19:59:48 +0200 Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>

> 

> Add devlink integration into the mscc_ocelot switchdev driver. Only the

> probed interfaces are registered with devlink, because for convenience,

> struct devlink_port was included into struct ocelot_port_private, which

> is only initialized for the ports that are used.


This sounds like something that DSA should have a general policy on.
I actually feel like it was discussed in the past.. Do you know what
other drivers do?
Vladimir Oltean Jan. 11, 2021, 5:13 p.m. UTC | #3
On Sat, Jan 09, 2021 at 05:44:39PM -0800, Jakub Kicinski wrote:
> On Fri,  8 Jan 2021 19:59:48 +0200 Vladimir Oltean wrote:

> > From: Vladimir Oltean <vladimir.oltean@nxp.com>

> > 

> > Add devlink integration into the mscc_ocelot switchdev driver. Only the

> > probed interfaces are registered with devlink, because for convenience,

> > struct devlink_port was included into struct ocelot_port_private, which

> > is only initialized for the ports that are used.

> > 

> > Since we use devlink_port_type_eth_set to link the devlink port to the

> > net_device, we can as well remove the .ndo_get_phys_port_name and

> > .ndo_get_port_parent_id implementations, since devlink takes care of

> > retrieving the port name and number automatically, once

> > .ndo_get_devlink_port is implemented.

> > 

> > Note that the felix DSA driver is already integrated with devlink by

> > default, since that is a thing that the DSA core takes care of. This is

> > the reason why these devlink stubs were put in ocelot_net.c and not in

> > the common library.

> > 

> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

> 

> > diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c

> > index 2bd2840d88bd..d0d98c6adea8 100644

> > --- a/drivers/net/ethernet/mscc/ocelot_net.c

> > +++ b/drivers/net/ethernet/mscc/ocelot_net.c

> > @@ -8,6 +8,116 @@

> >  #include "ocelot.h"

> >  #include "ocelot_vcap.h"

> >  

> > +struct ocelot_devlink_private {

> > +	struct ocelot *ocelot;

> > +};

> 

> Why not make struct ocelot part of devlink_priv?


I am not sure what you mean.

> > +static const struct devlink_ops ocelot_devlink_ops = {

> > +};

> > +

> > +static int ocelot_port_devlink_init(struct ocelot *ocelot, int port)

> > +{

> > +	struct ocelot_port *ocelot_port = ocelot->ports[port];

> > +	int id_len = sizeof(ocelot->base_mac);

> > +	struct devlink *dl = ocelot->devlink;

> > +	struct devlink_port_attrs attrs = {};

> > +	struct ocelot_port_private *priv;

> > +	struct devlink_port *dlp;

> > +	int err;

> > +

> > +	if (!ocelot_port)

> > +		return 0;

> > +

> > +	priv = container_of(ocelot_port, struct ocelot_port_private, port);

> > +	dlp = &priv->devlink_port;

> > +

> > +	memcpy(attrs.switch_id.id, &ocelot->base_mac, id_len);

> > +	attrs.switch_id.id_len = id_len;

> > +	attrs.phys.port_number = port;

> > +

> > +	if (priv->dev)

> > +		attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL;

> > +	else

> > +		attrs.flavour = DEVLINK_PORT_FLAVOUR_UNUSED;

> > +

> > +	devlink_port_attrs_set(dlp, &attrs);

> > +

> > +	err = devlink_port_register(dl, dlp, port);

> > +	if (err)

> > +		return err;

> > +

> > +	if (priv->dev)

> > +		devlink_port_type_eth_set(dlp, priv->dev);

> 

> devlink_port_attrs_set() should be called before netdev is registered,

> and devlink_port_type_eth_set() after. So this sequence makes me a tad

> suspicious.

> 

> In particular IIRC devlink's .ndo_get_phys_port_name depends on it,

> because udev event needs to carry the right info for interface renaming

> to work reliably. No?

> 


If I change the driver's Kconfig from tristate to bool, all is fine,
isn't it?

> > +	return 0;

> > +}

> > +

> > +static void ocelot_port_devlink_teardown(struct ocelot *ocelot, int port)

> > +{

> > +	struct ocelot_port *ocelot_port = ocelot->ports[port];

> > +	struct ocelot_port_private *priv;

> > +	struct devlink_port *dlp;

> > +

> > +	if (!ocelot_port)

> > +		return;

> > +

> > +	priv = container_of(ocelot_port, struct ocelot_port_private, port);

> > +	dlp = &priv->devlink_port;

> > +

> > +	devlink_port_unregister(dlp);

> > +}

> > +

> > +int ocelot_devlink_init(struct ocelot *ocelot)

> > +{

> > +	struct ocelot_devlink_private *dl_priv;

> > +	int port, err;

> > +

> > +	ocelot->devlink = devlink_alloc(&ocelot_devlink_ops, sizeof(*dl_priv));

> > +	if (!ocelot->devlink)

> > +		return -ENOMEM;

> > +	dl_priv = devlink_priv(ocelot->devlink);

> > +	dl_priv->ocelot = ocelot;

> > +

> > +	err = devlink_register(ocelot->devlink, ocelot->dev);

> > +	if (err)

> > +		goto free_devlink;

> > +

> > +	for (port = 0; port < ocelot->num_phys_ports; port++) {

> > +		err = ocelot_port_devlink_init(ocelot, port);

> > +		if (err) {

> > +			while (port-- > 0)

> > +				ocelot_port_devlink_teardown(ocelot, port);

> > +			goto unregister_devlink;

> 

> nit: should this also be on the error path below?

> 

> > +		}

> > +	}

> > +

> > +	return 0;

> > +

> > +unregister_devlink:

> > +	devlink_unregister(ocelot->devlink);

> > +free_devlink:

> > +	devlink_free(ocelot->devlink);

> > +	return err;

> > +}

> 

> > --- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c

> > +++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c

> > @@ -1293,6 +1293,12 @@ static int mscc_ocelot_probe(struct platform_device *pdev)

> >  		}

> >  	}

> >  

> > +	err = ocelot_devlink_init(ocelot);

> > +	if (err) {

> > +		mscc_ocelot_release_ports(ocelot);

> > +		goto out_ocelot_deinit;

> 

> No need to add ocelot_deinit_timestamp(ocelot); to the error path?


Yes, the error handling could be better.
Jakub Kicinski Jan. 11, 2021, 7:19 p.m. UTC | #4
On Mon, 11 Jan 2021 19:13:44 +0200 Vladimir Oltean wrote:
> On Sat, Jan 09, 2021 at 05:44:39PM -0800, Jakub Kicinski wrote:

> > On Fri,  8 Jan 2021 19:59:48 +0200 Vladimir Oltean wrote:  

> > > From: Vladimir Oltean <vladimir.oltean@nxp.com>

> > > 

> > > Add devlink integration into the mscc_ocelot switchdev driver. Only the

> > > probed interfaces are registered with devlink, because for convenience,

> > > struct devlink_port was included into struct ocelot_port_private, which

> > > is only initialized for the ports that are used.

> > > 

> > > Since we use devlink_port_type_eth_set to link the devlink port to the

> > > net_device, we can as well remove the .ndo_get_phys_port_name and

> > > .ndo_get_port_parent_id implementations, since devlink takes care of

> > > retrieving the port name and number automatically, once

> > > .ndo_get_devlink_port is implemented.

> > > 

> > > Note that the felix DSA driver is already integrated with devlink by

> > > default, since that is a thing that the DSA core takes care of. This is

> > > the reason why these devlink stubs were put in ocelot_net.c and not in

> > > the common library.

> > > 

> > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>  

> >   

> > > diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c

> > > index 2bd2840d88bd..d0d98c6adea8 100644

> > > --- a/drivers/net/ethernet/mscc/ocelot_net.c

> > > +++ b/drivers/net/ethernet/mscc/ocelot_net.c

> > > @@ -8,6 +8,116 @@

> > >  #include "ocelot.h"

> > >  #include "ocelot_vcap.h"

> > >  

> > > +struct ocelot_devlink_private {

> > > +	struct ocelot *ocelot;

> > > +};  

> > 

> > Why not make struct ocelot part of devlink_priv?  

> 

> I am not sure what you mean.


You put a pointer to struct ocelot inside devlink->priv, why not put
the actual struct ocelot there?

> > > +static const struct devlink_ops ocelot_devlink_ops = {

> > > +};

> > > +

> > > +static int ocelot_port_devlink_init(struct ocelot *ocelot, int port)

> > > +{

> > > +	struct ocelot_port *ocelot_port = ocelot->ports[port];

> > > +	int id_len = sizeof(ocelot->base_mac);

> > > +	struct devlink *dl = ocelot->devlink;

> > > +	struct devlink_port_attrs attrs = {};

> > > +	struct ocelot_port_private *priv;

> > > +	struct devlink_port *dlp;

> > > +	int err;

> > > +

> > > +	if (!ocelot_port)

> > > +		return 0;

> > > +

> > > +	priv = container_of(ocelot_port, struct ocelot_port_private, port);

> > > +	dlp = &priv->devlink_port;

> > > +

> > > +	memcpy(attrs.switch_id.id, &ocelot->base_mac, id_len);

> > > +	attrs.switch_id.id_len = id_len;

> > > +	attrs.phys.port_number = port;

> > > +

> > > +	if (priv->dev)

> > > +		attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL;

> > > +	else

> > > +		attrs.flavour = DEVLINK_PORT_FLAVOUR_UNUSED;

> > > +

> > > +	devlink_port_attrs_set(dlp, &attrs);

> > > +

> > > +	err = devlink_port_register(dl, dlp, port);

> > > +	if (err)

> > > +		return err;

> > > +

> > > +	if (priv->dev)

> > > +		devlink_port_type_eth_set(dlp, priv->dev);  

> > 

> > devlink_port_attrs_set() should be called before netdev is registered,

> > and devlink_port_type_eth_set() after. So this sequence makes me a tad

> > suspicious.

> > 

> > In particular IIRC devlink's .ndo_get_phys_port_name depends on it,

> > because udev event needs to carry the right info for interface renaming

> > to work reliably. No?

> 

> If I change the driver's Kconfig from tristate to bool, all is fine,

> isn't it?


How does Kconfig change the order of registration of objects to
subsystems _within_ the driver?

Can you unbind and bind the driver back and see if phys_port_name
always gets the correct value? (replay/udevadm test is not sufficient)
Vladimir Oltean Jan. 14, 2021, 10:34 a.m. UTC | #5
On Mon, Jan 11, 2021 at 11:19:09AM -0800, Jakub Kicinski wrote:
> > > devlink_port_attrs_set() should be called before netdev is registered,

> > > and devlink_port_type_eth_set() after. So this sequence makes me a tad

> > > suspicious.

> > >

> > > In particular IIRC devlink's .ndo_get_phys_port_name depends on it,

> > > because udev event needs to carry the right info for interface renaming

> > > to work reliably. No?

> >

> > If I change the driver's Kconfig from tristate to bool, all is fine,

> > isn't it?

>

> How does Kconfig change the order of registration of objects to

> subsystems _within_ the driver?


The registration order within the driver is not what matters. What
matters is that the devlink_port and net_device are both registered
_before_ udev is up.

> Can you unbind and bind the driver back and see if phys_port_name

> always gets the correct value? (replay/udevadm test is not sufficient)


Yes, and that udev renaming test failed miserably still.

I have dhcpcd in my system, and it races with my udev script by
auto-upping the interfaces when they probe. Then, dev_change_name()
returns -EBUSY because the interfaces are up but its priv_flags do not
declare IFF_LIVE_RENAME_OK.

How is that one solved?
Jakub Kicinski Jan. 14, 2021, 4:44 p.m. UTC | #6
On Thu, 14 Jan 2021 12:34:05 +0200 Vladimir Oltean wrote:
> On Mon, Jan 11, 2021 at 11:19:09AM -0800, Jakub Kicinski wrote:

> > > > devlink_port_attrs_set() should be called before netdev is registered,

> > > > and devlink_port_type_eth_set() after. So this sequence makes me a tad

> > > > suspicious.

> > > >

> > > > In particular IIRC devlink's .ndo_get_phys_port_name depends on it,

> > > > because udev event needs to carry the right info for interface renaming

> > > > to work reliably. No?  

> > >

> > > If I change the driver's Kconfig from tristate to bool, all is fine,

> > > isn't it?  

> >

> > How does Kconfig change the order of registration of objects to

> > subsystems _within_ the driver?  

> 

> The registration order within the driver is not what matters. What

> matters is that the devlink_port and net_device are both registered

> _before_ udev is up.


I see.

> > Can you unbind and bind the driver back and see if phys_port_name

> > always gets the correct value? (replay/udevadm test is not sufficient)  

> 

> Yes, and that udev renaming test failed miserably still.

> 

> I have dhcpcd in my system, and it races with my udev script by

> auto-upping the interfaces when they probe. Then, dev_change_name()

> returns -EBUSY because the interfaces are up but its priv_flags do not

> declare IFF_LIVE_RENAME_OK.

> 

> How is that one solved?


Yeah, that's one of those perennial problems we never found a strong
answer to. IIRC IFF_LIVE_RENAME_OK was more of a dirty hack than a
serious answer. I think we should allow renaming interfaces while
they're up, and see if anything breaks. We'd just need to add the right
netlink notifications to dev_change_name(), maybe?
Vladimir Oltean Jan. 15, 2021, 7:54 p.m. UTC | #7
On Fri, Jan 15, 2021 at 07:11:42PM +0200, Vladimir Oltean wrote:
> By the way I removed the if condition and added nothing in its place,

> just to see what would happen. I see a lot of these messages, I did not

> investigate where they are coming from and why they are emitted. They go

> away when I rename the interface back to swp0.


Looks like it's again related to dhcpcd. If I rename the interface while
it's down, the link-local IPv6 addresses don't get endlessly deleted as
they do if I rename it while it's up. Also, if I live-rename the interface
while dhcpcd isn't up, that behavior disappears too.

Still not sure why it happens.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/mscc/ocelot.h b/drivers/net/ethernet/mscc/ocelot.h
index 519335676c24..2e9a3c4697c8 100644
--- a/drivers/net/ethernet/mscc/ocelot.h
+++ b/drivers/net/ethernet/mscc/ocelot.h
@@ -57,6 +57,8 @@  struct ocelot_port_private {
 	struct phy *serdes;
 
 	struct ocelot_port_tc tc;
+
+	struct devlink_port devlink_port;
 };
 
 struct ocelot_dump_ctx {
@@ -121,6 +123,8 @@  void ocelot_port_writel(struct ocelot_port *port, u32 val, u32 reg);
 
 int ocelot_probe_port(struct ocelot *ocelot, int port, struct regmap *target,
 		      struct phy_device *phy);
+int ocelot_devlink_init(struct ocelot *ocelot);
+void ocelot_devlink_teardown(struct ocelot *ocelot);
 
 extern struct notifier_block ocelot_netdevice_nb;
 extern struct notifier_block ocelot_switchdev_nb;
diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
index 2bd2840d88bd..d0d98c6adea8 100644
--- a/drivers/net/ethernet/mscc/ocelot_net.c
+++ b/drivers/net/ethernet/mscc/ocelot_net.c
@@ -8,6 +8,116 @@ 
 #include "ocelot.h"
 #include "ocelot_vcap.h"
 
+struct ocelot_devlink_private {
+	struct ocelot *ocelot;
+};
+
+static const struct devlink_ops ocelot_devlink_ops = {
+};
+
+static int ocelot_port_devlink_init(struct ocelot *ocelot, int port)
+{
+	struct ocelot_port *ocelot_port = ocelot->ports[port];
+	int id_len = sizeof(ocelot->base_mac);
+	struct devlink *dl = ocelot->devlink;
+	struct devlink_port_attrs attrs = {};
+	struct ocelot_port_private *priv;
+	struct devlink_port *dlp;
+	int err;
+
+	if (!ocelot_port)
+		return 0;
+
+	priv = container_of(ocelot_port, struct ocelot_port_private, port);
+	dlp = &priv->devlink_port;
+
+	memcpy(attrs.switch_id.id, &ocelot->base_mac, id_len);
+	attrs.switch_id.id_len = id_len;
+	attrs.phys.port_number = port;
+
+	if (priv->dev)
+		attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL;
+	else
+		attrs.flavour = DEVLINK_PORT_FLAVOUR_UNUSED;
+
+	devlink_port_attrs_set(dlp, &attrs);
+
+	err = devlink_port_register(dl, dlp, port);
+	if (err)
+		return err;
+
+	if (priv->dev)
+		devlink_port_type_eth_set(dlp, priv->dev);
+
+	return 0;
+}
+
+static void ocelot_port_devlink_teardown(struct ocelot *ocelot, int port)
+{
+	struct ocelot_port *ocelot_port = ocelot->ports[port];
+	struct ocelot_port_private *priv;
+	struct devlink_port *dlp;
+
+	if (!ocelot_port)
+		return;
+
+	priv = container_of(ocelot_port, struct ocelot_port_private, port);
+	dlp = &priv->devlink_port;
+
+	devlink_port_unregister(dlp);
+}
+
+int ocelot_devlink_init(struct ocelot *ocelot)
+{
+	struct ocelot_devlink_private *dl_priv;
+	int port, err;
+
+	ocelot->devlink = devlink_alloc(&ocelot_devlink_ops, sizeof(*dl_priv));
+	if (!ocelot->devlink)
+		return -ENOMEM;
+	dl_priv = devlink_priv(ocelot->devlink);
+	dl_priv->ocelot = ocelot;
+
+	err = devlink_register(ocelot->devlink, ocelot->dev);
+	if (err)
+		goto free_devlink;
+
+	for (port = 0; port < ocelot->num_phys_ports; port++) {
+		err = ocelot_port_devlink_init(ocelot, port);
+		if (err) {
+			while (port-- > 0)
+				ocelot_port_devlink_teardown(ocelot, port);
+			goto unregister_devlink;
+		}
+	}
+
+	return 0;
+
+unregister_devlink:
+	devlink_unregister(ocelot->devlink);
+free_devlink:
+	devlink_free(ocelot->devlink);
+	return err;
+}
+
+void ocelot_devlink_teardown(struct ocelot *ocelot)
+{
+	int port;
+
+	for (port = 0; port < ocelot->num_phys_ports; port++)
+		ocelot_port_devlink_teardown(ocelot, port);
+
+	devlink_unregister(ocelot->devlink);
+	devlink_free(ocelot->devlink);
+}
+
+static struct devlink_port *ocelot_get_devlink_port(struct net_device *dev)
+{
+	struct ocelot_port_private *priv = netdev_priv(dev);
+
+	return &priv->devlink_port;
+}
+
 int ocelot_setup_tc_cls_flower(struct ocelot_port_private *priv,
 			       struct flow_cls_offload *f,
 			       bool ingress)
@@ -525,20 +635,6 @@  static void ocelot_set_rx_mode(struct net_device *dev)
 	__dev_mc_sync(dev, ocelot_mc_sync, ocelot_mc_unsync);
 }
 
-static int ocelot_port_get_phys_port_name(struct net_device *dev,
-					  char *buf, size_t len)
-{
-	struct ocelot_port_private *priv = netdev_priv(dev);
-	int port = priv->chip_port;
-	int ret;
-
-	ret = snprintf(buf, len, "p%d", port);
-	if (ret >= len)
-		return -EINVAL;
-
-	return 0;
-}
-
 static int ocelot_port_set_mac_address(struct net_device *dev, void *p)
 {
 	struct ocelot_port_private *priv = netdev_priv(dev);
@@ -689,18 +785,6 @@  static int ocelot_set_features(struct net_device *dev,
 	return 0;
 }
 
-static int ocelot_get_port_parent_id(struct net_device *dev,
-				     struct netdev_phys_item_id *ppid)
-{
-	struct ocelot_port_private *priv = netdev_priv(dev);
-	struct ocelot *ocelot = priv->port.ocelot;
-
-	ppid->id_len = sizeof(ocelot->base_mac);
-	memcpy(&ppid->id, &ocelot->base_mac, ppid->id_len);
-
-	return 0;
-}
-
 static int ocelot_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 {
 	struct ocelot_port_private *priv = netdev_priv(dev);
@@ -727,7 +811,6 @@  static const struct net_device_ops ocelot_port_netdev_ops = {
 	.ndo_stop			= ocelot_port_stop,
 	.ndo_start_xmit			= ocelot_port_xmit,
 	.ndo_set_rx_mode		= ocelot_set_rx_mode,
-	.ndo_get_phys_port_name		= ocelot_port_get_phys_port_name,
 	.ndo_set_mac_address		= ocelot_port_set_mac_address,
 	.ndo_get_stats64		= ocelot_get_stats64,
 	.ndo_fdb_add			= ocelot_port_fdb_add,
@@ -736,9 +819,9 @@  static const struct net_device_ops ocelot_port_netdev_ops = {
 	.ndo_vlan_rx_add_vid		= ocelot_vlan_rx_add_vid,
 	.ndo_vlan_rx_kill_vid		= ocelot_vlan_rx_kill_vid,
 	.ndo_set_features		= ocelot_set_features,
-	.ndo_get_port_parent_id		= ocelot_get_port_parent_id,
 	.ndo_setup_tc			= ocelot_setup_tc,
 	.ndo_do_ioctl			= ocelot_ioctl,
+	.ndo_get_devlink_port		= ocelot_get_devlink_port,
 };
 
 struct net_device *ocelot_port_to_netdev(struct ocelot *ocelot, int port)
diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
index ecd474476cc6..80fdf971d573 100644
--- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
+++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
@@ -1293,6 +1293,12 @@  static int mscc_ocelot_probe(struct platform_device *pdev)
 		}
 	}
 
+	err = ocelot_devlink_init(ocelot);
+	if (err) {
+		mscc_ocelot_release_ports(ocelot);
+		goto out_ocelot_deinit;
+	}
+
 	register_netdevice_notifier(&ocelot_netdevice_nb);
 	register_switchdev_notifier(&ocelot_switchdev_nb);
 	register_switchdev_blocking_notifier(&ocelot_switchdev_blocking_nb);
@@ -1314,6 +1320,7 @@  static int mscc_ocelot_remove(struct platform_device *pdev)
 {
 	struct ocelot *ocelot = platform_get_drvdata(pdev);
 
+	ocelot_devlink_teardown(ocelot);
 	ocelot_deinit_timestamp(ocelot);
 	mscc_ocelot_release_ports(ocelot);
 	ocelot_deinit(ocelot);
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index 9a46787c679b..75cd457b99b9 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -602,6 +602,7 @@  struct ocelot_port {
 
 struct ocelot {
 	struct device			*dev;
+	struct devlink			*devlink;
 
 	const struct ocelot_ops		*ops;
 	struct regmap			*targets[TARGET_MAX];