Re: [dpdk-dev] [PATCH] net/i40e: fix unchecked return value

Re: [dpdk-dev] [PATCH] net/i40e: fix unchecked return value

[Bruce Richardson]

On Wed, Feb 12, 2020 at 03:02:00AM +0800, Beilei Xing wrote:
> Check the return value of the i40e_xmit_cleanup function.
> 
> Coverity issue: 353617
> Fixes: 4861cde46116 ("i40e: new poll mode driver")
> 
> Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> ---
>  drivers/net/i40e/i40e_rxtx.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
> index fd1ae80..f43fc0f 100644
> --- a/drivers/net/i40e/i40e_rxtx.c
> +++ b/drivers/net/i40e/i40e_rxtx.c
> @@ -1038,8 +1038,9 @@ i40e_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
>  	txe = &sw_ring[tx_id];
>  
>  	/* Check if the descriptor ring needs to be cleaned. */
> -	if (txq->nb_tx_free < txq->tx_free_thresh)
> -		i40e_xmit_cleanup(txq);
> +	if ((txq->nb_tx_free < txq->tx_free_thresh) &&
> +	    (i40e_xmit_cleanup(txq) != 0))
> +		return 0;
>  

I don't think this should be fixed, and the original code is correct.

This cleanup is opportunistic and may not cause problems if it fails. For
example, if tx_free_thresh is 32, and nb_tx_free is 24, there is no reason
to return zero here if the total packets to be sent it 16 - since all
packets can feasibly fit. Even if we had 32 to transmit, we still should
not quit here, since any packets that can be transmitted should be sent,
and there is a subsequent cleanup call at line 1084 to handle failed
cleanup when it does become a problem.

Regards,
/Bruce

[dpdk-dev] [PATCH] net/i40e: fix unchecked return value

[Beilei Xing]

Check the return value of the i40e_xmit_cleanup function.

Coverity issue: 353617
Fixes: 4861cde46116 ("i40e: new poll mode driver")

Signed-off-by: Beilei Xing <beilei.xing@intel.com>
---
 drivers/net/i40e/i40e_rxtx.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
index fd1ae80..f43fc0f 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -1038,8 +1038,9 @@ i40e_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
 	txe = &sw_ring[tx_id];
 
 	/* Check if the descriptor ring needs to be cleaned. */
-	if (txq->nb_tx_free < txq->tx_free_thresh)
-		i40e_xmit_cleanup(txq);
+	if ((txq->nb_tx_free < txq->tx_free_thresh) &&
+	    (i40e_xmit_cleanup(txq) != 0))
+		return 0;
 
 	for (nb_tx = 0; nb_tx < nb_pkts; nb_tx++) {
 		td_cmd = 0;
-- 
2.7.4

Re: [dpdk-dev] [PATCH] net/i40e: fix unchecked return value

[Xing, Beilei]

> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Tuesday, February 11, 2020 7:42 PM
> To: Xing, Beilei <beilei.xing@intel.com>
> Cc: dev@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] net/i40e: fix unchecked return value
> 
> On Wed, Feb 12, 2020 at 03:02:00AM +0800, Beilei Xing wrote:
> > Check the return value of the i40e_xmit_cleanup function.
> >
> > Coverity issue: 353617
> > Fixes: 4861cde46116 ("i40e: new poll mode driver")
> >
> > Signed-off-by: Beilei Xing <beilei.xing@intel.com>
> > ---
> >  drivers/net/i40e/i40e_rxtx.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/i40e/i40e_rxtx.c
> > b/drivers/net/i40e/i40e_rxtx.c index fd1ae80..f43fc0f 100644
> > --- a/drivers/net/i40e/i40e_rxtx.c
> > +++ b/drivers/net/i40e/i40e_rxtx.c
> > @@ -1038,8 +1038,9 @@ i40e_xmit_pkts(void *tx_queue, struct rte_mbuf
> **tx_pkts, uint16_t nb_pkts)
> >  	txe = &sw_ring[tx_id];
> >
> >  	/* Check if the descriptor ring needs to be cleaned. */
> > -	if (txq->nb_tx_free < txq->tx_free_thresh)
> > -		i40e_xmit_cleanup(txq);
> > +	if ((txq->nb_tx_free < txq->tx_free_thresh) &&
> > +	    (i40e_xmit_cleanup(txq) != 0))
> > +		return 0;
> >
> 
> I don't think this should be fixed, and the original code is correct.
> 
> This cleanup is opportunistic and may not cause problems if it fails. For
> example, if tx_free_thresh is 32, and nb_tx_free is 24, there is no reason to
> return zero here if the total packets to be sent it 16 - since all packets can
> feasibly fit. Even if we had 32 to transmit, we still should not quit here, since
> any packets that can be transmitted should be sent, and there is a
> subsequent cleanup call at line 1084 to handle failed cleanup when it does
> become a problem.

Yes, agree. Will abandon the patch. Thanks.

> 
> Regards,
> /Bruce

[dpdk-dev] [PATCH v2] net/i40e: fix unchecked return value

[Beilei Xing]

This patch fixes unchecked return value
of the i40e_xmit_cleanup function.

Coverity issue: 353617
Fixes: 4861cde46116 ("i40e: new poll mode driver")

Signed-off-by: Beilei Xing <beilei.xing@intel.com>
---
V2 change:
 - Use cast.

 drivers/net/i40e/i40e_rxtx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
index fd1ae80..f6d23c9 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -1039,7 +1039,7 @@ i40e_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
 
 	/* Check if the descriptor ring needs to be cleaned. */
 	if (txq->nb_tx_free < txq->tx_free_thresh)
-		i40e_xmit_cleanup(txq);
+		(void)i40e_xmit_cleanup(txq);
 
 	for (nb_tx = 0; nb_tx < nb_pkts; nb_tx++) {
 		td_cmd = 0;
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2] net/i40e: fix unchecked return value

[Ye Xiaolong]

On 02/12, Beilei Xing wrote:
>This patch fixes unchecked return value
>of the i40e_xmit_cleanup function.
>
>Coverity issue: 353617
>Fixes: 4861cde46116 ("i40e: new poll mode driver")
>
>Signed-off-by: Beilei Xing <beilei.xing@intel.com>
>---
>V2 change:
> - Use cast.
>
> drivers/net/i40e/i40e_rxtx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
>index fd1ae80..f6d23c9 100644
>--- a/drivers/net/i40e/i40e_rxtx.c
>+++ b/drivers/net/i40e/i40e_rxtx.c
>@@ -1039,7 +1039,7 @@ i40e_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
> 
> 	/* Check if the descriptor ring needs to be cleaned. */
> 	if (txq->nb_tx_free < txq->tx_free_thresh)
>-		i40e_xmit_cleanup(txq);
>+		(void)i40e_xmit_cleanup(txq);
> 
> 	for (nb_tx = 0; nb_tx < nb_pkts; nb_tx++) {
> 		td_cmd = 0;
>-- 
>2.7.4
>

Acked-by: Xiaolong Ye <xiaolong.ye@intel.com>

Applied to dpdk-next-net-intel, Thanks.