netdev - Re: [PATCH v3 net-next 08/10] net: mscc: ocelot: register devlink ports

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]

Message-ID: <20210111111909.4cf0174f@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Mon, 11 Jan 2021 11:19:09 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     netdev@...r.kernel.org, alexandre.belloni@...tlin.com,
        andrew@...n.ch, f.fainelli@...il.com, vivien.didelot@...il.com,
        alexandru.marginean@....com, claudiu.manoil@....com,
        xiaoliang.yang_1@....com, hongbo.wang@....com, jiri@...nulli.us,
        idosch@...sch.org, UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH v3 net-next 08/10] net: mscc: ocelot: register devlink
 ports

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