Re: [dpdk-dev] [PATCH] net/iavf: release port upon close

[Thomas Monjalon <thomas@monjalon.net>]

Hi,

SteveX Yang <stevex.yang@intel.com> wrote:
> Set RTE_ETH_DEV_CLOSE_REMOVE upon probe so all the private resources
> for the port can be freed by rte_eth_dev_close().
> 
> Signed-off-by: SteveX Yang <stevex.yang@intel.com>

I guess the X is not part of your name.

[...]
> -static int
> -iavf_dev_uninit(struct rte_eth_dev *dev)
> -{
> -       struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
> -
> -       if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> -               return -EPERM;

All the code below is moved from iavf_dev_uninit() where it was
restricted to the primary process, to iavf_dev_close() which can be
called from the primary process.
It looks inconsistent.

> 
>         dev->dev_ops = NULL;
>         dev->rx_pkt_burst = NULL;
>         dev->tx_pkt_burst = NULL;
> 
> -       iavf_dev_close(dev);
> +
> +       if (vf->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_RSS_PF) {
> +               if (vf->rss_lut) {
> +                       rte_free(vf->rss_lut);
> +                       vf->rss_lut = NULL;
> +               }
> +               if (vf->rss_key) {
> +                       rte_free(vf->rss_key);
> +                       vf->rss_key = NULL;
> +               }
> +       }
> 
>         rte_free(vf->vf_res);
>         vf->vsi_res = NULL;
> 
> @@ -1449,15 +1456,15 @@ iavf_dev_uninit(struct rte_eth_dev *dev)
> 
>         rte_free(vf->aq_resp);
>         vf->aq_resp = NULL;
> 
> +}
> 
> -       if (vf->rss_lut) {
> -               rte_free(vf->rss_lut);
> -               vf->rss_lut = NULL;
> -       }
> -       if (vf->rss_key) {
> -               rte_free(vf->rss_key);
> -               vf->rss_key = NULL;
> -       }
> +static int
> +iavf_dev_uninit(struct rte_eth_dev *dev)
> +{
> +       if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +               return -EPERM;
> +
> +       iavf_dev_close(dev);

Are you sure about what should be freed in the secondary process?
If iavf_dev_close() should not be called by the secondary process,
you should move the check inside the function,
because iavf_dev_close() is also called by rte_eth_dev_close().

> 
>         return 0;
>  
>  }