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.olt...@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.olt...@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.
