[dpdk-dev] [PATCH v1 2/8] bus: introduce opaque control framework
Shreyansh Jain
shreyansh.jain at nxp.com
Mon Dec 11 14:36:55 CET 2017
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.
More information about the dev
mailing list