| Message ID | 20171109223449.3012-2-f.fainelli@gmail.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: dsa: b53: Turn on Broadcom tags | expand |
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
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).
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;
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(+)