[PATCH net-next v2 0/3] net: dsa: b53: Turn on Broadcom tags

[PATCH net-next v2 0/3] net: dsa: b53: Turn on Broadcom tags

[Florian Fainelli]

Hi all,

This was long overdue, with this patch series, the b53 driver now
turns on Broadcom tags except for 5325 and 5365 which use an older
format that we do not support yet (TBD).

First patch is necessary in order for bgmac, used on BCM5301X and Northstar
Plus to work correctly and successfully send ARP packets back to the requsester.

Second patch is actually a bug fix, but because net/master and net-next/master
diverge in that area, I am targeting net-next/master here.

Finally, the last patch enables Broadcom tags after checking that the CPU port
selected is either, 5, 7 or 8, since those are the only valid combinations
given currently supported HW.

Florian Fainelli (3):
  net: bgmac: Pad packets to a minimum size
  net: dsa: b53: Stop using dev->cpu_port incorrectly
  net: dsa: b53: Turn on Broadcom tags

 drivers/net/dsa/b53/b53_common.c      | 58 ++++++++++++++++++++++++++---------
 drivers/net/ethernet/broadcom/bgmac.c | 13 ++++++++
 2 files changed, 56 insertions(+), 15 deletions(-)

-- 
2.9.3

[PATCH net-next v2 1/3] net: bgmac: Pad packets to a minimum size

[Florian Fainelli]

In preparation for enabling Broadcom tags with b53, pad packets to a
minimum size of 64 bytes (sans FCS) in order for the Broadcom switch to
accept ingressing frames. Without this, we would typically be able to
DHCP, but not resolve with ARP because packets are too small and get
rejected by the switch.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/broadcom/bgmac.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index 48d672b204a4..5130fc96940d 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -127,6 +127,8 @@ bgmac_dma_tx_add_buf(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
 	dma_desc->ctl1 = cpu_to_le32(ctl1);
 }
 
+#define ENET_BRCM_TAG_LEN	4
+
 static netdev_tx_t bgmac_dma_tx_add(struct bgmac *bgmac,
 				    struct bgmac_dma_ring *ring,
 				    struct sk_buff *skb)
