Re: [dpdk-dev] [PATCH v3] ethdev: modifiy vlan_offload_set_t to return int

[Thomas Monjalon <thomas@monjalon.net>]

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