From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shreyansh Jain Subject: Re: [PATCH v1 2/8] bus: introduce opaque control framework Date: Mon, 11 Dec 2017 17:30:16 +0530 Message-ID: <088de7d2-9bd4-4a49-44ba-9df9c52b72d1@nxp.com> References: <91106540c460d22dd23a30dc2903d7e238ff9a3b.1507796085.git.gaetan.rivet@6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Cc: To: Gaetan Rivet Return-path: Received: from NAM01-SN1-obe.outbound.protection.outlook.com (mail-sn1nam01on0062.outbound.protection.outlook.com [104.47.32.62]) by dpdk.org (Postfix) with ESMTP id 135A2293B for ; Mon, 11 Dec 2017 12:46:44 +0100 (CET) In-Reply-To: <91106540c460d22dd23a30dc2903d7e238ff9a3b.1507796085.git.gaetan.rivet@6wind.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Thursday 12 October 2017 01:48 PM, Gaetan Rivet wrote: > New configuration elements are added to the buses. They make the ABI > unstable and will continue to do so. > > This new control scheme allows to add new bus operators without > breaking the ABI and by only expanding the API. > > This helps having more stability in core EAL subsystems, while allowing > flexibility for future evolutions. > > Signed-off-by: Gaetan Rivet > --- > lib/librte_eal/common/eal_common_bus.c | 9 +++++++ > lib/librte_eal/common/include/rte_bus.h | 46 +++++++++++++++++++++++++++++++++ > 2 files changed, 55 insertions(+) > > diff --git a/lib/librte_eal/common/eal_common_bus.c b/lib/librte_eal/common/eal_common_bus.c > index 3c66a02..65d7229 100644 > --- a/lib/librte_eal/common/eal_common_bus.c > +++ b/lib/librte_eal/common/eal_common_bus.c > @@ -42,6 +42,13 @@ > struct rte_bus_list rte_bus_list = > TAILQ_HEAD_INITIALIZER(rte_bus_list); > > +static rte_bus_ctrl_t > +rte_bus_default_ctrl(enum rte_bus_ctrl_op op __rte_unused, > + enum rte_bus_ctrl_item item __rte_unused) > +{ > + return NULL; > +} > + > void > rte_bus_register(struct rte_bus *bus) > { > @@ -53,6 +60,8 @@ rte_bus_register(struct rte_bus *bus) > RTE_VERIFY(bus->find_device); > /* Buses supporting driver plug also require unplug. */ > RTE_VERIFY(!bus->plug || bus->unplug); > + if (bus->ctrl == NULL) > + bus->ctrl = &rte_bus_default_ctrl; > > TAILQ_INSERT_TAIL(&rte_bus_list, bus, next); > RTE_LOG(DEBUG, EAL, "Registered [%s] bus.\n", bus->name); > 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 { 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? > + > +/** > + * Bus configuration operations. > + */ > +enum rte_bus_ctrl_op { > + RTE_BUS_CTRL_GET = 0, > + RTE_BUS_CTRL_SET, > + RTE_BUS_CTRL_RESET, > + RTE_BUS_CTRL_OP_MAX, > +}; Similarly, the driver implementation can choose to implement a operation which is not defined in the above structures. Obviously, the application is expected to know - it being a custom knob. [...]