Re: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get

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

On 2/3/2017 10:02 AM, Iremonger, Bernard wrote:
> Hi Konstantin,
> 
>> -----Original Message-----
>> From: Ananyev, Konstantin
>> Sent: Friday, February 3, 2017 9:50 AM
>> To: Iremonger, Bernard <bernard.iremonger@intel.com>; Yigit, Ferruh
>> <ferruh.yigit@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
>> dev@dpdk.org
>> Subject: RE: [dpdk-dev] [PATCH] net/ixgbe: clean up rte_eth_dev_info_get
>>
>> Hi Bernard,
>>
>>> -----Original Message-----
>>> From: Iremonger, Bernard
>>> Sent: Friday, February 3, 2017 9:21 AM
>>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Yigit, Ferruh
>>> <ferruh.yigit@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
>>> dev@dpdk.org
>>> Subject: RE: [dpdk-dev] [PATCH] net/ixgbe: clean up
>>> rte_eth_dev_info_get
>>>
>>> Hi Konstantin,
>>>
>>>> -----Original Message-----
>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
>>>> Konstantin
>>>> Sent: Wednesday, February 1, 2017 6:10 PM
>>>> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Lu, Wenzhuo
>>>> <wenzhuo.lu@intel.com>; dev@dpdk.org
>>>> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up
>>>> rte_eth_dev_info_get
>>>>
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Yigit, Ferruh
>>>>> Sent: Wednesday, February 1, 2017 5:40 PM
>>>>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Lu,
>>>>> Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org
>>>>> Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: clean up
>>>>> rte_eth_dev_info_get
>>>>>
>>>>> On 2/1/2017 4:24 PM, Ananyev, Konstantin wrote:
>>>>>> Hi Wenzhuo,
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wenzhuo
>> Lu
>>>>>>> Sent: Wednesday, January 25, 2017 2:39 AM
>>>>>>> To: dev@dpdk.org
>>>>>>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>
>>>>>>> Subject: [dpdk-dev] [PATCH] net/ixgbe: clean up
>>>>>>> rte_eth_dev_info_get
>>>>>>>
>>>>>>> It'not appropriate to call rte_eth_dev_info_get in PMD, as
>>>>>>> rte_eth_dev_info_get need to get info from PMD.
>>>>>>> Remove rte_eth_dev_info_get from PMD code and get the info
>>>>>>> directly.
>>>>>>>
>>>>>>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
>>>>>>> ---
>>>>>>>  drivers/net/ixgbe/ixgbe_ethdev.c | 144
>>>>>>> ++++++++++++++++++---------------------
>>>>>>>  1 file changed, 68 insertions(+), 76 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
>>>>>>> b/drivers/net/ixgbe/ixgbe_ethdev.c
>>>>>>> index 64ce55a..f14a68b 100644
>>>>>>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>>>>>>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>>>>>>> @@ -4401,17 +4401,17 @@ static int
>>>> ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
>>>>>>>  	int rar_entry;
>>>>>>>  	uint8_t *new_mac = (uint8_t *)(mac_addr);
>>>>>>>  	struct rte_eth_dev *dev;
>>>>>>> -	struct rte_eth_dev_info dev_info;
>>>>>>> +	struct rte_pci_device *pci_dev;
>>>>>>>
>>>>>>>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
>>>>>>>
>>>>>>>  	dev = &rte_eth_devices[port];
>>>>>>> -	rte_eth_dev_info_get(port, &dev_info);
>>>>>>> +	pci_dev = IXGBE_DEV_TO_PCI(dev);
>>>>>>>
>>>>>>> -	if (is_ixgbe_pmd(dev_info.driver_name) != 0)
>>>>>>> +	if (is_ixgbe_pmd(dev->data->drv_name))
>>>>>>>  		return -ENOTSUP;
>>>>>>
>>>>>> I wonder why do we need now that it is really an ixgbe device
>>>>>> all over the
>>>> place?
>>>>>
>>>>> This device specific API, so it is missing merits of abstraction
>>>>> layer, application can these APIs with any port_id, API should be
>>>>> protected
>>>> for it.
>>>>
>>>> Ah ok, my bad - didn't realize from the patch that it affects only
>>>> device specific API :) Would It be too much hassle to move these
>>>> functions into a separate file (rte_ixgbe_pmd.c or so)?
>>>> Konstantin
>>>>
>>>>>
>>>>>> Konstantin
>>>>>>
>>> All the device specific API functions are prefixed with rte_pmd_ixgbe
>>
>> That's ok.
>>
>>
>>> so I don't think a separate file is necessary.
>>
>> So far I didn't say it is necessary.
>> Though I think it would be good to group these functions in a separate file to
>> help avoid confusion (as happened to me) and keep ixgbe_ethdev.c smaller
>> and cleaner.
>> Again would be easier to maintain things in future, when more folks will
>> come up with some extensions for it.
>> That's why I am asking:  would it be a lot of work to do?
>> It is probably worth doing it now, while we have this API relatively small.
>> Konstantin
>>
> I would need to investigate what is involved in moving the rte_pmd_ixgbe_* functions to a separate file.
> They may be using some of the static functions and data in the ixgbe_ethdev.c  file which could be a problem.
> The rte_pmd_ixgbe_* functions are considered an "interim solution" until a "generic solution" is agreed.
> It might be best to postpone this work until the "generic solution" is agreed.

Still I believe it is good idea to move these functions into a separate
file, but not for this release J.

For PMD specific APIs, even we keep using direct API call method or
benefit from abstraction layer, means of using ioctl perhaps, these APIs
will be implemented in the PMD. And logically grouping them is good idea.
Briefly, I don't think we need to wait until "generic solution" agreed
for this.

But, of course an investigation needs to be done on required effort, and
decide based on that.

Thanks,
ferruh

> 
> Regards,
> 
> Bernard.
>