Re: [PATCH] pci: Add the class_id support in pci probe

[Panu Matilainen <pmatilai@redhat.com>]

On 01/29/2016 12:10 PM, Panu Matilainen wrote:
> On 01/29/2016 11:34 AM, Thomas Monjalon wrote:
>> 2016-01-29 11:21, Panu Matilainen:
>>> On 01/28/2016 11:38 PM, Thomas Monjalon wrote:
>>>> 2016-01-13 14:22, Panu Matilainen:
>>>>> On 01/13/2016 01:55 PM, Bruce Richardson wrote:
>>>>>> On Thu, Dec 31, 2015 at 09:12:14AM -0800, Stephen Hemminger wrote:
>>>>>>> On Tue, 29 Dec 2015 10:53:26 +0800
>>>>>>> Ziye Yang <ziye.yang@intel.com> wrote:
>>>>>>>
>>>>>>>> This patch is used to add the class_id support
>>>>>>>> for pci_probe since some devices need the class_info
>>>>>>>> (class_code, subclass_code, programming_interface)
>>>>>>>>
>>>>>>>> Signed-off-by: Ziye Yang <ziye.yang@intel.com>
>>>>>>>
>>>>>>> Since rte_pci is exposed to application this breaks the ABI.
>>>>>>
>>>>>> But applications are not going to be defining rte_pci_ids values
>>>>>> internally, are
>>>>>> they? That is for drivers to use. Is this really an ABI breakage
>>>>>> for applications that we
>>>>>> need to be concerned about?
>>>>>
>>>>> There might not be applications using it but drivers are ABI consumers
>>>>> too - think of 3rd party drivers and such.
>>>>
>>>> Drivers are not ABI consumers in the sense that ABI means
>>>> Application Binary Interface.
>>>> We are talking about drivers interface here.
>>>> When establishing the ABI policy we were discussing about
>>>> applications only.
>>>
>>> Generally speaking an ABI is an interface between two program (or
>>> software if you like) modules, its not specific to "applications".
>>> Looking at http://dpdk.org/doc/guides/contributing/versioning.html I see
>>> it does only talk about applications, but an ABI consumer can also be
>>> another library. A driver calling rte_malloc() is just as much
>>> librte_eal ABI consumer as anything else.
>>>
>>> Now, I understand that drivers use and need interface(s) that
>>> applications have no use for or simply cannot use, and those interfaces
>>> could be subject to different policies. As an extreme example, the Linux
>>> kernel has two isolated ABIs, one is the userland system call interface
>>> which is guaranteed to stay forever and the other is kernel module
>>> interface, guarantees nothing at all.
>>>
>>> In DPDK the difference is far muddier than that since all the interfaces
>>> live in common, versioned userland DSOs. So if there are two different
>>> interfaces following two different policies, it's all the more important
>>> to clearly document them. One simple way could be using a different
>>> prefix than rte_.
>>
>> Good suggestion. Or we can simply have different files with a clear
>> notice
>> in their headers and in the versioning doc.
>> It was well split in rte_cryptodev_pmd.h
>
> Using separate headers is also good. Optimally both? :)
>
>>>> I agree we must allow 3rd party drivers but there is no good reason
>>>> to try to upgrade DPDK without upgrading/porting the external drivers.
>>>> If someone does not want to release its driver and keep upgrading DPDK,
>>>> it is acceptable IMHO to force an upgrade of its driver.
>>>
>>> Note that I've no particular sympathy for 3rd party drivers as such.
>>> What I *do* care about is that breakage is made explicit, as in drivers
>>> built for an incompatible version refuse to load at all, instead of
>>> silently corrupting memory etc.
>>
>> OK I agree.
>
> Cool, the rest is just details then.
>
>> Anyway the ABI versionning does not cover the structure changes.
>> What about making a DPDK version check when registering a driver?
>> So a binary driver would be clearly bound to a DPDK version.
>
> That's one possibility. Another way to achieve essentially the same is
> to make rte_eal_driver_register() symbol version follow the DPDK
> version, in which case a driver built for another version will fail at
> dlopen() already.

Thinking about this a bit more, symbol versioning doesn't cut it because 
its not always used (static linkakage) and I guess we should cover that too.

Another similar possibility that blocks it at dlopen() level is to munge 
the actual function name to carry a version, so it becomes something 
like rte_eal_driver_register_v230() and later _v240() etc. AFAICS its 
only ever invoked via PMD_REGISTER_DRIVER() so the calling details can 
conveniently be hidden there.

	- Panu -