diff mbox

[dpdk-dev] event/rx_adapter: fix ignore return of event start

Message ID 1517352989-11720-1-git-send-email-vipin.varghese@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Jerin Jacob
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Varghese, Vipin Jan. 30, 2018, 10:56 p.m. UTC 
  Capture the return value for rte_event_dev_start. Return the
result back to user.

Coverity issue: 257000
Fixes: 9c38b704d280 ("eventdev: add eth Rx adapter implementation")
Cc: nikhil.rao@intel.com

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
---
 lib/librte_eventdev/rte_event_eth_rx_adapter.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Rao, Nikhil Jan. 31, 2018, 5:32 a.m. UTC | #1 
Adding eventdev PMD folks for their suggestions on how to handle the return value from rte_event_dev_start() below.

> -----Original Message-----
> From: Varghese, Vipin
> Sent: Wednesday, January 31, 2018 4:26 AM
> To: dev@dpdk.org; Rao, Nikhil <nikhil.rao@intel.com>
> Cc: Jain, Deepak K <deepak.k.jain@intel.com>; Varghese, Vipin
> <vipin.varghese@intel.com>
> Subject: [PATCH] event/rx_adapter: fix ignore return of event start
> 
> Capture the return value for rte_event_dev_start. Return the result back to
> user.
> 
> Coverity issue: 257000
> Fixes: 9c38b704d280 ("eventdev: add eth Rx adapter implementation")
> Cc: nikhil.rao@intel.com
> 
> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> ---
>  lib/librte_eventdev/rte_event_eth_rx_adapter.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_eventdev/rte_event_eth_rx_adapter.c
> b/lib/librte_eventdev/rte_event_eth_rx_adapter.c
> index 90106e6..a818bef 100644
> --- a/lib/librte_eventdev/rte_event_eth_rx_adapter.c
> +++ b/lib/librte_eventdev/rte_event_eth_rx_adapter.c
> @@ -603,7 +603,7 @@ static uint16_t gcd_u16(uint16_t a, uint16_t b)
>  		RTE_EDEV_LOG_ERR("failed to configure event dev %u\n",
>  						dev_id);
>  		if (started)
> -			rte_event_dev_start(dev_id);
> +			ret = rte_event_dev_start(dev_id);

Currently the a non-zero return value at this point signifies an error returned from rte_event_dev_configure(),  so I suggest that the return value is typecasted to void.

>  		return ret;
>  	}
> 
> @@ -617,7 +617,7 @@ static uint16_t gcd_u16(uint16_t a, uint16_t b)
>  	conf->event_port_id = port_id;
>  	conf->max_nb_rx = 128;
>  	if (started)
> -		rte_event_dev_start(dev_id);
> +		ret = rte_event_dev_start(dev_id);
This change looks good to me.

>  	rx_adapter->default_cb_arg = 1;
>  	return ret;
>  }
> --
> 1.9.1
Jerin Jacob Jan. 31, 2018, 6:54 a.m. UTC | #2 
-----Original Message-----
>
>
> Adding eventdev PMD folks for their suggestions on how to handle the return value from rte_event_dev_start() below.
>
> > -----Original Message-----
> > From: Varghese, Vipin
> > Sent: Wednesday, January 31, 2018 4:26 AM
> > To: dev@dpdk.org; Rao, Nikhil <nikhil.rao@intel.com>
> > Cc: Jain, Deepak K <deepak.k.jain@intel.com>; Varghese, Vipin
> > <vipin.varghese@intel.com>
> > Subject: [PATCH] event/rx_adapter: fix ignore return of event start
> >
> > Capture the return value for rte_event_dev_start. Return the result back to
> > user.
> >
> > Coverity issue: 257000
> > Fixes: 9c38b704d280 ("eventdev: add eth Rx adapter implementation")
> > Cc: nikhil.rao@intel.com
> >
> > Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> > ---
> >  lib/librte_eventdev/rte_event_eth_rx_adapter.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/librte_eventdev/rte_event_eth_rx_adapter.c
> > b/lib/librte_eventdev/rte_event_eth_rx_adapter.c
> > index 90106e6..a818bef 100644
> > --- a/lib/librte_eventdev/rte_event_eth_rx_adapter.c
> > +++ b/lib/librte_eventdev/rte_event_eth_rx_adapter.c
> > @@ -603,7 +603,7 @@ static uint16_t gcd_u16(uint16_t a, uint16_t b)
> >  		RTE_EDEV_LOG_ERR("failed to configure event dev %u\n",
> >  						dev_id);
> >  		if (started)
> > -			rte_event_dev_start(dev_id);
> > +			ret = rte_event_dev_start(dev_id);
>
> Currently the a non-zero return value at this point signifies an error returned from rte_event_dev_configure(),  so I suggest that the return value is typecasted to void.

