Re: [dpdk-dev] [PATCH v3 29/29] ethdev: allow close function to return an error

[Thomas Monjalon <thomas@monjalon.net>]

29/09/2020 13:05, Andrew Rybchenko:
> On 9/29/20 2:14 AM, Thomas Monjalon wrote:
> > The API function rte_eth_dev_close() was returning void.
> > The return type is changed to int for notifying of errors.
> > 
> > If an error happens during a close operation,
> > the status of the port is undefined,
> > a maximum of resources having been freed.
[...]
> > @@ -135,7 +135,6 @@ Deprecation Notices
> >    invalid port ID, unsupported operation, failed operation):
> >  
> >    - ``rte_eth_dev_stop``
> > -  - ``rte_eth_dev_close``
> 
> Many thanks for continuing my unfinished job.

It is not finished.
Would you take rte_eth_dev_stop?

> >  rte_pmd_mvneta_remove(struct rte_vdev_device *vdev)
> >  {
> >  	uint16_t port_id;
> > +	int ret = 0;
> >  
> >  	RTE_ETH_FOREACH_DEV(port_id) {
> >  		if (rte_eth_devices[port_id].device != &vdev->device)
> >  			continue;
> > -		rte_eth_dev_close(port_id);
> > +		ret = rte_eth_dev_close(port_id);
> 
> I guess |= should be used here as well (similar as above).

Yes

> > -void
> > +int
> >  rte_eth_dev_close(uint16_t port_id)
> >  {
> >  	struct rte_eth_dev *dev;
> > +	int ret;
> >  
> > -	RTE_ETH_VALID_PORTID_OR_RET(port_id);
> > +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
> >  	dev = &rte_eth_devices[port_id];
> >  
> > -	RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_close);
> > -	(*dev->dev_ops->dev_close)(dev);
> > +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_close, -ENOTSUP);
> > +	ret = (*dev->dev_ops->dev_close)(dev);
> >  
> >  	rte_ethdev_trace_close(port_id);
> > -	rte_eth_dev_release_port(dev);
> > +	ret = rte_eth_dev_release_port(dev);
> 
> It does not look good that it overwrites close return status.

Indeed, it seems I did not finish this patch,
or maybe I was drunk ;)

What do you think is the best strategy?
I think we must call rte_eth_dev_release_port()
even if dev_close returns an error.
Then which error to return? I would return the first non-zero one.