[PATCH net-next 0/2] lan9303: Add basic offloading of unicast traffic

[PATCH net-next 0/2] lan9303: Add basic offloading of unicast traffic

[Egil Hjelmeland]

This series add basic offloading of unicast traffic to the lan9303
DSA driver.

Comments welcome!

Egil Hjelmeland (2):
  net: dsa: lan9303: Move tag setup to new lan9303_setup_tagging
  net: dsa: lan9303: Add basic offloading of unicast traffic

 drivers/net/dsa/lan9303-core.c | 130 +++++++++++++++++++++++++++++++++++------
 drivers/net/dsa/lan9303.h      |   1 +
 2 files changed, 114 insertions(+), 17 deletions(-)

-- 
2.11.0

[PATCH net-next 1/2] net: dsa: lan9303: Move tag setup to new lan9303_setup_tagging

[Egil Hjelmeland]

Prepare for next patch:
Move tag setup from lan9303_separate_ports() to new function
lan9303_setup_tagging()

Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
---
 drivers/net/dsa/lan9303-core.c | 42 +++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 07355db2ad81..bba875e114e7 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -157,6 +157,7 @@
 # define LAN9303_SWE_PORT_MIRROR_ENABLE_RX_MIRRORING BIT(1)
 # define LAN9303_SWE_PORT_MIRROR_ENABLE_TX_MIRRORING BIT(0)
 #define LAN9303_SWE_INGRESS_PORT_TYPE 0x1847
+#define  LAN9303_SWE_INGRESS_PORT_TYPE_VLAN 3
 #define LAN9303_BM_CFG 0x1c00
 #define LAN9303_BM_EGRSS_PORT_TYPE 0x1c0c
 # define LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT2 (BIT(17) | BIT(16))
@@ -510,11 +511,30 @@ static int lan9303_enable_processing_port(struct lan9303 *chip,
 				LAN9303_MAC_TX_CFG_X_TX_ENABLE);
 }
 
+/* forward special tagged packets from port 0 to port 1 *or* port 2 */
+static int lan9303_setup_tagging(struct lan9303 *chip)
+{
+	int ret;
+	/* enable defining the destination port via special VLAN tagging
+	 * for port 0
+	 */
+	ret = lan9303_write_switch_reg(chip, LAN9303_SWE_INGRESS_PORT_TYPE,
+				       LAN9303_SWE_INGRESS_PORT_TYPE_VLAN);
+	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)
 {
@@ -529,22 +549,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 |
@@ -644,6 +648,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);
-- 
2.11.0

[PATCH net-next 2/2] net: dsa: lan9303: Add basic offloading of unicast traffic

[Egil Hjelmeland]

When both user ports are joined to the same bridge, the normal
HW MAC learning is enabled. This means that unicast traffic is forwarded
in HW.

If one of the user ports leave the bridge,
the ports goes back to the initial separated operation.

Port separation relies on disabled HW MAC learning. Hence the condition
that both ports must join same bridge.

Add brigde methods port_bridge_join, port_bridge_leave and
port_stp_state_set.

Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
---
 drivers/net/dsa/lan9303-core.c | 88 ++++++++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/lan9303.h      |  1 +
 2 files changed, 89 insertions(+)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index bba875e114e7..76112674fe6a 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -18,6 +18,7 @@
 #include <linux/mutex.h>
 #include <linux/mii.h>
 #include <linux/phy.h>
+#include <linux/if_bridge.h>
 
 #include "lan9303.h"
 
@@ -146,6 +147,7 @@
 # define LAN9303_SWE_PORT_STATE_FORWARDING_PORT0 (0)
 # define LAN9303_SWE_PORT_STATE_LEARNING_PORT0 BIT(1)
 # define LAN9303_SWE_PORT_STATE_BLOCKING_PORT0 BIT(0)
+# define LAN9303_SWE_PORT_STATE_DISABLED_PORT0 (3)
 #define LAN9303_SWE_PORT_MIRROR 0x1846
 # define LAN9303_SWE_PORT_MIRROR_SNIFF_ALL BIT(8)
 # define LAN9303_SWE_PORT_MIRROR_SNIFFER_PORT2 BIT(7)
