One more comment below
01/09/2017 00:04, Thomas Monjalon:
> Hi,
>
> 25/08/2017 15:47, David Harton:
> > Some devices may not support or fail setting VLAN offload
> > configuration based on dynamic circurmstances so the
> > vlan_offload_set_t vector is modified to return an int so
> > the caller can determine success or not.
>
> I agree with allowing to return an error.
>
> Comments on details below.
>
> The title could be changed to better reflect the purpose:
> ethdev: allow returning error on VLAN configuration
>
> > --- a/doc/guides/rel_notes/release_17_11.rst
> > +++ b/doc/guides/rel_notes/release_17_11.rst
> > @@ -124,7 +124,7 @@ ABI Changes
> > Also, make sure to start the actual text at the margin.
> > =========================================================
> >
> > -
> > +* Changed return type of ``vlan_offload_set_t`` from ``void`` to ``int``.
>
> It should be referenced as an API change (instead of ABI change).
> (and line spacing must be kept)
>
> We also need to bump the library version but it can be done with
> bigger changes in ethdev.
>
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > @@ -2049,10 +2049,16 @@ struct rte_eth_dev *
> > int ret = 0;
> > int mask = 0;
> > int cur, org = 0;
> > + uint8_t org_strip, org_filter, org_extend;
> >
> > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > dev = &rte_eth_devices[port_id];
> >
> > + /* save original values in case of failure */
> > + org_strip = dev->data->dev_conf.rxmode.hw_vlan_strip;
> > + org_filter = dev->data->dev_conf.rxmode.hw_vlan_filter;
> > + org_extend = dev->data->dev_conf.rxmode.hw_vlan_extend;
>
> Please waste one more char to write "orig" instead of "org".
>
> > - (*dev->dev_ops->vlan_offload_set)(dev, mask);
> > + ret = (*dev->dev_ops->vlan_offload_set)(dev, mask);
Which error codes can be returned by this op?
It is adding new error codes to rte_eth_dev_set_vlan_offload(),
and they must be documented in the doxygen.
> > + if (ret) {
> > + /* hit an error restore original values */
> > + dev->data->dev_conf.rxmode.hw_vlan_strip = org_strip;
> > + dev->data->dev_conf.rxmode.hw_vlan_filter = org_filter;
> > + dev->data->dev_conf.rxmode.hw_vlan_extend = org_extend;
> > + }
>
> Isn't it the responsibility of the PMD to restore values in case of error?
> I understand it is there to factorize error handling code, right?
> Do we want to document this behaviour with the ops prototype?
Thanks
