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

[Somnath Kotur]

On Fri, Sep 25, 2020 at 2:12 PM Ferruh Yigit <ferruh.yigit at intel.com> wrote:
>
> On 9/25/2020 3:04 AM, Somnath Kotur wrote:
> > On Thu, Sep 24, 2020 at 8:17 PM Ferruh Yigit <ferruh.yigit at 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 at broadcom.com>
> >>> Reviewed-by: Venkat Duvvuru <venkatkumar.duvvuru at 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 ?
>
> +1
>
> > 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?
>
> Please send a new version of this patch alone. Thanks.
Thanks Ferruh, Done