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.faine...@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
