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

[PATCH v2 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.

Review welcome!
 
Changes v1 -> v2:
 - Patch 1: Codestyle linting.
 - Patch 2: Remember SWE_PORT_STATE while not bridged.
            Added constant LAN9303_SWE_PORT_MIRROR_DISABLED.


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 | 124 +++++++++++++++++++++++++++++++++++------
 drivers/net/dsa/lan9303.h      |   2 +
 2 files changed, 109 insertions(+), 17 deletions(-)

-- 
2.11.0

[PATCH v2 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..2215ec1fbe1e 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;
+	u32 val;
+	/* 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
+	 */
+	val = LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0;
+	return lan9303_write_switch_reg(chip, LAN9303_BM_EGRSS_PORT_TYPE, val);
+}
+
 /* 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 v2 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 | 82 ++++++++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/lan9303.h      |  2 ++
 2 files changed, 84 insertions(+)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 2215ec1fbe1e..fecfe1fe67ea 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)
@@ -156,6 +158,7 @@
 # define LAN9303_SWE_PORT_MIRROR_MIRRORED_PORT0 BIT(2)
 # define LAN9303_SWE_PORT_MIRROR_ENABLE_RX_MIRRORING BIT(1)
 # define LAN9303_SWE_PORT_MIRROR_ENABLE_TX_MIRRORING BIT(0)
+# define LAN9303_SWE_PORT_MIRROR_DISABLED 0
 #define LAN9303_SWE_INGRESS_PORT_TYPE 0x1847
 #define  LAN9303_SWE_INGRESS_PORT_TYPE_VLAN 3
 #define LAN9303_BM_CFG 0x1c00
@@ -556,6 +559,16 @@ 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,
+				 LAN9303_SWE_PORT_MIRROR_DISABLED);
+
+	lan9303_write_switch_reg(chip, LAN9303_SWE_PORT_STATE,
+				 chip->swe_port_state);
+}
+
 static int lan9303_handle_reset(struct lan9303 *chip)
 {
 	if (!chip->reset_gpio)
@@ -844,6 +857,72 @@ 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);
+
+	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);
+
+	chip->swe_port_state = (chip->swe_port_state & ~portmask) | portstate;
+
+	if (chip->is_bridged)
+		lan9303_write_switch_reg(chip, LAN9303_SWE_PORT_STATE,
+					 chip->swe_port_state);
+	/* else: touching SWE_PORT_STATE would break port separation */
+}
+
 static const struct dsa_switch_ops lan9303_switch_ops = {
 	.get_tag_protocol = lan9303_get_tag_protocol,
 	.setup = lan9303_setup,
@@ -855,6 +934,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..68ecd544b658 100644
--- a/drivers/net/dsa/lan9303.h
+++ b/drivers/net/dsa/lan9303.h
@@ -21,6 +21,8 @@ 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 are bridged */
+	u32 swe_port_state; /* remember SWE_PORT_STATE while not bridged */
 };
 
 extern const struct regmap_access_table lan9303_register_set;
-- 
2.11.0

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

[Woojung.Huh]

> +/* forward special tagged packets from port 0 to port 1 *or* port 2 */
> +static int lan9303_setup_tagging(struct lan9303 *chip)
> +{
> +	int ret;
> +	u32 val;
> +	/* 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
> +	 */
> +	val = LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0;
> +	return lan9303_write_switch_reg(chip,
> LAN9303_BM_EGRSS_PORT_TYPE, val);
Specific reason to use val then using LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0
like previous line?

> @@ -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);
> +
Still move on when error happens?

>  	ret = lan9303_separate_ports(chip);
>  	if (ret)
>  		dev_err(chip->dev, "failed to separate ports %d\n", ret);
> --
> 2.11.0

- Woojung

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

[Egil Hjelmeland]

On 10. okt. 2017 17:14, Woojung.Huh@microchip.com wrote:
>> +/* forward special tagged packets from port 0 to port 1 *or* port 2 */
>> +static int lan9303_setup_tagging(struct lan9303 *chip)
>> +{
>> +	int ret;
>> +	u32 val;
>> +	/* 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
>> +	 */
>> +	val = LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0;
>> +	return lan9303_write_switch_reg(chip,
>> LAN9303_BM_EGRSS_PORT_TYPE, val);
> Specific reason to use val then using LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0
> like previous line?
> 
Specific reason was to please a reviewer that did not like my
indenting in first version. I did not agree with him, but since
nobody else spoke up, I changed the code.

>> @@ -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);
>> +
> Still move on when error happens?
> 
Good question. I just followed the pattern from the original function,
which was not made by me. Actually I did once reflect on whether this 
was the correct way. Perhaps it could be argued that it is better to 
allow the device to come up, so the problem can be investigated?