@@ -431,6 +433,20 @@ static int lan9303_read_switch_reg(struct lan9303 *chip, u16 regnum, u32 *val)
 	return ret;
 }
 
+static int lan9303_write_switch_reg_mask(
+	struct lan9303 *chip, u16 regnum, u32 val, u32 mask)
+{
+	int ret;
+	u32 reg;
+
+	ret = lan9303_read_switch_reg(chip, regnum, &reg);
+	if (ret)
+		return ret;
+	reg = (reg & ~mask) | val;
+
+	return lan9303_write_switch_reg(chip, regnum, reg);
+}
+
 static int lan9303_write_switch_port(struct lan9303 *chip, int port,
 				     u16 regnum, u32 val)
 {
@@ -556,6 +572,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)
@@ -844,6 +866,69 @@ 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; /* touching SWE_PORT_STATE will break port separation */
+
+	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:
+		portstate = LAN9303_SWE_PORT_STATE_DISABLED_PORT0;
+		dev_err(chip->dev, "unknown stp state: port %d, state %d\n",
+			port, state);
+	}
+
+	portmask = 0x3 << (port * 2);
+	portstate     <<= (port * 2);
+	lan9303_write_switch_reg_mask(chip, LAN9303_SWE_PORT_STATE,
+				      portstate, portmask);
+}
+
 static const struct dsa_switch_ops lan9303_switch_ops = {
 	.get_tag_protocol = lan9303_get_tag_protocol,
 	.setup = lan9303_setup,
@@ -855,6 +940,9 @@ static const struct dsa_switch_ops lan9303_switch_ops = {
 	.get_sset_count = lan9303_get_sset_count,
 	.port_enable = lan9303_port_enable,
 	.port_disable = lan9303_port_disable,
+	.port_bridge_join       = lan9303_port_bridge_join,
+	.port_bridge_leave      = lan9303_port_bridge_leave,
+	.port_stp_state_set     = lan9303_port_stp_state_set,
 };
 
 static int lan9303_register_switch(struct lan9303 *chip)
diff --git a/drivers/net/dsa/lan9303.h b/drivers/net/dsa/lan9303.h
index 4d8be555ff4d..5be246f05965 100644
--- a/drivers/net/dsa/lan9303.h
+++ b/drivers/net/dsa/lan9303.h
@@ -21,6 +21,7 @@ struct lan9303 {
 	struct dsa_switch *ds;
 	struct mutex indirect_mutex; /* protect indexed register access */
 	const struct lan9303_phy_ops *ops;
+	bool is_bridged; /* true if port 1 and 2 is bridged */
 };
 
 extern const struct regmap_access_table lan9303_register_set;
-- 
2.11.0

Re: [PATCH net-next 2/2] net: dsa: lan9303: Add basic offloading of unicast traffic

[Andrew Lunn]

Hi Egil

> +static void lan9303_bridge_ports(struct lan9303 *chip)
> +{
> +	/* ports bridged: remove mirroring */
> +	lan9303_write_switch_reg(chip, LAN9303_SWE_PORT_MIRROR, 0);
> +}

Could you replace the 0 with something symbolic which makes this
easier to understand.

#define LAN9303_SWE_PORT_MIRROR_DISABLED 0

> +
>  static int lan9303_handle_reset(struct lan9303 *chip)
>  {
>  	if (!chip->reset_gpio)
> @@ -844,6 +866,69 @@ 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; /* touching SWE_PORT_STATE will break port separation */

I'm wondering how this is supposed to work. Please add a good comment
here, since the hardware is forcing you to do something odd.

Maybe it would be a good idea to save the STP state in chip.  And then
when chip->is_bridged is set true, change the state in the hardware to
the saved value?

What happens when port 0 is added to the bridge, there is then a
minute pause and then port 1 is added? I would expect that as soon as
port 0 is added, the STP state machine for port 0 will start and move
into listening and then forwarding. Due to hardware limitations it
looks like you cannot do this. So what state is the hardware
effectively in? Blocking? Forwarding?

Then port 1 is added. You can then can respect the states. port 1 will
do blocking->listening->forwarding, but what about port 0? The calls
won't get repeated? How does it transition to forwarding?

  Andrew

> +
> +	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:
> +		portstate = LAN9303_SWE_PORT_STATE_DISABLED_PORT0;
> +		dev_err(chip->dev, "unknown stp state: port %d, state %d\n",
> +			port, state);
> +	}
> +
> +	portmask = 0x3 << (port * 2);
> +	portstate     <<= (port * 2);
> +	lan9303_write_switch_reg_mask(chip, LAN9303_SWE_PORT_STATE,
> +				      portstate, portmask);
> +}

