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: <20210109174439.404713f6@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Sat, 9 Jan 2021 17:44:39 -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 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?

> +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);