Re: [dpdk-dev] [PATCH 5/8] net/bnxt: add a null ptr check in bnxt PCI probe

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

On Thu, Sep 24, 2020 at 8:17 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> On 9/22/2020 8:06 AM, Somnath Kotur wrote:
> > Check for devargs before invoking rep port probe.
> >
> > Fixes: 6dc83230b43b ("net/bnxt: support port representor data path")
> >
> > Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
> > Reviewed-by: Venkat Duvvuru <venkatkumar.duvvuru@broadcom.com>
> > ---
> >   drivers/net/bnxt/bnxt_ethdev.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
> > index db2f0dd..84eba0b 100644
> > --- a/drivers/net/bnxt/bnxt_ethdev.c
> > +++ b/drivers/net/bnxt/bnxt_ethdev.c
> > @@ -6147,6 +6147,10 @@ static int bnxt_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
> >       }
> >       PMD_DRV_LOG(DEBUG, "BNXT Port:%d pci probe\n",
> >                   backing_eth_dev->data->port_id);
> > +
> > +     if (!pci_dev->device.devargs)
> > +             return ret;
> > +
>
> There is already a null check at the beginning of the function because
> of the same thing (port representors), should they be combined?
>
No, this is to catch the corner case if/when 'backing_eth_dev' is
already allocated , so code would unconditionally call
bnxt_rep_port_probe()
irrespective of devargs being there or not, the check at this point
helps prevent that
> And devargs being not NULL does not really mean it has arguments related
> to the port representors, it may have other device devargs. Perhaps
> 'eth_da' can  be used to check?
eth_da is a local var in this function, so perhaps 'num_rep' i.e
invoke bnxt_rep_port_probe only if num_rep > 0 ?
Please let me know if you want me to do a respin of this patch alone
or will you be doing this minor change while merging it in?

Thanks
Som