Re: [PATCH net-next v2 07/10] net: dsa: lan9303: Added basic offloading of unicast traffic

From: Andrew Lunn
Date: Wed Jul 26 2017 - 13:25:00 EST


Hi Egil

> +/* forward special tagged packets from port 0 to port 1 *or* port 2 */
> +static int lan9303_setup_tagging(struct lan9303 *chip)
> +{
> + int ret;

Blank line please.


> + /* enable defining the destination port via special VLAN tagging
> + * for port 0
> + */
> + ret = lan9303_write_switch_reg(chip, LAN9303_SWE_INGRESS_PORT_TYPE,
> + 0x03);

#define for 0x03.

> + if (ret)
> + return ret;
> +
> + /* tag incoming packets at port 1 and 2 on their way to port 0 to be
> + * able to discover their source port
> + */
> + return lan9303_write_switch_reg(
> + chip, LAN9303_BM_EGRSS_PORT_TYPE,
> + LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0);
> +}
> +
> /* We want a special working switch:
> * - do not forward packets between port 1 and 2
> * - forward everything from port 1 to port 0
> * - forward everything from port 2 to port 0
> - * - forward special tagged packets from port 0 to port 1 *or* port 2
> */
> static int lan9303_separate_ports(struct lan9303 *chip)
> {
> @@ -534,22 +555,6 @@ static int lan9303_separate_ports(struct lan9303 *chip)
> if (ret)
> return ret;
>
> - /* enable defining the destination port via special VLAN tagging
> - * for port 0
> - */
> - ret = lan9303_write_switch_reg(chip, LAN9303_SWE_INGRESS_PORT_TYPE,
> - 0x03);
> - if (ret)
> - return ret;
> -
> - /* tag incoming packets at port 1 and 2 on their way to port 0 to be
> - * able to discover their source port
> - */
> - ret = lan9303_write_switch_reg(chip, LAN9303_BM_EGRSS_PORT_TYPE,
> - LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0);
> - if (ret)
> - return ret;
> -
> /* prevent port 1 and 2 from forwarding packets by their own */
> return lan9303_write_switch_reg(chip, LAN9303_SWE_PORT_STATE,
> LAN9303_SWE_PORT_STATE_FORWARDING_PORT0 |
> @@ -557,6 +562,12 @@ static int lan9303_separate_ports(struct lan9303 *chip)
> LAN9303_SWE_PORT_STATE_BLOCKING_PORT2);
> }
>
> +static void lan9303_bridge_ports(struct lan9303 *chip)
> +{
> + /* ports bridged: remove mirroring */
> + lan9303_write_switch_reg(chip, LAN9303_SWE_PORT_MIRROR, 0);
> +}
> +
> static int lan9303_handle_reset(struct lan9303 *chip)
> {
> if (!chip->reset_gpio)
> @@ -707,6 +718,10 @@ static int lan9303_setup(struct dsa_switch *ds)
> return -EINVAL;
> }
>
> + ret = lan9303_setup_tagging(chip);
> + if (ret)
> + dev_err(chip->dev, "failed to setup port tagging %d\n", ret);
> +
> ret = lan9303_separate_ports(chip);
> if (ret)
> dev_err(chip->dev, "failed to separate ports %d\n", ret);
> @@ -898,17 +913,81 @@ static void lan9303_port_disable(struct dsa_switch *ds, int port,
> }
> }
>
> +static int lan9303_port_bridge_join(struct dsa_switch *ds, int port,
> + struct net_device *br)
> +{
> + struct lan9303 *chip = ds->priv;
> +
> + dev_dbg(chip->dev, "%s(port %d)\n", __func__, port);
> + if (ds->ports[1].bridge_dev == ds->ports[2].bridge_dev) {
> + lan9303_bridge_ports(chip);
> + chip->is_bridged = true; /* unleash stp_state_set() */
> + }
> +
> + return 0;
> +}
> +
> +static void lan9303_port_bridge_leave(struct dsa_switch *ds, int port,
> + struct net_device *br)
> +{
> + struct lan9303 *chip = ds->priv;
> +
> + dev_dbg(chip->dev, "%s(port %d)\n", __func__, port);
> + if (chip->is_bridged) {
> + lan9303_separate_ports(chip);
> + chip->is_bridged = false;
> + }
> +}
> +
> +static void lan9303_port_stp_state_set(struct dsa_switch *ds, int port,
> + u8 state)
> +{
> + int portmask, portstate;
> + struct lan9303 *chip = ds->priv;
> +
> + dev_dbg(chip->dev, "%s(port %d, state %d)\n",
> + __func__, port, state);
> + if (!chip->is_bridged)
> + return;

I think you are over-simplifying here. Say i have a layer 2 VPN and i
bridge port 1 and the VPN? The software bridge still wants to do STP
on port 1, in order to solve loops.

> +
> + switch (state) {
> + case BR_STATE_DISABLED:
> + portstate = LAN9303_SWE_PORT_STATE_DISABLED_PORT0;
> + break;
> + case BR_STATE_BLOCKING:
> + case BR_STATE_LISTENING:
> + portstate = LAN9303_SWE_PORT_STATE_BLOCKING_PORT0;
> + break;
> + case BR_STATE_LEARNING:
> + portstate = LAN9303_SWE_PORT_STATE_LEARNING_PORT0;
> + break;
> + case BR_STATE_FORWARDING:
> + portstate = LAN9303_SWE_PORT_STATE_FORWARDING_PORT0;
> + break;
> + default:
> + dev_err(chip->dev, "%s(port %d, state %d)\n",
> + __func__, port, state);
> + }
> + portmask = 0x3 << (port * 2);
> + portstate <<= (port * 2);
> + lan9303_write_switch_reg_mask(chip, LAN9303_SWE_PORT_STATE,
> + portstate, portmask);
> +}




> +
> static struct dsa_switch_ops lan9303_switch_ops = {
> .get_tag_protocol = lan9303_get_tag_protocol,
> .setup = lan9303_setup,
> - .get_strings = lan9303_get_strings,

????

> .phy_read = lan9303_phy_read,
> .phy_write = lan9303_phy_write,
> .adjust_link = lan9303_adjust_link,
> + .get_strings = lan9303_get_strings,

Please don't include other unrelated changes.

Andrew