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

["Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>]

Hi Thomas,

Thanks for reviewing this proposal.


> > Signed-off-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
> > Acked-by: Keith Wiles <keith.wiles@intel.com>
> > Acked-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> [...]
> > +enum rte_eth_capability {
> > +	RTE_ETH_CAPABILITY_FLOW = 0, /**< Flow */
> > +	RTE_ETH_CAPABILITY_TM, /**< Traffic Manager */
> > +	RTE_ETH_CAPABILITY_MAX
> > +};
> [...]
> >  /**
> > + * Take capability operations on an Ethernet device.
> > + *
> > + * @param port_id
> > + *   The port identifier of the Ethernet device.
> > + * @param cap
> > + *   The capability of the Ethernet device
> > + * @param arg
> > + *   A pointer to arguments defined specifically for the operation.
> > + * @return
> > + *   - (0) if successful.
> > + *   - (-ENOTSUP) if hardware doesn't support.
> > + *   - (-ENODEV) if *port_id* invalid.
> > + */
> > +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);

Regards,
Cristian