> On Apr 23, 2021, at 7:42 AM, Ferruh Yigit <ferruh.yi...@intel.com> wrote:
>
> On 12/15/2020 12:26 PM, Ferruh Yigit wrote:
>> On 12/10/2020 2:46 AM, Andrew Boyer wrote:
>>> This RFC is in response to the threads about testpmd mtu settings
>>> and the deprecation of max-rx-pkt-len.
>>>
>>> It took us a while to figure out what we were supposed to be doing
>>> in this part of the API. It is not clear if max_rx_pkt_len should be
>>> an input to or an output from the PMD.
>> 'max_rx_pkt_len' is input to the PMD, but it needs to be in sync with MTU
>> value, that is why PMDs update this value when MTU is updated to keep the
>> sync.
>>>
>>> The code below represents what we believe should happen today, and also
>>> happens to pass the DTS tests which were failing prior to this change
>>> (checksum and jumbo_frame at least).
>>>
>
> Hi Andrew,
>
> I am updating the status of the patch as "change requested", what is the
> status of this patch, will there be a new version?
> Is DTS still failing?
>
> Please see a few comments below if there will be new version.
>
Thank you for your thorough review!
I am working on a new feature at present so upstreaming has been delayed.
We were blocked from posting the next set of our patches by some arm64 build
stuff, but thanks to Juraj & co. I think that is cleared up. It’s just a matter
of when I will get to posting them.
-Andrew
> <...>
>
>>> diff --git a/drivers/net/ionic/ionic_ethdev.c
>>> b/drivers/net/ionic/ionic_ethdev.c
>>> index 925da3e29..7000de3f9 100644
>>> --- a/drivers/net/ionic/ionic_ethdev.c
>>> +++ b/drivers/net/ionic/ionic_ethdev.c
>>> @@ -357,25 +357,19 @@ ionic_dev_mtu_set(struct rte_eth_dev *eth_dev,
>>> uint16_t mtu)
>>> int err;
>>> IONIC_PRINT_CALL();
>>> + IONIC_PRINT(INFO, "Setting mtu %u\n", mtu);
>>> - /*
>>> - * Note: mtu check against IONIC_MIN_MTU, IONIC_MAX_MTU
>>> - * is done by the the API.
>>> - */
>>> -
>>> - /*
>>> - * Max frame size is MTU + Ethernet header + VLAN + QinQ
>>> - * (plus ETHER_CRC_LEN if the adapter is able to keep CRC)
>>> - */
>>> - max_frame_size = mtu + RTE_ETHER_HDR_LEN + 4 + 4;
>>> -
>>> - if (eth_dev->data->dev_conf.rxmode.max_rx_pkt_len < max_frame_size)
>>> - return -EINVAL;
>> The max frame size calculation depends on what HW support, but if VLAN
>> header is supported above calculation and check is correct.
>
> Removing above check seems correct thing to do, instead should check against
> 'dev_info.max_mtu' which is already done in ethdev layer, so nothing needed
> here.
>
>>> -
>>> + /* Note: mtu check against min/max is done by the API */
>>> err = ionic_lif_change_mtu(lif, mtu);
>>> if (err)
>>> return err;
>>> + /* Update max frame size */
>>> + max_frame_size = mtu + RTE_ETHER_HDR_LEN;
>> I guess you are removing the CRC because your HW strips the CRC in the Rx
>> buffer, but this limit is not for Rx buffer, it is for the frame size HW
>> accepts, and since recevied frame will have the CRC it should be included
>> into the calculation.
>>> + eth_dev->data->dev_conf.rxmode.max_rx_pkt_len = max_frame_size;
>>> +
>
> Although 'rxmode.max_rx_pkt_len' is input to driver, it is related with MTU,
> which is also input from application in this function, so OK to update
> 'rxmode.max_rx_pkt_len' here.
>
>>> + ionic_lif_set_rx_buf_size(lif);
>>> +
>
> This seems to keep the local copy of 'rxmode.max_rx_pkt_len' and use local
> copy instead, is this just refactoring or is there any other reason for local
> copy?
