Re: [dpdk-dev] [PATCH v5 01/51] net/bnxt: add basic infrastructure for VF reps

[Somnath Kotur <somnath.kotur@broadcom.com>]

On Mon, Jul 6, 2020 at 3:37 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> On 7/3/2020 10:01 PM, Ajit Khaparde wrote:
> > From: Somnath Kotur <somnath.kotur@broadcom.com>
> >
> > Defines data structures and code to init/uninit
> > VF representors during pci_probe and pci_remove
> > respectively.
> > Most of the dev_ops for the VF representor are just
> > stubs for now and will be will be filled out in next patch.
> >
> > To create a representor using testpmd:
> > testpmd -c 0xff -wB:D.F,representor=1 -- -i
> > testpmd -c 0xff -w05:02.0,representor=[1] -- -i
> >
> > To create a representor using ovs-dpdk:
> > 1. Firt add the trusted VF port to a bridge
> > ovs-vsctl add-port ovsbr0 vf_rep1 -- set Interface vf_rep1 type=dpdk
> > options:dpdk-devargs=0000:06:02.0
> > 2. Add the representor port to the bridge
> > ovs-vsctl add-port ovsbr0 vf_rep1 -- set Interface vf_rep1 type=dpdk
> > options:dpdk-devargs=0000:06:02.0,representor=1
> >
> > Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
> > Signed-off-by: Venkat Duvvuru <venkatkumar.duvvuru@broadcom.com>
> > Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
>
> <...>
>
> >  static int bnxt_pci_remove(struct rte_pci_device *pci_dev)
> >  {
> > -     if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> > -             return rte_eth_dev_pci_generic_remove(pci_dev,
> > -                             bnxt_dev_uninit);
> > -     else
> > +     struct rte_eth_dev *eth_dev;
> > +
> > +     eth_dev = rte_eth_dev_allocated(pci_dev->device.name);
> > +     if (!eth_dev)
> > +             return ENODEV; /* Invoked typically only by OVS-DPDK, by the
> > +                             * time it comes here the eth_dev is already
> > +                             * deleted by rte_eth_dev_close(), so returning
> > +                             * +ve value will atleast help in proper cleanup
> > +                             */
>
> Why returning a positive error value? It hides the error since the caller of the
> function does a "< 0" check.
> Better to be more explicit and return '0' if an error is not intendent in this case.
>
Sure, makes sense Ferruh