From mboxrd@z Thu Jan 1 00:00:00 1970 From: Florian Fainelli Subject: Re: [PATCH net-next v2 1/3] net: bgmac: Pad packets to a minimum size Date: Fri, 10 Nov 2017 09:55:55 -0800 Message-ID: References: <20171109223449.3012-1-f.fainelli@gmail.com> <20171109223449.3012-2-f.fainelli@gmail.com> <063D6719AE5E284EB5DD2968C1650D6DD00B846B@AcuExch.aculab.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: "davem@davemloft.net" , "andrew@lunn.ch" , "vivien.didelot@savoirfairelinux.com" To: David Laight , "netdev@vger.kernel.org" Return-path: Received: from mail-qt0-f195.google.com ([209.85.216.195]:48785 "EHLO mail-qt0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751541AbdKJRz6 (ORCPT ); Fri, 10 Nov 2017 12:55:58 -0500 Received: by mail-qt0-f195.google.com with SMTP id f8so12794553qta.5 for ; Fri, 10 Nov 2017 09:55:58 -0800 (PST) In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6DD00B846B@AcuExch.aculab.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: 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 >> --- >> 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