[dpdk-dev] [PATCH v1 2/8] bus: introduce opaque control framework

[Shreyansh Jain]

On Monday 11 December 2017 06:13 PM, Gaëtan Rivet wrote:
> On Mon, Dec 11, 2017 at 05:30:16PM +0530, Shreyansh Jain wrote:
>> On Thursday 12 October 2017 01:48 PM, Gaetan Rivet wrote:

[...]

>>> diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
>>> index 331d954..bd3c28e 100644
>>> --- a/lib/librte_eal/common/include/rte_bus.h
>>> +++ b/lib/librte_eal/common/include/rte_bus.h
>>> @@ -183,6 +183,51 @@ struct rte_bus_conf {
>>>    	enum rte_bus_probe_mode probe_mode; /**< Probe policy. */
>>>    };
>>> +/**
>>> + * Bus configuration items.
>>> + */
>>> +enum rte_bus_ctrl_item {
>>> +	RTE_BUS_CTRL_PROBE_MODE = 0,
>>> +	RTE_BUS_CTRL_ITEM_MAX,
>>> +};
>>
>> I am assuming that a driver implementation can take more than ITEM_MAX
>> control knobs. It is opaque to the library. Are we on same page?
>>
>> For example, a bus driver can implement:
>>
>> rte_bus_XXX_ctrl_item {
>> 	<Leaving space for allowing rte_bus.h implementations>
>> 	RTE_BUS_XYZ_KNOB_1 = 100,
>> 	RTE_BUS_XYZ_KNOB_2,
>> 	RTE_BUS_XYZ_KNOB_3,
>> };
>>
>> without the library knowing or restricting the API to RTE_BUS_CTRL_ITEM_MAX.
>>
>> I see that in your code for PCI (Patch 5/8: pci_ctrl) you have restricted
>> the control knob to RTE_BUS_CTRL_ITEM_MAX.
>> I hope that such restrictions would not float to library layer.
>>
>> If we are on same page, should this be documented as a code comment
>> somewhere?
>> if not, do you think what I am stating makes sense?
>>
> 
> I see what you mean, but I'm not sure it would be a good thing.
> Actually, I think proposing this ITEM_MAX was a mistake.
> 
> Regarding the specific bus knobs:
> 
> - If a single bus needs this knob, then it would be better for the dev
>    to add it as part of the bus' public API, following the correct
>    library versioning processes. This would not break this bus control
>    structure ABI.

Sorry, but can you elaborate on "...add it as part of bus' public API"?

This is what I had in mind:

ctrl_fn = rte_bus_control_get(busname, RTE_BUS_XYZ_KNOB_1);

(unlike specific functions like probe_mode_get/set and iova_mode_get/set)

Where ctrl_fn would then point to a method specific to bus for KNOB_1 
configuration parameter.
Thereafter, ctrl_fn(KNOB_1, void *arg).

What other public API method are you hinting at?


> 
> - If more than one bus implement this knob, then it should be proposed
>    as part of the library API. Buses adding this new knob would break
>    their ABI, other buses would be left untouched.

Agree, if more than one bus implements same operation.

> 
> This makes me realize that proposing this ITEM_MAX value is not good to
> the intended purpose of this patchset:
> 
> - If a bus implementation use a reference to ITEM_MAX, then the control
>    structure ABI would be broken by any new control knob added, even if the
>    bus does not implement it. Granted, it would not break the driver
>    structure itself, but still. My PCI implementation is thus incorrect.

Changes to enum wouldn't break ABI as far as I understand. Adding a new 
entry only expands it to a new declaration without impacting its size or 
signature.

> 
> Therefore I think that it would be best to remove this ITEM_MAX altogether,
> forcing bus developpers to use other ways that would not break their
> ABIs every other release.
> 

Removing ITEM_MAX is OK from my side. It doesn't serve much purpose. 
But, not for the "ABI break" reason.