Re: [PATCH net-next 2/2] net: dsa: lan9303: Add basic offloading of unicast traffic

[Vivien Didelot]

Hi Egil,

Egil Hjelmeland <privat@egil-hjelmeland.no> writes:

> When both user ports are joined to the same bridge, the normal
> HW MAC learning is enabled. This means that unicast traffic is forwarded
> in HW.
>
> If one of the user ports leave the bridge,
> the ports goes back to the initial separated operation.
>
> Port separation relies on disabled HW MAC learning. Hence the condition
> that both ports must join same bridge.
>
> Add brigde methods port_bridge_join, port_bridge_leave and
> port_stp_state_set.
>
> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>

Styling nitpicks below, other than that, the patch LGTM:

Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

>  #include <linux/mutex.h>
>  #include <linux/mii.h>
>  #include <linux/phy.h>
> +#include <linux/if_bridge.h>

It's nice to order header inclusions alphabetically.

>  
>  #include "lan9303.h"
>  
> @@ -146,6 +147,7 @@
>  # define LAN9303_SWE_PORT_STATE_FORWARDING_PORT0 (0)
>  # define LAN9303_SWE_PORT_STATE_LEARNING_PORT0 BIT(1)
>  # define LAN9303_SWE_PORT_STATE_BLOCKING_PORT0 BIT(0)
> +# define LAN9303_SWE_PORT_STATE_DISABLED_PORT0 (3)
>  #define LAN9303_SWE_PORT_MIRROR 0x1846
>  # define LAN9303_SWE_PORT_MIRROR_SNIFF_ALL BIT(8)
>  # define LAN9303_SWE_PORT_MIRROR_SNIFFER_PORT2 BIT(7)
> @@ -431,6 +433,20 @@ static int lan9303_read_switch_reg(struct lan9303 *chip, u16 regnum, u32 *val)
>  	return ret;
>  }
>  
> +static int lan9303_write_switch_reg_mask(
> +	struct lan9303 *chip, u16 regnum, u32 val, u32 mask)

That is unlikely to respect the Kernel Coding Style. Please fill the
line as much as possible and align with the opening parenthesis:

    static int lan9303_write_switch_reg_mask(struct lan9303 *chip, u16 regnum,
                                             u32 val, u32 mask)

> +{
> +	int ret;
> +	u32 reg;
> +
> +	ret = lan9303_read_switch_reg(chip, regnum, &reg);
> +	if (ret)
> +		return ret;
> +	reg = (reg & ~mask) | val;
> +
> +	return lan9303_write_switch_reg(chip, regnum, reg);
> +}

...

> +
> +	portmask = 0x3 << (port * 2);
> +	portstate     <<= (port * 2);

Unnecessary alignment, please remove the extra space characters.

