Messages in this thread | | | Date | Mon, 13 Dec 2021 07:43:47 -0800 | From | Jakub Kicinski <> | Subject | 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: > > On Mon, 6 Dec 2021 23:39:09 +0100 Aleksander Jan Bajkowski wrote: > >> +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.
> >> +} > >> + > >> +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. > > > 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. > > >> +} > >> + > > 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.
| |