[dpdk-dev,v1,2/8] bus: introduce opaque control framework
Checks
Commit Message
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 <gaetan.rivet@6wind.com>
---
lib/librte_eal/common/eal_common_bus.c | 9 +++++++
lib/librte_eal/common/include/rte_bus.h | 46 +++++++++++++++++++++++++++++++++
2 files changed, 55 insertions(+)
Comments
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 <gaetan.rivet@6wind.com>
> ---
> 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 {
<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?
> +
> +/**
> + * 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.
[...]
On Mon, Dec 11, 2017 at 05:30:16PM +0530, Shreyansh Jain wrote:
> 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 <gaetan.rivet@6wind.com>
> > ---
> > 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 {
> <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.
- 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.
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.
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.
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.
On Mon, Dec 11, 2017 at 07:06:55PM +0530, Shreyansh Jain wrote:
> 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?
>
>
I was thinking that buses would simply expose a function
rte_busname_xyz_knob1(void *arg);
as part of their public API. This would not require an ABI break for
this bus, as it would only be an API extension and would not use
callbacks within the bus structure.
Thus, I think that for buses tempted to propose a specific API, this would be
the cleanest way.
The bus proposing it as part of a custom control section would only be
interesting when the operation is expected to become a standard API for
other buses but was not yet accepted. Applications would be able to use
the interface and the ITEM could be added later. But I doubt this is
encouraging best practices as far as API evolution go.
> >
> > - 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.
Adding the enum would not break ABI indeed, but I was thinking about the
way the bus control structure would be declared.
However, upon second inspection on the my PCI implementation, I did not
actually use ITEM_MAX:
> static rte_bus_ctrl_t pci_ctrl_ops[][RTE_BUS_CTRL_OP_MAX] = {
> » [RTE_BUS_CTRL_PROBE_MODE] = {
> » » [RTE_BUS_CTRL_GET] = pci_probe_mode_get,
> » » [RTE_BUS_CTRL_SET] = pci_probe_mode_set,
> » },
> };
I just thought I had used RTE_BUS_CTRL_ITEM_MAX instead of RTE_BUS_CTRL_OP_MAX.
So my line of thought was simply that if any new item was declared, the control
structure would then change size.
But I was mistaken, so that's actually not a problem :)
Having ITEM_MAX available would still make those kind of mistakes
possible however. It might be better to prevent it completely by
removing it. This would however also prevent a custom control section.
Do you think this would be useful enough to justify the slightly more
complex maintenance and review of bus implementations?
On Monday 11 December 2017 08:08 PM, Gaëtan Rivet wrote:
> On Mon, Dec 11, 2017 at 07:06:55PM +0530, Shreyansh Jain wrote:
>> 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?
>>
>>
>
> I was thinking that buses would simply expose a function
>
> rte_busname_xyz_knob1(void *arg);
Yes, that is possible but only for cases where some very specific
functionality needs to be exposed which is not expected to be
generalized ever.
>
> as part of their public API. This would not require an ABI break for
> this bus, as it would only be an API extension and would not use
> callbacks within the bus structure.
Yes, agree with your point.
As such APIs are outside control of DPDK framework, they are something
which will never impact the library layer.
>
> Thus, I think that for buses tempted to propose a specific API, this would be
> the cleanest way. >
> The bus proposing it as part of a custom control section would only be
> interesting when the operation is expected to become a standard API for
> other buses but was not yet accepted. Applications would be able to use
> the interface and the ITEM could be added later. But I doubt this is
> encouraging best practices as far as API evolution go.
So, technically both are feasible: 1) having a bus specific API like
rte_busname_xyz_knob1 and 2) expanding OPS with bus specific values and
allowing application to use them.
But, in either case, if the APIs can be generalized and/or can be used
by multiple buses, they can definitely be moved into the library API
(e.g. rte_bus_probe_mode_set) and/or can be added to rte_bus_ctrl_item.
To summarize, I am OK with your approach.
>
>>>
>>> - 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.
>
> Adding the enum would not break ABI indeed, but I was thinking about the
> way the bus control structure would be declared.
>
> However, upon second inspection on the my PCI implementation, I did not
> actually use ITEM_MAX:
>
>> static rte_bus_ctrl_t pci_ctrl_ops[][RTE_BUS_CTRL_OP_MAX] = {
>> » [RTE_BUS_CTRL_PROBE_MODE] = {
>> » » [RTE_BUS_CTRL_GET] = pci_probe_mode_get,
>> » » [RTE_BUS_CTRL_SET] = pci_probe_mode_set,
>> » },
>> };
>
> I just thought I had used RTE_BUS_CTRL_ITEM_MAX instead of RTE_BUS_CTRL_OP_MAX.
> So my line of thought was simply that if any new item was declared, the control
> structure would then change size.
>
> But I was mistaken, so that's actually not a problem :)
>
> Having ITEM_MAX available would still make those kind of mistakes
> possible however. It might be better to prevent it completely by
> removing it. This would however also prevent a custom control section.
It might be a naive question, but, why do you think it would prevent a
custom control section?
Assuming that only RTE_BUS_CTRL_ITEM_MAX is not available
(RTE_BUS_CTRL_OPS_MAX is available), Bus driver can still define:
static rte_bus_ctrl_t pci_ctrl_ops[][RTE_BUS_CTRL_OP_MAX] = {
[ITEM_1] = {
{...},
{...},
},
[ITEM_2] = {
{...},
{...},
},
[ITEM_NOT_IN_RTE_BUS.H] = {
{...},
{...},
},
}
(I know you disagree with third element of above definition - but
somehow I feel it is a good addition for defining a knob which doesn't
require an additional API call. Just assume as an example for now, please!)
>
> Do you think this would be useful enough to justify the slightly more
> complex maintenance and review of bus implementations?
>
Having RTE_BUS_CTRL_ITEM_MAX is helpful if one has to iterate over all
ctrl_items - but, that might never be required in my perception. Other
than that, I don't have a strong reason to say that ITEM_MAX is required.
Though, same is not true for OP_MAX - which is required for definitions
like above.
@@ -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);
@@ -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,
+};
+
+/**
+ * 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,
+};
+
+/**
+ * Operator for a particular rte_bus configuration item.
+ *
+ * @param arg
+ * Operation parameter.
+ *
+ * @return
+ * 0 on success
+ * !0 otherwise
+ */
+typedef int (*rte_bus_ctrl_t)(void *arg);
+
+/**
+ * Accessor to bus configuration operators.
+ *
+ * @param op
+ * Operation type.
+ *
+ * @param item
+ * Operation element.
+ *
+ * @return
+ * Operator function on success.
+ * NULL if this item is not supported.
+ */
+typedef rte_bus_ctrl_t (*rte_bus_ctrl_get_t)(enum rte_bus_ctrl_op op,
+ enum rte_bus_ctrl_item item);
/**
* Get common iommu class of the all the devices on the bus. The bus may
@@ -211,6 +256,7 @@ struct rte_bus {
rte_bus_parse_t parse; /**< Parse a device name */
struct rte_bus_conf conf; /**< Bus configuration */
rte_bus_get_iommu_class_t get_iommu_class; /**< Get iommu class */
+ rte_bus_ctrl_get_t ctrl; /**< Get control operators */
};
/**