If I understand it correctly, Any one of the failure(configure() or start()) should result in bad state. Right?
i.e If some reason PMD is not able to start() even after failure configuration() would result in bad state.
If so, one option could be combine the error like ret |= operation or create a new logical error in Rx adapter
which denotes this new error.

>
> >  		return ret;
> >  	}
> >
> > @@ -617,7 +617,7 @@ static uint16_t gcd_u16(uint16_t a, uint16_t b)
> >  	conf->event_port_id = port_id;
> >  	conf->max_nb_rx = 128;
> >  	if (started)
> > -		rte_event_dev_start(dev_id);
> > +		ret = rte_event_dev_start(dev_id);
> This change looks good to me.
>
> >  	rx_adapter->default_cb_arg = 1;
> >  	return ret;
> >  }
> > --
> > 1.9.1
>
Varghese, Vipin Feb. 2, 2018, 8:08 a.m. UTC | #3 
> -----Original Message-----
> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> Sent: Wednesday, January 31, 2018 6:54 AM
> To: Rao, Nikhil <nikhil.rao@intel.com>
> Cc: Varghese, Vipin <vipin.varghese@intel.com>; dev@dpdk.org; Jacob, Jerin
> <Jerin.JacobKollanukkaran@cavium.com>; Van Haaren, Harry
> <harry.van.haaren@intel.com>; Hemant Agrawal <hemant.agrawal@nxp.com>;
> Jain, Deepak K <deepak.k.jain@intel.com>
> Subject: Re: [PATCH] event/rx_adapter: fix ignore return of event start
> 
> -----Original Message-----
> >
> >
> > Adding eventdev PMD folks for their suggestions on how to handle the return
> value from rte_event_dev_start() below.
> >
> > > -----Original Message-----
> > > From: Varghese, Vipin
> > > Sent: Wednesday, January 31, 2018 4:26 AM
> > > To: dev@dpdk.org; Rao, Nikhil <nikhil.rao@intel.com>
> > > Cc: Jain, Deepak K <deepak.k.jain@intel.com>; Varghese, Vipin
> > > <vipin.varghese@intel.com>
> > > Subject: [PATCH] event/rx_adapter: fix ignore return of event start
> > >
> > > Capture the return value for rte_event_dev_start. Return the result
> > > back to user.
> > >
> > > Coverity issue: 257000
> > > Fixes: 9c38b704d280 ("eventdev: add eth Rx adapter implementation")
> > > Cc: nikhil.rao@intel.com
> > >
> > > Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> > > ---
> > >  lib/librte_eventdev/rte_event_eth_rx_adapter.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/lib/librte_eventdev/rte_event_eth_rx_adapter.c
> > > b/lib/librte_eventdev/rte_event_eth_rx_adapter.c
> > > index 90106e6..a818bef 100644
> > > --- a/lib/librte_eventdev/rte_event_eth_rx_adapter.c
> > > +++ b/lib/librte_eventdev/rte_event_eth_rx_adapter.c
> > > @@ -603,7 +603,7 @@ static uint16_t gcd_u16(uint16_t a, uint16_t b)
> > >  		RTE_EDEV_LOG_ERR("failed to configure event dev %u\n",
> > >  						dev_id);
> > >  		if (started)
> > > -			rte_event_dev_start(dev_id);
> > > +			ret = rte_event_dev_start(dev_id);
> >
> > Currently the a non-zero return value at this point signifies an error returned
> from rte_event_dev_configure(),  so I suggest that the return value is typecasted
> to void.
> 
> If I understand it correctly, Any one of the failure(configure() or start()) should
> result in bad state. Right?
> i.e If some reason PMD is not able to start() even after failure configuration()
> would result in bad state.
> If so, one option could be combine the error like ret |= operation or create a
> new logical error in Rx adapter which denotes this new error.
> 

