> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monja...@6wind.com]
> Sent: Monday, March 6, 2017 4:57 PM
> To: Dumitrescu, Cristian <cristian.dumitre...@intel.com>
> Cc: dev@dpdk.org; jerin.ja...@caviumnetworks.com;
> balasubramanian.manoha...@cavium.com; hemant.agra...@nxp.com;
> shreyansh.j...@nxp.com; Wiles, Keith <keith.wi...@intel.com>; Richardson,
> Bruce <bruce.richard...@intel.com>
> Subject: Re: [PATCH v3 1/2] ethdev: add capability control API
>
> 2017-03-06 16:35, Dumitrescu, Cristian:
> > Hi Thomas,
> >
> > Thanks for reviewing this proposal.
> >
> >
> > > > Signed-off-by: Cristian Dumitrescu <cristian.dumitre...@intel.com>
> > > > Acked-by: Keith Wiles <keith.wi...@intel.com>
> > > > Acked-by: Jerin Jacob <jerin.ja...@caviumnetworks.com>
> > > > Acked-by: Hemant Agrawal <hemant.agra...@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);
>
> 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.
