lkml.org 
[lkml]   [2021]   [Dec]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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.

\
 
 \ /
  Last update: 2021-12-13 16:44    [W:0.693 / U:0.084 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site