diff mbox series

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

Message ID 20200922070632.17706-6-somnath.kotur@broadcom.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ajit Khaparde
Headers
Series bnxt patches |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Somnath Kotur Sept. 22, 2020, 7:06 a.m. UTC 
  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

Ferruh Yigit Sept. 24, 2020, 2:47 p.m. UTC | #1 
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?
Somnath Kotur Sept. 25, 2020, 2:04 a.m. UTC | #2 
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
Ferruh Yigit Sept. 25, 2020, 8:42 a.m. UTC | #3 
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.
Somnath Kotur Sept. 25, 2020, 10:46 a.m. UTC | #4 
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
Somnath Kotur Sept. 25, 2020, 10:49 a.m. UTC | #5 
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
diff mbox series

Patch

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;
+
 	/* probe representor ports now */
 	ret = bnxt_rep_port_probe(pci_dev, eth_da, backing_eth_dev);