Re: [PATCH net v2] net: lantiq_xrx200: increase buffer reservation

On Mon, 13 Dec 2021 00:05:16 +0100 Aleksander Bajkowski wrote:

On 12/8/21 5:54 AM, Jakub Kicinski wrote:

quoted

On Mon,  6 Dec 2021 23:39:09 +0100 Aleksander Jan Bajkowski wrote:  

quoted

+static int xrx200_max_frame_len(int mtu)
+{
+	return VLAN_ETH_HLEN + mtu + ETH_FCS_LEN;  

You sure the problem is not that this doesn't include ETH_HLEN? 
MTU is the length of the L2 _payload_.

VLAN_ETH_HLEN (14 + 4) contains ETH_HLEN (14). This function returns
the length of the frame that is written to the RX descriptor. Maybe
I don't understand the question and you are asking something else? 

Ah, right, misread that as VLAN_HLEN.

quoted

quoted

+}
+
+static int xrx200_buffer_size(int mtu)
+{
+	return round_up(xrx200_max_frame_len(mtu) - 1, 4 * XRX200_DMA_BURST_LEN);  

Why the - 1 ? 🤔
  

This is how the hardware behaves. I don't really know where the -1
comes from. Unfortunately, I do not have access to TRM. 

quoted

For a frame size 101 => max_frame_len 109 you'll presumably want 
the buffer to be 116, not 108?

For a frame size 101 => max_frame_len is 123 (18 + 101 + 4).

You get my point, tho right?

Infact, PMAC strips FCS and ETH_FCS_LEN may not be needed. This behavior
is controlled by the PMAC_HD_CTL_RC bit. This bit is enabled from
the beginning of this driver. Ethtool has the option to enable
FCS reception, but the ethtool interface is not yet supported
by this driver. 

quoted

quoted

+}
+  

Experiments show that the hardware starts to split the frame at
max_frame_len() - 1. Some examples:

pkt len	MTU	max_frame_size()	buffer_size()	desc1	desc2	desc3	desc4
----------------------------------------------------------------------------------------------
1506		1483		1505		1504		1502	4	X	X
1505		1483		1505		1504		1502	3	X	X
1504		1483		1505		1504		1504	X	X	X
1503		1483		1505		1504		1503	X	X	X
1502		1483		1505		1504		1502	X	X	X
1501		1483		1505		1504		1501	X	X	X
----------------------------------------------------------------------------------------------
1249		380		402		416		414	416	416	3
1248		380		402		416		414	416	416	2
1247		380		402		416		414	416	416	1
1246		380		402		416		414	416	416	X
1245		380		402		416		414	416	415	X
----------------------------------------------------------------------------------------------

Hm, doesn't the former example prove that the calculation in this
patch is incorrect? Shouldn't we be able to receive a 1505B frame 
into a single buffer for the MTU of 1483?

The HW doesn't split at max_frame_len - 1 in the latter example.
Maybe to save one bit of state the HW doesn't register the lowest 
bit of the buffer length? I thinks the buffer is 1504.

I'd lean towards dropping the -1 and letting the DMA alignment
calculation round the whole length up.

Also should we not take NET_IP_ALIGN into account?

Judging by the length of the first buffer when split happens
NET_IP_ALIGN is 2, and HW rounds off (2 + pkt-len) to the DMA
burst size. This makes me think we overrun the end of the buffer
by NET_IP_ALIGN.

In fact, this patch is a preparation for SG DMA support, which
I wrote some time ago.