This patch fixes core dump issue when entering safe mode with a wrong package file. Fixes: 5ad3db8d4bdd ("net/ice: enable advanced RSS") Signed-off-by: Simei Su <simei.su@intel.com> --- drivers/net/ice/ice_hash.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ice/ice_hash.c b/drivers/net/ice/ice_hash.c index 3381b45..04ec30f 100644 --- a/drivers/net/ice/ice_hash.c +++ b/drivers/net/ice/ice_hash.c @@ -236,7 +236,7 @@ struct ice_hash_match_type ice_hash_type_list[] = { static int ice_hash_init(struct ice_adapter *ad) { - struct ice_flow_parser *parser = NULL; + struct ice_flow_parser *parser; if (ad->active_pkg_type == ICE_PKG_TYPE_OS_DEFAULT) parser = &ice_hash_parser_os; -- 1.8.3.1
This patch fixes core dump issue when entering safe mode with a wrong package file. This patch also fixes build failure issue. Fixes: 5ad3db8d4bdd ("net/ice: enable advanced RSS") Signed-off-by: Simei Su <simei.su@intel.com> --- drivers/net/ice/ice_hash.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/net/ice/ice_hash.c b/drivers/net/ice/ice_hash.c index 3381b45..08b2502 100644 --- a/drivers/net/ice/ice_hash.c +++ b/drivers/net/ice/ice_hash.c @@ -236,12 +236,12 @@ struct ice_hash_match_type ice_hash_type_list[] = { static int ice_hash_init(struct ice_adapter *ad) { - struct ice_flow_parser *parser = NULL; + struct ice_flow_parser *parser; - if (ad->active_pkg_type == ICE_PKG_TYPE_OS_DEFAULT) - parser = &ice_hash_parser_os; - else if (ad->active_pkg_type == ICE_PKG_TYPE_COMMS) + if (ad->active_pkg_type == ICE_PKG_TYPE_COMMS) parser = &ice_hash_parser_comms; + else + parser = &ice_hash_parser_os; return ice_register_parser(parser, ad); } -- 1.8.3.1
Can you provide more details on how this fixes the issue? Looking at the code, the pointer *parser should get set by the code so I'm not sure how this change does anything.
Paul
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Simei Su
> Sent: Tuesday, November 5, 2019 11:09 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>;
> Yang, Qiming <qiming.yang@intel.com>
> Cc: dev@dpdk.org; Su, Simei <simei.su@intel.com>
> Subject: [dpdk-dev] [PATCH] net/ice: fix segmentation fault for RSS init
>
> This patch fixes core dump issue when entering safe mode with a wrong
> package file.
>
> Fixes: 5ad3db8d4bdd ("net/ice: enable advanced RSS")
>
> Signed-off-by: Simei Su <simei.su@intel.com>
> ---
> drivers/net/ice/ice_hash.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ice/ice_hash.c b/drivers/net/ice/ice_hash.c index
> 3381b45..04ec30f 100644
> --- a/drivers/net/ice/ice_hash.c
> +++ b/drivers/net/ice/ice_hash.c
> @@ -236,7 +236,7 @@ struct ice_hash_match_type ice_hash_type_list[] = {
> static int ice_hash_init(struct ice_adapter *ad) {
> - struct ice_flow_parser *parser = NULL;
> + struct ice_flow_parser *parser;
>
> if (ad->active_pkg_type == ICE_PKG_TYPE_OS_DEFAULT)
> parser = &ice_hash_parser_os;
> --
> 1.8.3.1
Disregard my previous email, I didn't see this one. See my comments inline. Paul > -----Original Message----- > From: dev <dev-bounces@dpdk.org> On Behalf Of Simei Su > Sent: Wednesday, November 6, 2019 12:26 AM > To: Zhang, Qi Z <qi.z.zhang@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>; > Yang, Qiming <qiming.yang@intel.com> > Cc: dev@dpdk.org; Su, Simei <simei.su@intel.com> > Subject: [dpdk-dev] [PATCH v2] net/ice: fix segmentation fault for RSS init > > This patch fixes core dump issue when entering safe mode with a wrong > package file. This patch also fixes build failure issue. > > Fixes: 5ad3db8d4bdd ("net/ice: enable advanced RSS") > > Signed-off-by: Simei Su <simei.su@intel.com> > --- > drivers/net/ice/ice_hash.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ice/ice_hash.c b/drivers/net/ice/ice_hash.c index > 3381b45..08b2502 100644 > --- a/drivers/net/ice/ice_hash.c > +++ b/drivers/net/ice/ice_hash.c > @@ -236,12 +236,12 @@ struct ice_hash_match_type ice_hash_type_list[] = { > static int ice_hash_init(struct ice_adapter *ad) { > - struct ice_flow_parser *parser = NULL; > + struct ice_flow_parser *parser; > > - if (ad->active_pkg_type == ICE_PKG_TYPE_OS_DEFAULT) > - parser = &ice_hash_parser_os; > - else if (ad->active_pkg_type == ICE_PKG_TYPE_COMMS) > + if (ad->active_pkg_type == ICE_PKG_TYPE_COMMS) > parser = &ice_hash_parser_comms; > + else > + parser = &ice_hash_parser_os; This doesn't make sense to me. Why set up a parser at all if we are in safe mode? Safe mode means 1 queue so there isn't anything to RSS to. It seems like if we are in safe mode we should just return an error here with a message that we are in safe mode. > > return ice_register_parser(parser, ad); } > -- > 1.8.3.1
Hi, Paul > -----Original Message----- > From: Stillwell Jr, Paul M <paul.m.stillwell.jr@intel.com> > Sent: Thursday, November 7, 2019 12:10 AM > To: Su, Simei <simei.su@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Ye, > Xiaolong <xiaolong.ye@intel.com>; Yang, Qiming <qiming.yang@intel.com> > Cc: dev@dpdk.org; Su, Simei <simei.su@intel.com> > Subject: RE: [dpdk-dev] [PATCH v2] net/ice: fix segmentation fault for RSS init > > Disregard my previous email, I didn't see this one. See my comments inline. > > Paul > > > -----Original Message----- > > From: dev <dev-bounces@dpdk.org> On Behalf Of Simei Su > > Sent: Wednesday, November 6, 2019 12:26 AM > > To: Zhang, Qi Z <qi.z.zhang@intel.com>; Ye, Xiaolong > > <xiaolong.ye@intel.com>; Yang, Qiming <qiming.yang@intel.com> > > Cc: dev@dpdk.org; Su, Simei <simei.su@intel.com> > > Subject: [dpdk-dev] [PATCH v2] net/ice: fix segmentation fault for RSS > > init > > > > This patch fixes core dump issue when entering safe mode with a wrong > > package file. This patch also fixes build failure issue. > > > > Fixes: 5ad3db8d4bdd ("net/ice: enable advanced RSS") > > > > Signed-off-by: Simei Su <simei.su@intel.com> > > --- > > drivers/net/ice/ice_hash.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/net/ice/ice_hash.c b/drivers/net/ice/ice_hash.c > > index > > 3381b45..08b2502 100644 > > --- a/drivers/net/ice/ice_hash.c > > +++ b/drivers/net/ice/ice_hash.c > > @@ -236,12 +236,12 @@ struct ice_hash_match_type > ice_hash_type_list[] > > = { static int ice_hash_init(struct ice_adapter *ad) { -struct > > ice_flow_parser *parser = NULL; > > +struct ice_flow_parser *parser; > > > > -if (ad->active_pkg_type == ICE_PKG_TYPE_OS_DEFAULT) -parser = > > &ice_hash_parser_os; -else if (ad->active_pkg_type == > > ICE_PKG_TYPE_COMMS) > > +if (ad->active_pkg_type == ICE_PKG_TYPE_COMMS) > > parser = &ice_hash_parser_comms; > > +else > > +parser = &ice_hash_parser_os; > > This doesn't make sense to me. Why set up a parser at all if we are in safe > mode? Safe mode means 1 queue so there isn't anything to RSS to. It seems > like if we are in safe mode we should just return an error here with a message > that we are in safe mode. > When application starts with a wrong ice.pkg, the output shows: ice_load_pkg(): failed to allocate buf of size 0 for package ice_dev_init(): Failed to load the DDP package,Entering Safe Mode Segmentation fault (core dumped) I used gdb to track this issue and found it cored dump at ice_hash_init(). It is caused by the pointer *parser is set NULL. If I don't set NULL, the output shows normally: ice_load_pkg(): failed to allocate buf of size 0 for package ice_dev_init(): Failed to load the DDP package,Entering Safe Mode ice_init_rss(): RSS is not supported in safe mode As to build failure issue, I used "if, else if" previously and didn't consider all cases other than "if, else if". So the test report shows: error: 'parser' may be used uninitialized in this function. I changed it to "if, else". Thanks! Br Simei > > > > return ice_register_parser(parser, ad); } > > -- > > 1.8.3.1 >
This patch fixes core dump issue when entering safe mode with a wrong ice.pkg. In safe mode, rte_flow is not supported and it won't initialize any flow engine. Fixes: 7615a6895009 ("net/ice: rework for generic flow enabling") Signed-off-by: Simei Su <simei.su@intel.com> --- drivers/net/ice/ice_ethdev.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c index d81eb5e..2a28d8e 100644 --- a/drivers/net/ice/ice_ethdev.c +++ b/drivers/net/ice/ice_ethdev.c @@ -2164,10 +2164,12 @@ static int ice_parse_devargs(struct rte_eth_dev *dev) /* get base queue pairs index in the device */ ice_base_queue_get(pf); - ret = ice_flow_init(ad); - if (ret) { - PMD_INIT_LOG(ERR, "Failed to initialize flow"); - return ret; + if (!ad->is_safe_mode) { + ret = ice_flow_init(ad); + if (ret) { + PMD_INIT_LOG(ERR, "Failed to initialize flow"); + return ret; + } } ret = ice_reset_fxp_resource(hw); @@ -2311,7 +2313,8 @@ static int ice_parse_devargs(struct rte_eth_dev *dev) ice_dev_stop(dev); - ice_flow_uninit(ad); + if (!ad->is_safe_mode) + ice_flow_uninit(ad); /* release all queue resource */ ice_free_queues(dev); -- 1.8.3.1
> -----Original Message-----
> From: Su, Simei <simei.su@intel.com>
> Sent: Thursday, November 7, 2019 1:47 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>;
> Yang, Qiming <qiming.yang@intel.com>
> Cc: dev@dpdk.org; Su, Simei <simei.su@intel.com>
> Subject: [PATCH v3] net/ice: fix segmentation fault with a wrong package
>
> This patch fixes core dump issue when entering safe mode with a wrong
> ice.pkg. In safe mode, rte_flow is not supported and it won't initialize any flow
> engine.
>
> Fixes: 7615a6895009 ("net/ice: rework for generic flow enabling")
>
> Signed-off-by: Simei Su <simei.su@intel.com>
Acked-by: Qi Zhang <qi.z.zhang@intel.com>
Hi, simei On 11/07, Simei Su wrote: >This patch fixes core dump issue when entering safe mode with a >wrong ice.pkg. In safe mode, rte_flow is not supported and it >won't initialize any flow engine. > >Fixes: 7615a6895009 ("net/ice: rework for generic flow enabling") > >Signed-off-by: Simei Su <simei.su@intel.com> >--- > drivers/net/ice/ice_ethdev.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > >diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c >index d81eb5e..2a28d8e 100644 >--- a/drivers/net/ice/ice_ethdev.c >+++ b/drivers/net/ice/ice_ethdev.c >@@ -2164,10 +2164,12 @@ static int ice_parse_devargs(struct rte_eth_dev *dev) > /* get base queue pairs index in the device */ > ice_base_queue_get(pf); > >- ret = ice_flow_init(ad); >- if (ret) { >- PMD_INIT_LOG(ERR, "Failed to initialize flow"); >- return ret; >+ if (!ad->is_safe_mode) { >+ ret = ice_flow_init(ad); >+ if (ret) { >+ PMD_INIT_LOG(ERR, "Failed to initialize flow"); >+ return ret; >+ } Do we need to print out some message indicates that now ice is in safe mode? Thanks, Xiaolong > } > > ret = ice_reset_fxp_resource(hw); >@@ -2311,7 +2313,8 @@ static int ice_parse_devargs(struct rte_eth_dev *dev) > > ice_dev_stop(dev); > >- ice_flow_uninit(ad); >+ if (!ad->is_safe_mode) >+ ice_flow_uninit(ad); > > /* release all queue resource */ > ice_free_queues(dev); >-- >1.8.3.1 >
Hi, xiaolong > -----Original Message----- > From: Ye, Xiaolong <xiaolong.ye@intel.com> > Sent: Monday, November 11, 2019 5:06 PM > To: Su, Simei <simei.su@intel.com> > Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; Yang, Qiming > <qiming.yang@intel.com>; dev@dpdk.org > Subject: Re: [PATCH v3] net/ice: fix segmentation fault with a wrong package > > Hi, simei > > On 11/07, Simei Su wrote: > >This patch fixes core dump issue when entering safe mode with a wrong > >ice.pkg. In safe mode, rte_flow is not supported and it won't > >initialize any flow engine. > > > >Fixes: 7615a6895009 ("net/ice: rework for generic flow enabling") > > > >Signed-off-by: Simei Su <simei.su@intel.com> > >--- > > drivers/net/ice/ice_ethdev.c | 13 ++++++++----- > > 1 file changed, 8 insertions(+), 5 deletions(-) > > > >diff --git a/drivers/net/ice/ice_ethdev.c > >b/drivers/net/ice/ice_ethdev.c index d81eb5e..2a28d8e 100644 > >--- a/drivers/net/ice/ice_ethdev.c > >+++ b/drivers/net/ice/ice_ethdev.c > >@@ -2164,10 +2164,12 @@ static int ice_parse_devargs(struct rte_eth_dev > *dev) > > /* get base queue pairs index in the device */ > > ice_base_queue_get(pf); > > > >- ret = ice_flow_init(ad); > >- if (ret) { > >- PMD_INIT_LOG(ERR, "Failed to initialize flow"); > >- return ret; > >+ if (!ad->is_safe_mode) { > >+ ret = ice_flow_init(ad); > >+ if (ret) { > >+ PMD_INIT_LOG(ERR, "Failed to initialize flow"); > >+ return ret; > >+ } > > Do we need to print out some message indicates that now ice is in safe mode? > We already have print info when ice is in safe mode. Thanks! Br Simei > Thanks, > Xiaolong > > > } > > > > ret = ice_reset_fxp_resource(hw); > >@@ -2311,7 +2313,8 @@ static int ice_parse_devargs(struct rte_eth_dev > >*dev) > > > > ice_dev_stop(dev); > > > >- ice_flow_uninit(ad); > >+ if (!ad->is_safe_mode) > >+ ice_flow_uninit(ad); > > > > /* release all queue resource */ > > ice_free_queues(dev); > >-- > >1.8.3.1 > >
On 11/07, Simei Su wrote:
>This patch fixes core dump issue when entering safe mode with a
>wrong ice.pkg. In safe mode, rte_flow is not supported and it
>won't initialize any flow engine.
>
>Fixes: 7615a6895009 ("net/ice: rework for generic flow enabling")
>
>Signed-off-by: Simei Su <simei.su@intel.com>
>---
> drivers/net/ice/ice_ethdev.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
>index d81eb5e..2a28d8e 100644
>--- a/drivers/net/ice/ice_ethdev.c
>+++ b/drivers/net/ice/ice_ethdev.c
>@@ -2164,10 +2164,12 @@ static int ice_parse_devargs(struct rte_eth_dev *dev)
> /* get base queue pairs index in the device */
> ice_base_queue_get(pf);
>
>- ret = ice_flow_init(ad);
>- if (ret) {
>- PMD_INIT_LOG(ERR, "Failed to initialize flow");
>- return ret;
>+ if (!ad->is_safe_mode) {
>+ ret = ice_flow_init(ad);
>+ if (ret) {
>+ PMD_INIT_LOG(ERR, "Failed to initialize flow");
>+ return ret;
>+ }
> }
>
> ret = ice_reset_fxp_resource(hw);
>@@ -2311,7 +2313,8 @@ static int ice_parse_devargs(struct rte_eth_dev *dev)
>
> ice_dev_stop(dev);
>
>- ice_flow_uninit(ad);
>+ if (!ad->is_safe_mode)
>+ ice_flow_uninit(ad);
>
> /* release all queue resource */
> ice_free_queues(dev);
>--
>1.8.3.1
>
Applied to dpdk-next-net-intel. Thanks.