[PATCH net-next 0/2] net: dsa: lan9303: IGMP handling

[PATCH net-next 0/2] net: dsa: lan9303: IGMP handling

[Egil Hjelmeland]

Set up the HW switch to trap IGMP packets to CPU port. 
And make sure skb->offload_fwd_mark is cleared for incoming IGMP packets.

skb->offload_fwd_mark calculation is a candidate for consolidation into the
DSA core. The calculation can probably be more polished when done at a point
where DSA has updated skb.  


Egil Hjelmeland (2):
  net: dsa: lan9303: Set up trapping of IGMP to CPU port
  net: dsa: lan9303: Clear offload_fwd_mark for IGMP

 drivers/net/dsa/lan9303-core.c | 26 ++++++++++++++++++++++++++
 net/dsa/tag_lan9303.c          | 13 +++++++++++++
 2 files changed, 39 insertions(+)

-- 
2.11.0

[PATCH net-next 1/2] net: dsa: lan9303: Set up trapping of IGMP to CPU port

[Egil Hjelmeland]

IGMP packets should be trapped to the CPU port. The SW bridge knows
whether to forward to other ports.

With "IGMP snooping for local traffic" merged, IGMP trapping is also
required for stable IGMPv2 operation.

LAN9303 does not trap IGMP packets by default.

Enable IGMP trapping in lan9303_setup.

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

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 320651a57c6f..6d7dee67d822 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -153,6 +153,8 @@
 # define LAN9303_SWE_VLAN_UNTAG_PORT0 BIT(12)
 #define LAN9303_SWE_VLAN_CMD_STS 0x1810
 #define LAN9303_SWE_GLB_INGRESS_CFG 0x1840
+# define LAN9303_SWE_GLB_INGR_IGMP_TRAP BIT(7)
+# define LAN9303_SWE_GLB_INGR_IGMP_PORT(p) BIT(10 + p)
 #define LAN9303_SWE_PORT_STATE 0x1843
 # define LAN9303_SWE_PORT_STATE_FORWARDING_PORT2 (0)
 # define LAN9303_SWE_PORT_STATE_LEARNING_PORT2 BIT(5)
@@ -450,6 +452,21 @@ 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)
 {
@@ -905,6 +922,15 @@ static int lan9303_setup(struct dsa_switch *ds)
 	if (ret)
 		dev_err(chip->dev, "failed to re-enable switching %d\n", ret);
 
+	/* Trap IGMP to port 0 */
+	ret = lan9303_write_switch_reg_mask(chip, LAN9303_SWE_GLB_INGRESS_CFG,
+					    LAN9303_SWE_GLB_INGR_IGMP_TRAP |
+					    LAN9303_SWE_GLB_INGR_IGMP_PORT(0),
+					    LAN9303_SWE_GLB_INGR_IGMP_PORT(1) |
+					    LAN9303_SWE_GLB_INGR_IGMP_PORT(2));
+	if (ret)
+		dev_err(chip->dev, "failed to setup IGMP trap %d\n", ret);
+
 	return 0;
 }
 
-- 
2.11.0

[PATCH net-next 2/2] net: dsa: lan9303: Clear offload_fwd_mark for IGMP

[Egil Hjelmeland]

Now that IGMP packets no longer is flooded in HW, we want the SW bridge to
forward packets based on bridge configuration. To make that happen,
IGMP packets must have skb->offload_fwd_mark = 0.

Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
---
 net/dsa/tag_lan9303.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/net/dsa/tag_lan9303.c b/net/dsa/tag_lan9303.c
