Re: [PATCH v3 1/2] ethdev: add capability control API

[Thomas Monjalon <thomas.monjalon@6wind.com>]

> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > 2017-03-06 16:35, Dumitrescu, Cristian:
> > > > > +int rte_eth_dev_capability_ops_get(uint8_t port_id,
> > > > > +	enum rte_eth_capability cap, void *arg);
> > > >
> > > > What is the benefit of getting different kind of capabilities with
> > > > the same function?
> > > > enum + void* = ioctl
> > > > A self-explanatory API should have a dedicated function for each kind
> > > > of features with different argument types.
> > >
> > > The advantage is providing a standard interface to query the capabilities of
> > the device rather than having each capability provide its own mechanism in a
> > slightly different way.
> > >
> > > IMO this mechanism is of great help to guide the developers of future
> > ethdev features on the clean path to add new features in a modular way,
> > extending the ethdev functionality while doing so in a separate name space
> > and file (that's why I tend to call this a plugin-like mechanism), as opposed to
> > the current monolithic approach for ethdev, where we have 100+ API
> > functions in a single name space and that are split into functional groups just
> > by blank lines in the header file. It is simply the generalization of the
> > mechanism introduced by rte_flow in release 17.02 (so all the credit should
> > go to Adrien and not me).
> > >
> > > IMO, having a standard function as above it cleaner than having a separate
> > and slightly different function per feature. People can quickly see the set of
> > standard ethdev capabilities and which ones are supported by a specific
> > device. Between A) and B) below, I definitely prefer A):
> > > A) status = rte_eth_dev_capability_ops_get(port_id,
> > RTE_ETH_CABABILITY_TM, &tm_ops);
> > > B) status = rte_eth_dev_tm_ops_get(port_id, &tm_ops);
> > 
> > I prefer B because instead of tm_ops, you can use some specific tm
> > arguments,
> > show their types and properly document each parameter.
> 
> Note that rte_flow already returns the flow ops as a void * with no strong argument type checking (approach A from above). Are you saying this is wrong?
> 
> 	rte_eth_dev_filter_ctrl(port_id, RTE_ETH_FILTER_GENERIC, RTE_ETH_FILTER_GET, void *eth_flow_ops);
> 
> Personally, I am in favour of allowing the standard interface at the expense of strong build-time type checking. Especially that this API function is between ethdev and the drivers, as opposed to between app and ethdev.

rte_eth_dev_filter_ctrl is going to be specialized in rte_flow operations.
I agree with you on having independent API blocks in ethdev like rte_flow.
But this function rte_eth_dev_capability_ops_get that you propose would be
cross-blocks. I don't see the benefit.
I especially don't think there is a sense in the enum
	enum rte_eth_capability {
		RTE_ETH_CAPABILITY_FLOW = 0, /**< Flow */
		RTE_ETH_CAPABILITY_TM, /**< Traffic Manager */
		RTE_ETH_CAPABILITY_MAX
	}

I won't debate more on this. We have to read opinions of other reviewers.