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.
