On 11/5/19 5:18 PM, Dekel Peled wrote:
> Thanks, PSB.
>
>> -----Original Message-----
>> From: Andrew Rybchenko <arybche...@solarflare.com>
>> Sent: Tuesday, November 5, 2019 2:40 PM
>> To: Dekel Peled <dek...@mellanox.com>; john.mcnam...@intel.com;
>> marko.kovace...@intel.com; nhor...@tuxdriver.com;
>> ajit.khapa...@broadcom.com; somnath.ko...@broadcom.com;
>> anatoly.bura...@intel.com; xuanziya...@huawei.com;
>> cloud.wangxiao...@huawei.com; zhouguoy...@huawei.com;
>> wenzhuo...@intel.com; konstantin.anan...@intel.com; Matan Azrad
>> <ma...@mellanox.com>; Shahaf Shuler <shah...@mellanox.com>; Slava
>> Ovsiienko <viachesl...@mellanox.com>; rm...@marvell.com;
>> shsha...@marvell.com; maxime.coque...@redhat.com;
>> tiwei....@intel.com; zhihong.w...@intel.com; yongw...@vmware.com;
>> Thomas Monjalon <tho...@monjalon.net>; ferruh.yi...@intel.com;
>> jingjing...@intel.com; bernard.iremon...@intel.com
>> Cc: dev@dpdk.org
>> Subject: Re: [PATCH 1/3] ethdev: support API to set max LRO packet size
>>
>> On 11/5/19 11:40 AM, Dekel Peled wrote:
>>> This patch implements [1], to support API for configuration and
>>> validation of max size for LRO aggregated packet.
>>> API change notice [2] is removed, and release notes for 19.11 are
>>> updated accordingly.
>>>
>>> Various PMDs using LRO offload are updated, the new data members are
>>> initialized to ensure they don't fail validation.
>>>
>>> [1]
>>>
>> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatch
>>>
>> es.dpdk.org%2Fpatch%2F58217%2F&data=02%7C01%7Cdekelp%40mell
>> anox.co
>>>
>> m%7C751aa0cb18b94b8a447c08d761ed4051%7Ca652971c7d2e4d9ba6a4d149
>> 256f461
>>>
>> b%7C0%7C1%7C637085543948425032&sdata=C2laHnaMCQZbDUneQD0
>> 2Kpi5iAcr%
>>> 2FYDAS%2BMuO8IcV9s%3D&reserved=0
>>> [2]
>>>
>> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatch
>>>
>> es.dpdk.org%2Fpatch%2F57492%2F&data=02%7C01%7Cdekelp%40mell
>> anox.co
>>>
>> m%7C751aa0cb18b94b8a447c08d761ed4051%7Ca652971c7d2e4d9ba6a4d149
>> 256f461
>>>
>> b%7C0%7C1%7C637085543948435028&sdata=XnexdrRYNmFyLqT9IL6ZKa
>> CLF2JKr
>>> oKPDVML7gXKceE%3D&reserved=0
>>>
>>> Signed-off-by: Dekel Peled <dek...@mellanox.com>
>>
>> Few comments below, otherwise
>>
>> Reviewed-by: Andrew Rybchenko <arybche...@solarflare.com>
[snip]
>>> diff --git a/lib/librte_ethdev/rte_ethdev.c
>>> b/lib/librte_ethdev/rte_ethdev.c index 85ab5f0..2f52090 100644
>>> --- a/lib/librte_ethdev/rte_ethdev.c
>>> +++ b/lib/librte_ethdev/rte_ethdev.c
>>> @@ -1156,6 +1156,26 @@ struct rte_eth_dev *
>>> return name;
>>> }
>>>
>>> +static inline int
>>> +rte_eth_check_lro_pkt_size(uint16_t port_id, uint32_t config_size,
>>> + uint32_t dev_info_size)
>>
>> As I understand Thomas prefers static functions without rte_eth_ prefix.
>> I think it is reasonable.
>
> Will remove prefix.
>
>>
>>> +{
>>> + int ret = 0;
>>> +
>>> + if (config_size > dev_info_size) {
>>> + RTE_ETHDEV_LOG(ERR, "Ethdev port_id=%d
>> max_lro_pkt_size %u > "
>>> + "max allowed value %u\n",
>>> + port_id, config_size, dev_info_size);
>>> + ret = -EINVAL;
>>> + } else if (config_size < RTE_ETHER_MIN_LEN) {
>>
>> Shouldn't config_size == 0 fallback to maximum?
>> (I don't know and I simply would like to get comments on it)
>>
>
> This check is for value smaller than minimum, not just 0.
Yes, I know, but the question still remains.
>>> + RTE_ETHDEV_LOG(ERR, "Ethdev port_id=%d
>> max_lro_pkt_size %u < "
>>> + "min allowed value %u\n", port_id, config_size,
>>> + (unsigned int)RTE_ETHER_MIN_LEN);
>>> + ret = -EINVAL;
>>> + }
>>> + return ret;
>>> +}
>>> +
>>> int
>>> rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t
>> nb_tx_q,
>>> const struct rte_eth_conf *dev_conf) @@ -1286,6
[snip]
