netdev - [RFC PATCH net-next 0/4] Remove the "dsa_to

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-next>] [day] [month] [year] [list]
Message-Id: <20210809190320.1058373-1-vladimir.oltean@nxp.com>
Date:   Mon,  9 Aug 2021 22:03:16 +0300
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     netdev@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        Kurt Kanzenbach <kurt@...utronix.de>,
        Woojung Huh <woojung.huh@...rochip.com>,
        UNGLinuxDriver@...rochip.com, Sean Wang <sean.wang@...iatek.com>,
        Landen Chao <Landen.Chao@...iatek.com>,
        DENG Qingfang <dqfext@...il.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        George McCollister <george.mccollister@...il.com>
Subject: [RFC PATCH net-next 0/4] Remove the "dsa_to_port in a loop" antipattern

The DSA core and drivers currently iterate too much through the port
list of a switch. For example, this snippet:

	for (port = 0; port < ds->num_ports; port++) {
		if (!dsa_is_cpu_port(ds, port))
			continue;

		ds->ops->change_tag_protocol(ds, port, tag_ops->proto);
	}

iterates through ds->num_ports once, and then calls dsa_is_cpu_port to
filter out the other types of ports. But that function has a hidden call
to dsa_to_port() in it, which contains:

	list_for_each_entry(dp, &dst->ports, list)
		if (dp->ds == ds && dp->index == p)
			return dp;

where the only thing we wanted to know in the first place was whether
dp->type == DSA_PORT_TYPE_CPU or not.

So it seems that the problem is that we are not iterating with the right
variable. We have an "int port" but in fact need a "struct dsa_port *dp".

This has started being an issue since this patch series:
https://patchwork.ozlabs.org/project/netdev/cover/20191020031941.3805884-1-vivien.didelot@gmail.com/

The currently proposed set of changes iterates like this:

	dsa_switch_for_each_cpu_port(cpu_dp, ds)
		err = ds->ops->change_tag_protocol(ds, cpu_dp->index,
						   tag_ops->proto);

which iterates directly over ds->dst->ports, which is a list of struct
dsa_port *dp. This makes it much easier and more efficient to check
dp->type.

As a nice side effect, with the proposed driver API, driver writers are
now encouraged to use more efficient patterns, and not only due to less
iterations through the port list. For example, something like this:

	for (port = 0; port < ds->num_ports; port++)
		do_something();

probably does not need to do_something() for the ports that are disabled
in the device tree. But adding extra code for that would look like this:

	for (port = 0; port < ds->num_ports; port++) {
		if (!dsa_is_unused_port(ds, port))
			continue;

		do_something();
	}

and therefore, it is understandable that some driver writers may decide
to not bother. This patch series introduces a "dsa_switch_for_each_available_port"
macro which comes at no extra cost in terms of lines of code / number of
braces to the driver writer, but it has the "dsa_is_unused_port" check
embedded within it.

I changed as much code as I could, probably not all, but a start anyway.

Vladimir Oltean (4):
  net: dsa: introduce a dsa_port_is_unused helper
  net: dsa: remove the "dsa_to_port in a loop" antipattern from the core
  net: dsa: remove the "dsa_to_port in a loop" antipattern from drivers
  net: dsa: b53: express b53_for_each_port in terms of
    dsa_switch_for_each_port

 drivers/net/dsa/b53/b53_common.c              |  26 ++-
 drivers/net/dsa/b53/b53_priv.h                |   6 +-
 drivers/net/dsa/bcm_sf2.c                     |   8 +-
 drivers/net/dsa/hirschmann/hellcreek.c        |  27 +--
 .../net/dsa/hirschmann/hellcreek_hwtstamp.c   |  19 +-
 drivers/net/dsa/microchip/ksz9477.c           |  19 +-
 drivers/net/dsa/microchip/ksz_common.c        |  19 +-
 drivers/net/dsa/mt7530.c                      |  58 +++---
 drivers/net/dsa/mv88e6xxx/chip.c              |  37 ++--
 drivers/net/dsa/mv88e6xxx/hwtstamp.c          |  10 +-
 drivers/net/dsa/mv88e6xxx/port.c              |  12 +-
 drivers/net/dsa/ocelot/felix.c                |  79 +++-----
 drivers/net/dsa/ocelot/felix_vsc9959.c        |  10 +-
 drivers/net/dsa/ocelot/seville_vsc9953.c      |  14 +-
 drivers/net/dsa/qca8k.c                       |  32 ++--
 drivers/net/dsa/sja1105/sja1105_main.c        | 176 ++++++++----------
 drivers/net/dsa/sja1105/sja1105_mdio.c        |  12 +-
 drivers/net/dsa/xrs700x/xrs700x.c             |  37 ++--
 include/net/dsa.h                             |  43 ++++-
 net/dsa/dsa.c                                 |  22 +--
 net/dsa/dsa2.c                                |  11 +-
 net/dsa/port.c                                |   7 +-
 net/dsa/switch.c                              |  41 ++--
 net/dsa/tag_8021q.c                           |  29 ++-
 24 files changed, 360 insertions(+), 394 deletions(-)

-- 
2.25.1