@@ -139,6 +141,16 @@ static netdev_tx_t bgmac_dma_tx_add(struct bgmac *bgmac,
 	u32 flags;
 	int i;
 
+	/* The Ethernet switch we are interfaced with needs packets to be at
+	 * least 64 bytes (including FCS) otherwise they will be discarded when
+	 * they enter the switch port logic. When Broadcom tags are enabled, we
+	 * need to make sure that packets are at least 68 bytes
+	 * (including FCS and tag) because the length verification is done after
+	 * the Broadcom tag is stripped off the ingress packet.
+	 */
+	if (skb_put_padto(skb, ETH_ZLEN + ENET_BRCM_TAG_LEN))
+		goto err_stats;
+
 	if (skb->len > BGMAC_DESC_CTL1_LEN) {
 		netdev_err(bgmac->net_dev, "Too long skb (%d)\n", skb->len);
 		goto err_drop;
@@ -225,6 +237,7 @@ static netdev_tx_t bgmac_dma_tx_add(struct bgmac *bgmac,
 
 err_drop:
 	dev_kfree_skb(skb);
+err_stats:
 	net_dev->stats.tx_dropped++;
 	net_dev->stats.tx_errors++;
 	return NETDEV_TX_OK;
-- 
2.9.3

RE: [PATCH net-next v2 1/3] net: bgmac: Pad packets to a minimum size

[David Laight]

From: Florian Fainelli
> Sent: 09 November 2017 22:35
> 
> In preparation for enabling Broadcom tags with b53, pad packets to a
> minimum size of 64 bytes (sans FCS) in order for the Broadcom switch to
> accept ingressing frames. Without this, we would typically be able to
> DHCP, but not resolve with ARP because packets are too small and get
> rejected by the switch.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/net/ethernet/broadcom/bgmac.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
> index 48d672b204a4..5130fc96940d 100644
> --- a/drivers/net/ethernet/broadcom/bgmac.c
> +++ b/drivers/net/ethernet/broadcom/bgmac.c
> @@ -127,6 +127,8 @@ bgmac_dma_tx_add_buf(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
>  	dma_desc->ctl1 = cpu_to_le32(ctl1);
>  }
> 
> +#define ENET_BRCM_TAG_LEN	4
> +
>  static netdev_tx_t bgmac_dma_tx_add(struct bgmac *bgmac,
>  				    struct bgmac_dma_ring *ring,
>  				    struct sk_buff *skb)
> @@ -139,6 +141,16 @@ static netdev_tx_t bgmac_dma_tx_add(struct bgmac *bgmac,
>  	u32 flags;
>  	int i;
> 
> +	/* The Ethernet switch we are interfaced with needs packets to be at
> +	 * least 64 bytes (including FCS) otherwise they will be discarded when
> +	 * they enter the switch port logic. When Broadcom tags are enabled, we
> +	 * need to make sure that packets are at least 68 bytes
> +	 * (including FCS and tag) because the length verification is done after
> +	 * the Broadcom tag is stripped off the ingress packet.
> +	 */

I think that that would be better as:
	/* Ethernet packets are padded to 64 bytes (including FCS).
	 * If 'Broadcom tags' are enabled they must still be 64 bytes
	 * long after the 4 byte tag is removed.
	 * Since the hardware doesn't do it, we must pad them before
	 * transmit.
	 */

Which seems to be to be a bug in the chip.

> +	if (skb_put_padto(skb, ETH_ZLEN + ENET_BRCM_TAG_LEN))
> +		goto err_stats;
> +

But you shouldn't overpad packets that don't have the extra tag.

	David

Re: [PATCH net-next v2 1/3] net: bgmac: Pad packets to a minimum size

[Florian Fainelli]

On 11/10/2017 03:10 AM, David Laight wrote:
> From: Florian Fainelli
>> Sent: 09 November 2017 22:35
>>
>> In preparation for enabling Broadcom tags with b53, pad packets to a
>> minimum size of 64 bytes (sans FCS) in order for the Broadcom switch to
>> accept ingressing frames. Without this, we would typically be able to
>> DHCP, but not resolve with ARP because packets are too small and get
>> rejected by the switch.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  drivers/net/ethernet/broadcom/bgmac.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
>> index 48d672b204a4..5130fc96940d 100644
>> --- a/drivers/net/ethernet/broadcom/bgmac.c
>> +++ b/drivers/net/ethernet/broadcom/bgmac.c
>> @@ -127,6 +127,8 @@ bgmac_dma_tx_add_buf(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
>>  	dma_desc->ctl1 = cpu_to_le32(ctl1);
>>  }
>>
>> +#define ENET_BRCM_TAG_LEN	4
>> +
>>  static netdev_tx_t bgmac_dma_tx_add(struct bgmac *bgmac,
>>  				    struct bgmac_dma_ring *ring,
>>  				    struct sk_buff *skb)
>> @@ -139,6 +141,16 @@ static netdev_tx_t bgmac_dma_tx_add(struct bgmac *bgmac,
>>  	u32 flags;
>>  	int i;
>>
>> +	/* The Ethernet switch we are interfaced with needs packets to be at
>> +	 * least 64 bytes (including FCS) otherwise they will be discarded when
>> +	 * they enter the switch port logic. When Broadcom tags are enabled, we
>> +	 * need to make sure that packets are at least 68 bytes
>> +	 * (including FCS and tag) because the length verification is done after
>> +	 * the Broadcom tag is stripped off the ingress packet.
>> +	 */
> 
> I think that that would be better as:
> 	/* Ethernet packets are padded to 64 bytes (including FCS).
> 	 * If 'Broadcom tags' are enabled they must still be 64 bytes
> 	 * long after the 4 byte tag is removed.
> 	 * Since the hardware doesn't do it, we must pad them before
> 	 * transmit.
> 	 */
> 
> Which seems to be to be a bug in the chip.

The same comment exists in drivers/net/ethernet/broadcom/bcmsysport.c,
and almost 4 years ago, you made pretty much the same type of drive-by
review, so no, I am not changing any of that comment which explains
exactly what is going on.

> 
>> +	if (skb_put_padto(skb, ETH_ZLEN + ENET_BRCM_TAG_LEN))
>> +		goto err_stats;
>> +
> 
> But you shouldn't overpad packets that don't have the extra tag.

I don't see a point in putting a netdev_uses_dsa() check to know whether
we should pad or not to an extra ENET_BRCM_TAG_LEN, the packets are
already hot in cache at that point, and they are small, so sure, this
will put some more cache pressure for host originating 64bytes packets,
but I don't see this being a real use case (except for benchmarks).
-- 
Florian

[PATCH net-next v2 2/3] net: dsa: b53: Stop using dev->cpu_port incorrectly

[Florian Fainelli]

dev->cpu_port is the driver local information that should only be used
to look up register offsets for a particular port, when they differ
(e.g: IMP port override), but it should certainly not be used in place
of the DSA configured CPU port.

Since the DSA switch layer calls port_vlan_{add,del}() on the CPU port
as well, we can remove the specific setting of the CPU port within
port_vlan_{add,del}.

Fixes: ff39c2d68679 ("net: dsa: b53: Add bridge support")
Fixes: 967dd82ffc52 ("net: dsa: b53: Add support for Broadcom RoboSwitch")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/b53/b53_common.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index a7ca62ba27b7..17f12484ce24 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -505,7 +505,7 @@ EXPORT_SYMBOL(b53_imp_vlan_setup);
 int b53_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy)
 {
 	struct b53_device *dev = ds->priv;
-	unsigned int cpu_port = dev->cpu_port;
+	unsigned int cpu_port = ds->ports[port].cpu_dp->index;
 	u16 pvlan;
 
 	/* Clear the Rx and Tx disable bits and set to no spanning tree */
@@ -1054,7 +1054,6 @@ void b53_vlan_add(struct dsa_switch *ds, int port,
 	struct b53_device *dev = ds->priv;
 	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
 	bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
-	unsigned int cpu_port = dev->cpu_port;
 	struct b53_vlan *vl;
 	u16 vid;
 
@@ -1063,12 +1062,11 @@ void b53_vlan_add(struct dsa_switch *ds, int port,
 
 		b53_get_vlan_entry(dev, vid, vl);
 
-		vl->members |= BIT(port) | BIT(cpu_port);
+		vl->members |= BIT(port);
 		if (untagged)
 			vl->untag |= BIT(port);
 		else
 			vl->untag &= ~BIT(port);
-		vl->untag &= ~BIT(cpu_port);
 
 		b53_set_vlan_entry(dev, vid, vl);
 		b53_fast_age_vlan(dev, vid);
@@ -1432,8 +1430,8 @@ void b53_br_leave(struct dsa_switch *ds, int port, struct net_device *br)
 		b53_write16(dev, B53_VLAN_PAGE, B53_JOIN_ALL_VLAN_EN, reg);
 	} else {
 		b53_get_vlan_entry(dev, pvid, vl);
-		vl->members |= BIT(port) | BIT(dev->cpu_port);
-		vl->untag |= BIT(port) | BIT(dev->cpu_port);
+		vl->members |= BIT(port) | BIT(cpu_port);
+		vl->untag |= BIT(port) | BIT(cpu_port);
 		b53_set_vlan_entry(dev, pvid, vl);
 	}
 }
-- 
2.9.3

[PATCH net-next v2 3/3] net: dsa: b53: Turn on Broadcom tags

[Florian Fainelli]

Enable Broadcom tags for b53 devices, except 5325 and 5365 which use a
different Broadcom tag format not yet supported by net/dsa/tag_brcm.c.

We also make sure that we can turn on Broadcom tags on a CPU port number
that is capable of that: 5, 7 or 8.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/b53/b53_common.c | 48 ++++++++++++++++++++++++++++++++--------
 1 file changed, 39 insertions(+), 9 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 17f12484ce24..44a9a03bff55 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -325,7 +325,6 @@ static void b53_get_vlan_entry(struct b53_device *dev, u16 vid,
 
 static void b53_set_forwarding(struct b53_device *dev, int enable)
 {
-	struct dsa_switch *ds = dev->ds;
 	u8 mgmt;
 
 	b53_read8(dev, B53_CTRL_PAGE, B53_SWITCH_MODE, &mgmt);
@@ -337,14 +336,11 @@ static void b53_set_forwarding(struct b53_device *dev, int enable)
 
 	b53_write8(dev, B53_CTRL_PAGE, B53_SWITCH_MODE, mgmt);
 
-	/* Include IMP port in dumb forwarding mode when no tagging protocol is
-	 * set
+	/* Include IMP port in dumb forwarding mode
 	 */
-	if (ds->ops->get_tag_protocol(ds) == DSA_TAG_PROTO_NONE) {
-		b53_read8(dev, B53_CTRL_PAGE, B53_SWITCH_CTRL, &mgmt);
-		mgmt |= B53_MII_DUMB_FWDG_EN;
-		b53_write8(dev, B53_CTRL_PAGE, B53_SWITCH_CTRL, mgmt);
-	}
+	b53_read8(dev, B53_CTRL_PAGE, B53_SWITCH_CTRL, &mgmt);
+	mgmt |= B53_MII_DUMB_FWDG_EN;
+	b53_write8(dev, B53_CTRL_PAGE, B53_SWITCH_CTRL, mgmt);
 }
 
 static void b53_enable_vlan(struct b53_device *dev, bool enable)
@@ -612,6 +608,8 @@ static void b53_enable_cpu_port(struct b53_device *dev, int port)
 		    PORT_CTRL_RX_MCST_EN |
 		    PORT_CTRL_RX_UCST_EN;
 	b53_write8(dev, B53_CTRL_PAGE, B53_PORT_CTRL(port), port_ctrl);
+
+	b53_brcm_hdr_setup(dev->ds, port);
 }
 
 static void b53_enable_mib(struct b53_device *dev)
@@ -1480,9 +1478,41 @@ void b53_br_fast_age(struct dsa_switch *ds, int port)
 }
 EXPORT_SYMBOL(b53_br_fast_age);
 
+static bool b53_can_enable_brcm_tags(struct dsa_switch *ds)
+{
+	unsigned int brcm_tag_mask;
+	unsigned int i;
+
+	/* Broadcom switches will accept enabling Broadcom tags on the
+	 * following ports: 5, 7 and 8, any other port is not supported
+	 */
+	brcm_tag_mask = BIT(B53_CPU_PORT_25) | BIT(7) | BIT(B53_CPU_PORT);
+
+	for (i = 0; i < ds->num_ports; i++) {
+		if (dsa_is_cpu_port(ds, i)) {
+			if (!(BIT(i) & brcm_tag_mask)) {
+				dev_warn(ds->dev,
+					 "Port %d is not Broadcom tag capable\n",
+					 i);
+				return false;
+			}
+		}
+	}
+
+	return true;
+}
+
 static enum dsa_tag_protocol b53_get_tag_protocol(struct dsa_switch *ds)
 {
-	return DSA_TAG_PROTO_NONE;
+	struct b53_device *dev = ds->priv;
+
+	/* Older models support a different tag format that we do not
+	 * support in net/dsa/tag_brcm.c yet.
+	 */
+	if (is5325(dev) || is5365(dev) || !b53_can_enable_brcm_tags(ds))
+		return DSA_TAG_PROTO_NONE;
+	else
+		return DSA_TAG_PROTO_BRCM;
 }
 
 int b53_mirror_add(struct dsa_switch *ds, int port,
-- 
2.9.3

Re: [PATCH net-next v2 0/3] net: dsa: b53: Turn on Broadcom tags

[Florian Fainelli]

On 11/09/2017 02:34 PM, Florian Fainelli wrote:
> Hi all,
> 
> This was long overdue, with this patch series, the b53 driver now
> turns on Broadcom tags except for 5325 and 5365 which use an older
> format that we do not support yet (TBD).
> 
> First patch is necessary in order for bgmac, used on BCM5301X and Northstar
> Plus to work correctly and successfully send ARP packets back to the requsester.
> 
> Second patch is actually a bug fix, but because net/master and net-next/master
> diverge in that area, I am targeting net-next/master here.
> 
> Finally, the last patch enables Broadcom tags after checking that the CPU port
> selected is either, 5, 7 or 8, since those are the only valid combinations
> given currently supported HW.

David, I forgot to update drivers/net/dsa/b53/Kconfig to have a select
NET_DSA_TAG_BRCM, let me re-submit that.

> 
> Florian Fainelli (3):
>   net: bgmac: Pad packets to a minimum size
>   net: dsa: b53: Stop using dev->cpu_port incorrectly
>   net: dsa: b53: Turn on Broadcom tags
> 
>  drivers/net/dsa/b53/b53_common.c      | 58 ++++++++++++++++++++++++++---------
>  drivers/net/ethernet/broadcom/bgmac.c | 13 ++++++++
>  2 files changed, 56 insertions(+), 15 deletions(-)
> 


-- 
Florian