[dpdk-dev] [PATCH v4 21/23] ethdev: Move filling of rte_eth_dev_info->pci_dev to dev_infos_get()

[Shreyansh Jain]

On Friday 23 December 2016 04:57 PM, Jan Blunck wrote:
> On Fri, Dec 23, 2016 at 12:11 PM, Shreyansh Jain <shreyansh.jain at nxp.com> wrote:
>> On Friday 23 December 2016 04:20 PM, Jan Blunck wrote:
>>>
>>> On Thu, Dec 22, 2016 at 9:11 AM, Shreyansh Jain <shreyansh.jain at nxp.com>
>>> wrote:
>>>>
>>>> On Thursday 22 December 2016 01:39 AM, Stephen Hemminger wrote:
>>>>>
>>>>>
>>>>> On Wed, 21 Dec 2016 16:09:44 +0100
>>>>> Jan Blunck <jblunck at infradead.org> wrote:
>>>>>
>>>>>> Only the device itself can decide its PCI or not.
>>>>>>
>>>>>> Signed-off-by: Jan Blunck <jblunck at infradead.org>
>>>>>> Acked-by: Shreyansh Jain <shreyansh.jain at nxp.com>
>>>>>> ---
>>>>>
>>>>>
>>>>>
>>>>> I would still like to kill dev_pci from the dev_info API.
>>>>>
>>>
>>> I'm fine with that too.
>>>
>>>>
>>>> +1. It should be rte_dev reference instead.
>>>>
>>>
>>> Only if you can give use-cases for what users should be able to do
>>> with it. If that is the case we need to clearly define what that
>>> means. Do we want to enable users to control the low-level EAL device
>>> directly and shortcut the ethdev driver? If that is necessary we need
>>> to give control to the driver first to decide if it is safe to do so.
>>>
>>
>> An ethernet device is not necessarily a PCI device. With planned removal of
>> rte_pci_device from rte_eth_device, this will be realized.
>> Similarly, the info is also not PCI device specific.
>>
>> With the '+1', my intention was not to say we should do it in this patch. We
>> should prepare eth_dev_info in similar manner as done for pci_dev of
>> rte_eth_dev (ETH_DEV_PCI_DEV() style macro, or inline).
>
> Which is exactly what this patch is doing. I'm moving the filling of
> the PCI information out of the generic code because only the driver
> could know if it is actually handling a PCI device. The generic code
> can not use the ETH_DEV_PCI_DEV() macro in a safe manner.

I fully agree with you and support this series.

Probably I went a step further in my eagerness - I was only hinting that 
now info filling part is relatively free of pci-ness, we should remove 
eth_dev_info linkage to PCI.

There is no change I am expecting in your patch in this regard. Yours is 
the first step to this change.

>
>> And now for whether we should expose lower level device details or not, I
>> was of the view that keeping pci_dev linked to this structure exposes more
>> lower level info than keeping rte_dev. Another view point could be to
>> completely do away with pci_info within eth_dev_info - but, I am not sure of
>> dependencies on it.
>
> If I understand Stephen correctly he questions the benefit of pushing
> down the code to the drivers instead of killing that code completely.
> I'll see what I can do here.
>

Ok. Then I read it wrong. I read it in the sense that we should remove 
PCI dependency of eth_info. Except, I considered that removal more as 
replacement with rte_device. My mistake!