> +	lan9303_write_switch_reg_mask(chip, LAN9303_SWE_PORT_STATE,
> +				      portstate, portmask);
> +}
> +
>  static const struct dsa_switch_ops lan9303_switch_ops = {
>  	.get_tag_protocol = lan9303_get_tag_protocol,
>  	.setup = lan9303_setup,
> @@ -855,6 +940,9 @@ static const struct dsa_switch_ops lan9303_switch_ops = {
>  	.get_sset_count = lan9303_get_sset_count,
>  	.port_enable = lan9303_port_enable,
>  	.port_disable = lan9303_port_disable,
> +	.port_bridge_join       = lan9303_port_bridge_join,
> +	.port_bridge_leave      = lan9303_port_bridge_leave,
> +	.port_stp_state_set     = lan9303_port_stp_state_set,

Same here, alignment unnecessary, especially since the rest of the
structure does not do that.

>  };
>  
>  static int lan9303_register_switch(struct lan9303 *chip)
> diff --git a/drivers/net/dsa/lan9303.h b/drivers/net/dsa/lan9303.h
> index 4d8be555ff4d..5be246f05965 100644
> --- a/drivers/net/dsa/lan9303.h
> +++ b/drivers/net/dsa/lan9303.h
> @@ -21,6 +21,7 @@ struct lan9303 {
>  	struct dsa_switch *ds;
>  	struct mutex indirect_mutex; /* protect indexed register access */
>  	const struct lan9303_phy_ops *ops;
> +	bool is_bridged; /* true if port 1 and 2 is bridged */
>  };
>  
>  extern const struct regmap_access_table lan9303_register_set;

Please use the checkpatch.pl script to ensure your patch respects the
kernel conventions before submitting, it can spot nice stuffs!

I use a Git alias(*) to check a commit which does basically this:

    git format-patch --stdout -1 | ./scripts/checkpatch.pl -


(*) in details, especially convenient during interactive rebases:

    $ git config alias.checkcommit '!f () { git format-patch --stdout -1 ${1:-HEAD} | ./scripts/checkpatch.pl - ; }; f'
    $ git checkcommit # i.e. current one
    $ git checkcommit HEAD^^
    $ git checkcommit d329ac88eb21
    ...


Thanks,

        Vivien

Re: [PATCH net-next 1/2] net: dsa: lan9303: Move tag setup to new lan9303_setup_tagging

[Vivien Didelot]

Hi Egil,

Egil Hjelmeland <privat@egil-hjelmeland.no> writes:

> Prepare for next patch:
> Move tag setup from lan9303_separate_ports() to new function
> lan9303_setup_tagging()
>
> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>

Minor styling issues, otherwise LGTM:

Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

> +/* forward special tagged packets from port 0 to port 1 *or* port 2 */
> +static int lan9303_setup_tagging(struct lan9303 *chip)
> +{
> +	int ret;
> +	/* enable defining the destination port via special VLAN tagging
> +	 * for port 0
> +	 */
> +	ret = lan9303_write_switch_reg(chip, LAN9303_SWE_INGRESS_PORT_TYPE,
> +				       LAN9303_SWE_INGRESS_PORT_TYPE_VLAN);
> +	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);

Please avoid this kind of alignment as much as possible.

    u32 val = LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0;

would do the trick for the +80 chars issue.

BTW, it'd be great to see sometime soon a cleanup patch which makes use
of such temporary u32 val variable for most of the
lan9303_write_switch_reg and lan9303_write_switch_port calls. ;-)


Thanks,

        Vivien

Re: [PATCH net-next 2/2] net: dsa: lan9303: Add basic offloading of unicast traffic

[Egil Hjelmeland]

Den 21. sep. 2017 16:21, skrev Andrew Lunn:
> Hi Egil
> 
>> +static void lan9303_bridge_ports(struct lan9303 *chip)
>> +{
>> +	/* ports bridged: remove mirroring */
>> +	lan9303_write_switch_reg(chip, LAN9303_SWE_PORT_MIRROR, 0);
>> +}
> 
> Could you replace the 0 with something symbolic which makes this
> easier to understand.
> 
> #define LAN9303_SWE_PORT_MIRROR_DISABLED 0
> 

OK

