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 19:06:55 +0530 Message-ID: <665f56dc-3cd5-e199-59b5-2021facd0bba@nxp.com> References: <91106540c460d22dd23a30dc2903d7e238ff9a3b.1507796085.git.gaetan.rivet@6wind.com> <088de7d2-9bd4-4a49-44ba-9df9c52b72d1@nxp.com> <20171211124359.zhyeaveywobmobef@bidouze.vm.6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit Cc: To: =?UTF-8?Q?Ga=c3=abtan_Rivet?= Return-path: Received: from NAM01-BN3-obe.outbound.protection.outlook.com (mail-bn3nam01on0048.outbound.protection.outlook.com [104.47.33.48]) by dpdk.org (Postfix) with ESMTP id 858EC2BAE for ; Mon, 11 Dec 2017 14:23:23 +0100 (CET) In-Reply-To: <20171211124359.zhyeaveywobmobef@bidouze.vm.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 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 { >> >> 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.