Re: [dpdk-stable] [dpdk-dev] [PATCH] app/testpmd: fix ixgbe private API calling

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 1/11/2017 4:29 PM, Iremonger, Bernard wrote:
> Hi Ferruh,
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Wednesday, January 11, 2017 4:19 PM
>> To: Iremonger, Bernard <bernard.iremonger@intel.com>; Lu, Wenzhuo
>> <wenzhuo.lu@intel.com>; dev@dpdk.org
>> Cc: Wu, Jingjing <jingjing.wu@intel.com>; stable@dpdk.org
>> Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH] app/testpmd: fix ixgbe
>> private API calling
>>
>> On 1/11/2017 3:47 PM, Iremonger, Bernard wrote:
>>> Hi Ferruh,
>>>
>>>> -----Original Message-----
>>>> From: Yigit, Ferruh
>>>> Sent: Wednesday, January 11, 2017 3:27 PM
>>>> To: Iremonger, Bernard <bernard.iremonger@intel.com>; Lu, Wenzhuo
>>>> <wenzhuo.lu@intel.com>; dev@dpdk.org
>>>> Cc: Wu, Jingjing <jingjing.wu@intel.com>; stable@dpdk.org
>>>> Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH] app/testpmd: fix ixgbe
>>>> private API calling
>>>>
>>>> On 1/11/2017 3:20 PM, Iremonger, Bernard wrote:
>>>>> Hi Wenzhuo,
>>>>>
>>>>> <snip>
>>>>>>> Subject: [dpdk-dev] [PATCH] app/testpmd: fix ixgbe private API
>>>>>>> calling
>>>>>>>
>>>>>>> Some ixgbe private APIs are added to expose ixgbe specific functions.
>>>>>>> When they're used by testpmd, there's no check for if the NICs are
>>>> ixgbe.
>>>>>>> Other NICs also have chance to  call these APIs.
>>>>>>> This patch add the check and the feedback print.
>>>>>>
>>>>>> I am not sure that testpmd is the right place to do this.
>>>>>> The rte_pmd_ixgbe_* functions are public API's which can be called
>>>>>> by other applications.
>>>>>> The checks should be in the rte_pmd_ixgbe_* API's
>>>>>
>>>>> It is useful to handle the return code -ENOTSUP in testpmd.
>>>>>
>>>>
>>>> Makes sense, and I think it is good idea to add them in your patch,
>>>> since it introduces returning -ENOTSUP, would you mind sending a new
>>>> version of your patch with this update?
>>>> So we can drop this patch completely.
>>>>
>>>> Thanks,
>>>> ferruh
>>>>
>>> I don't think this patch should be dropped.
>>> Testpmd is already handling -EINVAL and -ENODEV.
>>> It makes sense for it to handle -ENOTSUP for the cases in the patch.
>>
>> This patch adds driver check [1] before ixgbe APIs, since now that check is
>> done within ixgbe APIs by your patch. Why do we need this patch at all?
>>
>> [1]
>> if (strstr(dev_info.driver_name, "ixgbe") != NULL)
>  
> I think our lines have got slightly crossed.
> This patch is doing two things, the check [1] above which is not needed now (after my ixgbe patch).
> Secondly it is handling the ret code of the rte_pmd_ixgbe_* API's, I think this is useful.

I was thinking returning -ENOTSUP introduced for your patch, but it
seems this is not true for all functions, some is already returning
-ENOTSUP, so testpmd should cover them.

I believe intention of this patch is the first item you have mentioned,
but can be converted to second one too.

Wenzhuo,

When do you have time, can you convert this patch to one that covers
"-ENOTSUP" error messages for ixgbe APIs please? If you don't have time,
please let me know I can do the patch?

Thanks,
ferruh

> 
> Regards,
> 
> Bernard.
> 
>   
>