>> +
>>   static int lan9303_handle_reset(struct lan9303 *chip)
>>   {
>>   	if (!chip->reset_gpio)
>> @@ -844,6 +866,69 @@ 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; /* touching SWE_PORT_STATE will break port separation */
> 
> I'm wondering how this is supposed to work. Please add a good comment
> here, since the hardware is forcing you to do something odd.
> 
> Maybe it would be a good idea to save the STP state in chip.  And then
> when chip->is_bridged is set true, change the state in the hardware to
> the saved value?
> 
> What happens when port 0 is added to the bridge, there is then a
> minute pause and then port 1 is added? I would expect that as soon as
> port 0 is added, the STP state machine for port 0 will start and move
> into listening and then forwarding. Due to hardware limitations it
> looks like you cannot do this. So what state is the hardware
> effectively in? Blocking? Forwarding?
> 
> Then port 1 is added. You can then can respect the states. port 1 will
> do blocking->listening->forwarding, but what about port 0? The calls
> won't get repeated? How does it transition to forwarding?
> 
>    Andrew
> 

I see your point with the "minute pause" argument. Although a bit
contrived use case, it is easy to fix by caching the STP state, as
you suggest. So I can do that.

The port separation method is from the original version of the driver,
not by me.

I have read through the datasheet to see if there are other ways
to make port separation, but I could not find anything.
If anybody care to read through the 300+ page lan9303.pdf and prove
me wrong, I am happy to do it differently.

How does other DSA HW chips handle port separation? Knowing that
could perhaps help me know what to look for.

Egil
'

>> +
>> +	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:
>> +		portstate = LAN9303_SWE_PORT_STATE_DISABLED_PORT0;
>> +		dev_err(chip->dev, "unknown stp state: port %d, state %d\n",
>> +			port, state);
>> +	}
>> +
>> +	portmask = 0x3 << (port * 2);
>> +	portstate     <<= (port * 2);
>> +	lan9303_write_switch_reg_mask(chip, LAN9303_SWE_PORT_STATE,
>> +				      portstate, portmask);
>> +}
> 

Re: [PATCH net-next 2/2] net: dsa: lan9303: Add basic offloading of unicast traffic

[Egil Hjelmeland]

Den 21. sep. 2017 16:26, skrev Vivien Didelot:
> Hi Egil,
> 
> Egil Hjelmeland <privat@egil-hjelmeland.no> writes:
> 
>> When both user ports are joined to the same bridge, the normal
>> HW MAC learning is enabled. This means that unicast traffic is forwarded
>> in HW.
>>
>> If one of the user ports leave the bridge,
>> the ports goes back to the initial separated operation.
>>
>> Port separation relies on disabled HW MAC learning. Hence the condition
>> that both ports must join same bridge.
>>
>> Add brigde methods port_bridge_join, port_bridge_leave and
>> port_stp_state_set.
>>
>> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
> 
> Styling nitpicks below, other than that, the patch LGTM:
> 
> Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> 
>>   #include <linux/mutex.h>
>>   #include <linux/mii.h>
>>   #include <linux/phy.h>
>> +#include <linux/if_bridge.h>
> 
> It's nice to order header inclusions alphabetically.
> 
>>   
>>   #include "lan9303.h"
>>   
>> @@ -146,6 +147,7 @@
>>   # define LAN9303_SWE_PORT_STATE_FORWARDING_PORT0 (0)
>>   # define LAN9303_SWE_PORT_STATE_LEARNING_PORT0 BIT(1)
>>   # define LAN9303_SWE_PORT_STATE_BLOCKING_PORT0 BIT(0)
>> +# define LAN9303_SWE_PORT_STATE_DISABLED_PORT0 (3)
>>   #define LAN9303_SWE_PORT_MIRROR 0x1846
>>   # define LAN9303_SWE_PORT_MIRROR_SNIFF_ALL BIT(8)
>>   # define LAN9303_SWE_PORT_MIRROR_SNIFFER_PORT2 BIT(7)
>> @@ -431,6 +433,20 @@ static int lan9303_read_switch_reg(struct lan9303 *chip, u16 regnum, u32 *val)
>>   	return ret;
>>   }
>>   
>> +static int lan9303_write_switch_reg_mask(
>> +	struct lan9303 *chip, u16 regnum, u32 val, u32 mask)
> 
> That is unlikely to respect the Kernel Coding Style. Please fill the
> line as much as possible and align with the opening parenthesis:
> 
>      static int lan9303_write_switch_reg_mask(struct lan9303 *chip, u16 regnum,
>                                               u32 val, u32 mask)
> 
OK. Probably this function will go away in v2 due to Andrews comment.