index 5ba01fc3c6ba..b8c5e52b2eff 100644
--- a/net/dsa/tag_lan9303.c
+++ b/net/dsa/tag_lan9303.c
@@ -92,6 +92,8 @@ static struct sk_buff *lan9303_rcv(struct sk_buff *skb, struct net_device *dev,
 {
 	u16 *lan9303_tag;
 	unsigned int source_port;
+	u16 ether_type_nw;
+	u8 ip_protocol;
 
 	if (unlikely(!pskb_may_pull(skb, LAN9303_TAG_LEN))) {
 		dev_warn_ratelimited(&dev->dev,
@@ -129,6 +131,17 @@ static struct sk_buff *lan9303_rcv(struct sk_buff *skb, struct net_device *dev,
 	skb->offload_fwd_mark = !ether_addr_equal(skb->data - ETH_HLEN,
 						  eth_stp_addr);
 
+	/* We also need IGMP packets to have skb->offload_fwd_mark = 0.
+	 * Solving this for all conceivable situations would add more cost to
+	 * every packet. Instead we handle just the common case:
+	 * No VLAN tag + Ethernet II framing.
+	 * Test least probable term first.
+	 */
+	ether_type_nw = lan9303_tag[2];
+	ip_protocol = *(skb->data + 9);
+	if (ip_protocol == IPPROTO_IGMP && ether_type_nw == htons(ETH_P_IP))
+		skb->offload_fwd_mark = 0;
+
 	return skb;
 }
 
-- 
2.11.0

Re: [PATCH net-next 0/2] net: dsa: lan9303: IGMP handling

[Andrew Lunn]

> skb->offload_fwd_mark calculation is a candidate for consolidation into the
> DSA core. The calculation can probably be more polished when done at a point
> where DSA has updated skb.  

Hi Egil

Yes, at some point we should do this. But at the moment it is too
early. We don't have enough experience with what the different
switches need. I would like to see 4 or more devices doing it before
we consolidate the code into the core.

	    Andrew

Re: [PATCH net-next 1/2] net: dsa: lan9303: Set up trapping of IGMP to CPU port

[Andrew Lunn]

On Fri, Nov 10, 2017 at 12:54:34PM +0100, Egil Hjelmeland wrote:
> IGMP packets should be trapped to the CPU port. The SW bridge knows
> whether to forward to other ports.
> 
> With "IGMP snooping for local traffic" merged, IGMP trapping is also
> required for stable IGMPv2 operation.
> 
> LAN9303 does not trap IGMP packets by default.
> 
> Enable IGMP trapping in lan9303_setup.
> 
> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 0/2] net: dsa: lan9303: IGMP handling

[Egil Hjelmeland]

Den 10. nov. 2017 15:07, skrev Andrew Lunn:
>> skb->offload_fwd_mark calculation is a candidate for consolidation into the
>> DSA core. The calculation can probably be more polished when done at a point
>> where DSA has updated skb.
> 
> Hi Egil
> 
> Yes, at some point we should do this. But at the moment it is too
> early. We don't have enough experience with what the different
> switches need. I would like to see 4 or more devices doing it before
> we consolidate the code into the core.
> 

Absolutely. My point was rather I prefer not spend time polishing this 
snippet right now, because it will probably be refactored later anyway.

One steppingstone could be to make reusable helper-functions in 
net/dsa/dsa_priv.h. But I leave that to device #2 that implement this ;-)

> 	    Andrew
> 

Egil

Re: [PATCH net-next 0/2] net: dsa: lan9303: IGMP handling

[David Miller]

From: Egil Hjelmeland <privat@egil-hjelmeland.no>
Date: Fri, 10 Nov 2017 12:54:33 +0100

> Set up the HW switch to trap IGMP packets to CPU port. 
> And make sure skb->offload_fwd_mark is cleared for incoming IGMP packets.
> 
> skb->offload_fwd_mark calculation is a candidate for consolidation into the
> DSA core. The calculation can probably be more polished when done at a point
> where DSA has updated skb.  

Series applied, thank you.

Re: [PATCH net-next 2/2] net: dsa: lan9303: Clear offload_fwd_mark for IGMP

[Egil Hjelmeland]

On 10. nov. 2017 12:54, Egil Hjelmeland wrote:
> Now that IGMP packets no longer is flooded in HW, we want the SW bridge to
> forward packets based on bridge configuration. To make that happen,
> IGMP packets must have skb->offload_fwd_mark = 0.
> 
> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
> ---
>   net/dsa/tag_lan9303.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/net/dsa/tag_lan9303.c b/net/dsa/tag_lan9303.c
> index 5ba01fc3c6ba..b8c5e52b2eff 100644
> --- a/net/dsa/tag_lan9303.c
> +++ b/net/dsa/tag_lan9303.c
> @@ -92,6 +92,8 @@ static struct sk_buff *lan9303_rcv(struct sk_buff *skb, struct net_device *dev,
>   {
>   	u16 *lan9303_tag;
>   	unsigned int source_port;
> +	u16 ether_type_nw;
> +	u8 ip_protocol;
>   
>   	if (unlikely(!pskb_may_pull(skb, LAN9303_TAG_LEN))) {
>   		dev_warn_ratelimited(&dev->dev,
> @@ -129,6 +131,17 @@ static struct sk_buff *lan9303_rcv(struct sk_buff *skb, struct net_device *dev,
>   	skb->offload_fwd_mark = !ether_addr_equal(skb->data - ETH_HLEN,
>   						  eth_stp_addr);
>   
> +	/* We also need IGMP packets to have skb->offload_fwd_mark = 0.
> +	 * Solving this for all conceivable situations would add more cost to
> +	 * every packet. Instead we handle just the common case:
> +	 * No VLAN tag + Ethernet II framing.
> +	 * Test least probable term first.
> +	 */
> +	ether_type_nw = lan9303_tag[2];
> +	ip_protocol = *(skb->data + 9);
> +	if (ip_protocol == IPPROTO_IGMP && ether_type_nw == htons(ETH_P_IP))
> +		skb->offload_fwd_mark = 0;
> +
>   	return skb;
>   }
>   
> 

RTFM, my bad. The lan9303 has both STP and IGMP bits in the receive tag. 
It is as simple as:

u16 lan9303_tag1 = ntohs(lan9303_tag[1]);
skb->offload_fwd_mark = !(lan9303_tag1 & 0x18);

Egil

Re: [PATCH net-next 2/2] net: dsa: lan9303: Clear offload_fwd_mark for IGMP

[Andrew Lunn]

> RTFM, my bad. The lan9303 has both STP and IGMP bits in the receive tag. It
> is as simple as:
> 
> u16 lan9303_tag1 = ntohs(lan9303_tag[1]);
> skb->offload_fwd_mark = !(lan9303_tag1 & 0x18);

Hi Egil

That is much nicer. But please add a couple of #defines for the 0x18
bits.

	Andrew

Re: [PATCH net-next 2/2] net: dsa: lan9303: Clear offload_fwd_mark for IGMP

[Egil Hjelmeland]

On 13. nov. 2017 14:02, Andrew Lunn wrote:
>> RTFM, my bad. The lan9303 has both STP and IGMP bits in the receive tag. It
>> is as simple as:
>>
>> u16 lan9303_tag1 = ntohs(lan9303_tag[1]);
>> skb->offload_fwd_mark = !(lan9303_tag1 & 0x18);
> 
> Hi Egil
> 
> That is much nicer. But please add a couple of #defines for the 0x18
> bits.
> 
Of course!

> 	Andrew
> 

Egil