So do we agree to ACK these changes to get the code fix to the mainline? Then rework the logic as required?

> >
> > >  		return ret;
> > >  	}
> > >
> > > @@ -617,7 +617,7 @@ static uint16_t gcd_u16(uint16_t a, uint16_t b)
> > >  	conf->event_port_id = port_id;
> > >  	conf->max_nb_rx = 128;
> > >  	if (started)
> > > -		rte_event_dev_start(dev_id);
> > > +		ret = rte_event_dev_start(dev_id);
> > This change looks good to me.
> >
> > >  	rx_adapter->default_cb_arg = 1;
> > >  	return ret;
> > >  }
> > > --
> > > 1.9.1
> >
Rao, Nikhil Feb. 2, 2018, 10:04 a.m. UTC | #4 
> -----Original Message-----
> From: Varghese, Vipin
> Sent: Friday, February 2, 2018 1:39 PM
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>; Rao, Nikhil
> <nikhil.rao@intel.com>
> Cc: dev@dpdk.org; Jacob, Jerin <Jerin.JacobKollanukkaran@cavium.com>; Van
> Haaren, Harry <harry.van.haaren@intel.com>; Hemant Agrawal
> <hemant.agrawal@nxp.com>; Jain, Deepak K <deepak.k.jain@intel.com>
> Subject: RE: [PATCH] event/rx_adapter: fix ignore return of event start
> 
> 
> 
> > -----Original Message-----
> > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> > Sent: Wednesday, January 31, 2018 6:54 AM
> > To: Rao, Nikhil <nikhil.rao@intel.com>
> > Cc: Varghese, Vipin <vipin.varghese@intel.com>; dev@dpdk.org; Jacob,
> > Jerin <Jerin.JacobKollanukkaran@cavium.com>; Van Haaren, Harry
> > <harry.van.haaren@intel.com>; Hemant Agrawal
> <hemant.agrawal@nxp.com>;
> > Jain, Deepak K <deepak.k.jain@intel.com>
> > Subject: Re: [PATCH] event/rx_adapter: fix ignore return of event
> > start
> >
> > -----Original Message-----
> > >
> > >
> > > Adding eventdev PMD folks for their suggestions on how to handle the
> > > return
> > value from rte_event_dev_start() below.
> > >
> > > > -----Original Message-----
> > > > From: Varghese, Vipin
> > > > Sent: Wednesday, January 31, 2018 4:26 AM
> > > > To: dev@dpdk.org; Rao, Nikhil <nikhil.rao@intel.com>
> > > > Cc: Jain, Deepak K <deepak.k.jain@intel.com>; Varghese, Vipin
> > > > <vipin.varghese@intel.com>
> > > > Subject: [PATCH] event/rx_adapter: fix ignore return of event
> > > > start
> > > >
> > > > Capture the return value for rte_event_dev_start. Return the
> > > > result back to user.
> > > >
> > > > Coverity issue: 257000
> > > > Fixes: 9c38b704d280 ("eventdev: add eth Rx adapter
> > > > implementation")
> > > > Cc: nikhil.rao@intel.com
> > > >
> > > > Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> > > > ---
> > > >  lib/librte_eventdev/rte_event_eth_rx_adapter.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/lib/librte_eventdev/rte_event_eth_rx_adapter.c
> > > > b/lib/librte_eventdev/rte_event_eth_rx_adapter.c
> > > > index 90106e6..a818bef 100644
> > > > --- a/lib/librte_eventdev/rte_event_eth_rx_adapter.c
> > > > +++ b/lib/librte_eventdev/rte_event_eth_rx_adapter.c
> > > > @@ -603,7 +603,7 @@ static uint16_t gcd_u16(uint16_t a, uint16_t b)
> > > >  		RTE_EDEV_LOG_ERR("failed to configure event dev %u\n",
> > > >  						dev_id);
> > > >  		if (started)
> > > > -			rte_event_dev_start(dev_id);
> > > > +			ret = rte_event_dev_start(dev_id);
> > >
> > > Currently the a non-zero return value at this point signifies an
> > > error returned
> > from rte_event_dev_configure(),  so I suggest that the return value is
> > typecasted to void.
> >
> > If I understand it correctly, Any one of the failure(configure() or
> > start()) should result in bad state. Right?
> > i.e If some reason PMD is not able to start() even after failure
> > configuration() would result in bad state.
> > If so, one option could be combine the error like ret |= operation or
> > create a new logical error in Rx adapter which denotes this new error.
> >
> 
> So do we agree to ACK these changes to get the code fix to the mainline? 

Sorry, if my original email wasn't clear,  if rte_event_dev_configure() returns an error and rte_eventdev_start() returns success that would be a problem, i.e., the fix is incorrect.

Of the 2 options suggested by Jerin - Since ret is not a bitmask  ret |= wouldn't work, if I understand the option correctly . A new error would work.

How about EIO ? and we also update the documentation to indicate that the event device would be in a stopped state if the return code is EIO.

> rework the logic as required?
> 
> > >
> > > >  		return ret;
> > > >  	}
> > > >
> > > > @@ -617,7 +617,7 @@ static uint16_t gcd_u16(uint16_t a, uint16_t b)
> > > >  	conf->event_port_id = port_id;
> > > >  	conf->max_nb_rx = 128;
> > > >  	if (started)
> > > > -		rte_event_dev_start(dev_id);
> > > > +		ret = rte_event_dev_start(dev_id);
> > > This change looks good to me.
> > >
> > > >  	rx_adapter->default_cb_arg = 1;
> > > >  	return ret;
> > > >  }
> > > > --
> > > > 1.9.1
> > >
Jerin Jacob Feb. 2, 2018, 12:12 p.m. UTC | #5 
-----Original Message-----
> Date: Fri, 2 Feb 2018 10:04:20 +0000
> From: "Rao, Nikhil" <nikhil.rao@intel.com>
> To: "Varghese, Vipin" <vipin.varghese@intel.com>, Jerin Jacob
>  <jerin.jacob@caviumnetworks.com>
> CC: "dev@dpdk.org" <dev@dpdk.org>, "Jacob,  Jerin"
>  <Jerin.JacobKollanukkaran@cavium.com>, "Van Haaren, Harry"
>  <harry.van.haaren@intel.com>, Hemant Agrawal <hemant.agrawal@nxp.com>,
>  "Jain, Deepak K" <deepak.k.jain@intel.com>
> Subject: RE: [PATCH] event/rx_adapter: fix ignore return of event start
> 
> 
> > -----Original Message-----
> > From: Varghese, Vipin
> > Sent: Friday, February 2, 2018 1:39 PM
> > To: Jerin Jacob <jerin.jacob@caviumnetworks.com>; Rao, Nikhil
> > <nikhil.rao@intel.com>
> > Cc: dev@dpdk.org; Jacob, Jerin <Jerin.JacobKollanukkaran@cavium.com>; Van
> > Haaren, Harry <harry.van.haaren@intel.com>; Hemant Agrawal
> > <hemant.agrawal@nxp.com>; Jain, Deepak K <deepak.k.jain@intel.com>
> > Subject: RE: [PATCH] event/rx_adapter: fix ignore return of event start
> > 
> > 
> > 
> > > -----Original Message-----
> > > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> > > Sent: Wednesday, January 31, 2018 6:54 AM
> > > To: Rao, Nikhil <nikhil.rao@intel.com>
> > > Cc: Varghese, Vipin <vipin.varghese@intel.com>; dev@dpdk.org; Jacob,
> > > Jerin <Jerin.JacobKollanukkaran@cavium.com>; Van Haaren, Harry
> > > <harry.van.haaren@intel.com>; Hemant Agrawal
> > <hemant.agrawal@nxp.com>;
> > > Jain, Deepak K <deepak.k.jain@intel.com>
> > > Subject: Re: [PATCH] event/rx_adapter: fix ignore return of event
> > > start
> > >
> > > -----Original Message-----
> > > >
> > > >
> > > > Adding eventdev PMD folks for their suggestions on how to handle the
> > > > return
> > > value from rte_event_dev_start() below.
> > > >
> > > > > -----Original Message-----
> > > > > From: Varghese, Vipin
> > > > > Sent: Wednesday, January 31, 2018 4:26 AM
> > > > > To: dev@dpdk.org; Rao, Nikhil <nikhil.rao@intel.com>
> > > > > Cc: Jain, Deepak K <deepak.k.jain@intel.com>; Varghese, Vipin
> > > > > <vipin.varghese@intel.com>
> > > > > Subject: [PATCH] event/rx_adapter: fix ignore return of event
> > > > > start
> > > > >
> > > > > Capture the return value for rte_event_dev_start. Return the
> > > > > result back to user.
> > > > >
> > > > > Coverity issue: 257000
> > > > > Fixes: 9c38b704d280 ("eventdev: add eth Rx adapter
> > > > > implementation")
> > > > > Cc: nikhil.rao@intel.com
> > > > >
> > > > > Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> > > > > ---
> > > > >  lib/librte_eventdev/rte_event_eth_rx_adapter.c | 4 ++--
> > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/lib/librte_eventdev/rte_event_eth_rx_adapter.c
> > > > > b/lib/librte_eventdev/rte_event_eth_rx_adapter.c
> > > > > index 90106e6..a818bef 100644
> > > > > --- a/lib/librte_eventdev/rte_event_eth_rx_adapter.c
> > > > > +++ b/lib/librte_eventdev/rte_event_eth_rx_adapter.c
> > > > > @@ -603,7 +603,7 @@ static uint16_t gcd_u16(uint16_t a, uint16_t b)
> > > > >  		RTE_EDEV_LOG_ERR("failed to configure event dev %u\n",
> > > > >  						dev_id);
> > > > >  		if (started)
> > > > > -			rte_event_dev_start(dev_id);
> > > > > +			ret = rte_event_dev_start(dev_id);
> > > >
> > > > Currently the a non-zero return value at this point signifies an
> > > > error returned
> > > from rte_event_dev_configure(),  so I suggest that the return value is
> > > typecasted to void.
> > >
> > > If I understand it correctly, Any one of the failure(configure() or
> > > start()) should result in bad state. Right?
> > > i.e If some reason PMD is not able to start() even after failure
> > > configuration() would result in bad state.
> > > If so, one option could be combine the error like ret |= operation or
> > > create a new logical error in Rx adapter which denotes this new error.
> > >
> > 
> > So do we agree to ACK these changes to get the code fix to the mainline? 
> 
> Sorry, if my original email wasn't clear,  if rte_event_dev_configure() returns an error and rte_eventdev_start() returns success that would be a problem, i.e., the fix is incorrect.
> 
> Of the 2 options suggested by Jerin - Since ret is not a bitmask  ret |= wouldn't work, if I understand the option correctly . A new error would work.
> 
> How about EIO ? and we also update the documentation to indicate that the event device would be in a stopped state if the return code is EIO.

+1 for new error. You may consider EBUSY or EINPROGRESS also.No strong opinion for the name.
diff mbox

Patch

diff --git a/lib/librte_eventdev/rte_event_eth_rx_adapter.c b/lib/librte_eventdev/rte_event_eth_rx_adapter.c
index 90106e6..a818bef 100644
--- a/lib/librte_eventdev/rte_event_eth_rx_adapter.c
+++ b/lib/librte_eventdev/rte_event_eth_rx_adapter.c
@@ -603,7 +603,7 @@  static uint16_t gcd_u16(uint16_t a, uint16_t b)
 		RTE_EDEV_LOG_ERR("failed to configure event dev %u\n",
 						dev_id);
 		if (started)
-			rte_event_dev_start(dev_id);
+			ret = rte_event_dev_start(dev_id);
 		return ret;
 	}
 
@@ -617,7 +617,7 @@  static uint16_t gcd_u16(uint16_t a, uint16_t b)
 	conf->event_port_id = port_id;
 	conf->max_nb_rx = 128;
 	if (started)
-		rte_event_dev_start(dev_id);
+		ret = rte_event_dev_start(dev_id);
 	rx_adapter->default_cb_arg = 1;
 	return ret;
 }