On 4/30/2019 8:55 AM, Thomas Monjalon wrote: > 30/04/2019 09:33, Ferruh Yigit: >> On 4/29/2019 11:34 PM, Thomas Monjalon wrote: >>> 30/04/2019 00:28, Ferruh Yigit: >>>> On 4/29/2019 9:14 PM, Thomas Monjalon wrote: >>>>> 29/04/2019 19:00, Ferruh Yigit: >>>>>> On 4/26/2019 6:09 AM, Xiaolong Ye wrote: >>>>>>> Since 18.11, it is suggested that driver should release all its private >>>>>>> resources at the dev_close routine. So all resources previously released >>>>>>> in remove routine are now released at the dev_close routine, and the >>>>>>> dev_close routine will be called in driver remove routine in order to >>>>>>> support removing a device without closing its ports. >>>>>>> >>>>>>> Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE >>>>>>> flag during probe stage. >>>>>>> >>>>>>> Signed-off-by: Xiaolong Ye <xiaolong...@intel.com> >>>>>> >>>>>> <...> >>>>>> >>>>>>> @@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev) >>>>>>> if (eth_dev == NULL) >>>>>>> return -1; >>>>>>> >>>>>>> - internals = eth_dev->data->dev_private; >>>>>>> - >>>>>>> - rte_ring_free(internals->umem->buf_ring); >>>>>>> - rte_memzone_free(internals->umem->mz); >>>>>>> - rte_free(internals->umem); >>>>>>> - >>>>>>> - rte_eth_dev_release_port(eth_dev); >>>>>> >>>>>> I thinks we should keep 'rte_eth_dev_release_port()' in '.remove()' path, >>>>>> the 'RTE_ETH_DEV_CLOSE_REMOVE' flag will take care of this in >>>>>> 'rte_eth_dev_close()' but still needed in '.remove()' path. >>>>> >>>>> I don't understand your comment. >>>>> Calling the close function looks the right thing to do in "remove". >>>> >>>> No concern on calling the 'close'. >>>> My comment was to keep 'rte_eth_dev_release_port()' which this patch >>>> removes. >>> >>> rte_eth_dev_release_port() is called in eth_dev_close(), isn't it? >> >> No, 'eth_dev_close()' is local 'dev_close()' ops, the one to clear driver >> private resources. >> I assume it is confused with 'rte_eth_dev_close()' ... >> >>>>>>> - >>>>>>> + eth_dev_close(eth_dev); > > Ah yes, I overlooked it. > Why not calling rte_eth_dev_close()? >
It may work, but I am for keeping 'dev_close()' and 'rte_eth_dev_release_port()' as two steps in '.remove()', I think there is no need to include 'rte_eth_dev_close()' API here and its possible/future side affects etc...
