netdev - Re: [PATCH v5 net-next 07/10] net: dsa: allow changing the tag protocol via the "tagging" device attribute

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] [day] [month] [year] [list]

Message-ID: <20210121122314.2jfxuwu2cpokx3w5@skbuf>
Date:   Thu, 21 Jan 2021 14:23:14 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Richard Cochran <richardcochran@...il.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandru Marginean <alexandru.marginean@....com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Xiaoliang Yang <xiaoliang.yang_1@....com>,
        Hongbo Wang <hongbo.wang@....com>,
        Vladimir Oltean <vladimir.oltean@....com>,
        Po Liu <po.liu@....com>, Yangbo Lu <yangbo.lu@....com>,
        Maxim Kochetkov <fido_max@...ox.ru>,
        Eldar Gasanov <eldargasanov2@...il.com>,
        Andrey L <al@...omtech.com>,
        Tobias Waldekranz <tobias@...dekranz.com>,
        UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH v5 net-next 07/10] net: dsa: allow changing the tag
 protocol via the "tagging" device attribute

Hi Florian,

On Wed, Jan 20, 2021 at 07:53:41PM -0800, Florian Fainelli wrote:
> > +static int dsa_switch_tag_proto_del(struct dsa_switch *ds,
> > +				    struct dsa_notifier_tag_proto_info *info)
> > +{
> > +	int err = 0, port;
> > +
> > +	for (port = 0; port < ds->num_ports; port++) {
> > +		if (!dsa_switch_tag_proto_match(ds, port, info))
> > +			continue;
> > +
> > +		/* Check early if we can replace it, so we don't delete it
> > +		 * for nothing and leave the switch dangling.
> > +		 */
> > +		if (!ds->ops->set_tag_protocol) {
> > +			err = -EOPNOTSUPP;
> > +			break;
> > +		}
> 
> This can be moved out of the loop.

Thanks a lot for reviewing.
Yes, you are right, this can be moved out. It is a left-over from using
dsa_broadcast in the previous version. It is not the only left-over: the
info->tree_index is now redundant because we are notifying within a
single DSA switch tree.

> > +
> > +		/* The delete method is optional, just the setter
> > +		 * is mandatory
> > +		 */
> > +		if (ds->ops->del_tag_protocol)
> > +			ds->ops->del_tag_protocol(ds, port,
> > +						  info->tag_ops->proto);
> > +	}
> > +
> > +	return err;
> > +}
> > +
> > +static int dsa_switch_tag_proto_set(struct dsa_switch *ds,
> > +				    struct dsa_notifier_tag_proto_info *info)
> > +{
> > +	bool proto_changed = false;
> > +	int port, err;
> > +
> > +	for (port = 0; port < ds->num_ports; port++) {
> > +		struct dsa_port *cpu_dp = dsa_to_port(ds, port);
> > +
> > +		if (!dsa_switch_tag_proto_match(ds, port, info))
> > +			continue;
> > +
> > +		err = ds->ops->set_tag_protocol(ds, cpu_dp->index,
> > +						info->tag_ops->proto);
> > +		if (err)
> > +			return err;
> 
> Don't you need to test for ds->ops->set_tag_protocol to be implemented
> before calling it? Similar comment to earlier, can we do an early check
> for the operation being supported outside of the loop?

My assumption was that I had already added the check in tag_proto_del.
There are no code paths that call one but not the other. I can add the
check here too.

There's one more thing I would change: the dsa_switch_tag_proto_match.
Right now I am matching only on the CPU port, but if I take a look at
the mv88e6xxx driver:

static int mv88e6xxx_setup_port_mode(struct mv88e6xxx_chip *chip, int port)
{
	if (dsa_is_dsa_port(chip->ds, port))
		return mv88e6xxx_set_port_mode_dsa(chip, port);

	if (dsa_is_user_port(chip->ds, port))
		return mv88e6xxx_set_port_mode_normal(chip, port);

	/* Setup CPU port mode depending on its supported tag format */
	if (chip->info->tag_protocol == DSA_TAG_PROTO_DSA)
		return mv88e6xxx_set_port_mode_dsa(chip, port);

	if (chip->info->tag_protocol == DSA_TAG_PROTO_EDSA)
		return mv88e6xxx_set_port_mode_edsa(chip, port);

	return -EINVAL;
}

DSA links call the same function as CPU ports configured for
DSA_TAG_PROTO_DSA. So to cater to Marvell switches too, and to ease a
potential conversion to this API, I could add dsa_is_dsa_port to the
matching function too.