[5/8] net/bnxt: add a null ptr check in bnxt PCI probe
Checks
Commit Message
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(+)
Comments
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?
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?
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
On 9/25/2020 3:04 AM, Somnath Kotur wrote:
> 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 ?
+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.
On Fri, Sep 25, 2020 at 2:12 PM Ferruh Yigit <ferruh.yigit@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@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 ?
>
> +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
On Fri, Sep 25, 2020 at 4:16 PM Somnath Kotur
<somnath.kotur@broadcom.com> wrote:
>
> On Fri, Sep 25, 2020 at 2:12 PM Ferruh Yigit <ferruh.yigit@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@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 ?
> >
> > +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
Sorry Ferruh, the first one i sent did not have the in-reply-to,
re-sent it with the correct in-reply-to format again, so please pick
that up
Thanks a lot
-Som
@@ -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;
+
/* probe representor ports now */
ret = bnxt_rep_port_probe(pci_dev, eth_da, backing_eth_dev);