>>   	ret = lan9303_separate_ports(chip);
>>   	if (ret)
>>   		dev_err(chip->dev, "failed to separate ports %d\n", ret);
>> --
>> 2.11.0
> 
> - Woojung
> 

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

[Woojung.Huh]

> > Specific reason to use val then using
> LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0
> > like previous line?
> >
> Specific reason was to please a reviewer that did not like my
> indenting in first version. I did not agree with him, but since
> nobody else spoke up, I changed the code.
Got it. Missed previous patch/comment.

> >> @@ -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);
> >> +
> > Still move on when error happens?
> >
> Good question. I just followed the pattern from the original function,
> which was not made by me. Actually I did once reflect on whether this
> was the correct way. Perhaps it could be argued that it is better to
> allow the device to come up, so the problem can be investigated?
Maybe depends on severity of setting?
BTW, lan9303_setup() still returns ZERO at the end?

Thanks.
Woojung

Re: [PATCH v2 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:

>>> +	val = LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0;
>>> +	return lan9303_write_switch_reg(chip,
>>> LAN9303_BM_EGRSS_PORT_TYPE, val);
>> Specific reason to use val then using LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0
>> like previous line?
>> 
> Specific reason was to please a reviewer that did not like my
> indenting in first version. I did not agree with him, but since
> nobody else spoke up, I changed the code.

Your indentation was broken and did not respect the Kernel Coding
Style. Using a temporary variable here ensures you respect both the
80-char limit and the vertical alignment on opening parenthesis.

You'd like to use ./scripts/checkpatch.pl before submitting patches.


Thanks,

        Vivien

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

[Egil Hjelmeland]

On 10. okt. 2017 17:51, Woojung.Huh@microchip.com wrote:
>>> Specific reason to use val then using
>> LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0
>>> like previous line?
>>>
>> Specific reason was to please a reviewer that did not like my
>> indenting in first version. I did not agree with him, but since
>> nobody else spoke up, I changed the code.
> Got it. Missed previous patch/comment.
> 
>>>> @@ -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);
>>>> +
>>> Still move on when error happens?
>>>
>> Good question. I just followed the pattern from the original function,
>> which was not made by me. Actually I did once reflect on whether this
>> was the correct way. Perhaps it could be argued that it is better to
>> allow the device to come up, so the problem can be investigated?
> Maybe depends on severity of setting?
> BTW, lan9303_setup() still returns ZERO at the end?
I did quick survey of the _setup functions of the other dsa drivers.
Some return on error, some ignore errors.
If you think so, I can make a v3 series that return on error. Otherwise
I leave it as it is.

> 
> Thanks.
> Woojung
> 

Thank you for polite review.

Egil

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

[Woojung.Huh]

> >>>> @@ -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);
> >>>> +
> >>> Still move on when error happens?
> >>>
> >> Good question. I just followed the pattern from the original function,
> >> which was not made by me. Actually I did once reflect on whether this
> >> was the correct way. Perhaps it could be argued that it is better to
> >> allow the device to come up, so the problem can be investigated?
> > Maybe depends on severity of setting?
> > BTW, lan9303_setup() still returns ZERO at the end?
> I did quick survey of the _setup functions of the other dsa drivers.
> Some return on error, some ignore errors.
> If you think so, I can make a v3 series that return on error. Otherwise
> I leave it as it is.

Unless Andrew, Vivien or Florian raises flag, I guess it will be fine as-is.

Thanks.
Woojung

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

[David Miller]

From: Egil Hjelmeland <privat@egil-hjelmeland.no>
Date: Tue, 10 Oct 2017 14:49:51 +0200

> This series add basic offloading of unicast traffic to the lan9303
> DSA driver.
> 
> Review welcome!
>  
> Changes v1 -> v2:
>  - Patch 1: Codestyle linting.
>  - Patch 2: Remember SWE_PORT_STATE while not bridged.
>             Added constant LAN9303_SWE_PORT_MIRROR_DISABLED.

Series applied, thanks.