Re: [dpdk-dev] [dpdk-stable] [PATCH 6/8] net/qede: correct offload not supported mask
> -Original Message-
> From: dev On Behalf Of Ferruh Yigit
> Sent: Saturday, November 3, 2018 1:32 AM
> To: Patil, Harish
> Cc: Xiaolong Ye ; Qi Zhang ;
> Beilei Xing ; dev@dpdk.org; sta...@dpdk.org
> Subject: Re: [dpdk-dev] [dpdk-stable] [PATCH 6/8] net/qede: correct offload
> not
> supported mask
>
>
> On 10/27/2018 11:40 AM, Xiaolong Ye wrote:
> > Previously XXX_TX_OFFLOAD_NOTSUP_MASK is obtained via xor which would
> lead
> > to unexpected result, correct it by using a NOT-AND operation.
> >
> > Fixes: 29540be7efce ("net/qede: support LRO/TSO offloads")
> >
> > Cc: harish.pa...@qlogic.com
> > Cc: sta...@dpdk.org
> > Signed-off-by: Xiaolong Ye
> > ---
> > drivers/net/qede/qede_rxtx.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/qede/qede_rxtx.h b/drivers/net/qede/qede_rxtx.h
> > index d3a41e92e..9da059564 100644
> > --- a/drivers/net/qede/qede_rxtx.h
> > +++ b/drivers/net/qede/qede_rxtx.h
> > @@ -159,7 +159,7 @@
> > PKT_TX_TUNNEL_GRE)
> >
> > #define QEDE_TX_OFFLOAD_NOTSUP_MASK \
> > - (PKT_TX_OFFLOAD_MASK ^ QEDE_TX_OFFLOAD_MASK)
> > + ~(PKT_TX_OFFLOAD_MASK & QEDE_TX_OFFLOAD_MASK)
>
> Hi Harish,
>
> The qede usage can be problematic, because of how QEDE_TX_OFFLOAD_MASK
> set:
>
> #define QEDE_TX_OFFLOAD_MASK (QEDE_TX_CSUM_OFFLOAD_MASK | \
> PKT_TX_VLAN_PKT | \
> PKT_TX_TUNNEL_VXLAN | \
> PKT_TX_TUNNEL_GENEVE | \
> PKT_TX_TUNNEL_MPLSINUDP | \
> PKT_TX_TUNNEL_GRE)
>
> I am not sure if OFFLOAD_NOTSUP_MASK works fine with multi-bit values, I
> think
> you should set `PKT_TX_TUNNEL_MASK` here.
>
> But please double check in case I am missing something.
Hi Ferruh,
You are right. Tunnel types are multi bit values. Qede PMD does not support all
tunneling protocol offloads which fall under PKT_TX_TUNNEL_MASK.
Not sure how to handle unsupported tunneling protocols in OFFLOAD_NOTSUP_MASK.
One way would be to set TX_TUNNEL_MASK in QEDE_TX_OFFLOAD_MASK and then check
for unsupported tunnel protocols in tx_pkt_prepare().
Thanks,
Shahed
Re: [dpdk-dev] [PATCH 09/18] drivers: net: qede: fix strncpy constant and NUL
> -Original Message-
> From: dev On Behalf Of Andy Green
> Sent: Monday, May 7, 2018 11:30 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH 09/18] drivers: net: qede: fix strncpy constant and
> NUL
>
>
> ---
> drivers/net/qede/base/ecore_int.c | 10 ++
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/qede/base/ecore_int.c
> b/drivers/net/qede/base/ecore_int.c
> index f43781ba4..c809d84ef 100644
> --- a/drivers/net/qede/base/ecore_int.c
> +++ b/drivers/net/qede/base/ecore_int.c
> @@ -1103,10 +1103,12 @@ static enum _ecore_status_t
> ecore_int_deassertion(struct ecore_hwfn *p_hwfn,
> OSAL_SNPRINTF(bit_name, 30,
> p_aeu->bit_name,
> num);
> - else
> - OSAL_STRNCPY(bit_name,
> - p_aeu->bit_name,
> - 30);
> + else {
> + strncpy(bit_name,
> + p_aeu->bit_name,
> + sizeof(bit_name) - 1);
> + bit_name[sizeof(bit_name) - 1]
> = '\0';
> + }
I think you can retain OSAL_STRNCPY and just replace 30 with
'bit_name[sizeof(bit_name) - 1' and then set last byte with '\0' just like you
did.
Thanks,
Shahed
>
> /* We now need to pass bitmask in its
>* correct position.
Re: [dpdk-dev] [PATCH 05/15] build: add Meson files for qede PMD
> -Original Message-
> From: Luca Boccassi
> Sent: Tuesday, September 11, 2018 1:34 AM
> To: dev@dpdk.org
> Cc: keith.wi...@intel.com; roy.fan.zh...@intel.com; jingjing...@intel.com;
> wenzhuo...@intel.com; Mody, Rasesh ; Patil,
> Harish ; Shaikh, Shahed
> ; amr.mokh...@intel.com; Thotton, Shijith
> ; Srinivasan, Srisivasubramanian
> ; liang.j...@intel.com;
> peter.mccar...@intel.com; Jacob, Jerin
> ; Czekaj, Maciej
> ; arybche...@solarflare.com;
> antosh.shu...@caviumnetworks.com; Gupta, Ashish
> ; yongw...@vmware.com;
> bruce.richard...@intel.com; tho...@monjalon.net
> Subject: [PATCH 05/15] build: add Meson files for qede PMD
.
.
> +subdir('base')
> +objs = [base_objs]
> +
> +sources = files(
> + 'qede_ethdev.c',
> + 'qede_fdir.c',
Heads up -
We have submitted a patch series in which qede_fdir.c gets renamed to
qede_filter.c.
Series has not be accepted yet, so you may have to change this if our patch
series gets applied before this one.
> + 'qede_main.c',
> + 'qede_rxtx.c',
> +)
> +
> +#deps += ['ethdev']
> --
> 2.18.0
Acked-by: Shahed Shaikh
Re: [dpdk-dev] [PATCH] drivers/net: do not redefine bool
> -Original Message-
> From: Thomas Monjalon
> Sent: Thursday, September 20, 2018 5:49 AM
> To: Ferruh Yigit ; Rahul Lakkireddy
> ; Wenzhuo Lu ; Qi
> Zhang ; Xiao Wang ;
> Konstantin Ananyev ; Mody, Rasesh
> ; Patil, Harish ; Shaikh,
> Shahed ; Yong Wang
> Cc: dev@dpdk.org
> Subject: [PATCH] drivers/net: do not redefine bool
>
> External Email
>
> When trying to include stdbool.h in DPDK base headers, there are a lot
> of conflicts with drivers which redefine bool/true/false
> in their compatibility layer.
>
> It is fixed by including stdbool.h in these drivers.
> Some errors with usage of bool type are also fixed in some drivers.
>
> Note: the driver qede has a surprising mix of bool and int:
> (~p_iov->b_pre_fp_hsi & ETH_HSI_VER_MINOR)
> where the first variable is boolean and the version is a number.
> It is replaced by
> !p_iov->b_pre_fp_hsi
>
> Signed-off-by: Thomas Monjalon
> ---
> drivers/net/cxgbe/cxgbe_compat.h | 2 +-
> drivers/net/e1000/base/e1000_osdep.h | 5 +
> drivers/net/fm10k/base/fm10k_osdep.h | 8 +---
> drivers/net/fm10k/fm10k_ethdev.c | 4 ++--
> drivers/net/ixgbe/base/ixgbe_osdep.h | 6 +-
> drivers/net/ixgbe/ixgbe_ethdev.c | 16 +---
> drivers/net/ixgbe/ixgbe_rxtx.c | 2 +-
> drivers/net/qede/base/bcm_osal.h | 6 ++
> drivers/net/qede/base/ecore_vf.c | 3 +--
> drivers/net/qede/qede_ethdev.c | 2 +-
> drivers/net/vmxnet3/base/vmxnet3_osdep.h | 3 ++-
> 11 files changed, 22 insertions(+), 35 deletions(-)
>
...
>
> /* Delays */
> diff --git a/drivers/net/qede/base/ecore_vf.c
> b/drivers/net/qede/base/ecore_vf.c
> index d2213f793..f5deb2916 100644
> --- a/drivers/net/qede/base/ecore_vf.c
> +++ b/drivers/net/qede/base/ecore_vf.c
> @@ -445,8 +445,7 @@ static enum _ecore_status_t ecore_vf_pf_acquire(struct
> ecore_hwfn *p_hwfn)
> }
>
> /* @DPDK */
> - if ((~p_iov->b_pre_fp_hsi &
> - ETH_HSI_VER_MINOR) &&
> + if (!p_iov->b_pre_fp_hsi &&
> (resp->pfdev_info.minor_fp_hsi < ETH_HSI_VER_MINOR))
> DP_INFO(p_hwfn,
> "PF is using older fastpath HSI;"
> diff --git a/drivers/net/qede/qede_ethdev.c b/drivers/net/qede/qede_ethdev.c
> index 7bb52b157..53a767b3e 100644
> --- a/drivers/net/qede/qede_ethdev.c
> +++ b/drivers/net/qede/qede_ethdev.c
> @@ -534,7 +534,7 @@ int qede_activate_vport(struct rte_eth_dev *eth_dev,
> bool flg)
> params.update_vport_active_tx_flg = 1;
> params.vport_active_rx_flg = flg;
> params.vport_active_tx_flg = flg;
> - if (~qdev->enable_tx_switching & flg) {
> + if (!qdev->enable_tx_switching && flg) {
> params.update_tx_switching_flg = 1;
> params.tx_switching_flg = !flg;
> }
For qede changes -
Acked-by: Shahed Shaikh
Re: [dpdk-dev] [PATCH 09/18] drivers: net: qede: fix strncpy constant and NUL
> -Original Message-
> From: dev On Behalf Of Bruce Richardson
> Sent: Tuesday, May 8, 2018 2:53 PM
> To: dev-boun...@dpdk.org
> Cc: Andy Green ; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 09/18] drivers: net: qede: fix strncpy constant
> and NUL
>
> On Tue, May 08, 2018 at 05:59:47PM +, dev-boun...@dpdk.org wrote:
> >
> >
> > > -Original Message-
> > > From: dev On Behalf Of Andy Green
> > > Sent: Monday, May 7, 2018 11:30 PM
> > > To: dev@dpdk.org
> > > Subject: [dpdk-dev] [PATCH 09/18] drivers: net: qede: fix strncpy
> > > constant and NUL
> > >
> > >
> > > ---
> > > drivers/net/qede/base/ecore_int.c | 10 ++
> > > 1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/net/qede/base/ecore_int.c
> > > b/drivers/net/qede/base/ecore_int.c
> > > index f43781ba4..c809d84ef 100644
> > > --- a/drivers/net/qede/base/ecore_int.c
> > > +++ b/drivers/net/qede/base/ecore_int.c
> > > @@ -1103,10 +1103,12 @@ static enum _ecore_status_t
> > > ecore_int_deassertion(struct ecore_hwfn *p_hwfn,
> > > OSAL_SNPRINTF(bit_name, 30,
> > > p_aeu->bit_name,
> > > num);
> > > - else
> > > - OSAL_STRNCPY(bit_name,
> > > - p_aeu->bit_name,
> > > - 30);
> > > + else {
> > > + strncpy(bit_name,
> > > + p_aeu->bit_name,
> > > + sizeof(bit_name) - 1);
> > > + bit_name[sizeof(bit_name) - 1]
> > > = '\0';
> > > + }
> >
> > I think you can retain OSAL_STRNCPY and just replace 30 with
> 'bit_name[sizeof(bit_name) - 1' and then set last byte with '\0' just like
> you did.
>
> Can that actually be fixed inside OSAL_STRNCPY itself, rather than having each
> use needing to explicitly null-terminate?
Although there is only instance of OSAL_STRNCPY, it makes sense to modify it.
Thanks,
Shahed
Re: [dpdk-dev] [PATCH 09/18] drivers: net: qede: fix strncpy constant and NUL
> >>> I think you can retain OSAL_STRNCPY and just replace 30 with > >> 'bit_name[sizeof(bit_name) - 1' and then set last byte with '\0' just > >> like you > did. > >> > >> Can that actually be fixed inside OSAL_STRNCPY itself, rather than > >> having each use needing to explicitly null-terminate? > > > > Although there is only instance of OSAL_STRNCPY, it makes sense to modify > > it. > > Doesn't it make more sense to get rid of OSAL_* that bring nothing at all to > the > party? > > #define OSAL_SPRINTF(name, pattern, ...) \ > sprintf(name, pattern, ##__VA_ARGS__) #define OSAL_SNPRINTF(buf, > size, > format, ...) \ > snprintf(buf, size, format, ##__VA_ARGS__) #define > OSAL_STRLEN(string) > strlen(string) #define OSAL_STRCPY(dst, string) strcpy(dst, string) #define > OSAL_STRNCPY(dst, string, len) strncpy(dst, string, len) #define > OSAL_STRCMP(str1, str2) strcmp(str1, str2) > > Do I miss the point or these are just cruft? Hi Andy, I'll send a cleanup patch for this. For now, you can go ahead with original patch. Thanks, Shahed Acked-by: Shahed Shaikh > > -Andy > > > Thanks, > > Shahed > >
Re: [dpdk-dev] [PATCH v4 09/18] net/qede: strncpy length constant and NUL
> > >> > > >> +#include > > >> + > > >> #include "bcm_osal.h" > > >> #include "ecore.h" > > >> #include "ecore_spq.h" > > >> @@ -1104,9 +1106,9 @@ static enum _ecore_status_t > > >> ecore_int_deassertion(struct ecore_hwfn *p_hwfn, > > >> > > >> p_aeu->bit_name, > > >>num); > > >> else > > >> -OSAL_STRNCPY(bit_name, > > >> - > > >> p_aeu->bit_name, > > >> - 30); > > >> +strlcpy(bit_name, > > >> +p_aeu->bit_name, > > >> + > > >> sizeof(bit_name)); > > >> > > >> /* We now need to pass bitmask > > >> in its > > >> * correct position. > > > > > > I'd say it should be better to change OSAL_STRNCPY to OSAL_STRLCPY > > > and modify the macro to use strlcpy, so we avoid further uses of that > > > strlcpy. > > > > > > However, this modifies base driver code, so it is up to the > > > maintainers to make > > that decision. > > > (CC'ing maintainers here). > > > > There's no value for any OSAL_* that simply defines itself to be the > > same as the direct api, as does OSAL_STRNCPY. > > > > It's better to just remove any OSAL_* that calls straight through > > since all it does is obfuscate what the code does, for no benefit. > > I agree. Since this is modifying base driver code, the maintainers can decide > what to do with this. Hi, For this series, you can continue with s/OSAL_STRNCPY/strlcpy/ for this instance. I will send a patch to cleanup OSAL_* once your series gets applied. Thanks, Shahed
Re: [dpdk-dev] [PATCH 4/5] app/testpmd: add configuration for udp port tunnel type
> -Original Message- > From: Ferruh Yigit [mailto:ferruh.yi...@intel.com] > Sent: Thursday, December 7, 2017 6:09 AM > To: Mody, Rasesh ; dev@dpdk.org > Cc: Shaikh, Shahed ; Dept-Eng DPDK Dev engdpdk...@cavium.com> > Subject: Re: [dpdk-dev] [PATCH 4/5] app/testpmd: add configuration for udp > port tunnel type > > On 11/24/2017 12:35 PM, Rasesh Mody wrote: > > From: Shahed Shaikh > > > > Replace rx_vxlan_port command with rx_tunnel_udp_port to support both > > VXLAN and GENEVE udp ports. > > Also updates tunnel_filter command to accept "geneve" argument, can you > please separate to another patch. > > And to prevent these patches hold PMD patches, you can send a new version of > the patchset splitting qede PMD patches into their own patchset. Sure. We'll send separate patchset for qede PMD. > > > > > Signed-off-by: Shahed Shaikh > > --- > > app/test-pmd/cmdline.c | 28 +++- > > 1 file changed, 19 insertions(+), 9 deletions(-) > > > > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index > > f71d963..4b5a8cd 100644 > > --- a/app/test-pmd/cmdline.c > > +++ b/app/test-pmd/cmdline.c > > @@ -402,11 +402,11 @@ static void cmd_help_long_parsed(void > *parsed_result, > > "imac-tenid|imac|omac-imac-tenid|oip|iip) (tenant_id) > (queue_id)\n" > > " remove a tunnel filter of a port.\n\n" > > > > - "rx_vxlan_port add (udp_port) (port_id)\n" > > - "Add an UDP port for VXLAN packet filter on a > port\n\n" > > + "rx_tunnel_udp_port add vxlan|geneve (udp_port) > (port_id)\n" > > Not sure about "rx_tunnel_udp_port" command. > > What do you think something like: > "port config (port_id) udp_tunnel_port add|rm vxlan|geneve (udp_port)" > > to expand ""port config (port_id) ..." command instead of introducing a new > one? Makes sense. I'll update port_config command. Thanks, Shahed