>> +{
>> +	int ret;
>> +	u32 reg;
>> +
>> +	ret = lan9303_read_switch_reg(chip, regnum, &reg);
>> +	if (ret)
>> +		return ret;
>> +	reg = (reg & ~mask) | val;
>> +
>> +	return lan9303_write_switch_reg(chip, regnum, reg);
>> +}
> 
> ...
> 
>> +
>> +	portmask = 0x3 << (port * 2);
>> +	portstate     <<= (port * 2);
> 
> Unnecessary alignment, please remove the extra space characters.
> 

I think the alignment makes the logic more clear.


>> +	lan9303_write_switch_reg_mask(chip, LAN9303_SWE_PORT_STATE,
>> +				      portstate, portmask);
>> +}
>> +
>>   static const struct dsa_switch_ops lan9303_switch_ops = {
>>   	.get_tag_protocol = lan9303_get_tag_protocol,
>>   	.setup = lan9303_setup,
>> @@ -855,6 +940,9 @@ static const struct dsa_switch_ops lan9303_switch_ops = {
>>   	.get_sset_count = lan9303_get_sset_count,
>>   	.port_enable = lan9303_port_enable,
>>   	.port_disable = lan9303_port_disable,
>> +	.port_bridge_join       = lan9303_port_bridge_join,
>> +	.port_bridge_leave      = lan9303_port_bridge_leave,
>> +	.port_stp_state_set     = lan9303_port_stp_state_set,
> 
> Same here, alignment unnecessary, especially since the rest of the
> structure does not do that.
> 
>>   };
>>   
>>   static int lan9303_register_switch(struct lan9303 *chip)
>> diff --git a/drivers/net/dsa/lan9303.h b/drivers/net/dsa/lan9303.h
>> index 4d8be555ff4d..5be246f05965 100644
>> --- a/drivers/net/dsa/lan9303.h
>> +++ b/drivers/net/dsa/lan9303.h
>> @@ -21,6 +21,7 @@ struct lan9303 {
>>   	struct dsa_switch *ds;
>>   	struct mutex indirect_mutex; /* protect indexed register access */
>>   	const struct lan9303_phy_ops *ops;
>> +	bool is_bridged; /* true if port 1 and 2 is bridged */
>>   };
>>   
>>   extern const struct regmap_access_table lan9303_register_set;
> 
> Please use the checkpatch.pl script to ensure your patch respects the
> kernel conventions before submitting, it can spot nice stuffs!

I have checked _every_ patch with checkpatch.pl and weeded all warnings
before I submitted them.

> 
> I use a Git alias(*) to check a commit which does basically this:
> 
>      git format-patch --stdout -1 | ./scripts/checkpatch.pl -
> 
> 
> (*) in details, especially convenient during interactive rebases:
> 
>      $ git config alias.checkcommit '!f () { git format-patch --stdout -1 ${1:-HEAD} | ./scripts/checkpatch.pl - ; }; f'
>      $ git checkcommit # i.e. current one
>      $ git checkcommit HEAD^^
>      $ git checkcommit d329ac88eb21
>      ...
> 
> 
> Thanks,
> 
>          Vivien
> 

Re: [PATCH net-next 2/2] net: dsa: lan9303: Add basic offloading of unicast traffic

[Andrew Lunn]

> >I'm wondering how this is supposed to work. Please add a good comment
> >here, since the hardware is forcing you to do something odd.
> >
> >Maybe it would be a good idea to save the STP state in chip.  And then
> >when chip->is_bridged is set true, change the state in the hardware to
> >the saved value?
> >
> >What happens when port 0 is added to the bridge, there is then a
> >minute pause and then port 1 is added? I would expect that as soon as
> >port 0 is added, the STP state machine for port 0 will start and move
> >into listening and then forwarding. Due to hardware limitations it
> >looks like you cannot do this. So what state is the hardware
> >effectively in? Blocking? Forwarding?
> >
> >Then port 1 is added. You can then can respect the states. port 1 will
> >do blocking->listening->forwarding, but what about port 0? The calls
> >won't get repeated? How does it transition to forwarding?
> >
> >   Andrew
> >
> 
> I see your point with the "minute pause" argument. Although a bit
> contrived use case, it is easy to fix by caching the STP state, as
> you suggest. So I can do that.

I don't think it is contrived. I've done bridge configuration by hand
for testing purposes. I've also set the forwarding delay to very small
values, so there is a clear race condition here.

> How does other DSA HW chips handle port separation? Knowing that
> could perhaps help me know what to look for.

They have better hardware :-)

