From: Shreyansh Jain <shreyansh.jain@nxp.com>
To: "Gaëtan Rivet" <gaetan.rivet@6wind.com>
Cc: <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v1 2/8] bus: introduce opaque control framework
Date: Mon, 11 Dec 2017 19:06:55 +0530 [thread overview]
Message-ID: <665f56dc-3cd5-e199-59b5-2021facd0bba@nxp.com> (raw)
In-Reply-To: <20171211124359.zhyeaveywobmobef@bidouze.vm.6wind.com>
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.
next prev parent reply other threads:[~2017-12-11 13:23 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-10-12 8:18 [dpdk-dev] [PATCH v1 0/8] Bus " Gaetan Rivet
2017-10-12 8:18 ` [dpdk-dev] [PATCH v1 1/8] bus: rename scan policy as probe policy Gaetan Rivet
2017-10-12 8:18 ` [dpdk-dev] [PATCH v1 2/8] bus: introduce opaque control framework Gaetan Rivet
2017-12-11 12:00 ` Shreyansh Jain
2017-12-11 12:43 ` Gaëtan Rivet
2017-12-11 13:36 ` Shreyansh Jain [this message]
2017-12-11 14:38 ` Gaëtan Rivet
2017-12-12 7:21 ` Shreyansh Jain
2017-10-12 8:18 ` [dpdk-dev] [PATCH v1 3/8] bus: remove probe mode configuration structure Gaetan Rivet
2017-10-12 8:18 ` [dpdk-dev] [PATCH v1 4/8] bus: add probe mode setter Gaetan Rivet
2017-12-11 12:39 ` Shreyansh Jain
2017-12-11 12:43 ` Shreyansh Jain
2017-10-12 8:18 ` [dpdk-dev] [PATCH v1 5/8] bus/pci: implement ctrl operator Gaetan Rivet
2017-10-12 8:18 ` [dpdk-dev] [PATCH v1 6/8] bus: add IOVA mode as a ctrl operation Gaetan Rivet
2017-10-12 8:18 ` [dpdk-dev] [PATCH v1 7/8] bus/pci: implement IOVA mode getter Gaetan Rivet
2017-10-12 8:18 ` [dpdk-dev] [PATCH v1 8/8] bus: remove redundant " Gaetan Rivet
2017-12-11 11:53 ` [dpdk-dev] [PATCH v1 0/8] Bus control framework Shreyansh Jain
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=665f56dc-3cd5-e199-59b5-2021facd0bba@nxp.com \
--to=shreyansh.jain@nxp.com \
--cc=dev@dpdk.org \
--cc=gaetan.rivet@6wind.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).