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

[Florian Fainelli <f.fainelli@gmail.com>]

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