netdev - Re: [PATCH v2 net-next] net: dsa: felix: enable cut-through forwarding between ports by default

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]

Date:   Wed, 24 Nov 2021 18:39:00 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        UNGLinuxDriver@...rochip.com, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Xiaoliang Yang <xiaoliang.yang_1@....com>,
        Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [PATCH v2 net-next] net: dsa: felix: enable cut-through
 forwarding between ports by default

On Tue, 23 Nov 2021 15:21:16 +0200 Vladimir Oltean wrote:
> +static void vsc9959_cut_through_fwd(struct ocelot *ocelot)
> +{
> +	struct felix *felix = ocelot_to_felix(ocelot);
> +	struct dsa_switch *ds = felix->ds;
> +	int port, other_port;
> +
> +	for (port = 0; port < ocelot->num_phys_ports; port++) {
> +		struct ocelot_port *ocelot_port = ocelot->ports[port];
> +		unsigned long mask;
> +		int min_speed;
> +		u32 val = 0;
> +
> +		if (ocelot_port->speed <= 0)
> +			continue;

Would it not be safer to disable cut-thru for ports which are down?

	goto set;

> +		min_speed = ocelot_port->speed;
> +		if (port == ocelot->npi) {
> +			/* Ocelot switches forward from the NPI port towards
> +			 * any port, regardless of it being in the NPI port's
> +			 * forwarding domain or not.
> +			 */
> +			mask = dsa_user_ports(ds);
> +		} else {
> +			mask = ocelot_read_rix(ocelot, ANA_PGID_PGID,
> +					       PGID_SRC + port);
> +			/* Ocelot switches forward to the NPI port despite it
> +			 * not being in the source ports' forwarding domain.
> +			 */
> +			if (ocelot->npi >= 0)
> +				mask |= BIT(ocelot->npi);
> +		}
> +
> +		for_each_set_bit(other_port, &mask, ocelot->num_phys_ports) {
> +			struct ocelot_port *other_ocelot_port;
> +
> +			other_ocelot_port = ocelot->ports[other_port];
> +			if (other_ocelot_port->speed <= 0)
> +				continue;
> +
> +			if (min_speed > other_ocelot_port->speed)
> +				min_speed = other_ocelot_port->speed;

break; ?

> +		}
> +
> +		/* Enable cut-through forwarding for all traffic classes. */
> +		if (ocelot_port->speed == min_speed)

Any particular reason this is not <= ?

> +			val = GENMASK(7, 0);

set: ?

> +		ocelot_write_rix(ocelot, val, ANA_CUT_THRU_CFG, port);
> +	}
> +}

>  static const struct felix_info felix_info_vsc9959 = {
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index 95920668feb0..30c790f401be 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c

> @@ -697,6 +702,8 @@ void ocelot_phylink_mac_link_up(struct ocelot *ocelot, int port,
>  	int mac_speed, mode = 0;
>  	u32 mac_fc_cfg;
>  
> +	ocelot_port->speed = speed;
> +
>  	/* The MAC might be integrated in systems where the MAC speed is fixed
>  	 * and it's the PCS who is performing the rate adaptation, so we have
>  	 * to write "1000Mbps" into the LINK_SPEED field of DEV_CLOCK_CFG
> @@ -772,6 +779,9 @@ void ocelot_phylink_mac_link_up(struct ocelot *ocelot, int port,
>  	/* Core: Enable port for frame transfer */
>  	ocelot_fields_write(ocelot, port,
>  			    QSYS_SWITCH_PORT_MODE_PORT_ENA, 1);

Does this enable forwarding? Is there a window here with forwarding
enabled and old cut-thru masks if we don't clear cut-thru when port
goes down?

> +	if (ocelot->ops->cut_through_fwd)
> +		ocelot->ops->cut_through_fwd(ocelot);
>  }
>  EXPORT_SYMBOL_GPL(ocelot_phylink_mac_link_up);
>  
> @@ -1637,6 +1647,9 @@ void ocelot_apply_bridge_fwd_mask(struct ocelot *ocelot)
>  
>  		ocelot_write_rix(ocelot, mask, ANA_PGID_PGID, PGID_SRC + port);
>  	}

Obviously shooting from the hip here, but I was expecting the cut-thru
update to be before the bridge reconfig if port is joining, and after
if port is leaving. Do you know what I'm getting at?

> +	if (ocelot->ops->cut_through_fwd)
> +		ocelot->ops->cut_through_fwd(ocelot);
>  }
>  EXPORT_SYMBOL(ocelot_apply_bridge_fwd_mask);