netdev - Re: [PATCH net-next v5 2/3] net: dsa: add Arrow SpeedChips XRS700x driver

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: <20210114224843.374dmyzvtszat6m4@skbuf>
Date:   Fri, 15 Jan 2021 00:48:43 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     George McCollister <george.mccollister@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Rob Herring <robh@...nel.org>,
        "David S . Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        devicetree@...r.kernel.org
Subject: Re: [PATCH net-next v5 2/3] net: dsa: add Arrow SpeedChips XRS700x
 driver

On Thu, Jan 14, 2021 at 01:57:33PM -0600, George McCollister wrote:
> Add a driver with initial support for the Arrow SpeedChips XRS7000
> series of gigabit Ethernet switch chips which are typically used in
> critical networking applications.
> 
> The switches have up to three RGMII ports and one RMII port.
> Management to the switches can be performed over i2c or mdio.
> 
> Support for advanced features such as PTP and
> HSR/PRP (IEC 62439-3 Clause 5 & 4) is not included in this patch and
> may be added at a later date.
> 
> Signed-off-by: George McCollister <george.mccollister@...il.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@...il.com>

This driver is good to go, just one small nitpick below, you can fix it
up afterwards if you want.

> +static void xrs700x_port_stp_state_set(struct dsa_switch *ds, int port,
> +				       u8 state)
> +{
> +	struct xrs700x *priv = ds->priv;
> +	unsigned int bpdus = 1;
> +	unsigned int val;
> +
> +	switch (state) {
> +	case BR_STATE_DISABLED:
> +		bpdus = 0;
> +		fallthrough;
> +	case BR_STATE_BLOCKING:
> +	case BR_STATE_LISTENING:
> +		val = XRS_PORT_DISABLED;
> +		break;
> +	case BR_STATE_LEARNING:
> +		val = XRS_PORT_LEARNING;
> +		break;
> +	case BR_STATE_FORWARDING:
> +		val = XRS_PORT_FORWARDING;
> +		break;
> +	default:
> +		dev_err(ds->dev, "invalid STP state: %d\n", state);
> +		return;
> +	}
> +
> +	regmap_fields_write(priv->ps_forward, port, val);
> +
> +	/* Enable/disable inbound policy added by xrs700x_port_add_bpdu_ipf()
> +	 * which allows BPDU forwarding to the CPU port when the front facing
> +	 * port is in disabled/learning state.
                      ~~~~~~~~
You probably mean blocking. When the port is in BR_STATE_DISABLED, you
set bpdus = 1, which makes sense.

> +	 */
> +	regmap_update_bits(priv->regmap, XRS_ETH_ADDR_CFG(port, 0), 1, bpdus);
> +
> +	dev_dbg_ratelimited(priv->dev, "%s - port: %d, state: %u, val: 0x%x\n",
> +			    __func__, port, state, val);
> +}