Re: [dpdk-dev] [dpdk-stable] [PATCH] app/testpmd: fix offloads overwrite by default configuration

["Zhao1, Wei" <wei.zhao1@intel.com>]

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Friday, June 14, 2019 11:42 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Peng, Yuan <yuan.peng@intel.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; Kevin Traynor <ktraynor@redhat.com>
> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads overwrite by
> default configuration
> 
> On 6/12/2019 2:17 AM, Zhao1, Wei wrote:
> >
> >
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Tuesday, June 11, 2019 10:37 PM
> >> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> >> Cc: stable@dpdk.org; Peng, Yuan <yuan.peng@intel.com>; Lu, Wenzhuo
> >> <wenzhuo.lu@intel.com>; Kevin Traynor <ktraynor@redhat.com>
> >> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads
> >> overwrite by default configuration
> >>
> >> On 5/9/2019 8:20 AM, Wei Zhao wrote:
> >>> There is an error in function rxtx_port_config(), which may
> >>> overwrite offloads configuration get from function
> >>> launch_args_parse() when run testpmd app. So rxtx_port_config() should
> do "or" for port offloads.
> >>>
> >>> Fixes: d44f8a485f5d ("app/testpmd: enable per queue configure")
> >>> cc: stable@dpdk.org
> >>>
> >>> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> >>> ---
> >>>  app/test-pmd/testpmd.c | 5 +++++
> >>>  1 file changed, 5 insertions(+)
> >>>
> >>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> >>> 6fbfd29..f0061d9 100644
> >>> --- a/app/test-pmd/testpmd.c
> >>> +++ b/app/test-pmd/testpmd.c
> >>> @@ -2809,9 +2809,12 @@ static void
> >>>  rxtx_port_config(struct rte_port *port)  {
> >>>  	uint16_t qid;
> >>> +	uint64_t offloads;
> >>>
> >>>  	for (qid = 0; qid < nb_rxq; qid++) {
> >>> +		offloads = port->rx_conf[qid].offloads;
> >>>  		port->rx_conf[qid] = port->dev_info.default_rxconf;
> >>> +		port->rx_conf[qid].offloads |= offloads;
> >>
> >> While talking with Kevin, he pointed out the error in this code.
> >>
> >> We are updating queue level offloads, with whatever in the 'offloads'
> >> and it can be non-queue level offloads in it, next time ethdev API
> >> called these values are caught by the API checks and causing an error.
> >>
> >> It looks like port level offload flags needs to be masted out before
> >> writing to queue level 'offloads' variable.
> >
> >
> > By the way, this error in not introduced in this patch, it seems has exist long
> before this patch.
> > This patch is just fix for overwrite problem.
> 
> I disagree, writing 'offloads' to "rx_conf[].offloads" without checking if they
> queue offloads or not causing this problem. And that write introduced in this
> patch.
> 

But if delete the patch I submitted, this bug still exists there.

> 
> >
> >
> >
> >>
> >>>
> >>>  		/* Check if any Rx parameters have been passed */
> >>>  		if (rx_pthresh != RTE_PMD_PARAM_UNSET) @@ -2833,7
> >> +2836,9 @@
> >>> rxtx_port_config(struct rte_port *port)
> >>>  	}
> >>>
> >>>  	for (qid = 0; qid < nb_txq; qid++) {
> >>> +		offloads = port->tx_conf[qid].offloads;
> >>>  		port->tx_conf[qid] = port->dev_info.default_txconf;
> >>> +		port->tx_conf[qid].offloads |= offloads;
> >>>
> >>>  		/* Check if any Tx parameters have been passed */
> >>>  		if (tx_pthresh != RTE_PMD_PARAM_UNSET)
> >>>
> >