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.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.
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)