Generally each port is totally independent. You can change the STP
state per port without restrictions.

      Andrew

Re: [PATCH net-next 2/2] net: dsa: lan9303: Add basic offloading of unicast traffic

[Egil Hjelmeland]

Den 22. sep. 2017 22:08, skrev Andrew Lunn:
>>> I'm wondering how this is supposed to work. Please add a good comment
>>> here, since the hardware is forcing you to do something odd.
>>>
>>> Maybe it would be a good idea to save the STP state in chip.  And then
>>> when chip->is_bridged is set true, change the state in the hardware to
>>> the saved value?
>>>
>>> What happens when port 0 is added to the bridge, there is then a
>>> minute pause and then port 1 is added? I would expect that as soon as
>>> port 0 is added, the STP state machine for port 0 will start and move
>>> into listening and then forwarding. Due to hardware limitations it
>>> looks like you cannot do this. So what state is the hardware
>>> effectively in? Blocking? Forwarding?
>>>
>>> Then port 1 is added. You can then can respect the states. port 1 will
>>> do blocking->listening->forwarding, but what about port 0? The calls
>>> won't get repeated? How does it transition to forwarding?
>>>
>>>    Andrew
>>>
>>
>> I see your point with the "minute pause" argument. Although a bit
>> contrived use case, it is easy to fix by caching the STP state, as
>> you suggest. So I can do that.
> 
> I don't think it is contrived. I've done bridge configuration by hand
> for testing purposes. I've also set the forwarding delay to very small
> values, so there is a clear race condition here.
> 
>> How does other DSA HW chips handle port separation? Knowing that
>> could perhaps help me know what to look for.
> 
> They have better hardware :-)
> 
> Generally each port is totally independent. You can change the STP
> state per port without restrictions.
> 
We can indeed change the STP state per lan9303 port "without
restrictions".

The point is: Once both external ports are in "forwarding", I see no way
to prevent traffic flowing directly between the external ports.


>        Andrew
> 

Egil

Re: [PATCH net-next 2/2] net: dsa: lan9303: Add basic offloading of unicast traffic

[Andrew Lunn]

> The point is: Once both external ports are in "forwarding", I see no way
> to prevent traffic flowing directly between the external ports.

Generally, there are port vectors. Port X can send frames only to Port
Y.

If you don't have that, there are possibilities with VLANs. Each port
is given a unique VLAN. All incoming untagged traffic is tagged with
the VLAN. You just need to keep the VLAN separated and add/remove the
VLAN tag in the dsa tag driver.

     Andrew

Re: [PATCH net-next 2/2] net: dsa: lan9303: Add basic offloading of unicast traffic

[Egil Hjelmeland]

Den 23. sep. 2017 16:31, skrev Andrew Lunn:
>> The point is: Once both external ports are in "forwarding", I see no way
>> to prevent traffic flowing directly between the external ports.
> 
> Generally, there are port vectors. Port X can send frames only to Port
> Y.
> 
> If you don't have that, there are possibilities with VLANs. Each port
> is given a unique VLAN. All incoming untagged traffic is tagged with
> the VLAN. You just need to keep the VLAN separated and add/remove the
> VLAN tag in the dsa tag driver.
> 
>       Andrew
> 
Thanks. The lan9303 has nothing like "port vectors". The port tagging
scheme is VLAN based, but is does not prevent direct forwarding between
the external ports.

In order to not break the strong port separation in the current driver;
I will stick to my solution, and only add caching of the STP state
register.

Egil