This patch series adds HW offloading support for TC flows with VxLAN GBP encap/decap.
Patch-1: Remove unused argument from functions.
Patch-2: Expose helper function vxlan_build_gbp_hdr.
Patch-3: Add helper function for encap_info_equal for tunnels with options.
Patch-4: constify input argument of ip_tunnel_info_opts.
Patch-5: Add HW offloading support for TC flows with VxLAN GBP encap/decap
in mlx ethernet driver.
Gavin Li (5):
vxlan: Remove unused argument from vxlan_build_gbp_hdr( ) and
vxlan_build_gpe_hdr( )
---
changelog:
v2->v3
- Addressed comments from Paolo Abeni
- Add new patch
---
vxlan: Expose helper vxlan_build_gbp_hdr
---
changelog:
v1->v2
- Addressed comments from Alexander Lobakin
- Use const to annotate read-only the pointer parameter
---
net/mlx5e: Add helper for encap_info_equal for tunnels with options
changelog:
v1->v2
- Addressed comments from Alexander Lobakin
- Replace confusing pointer arithmetic with function call
- Use boolean operator NOT to check if the function return value is not zero
---
ip_tunnel: constify input argument of ip_tunnel_info_opts( )
---
changelog:
v2->v3
- Addressed comments from Alexander Lobakin
- Add new patch
---
net/mlx5e: TC, Add support for VxLAN GBP encap/decap flows offload
---
changelog:
v2->v3
- Addressed comments from Alexander Lobakin
- Remove the WA by casting away
v1->v2
- Addressed comments from Alexander Lobakin
- Add a separate pair of braces around bitops
- Remove the WA by casting away
- Fit all log messages into one line
- Use NL_SET_ERR_MSG_FMT_MOD to print the invalid value on error
---
Gavin Li (5):
vxlan: Remove unused argument from vxlan_build_gbp_hdr( ) and
vxlan_build_gpe_hdr( )
vxlan: Expose helper vxlan_build_gbp_hdr
net/mlx5e: Add helper for encap_info_equal for tunnels with options
ip_tunnel: constify input argument of ip_tunnel_info_opts( )
net/mlx5e: TC, Add support for VxLAN GBP encap/decap flows offload
.../ethernet/mellanox/mlx5/core/en/tc_tun.h | 3 +
.../mellanox/mlx5/core/en/tc_tun_encap.c | 32 ++++++++
.../mellanox/mlx5/core/en/tc_tun_geneve.c | 24 +-----
.../mellanox/mlx5/core/en/tc_tun_vxlan.c | 76 ++++++++++++++++++-
drivers/net/vxlan/vxlan_core.c | 27 +------
include/linux/mlx5/device.h | 6 ++
include/linux/mlx5/mlx5_ifc.h | 13 +++-
include/net/ip_tunnels.h | 4 +-
include/net/vxlan.h | 19 +++++
9 files changed, 151 insertions(+), 53 deletions(-)
--
2.31.1
For tunnels with options, eg, geneve and vxlan with gbp, they share the
same way to compare the headers and options. Extract the code as a common
function for them.
Signed-off-by: Gavin Li <[email protected]>
Reviewed-by: Gavi Teitz <[email protected]>
Reviewed-by: Roi Dayan <[email protected]>
Reviewed-by: Maor Dickman <[email protected]>
Acked-by: Saeed Mahameed <[email protected]>
---
.../ethernet/mellanox/mlx5/core/en/tc_tun.h | 3 ++
.../mellanox/mlx5/core/en/tc_tun_encap.c | 32 +++++++++++++++++++
.../mellanox/mlx5/core/en/tc_tun_geneve.c | 24 +-------------
3 files changed, 36 insertions(+), 23 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.h b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.h
index b38f693bbb52..92065568bb19 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.h
@@ -115,6 +115,9 @@ int mlx5e_tc_tun_parse_udp_ports(struct mlx5e_priv *priv,
bool mlx5e_tc_tun_encap_info_equal_generic(struct mlx5e_encap_key *a,
struct mlx5e_encap_key *b);
+bool mlx5e_tc_tun_encap_info_equal_options(struct mlx5e_encap_key *a,
+ struct mlx5e_encap_key *b,
+ __be16 tun_flags);
#endif /* CONFIG_MLX5_ESWITCH */
#endif //__MLX5_EN_TC_TUNNEL_H__
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c
index 780224fd67a1..f1d132f16fcc 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c
@@ -3,6 +3,7 @@
#include <net/fib_notifier.h>
#include <net/nexthop.h>
+#include <net/ip_tunnels.h>
#include "tc_tun_encap.h"
#include "en_tc.h"
#include "tc_tun.h"
@@ -571,6 +572,37 @@ bool mlx5e_tc_tun_encap_info_equal_generic(struct mlx5e_encap_key *a,
a->tc_tunnel->tunnel_type == b->tc_tunnel->tunnel_type;
}
+bool mlx5e_tc_tun_encap_info_equal_options(struct mlx5e_encap_key *a,
+ struct mlx5e_encap_key *b,
+ __be16 tun_flags)
+{
+ struct ip_tunnel_info *a_info;
+ struct ip_tunnel_info *b_info;
+ bool a_has_opts, b_has_opts;
+
+ if (!mlx5e_tc_tun_encap_info_equal_generic(a, b))
+ return false;
+
+ a_has_opts = !!(a->ip_tun_key->tun_flags & tun_flags);
+ b_has_opts = !!(b->ip_tun_key->tun_flags & tun_flags);
+
+ /* keys are equal when both don't have any options attached */
+ if (!a_has_opts && !b_has_opts)
+ return true;
+
+ if (a_has_opts != b_has_opts)
+ return false;
+
+ /* options stored in memory next to ip_tunnel_info struct */
+ a_info = container_of(a->ip_tun_key, struct ip_tunnel_info, key);
+ b_info = container_of(b->ip_tun_key, struct ip_tunnel_info, key);
+
+ return a_info->options_len == b_info->options_len &&
+ !memcmp(ip_tunnel_info_opts(a_info),
+ ip_tunnel_info_opts(b_info),
+ a_info->options_len);
+}
+
static int cmp_decap_info(struct mlx5e_decap_key *a,
struct mlx5e_decap_key *b)
{
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_geneve.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_geneve.c
index 054d80c4e65c..2bcd10b6d653 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_geneve.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_geneve.c
@@ -337,29 +337,7 @@ static int mlx5e_tc_tun_parse_geneve(struct mlx5e_priv *priv,
static bool mlx5e_tc_tun_encap_info_equal_geneve(struct mlx5e_encap_key *a,
struct mlx5e_encap_key *b)
{
- struct ip_tunnel_info *a_info;
- struct ip_tunnel_info *b_info;
- bool a_has_opts, b_has_opts;
-
- if (!mlx5e_tc_tun_encap_info_equal_generic(a, b))
- return false;
-
- a_has_opts = !!(a->ip_tun_key->tun_flags & TUNNEL_GENEVE_OPT);
- b_has_opts = !!(b->ip_tun_key->tun_flags & TUNNEL_GENEVE_OPT);
-
- /* keys are equal when both don't have any options attached */
- if (!a_has_opts && !b_has_opts)
- return true;
-
- if (a_has_opts != b_has_opts)
- return false;
-
- /* geneve options stored in memory next to ip_tunnel_info struct */
- a_info = container_of(a->ip_tun_key, struct ip_tunnel_info, key);
- b_info = container_of(b->ip_tun_key, struct ip_tunnel_info, key);
-
- return a_info->options_len == b_info->options_len &&
- memcmp(a_info + 1, b_info + 1, a_info->options_len) == 0;
+ return mlx5e_tc_tun_encap_info_equal_options(a, b, TUNNEL_GENEVE_OPT);
}
struct mlx5e_tc_tunnel geneve_tunnel = {
--
2.31.1
Constify input argument(i.e. struct ip_tunnel_info *info) of
ip_tunnel_info_opts( ) so that it wouldn't be needed to W/A it each time
in each driver.
Signed-off-by: Gavin Li <[email protected]>
---
include/net/ip_tunnels.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
index fca357679816..32c77f149c6e 100644
--- a/include/net/ip_tunnels.h
+++ b/include/net/ip_tunnels.h
@@ -485,9 +485,9 @@ static inline void iptunnel_xmit_stats(struct net_device *dev, int pkt_len)
}
}
-static inline void *ip_tunnel_info_opts(struct ip_tunnel_info *info)
+static inline void *ip_tunnel_info_opts(const struct ip_tunnel_info *info)
{
- return info + 1;
+ return (void *)(info + 1);
}
static inline void ip_tunnel_info_opts_get(void *to,
--
2.31.1
Add HW offloading support for TC flows with VxLAN GBP encap/decap.
Example of encap rule:
tc filter add dev eth0 protocol ip ingress flower \
action tunnel_key set id 42 vxlan_opts 512 \
action mirred egress redirect dev vxlan1
Example of decap rule:
tc filter add dev vxlan1 protocol ip ingress flower \
enc_key_id 42 enc_dst_port 4789 vxlan_opts 1024 \
action tunnel_key unset action mirred egress redirect dev eth0
Signed-off-by: Gavin Li <[email protected]>
Reviewed-by: Gavi Teitz <[email protected]>
Reviewed-by: Roi Dayan <[email protected]>
Reviewed-by: Maor Dickman <[email protected]>
Acked-by: Saeed Mahameed <[email protected]>
---
.../mellanox/mlx5/core/en/tc_tun_vxlan.c | 76 ++++++++++++++++++-
include/linux/mlx5/device.h | 6 ++
include/linux/mlx5/mlx5_ifc.h | 13 +++-
3 files changed, 91 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
index 1f62c702b625..381b6090b759 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
/* Copyright (c) 2018 Mellanox Technologies. */
+#include <net/ip_tunnels.h>
#include <net/vxlan.h>
#include "lib/vxlan.h"
#include "en/tc_tun.h"
@@ -86,9 +87,11 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char buf[],
const struct ip_tunnel_key *tun_key = &e->tun_info->key;
__be32 tun_id = tunnel_id_to_key32(tun_key->tun_id);
struct udphdr *udp = (struct udphdr *)(buf);
+ struct vxlan_metadata *md;
struct vxlanhdr *vxh;
- if (tun_key->tun_flags & TUNNEL_VXLAN_OPT)
+ if ((tun_key->tun_flags & TUNNEL_VXLAN_OPT) &&
+ e->tun_info->options_len != sizeof(*md))
return -EOPNOTSUPP;
vxh = (struct vxlanhdr *)((char *)udp + sizeof(struct udphdr));
*ip_proto = IPPROTO_UDP;
@@ -96,6 +99,61 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char buf[],
udp->dest = tun_key->tp_dst;
vxh->vx_flags = VXLAN_HF_VNI;
vxh->vx_vni = vxlan_vni_field(tun_id);
+ if (tun_key->tun_flags & TUNNEL_VXLAN_OPT) {
+ md = ip_tunnel_info_opts(e->tun_info);
+ vxlan_build_gbp_hdr(vxh, md);
+ }
+
+ return 0;
+}
+
+static int mlx5e_tc_tun_parse_vxlan_gbp_option(struct mlx5e_priv *priv,
+ struct mlx5_flow_spec *spec,
+ struct flow_cls_offload *f)
+{
+ struct flow_rule *rule = flow_cls_offload_flow_rule(f);
+ struct netlink_ext_ack *extack = f->common.extack;
+ struct flow_match_enc_opts enc_opts;
+ void *misc5_c, *misc5_v;
+ u32 *gbp, *gbp_mask;
+
+ flow_rule_match_enc_opts(rule, &enc_opts);
+
+ if (memchr_inv(&enc_opts.mask->data, 0, sizeof(enc_opts.mask->data)) &&
+ !MLX5_CAP_ESW_FT_FIELD_SUPPORT_2(priv->mdev, tunnel_header_0_1)) {
+ NL_SET_ERR_MSG_MOD(extack, "Matching on VxLAN GBP is not supported");
+ netdev_warn(priv->netdev, "Matching on VxLAN GBP is not supported\n");
+ return -EOPNOTSUPP;
+ }
+
+ if (enc_opts.key->dst_opt_type != TUNNEL_VXLAN_OPT) {
+ NL_SET_ERR_MSG_MOD(extack, "Wrong VxLAN option type: not GBP");
+ netdev_warn(priv->netdev, "Wrong VxLAN option type: not GBP\n");
+ return -EOPNOTSUPP;
+ }
+
+ if (enc_opts.key->len != sizeof(*gbp) ||
+ enc_opts.mask->len != sizeof(*gbp_mask)) {
+ NL_SET_ERR_MSG_MOD(extack, "VxLAN GBP option/mask len is not 32 bits");
+ netdev_warn(priv->netdev, "VxLAN GBP option/mask len is not 32 bits\n");
+ return -EINVAL;
+ }
+
+ gbp = (u32 *)&enc_opts.key->data[0];
+ gbp_mask = (u32 *)&enc_opts.mask->data[0];
+
+ if (*gbp_mask & ~VXLAN_GBP_MASK) {
+ NL_SET_ERR_MSG_FMT_MOD(extack, "Wrong VxLAN GBP mask(0x%08X)\n", *gbp_mask);
+ netdev_warn(priv->netdev, "Wrong VxLAN GBP mask(0x%08X)\n", *gbp_mask);
+ return -EINVAL;
+ }
+
+ misc5_c = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, misc_parameters_5);
+ misc5_v = MLX5_ADDR_OF(fte_match_param, spec->match_value, misc_parameters_5);
+ MLX5_SET(fte_match_set_misc5, misc5_c, tunnel_header_0, *gbp_mask);
+ MLX5_SET(fte_match_set_misc5, misc5_v, tunnel_header_0, *gbp);
+
+ spec->match_criteria_enable |= MLX5_MATCH_MISC_PARAMETERS_5;
return 0;
}
@@ -122,6 +180,14 @@ static int mlx5e_tc_tun_parse_vxlan(struct mlx5e_priv *priv,
if (!enc_keyid.mask->keyid)
return 0;
+ if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ENC_OPTS)) {
+ int err;
+
+ err = mlx5e_tc_tun_parse_vxlan_gbp_option(priv, spec, f);
+ if (err)
+ return err;
+ }
+
/* match on VNI is required */
if (!MLX5_CAP_ESW_FLOWTABLE_FDB(priv->mdev,
@@ -143,6 +209,12 @@ static int mlx5e_tc_tun_parse_vxlan(struct mlx5e_priv *priv,
return 0;
}
+static bool mlx5e_tc_tun_encap_info_equal_vxlan(struct mlx5e_encap_key *a,
+ struct mlx5e_encap_key *b)
+{
+ return mlx5e_tc_tun_encap_info_equal_options(a, b, TUNNEL_VXLAN_OPT);
+}
+
static int mlx5e_tc_tun_get_remote_ifindex(struct net_device *mirred_dev)
{
const struct vxlan_dev *vxlan = netdev_priv(mirred_dev);
@@ -160,6 +232,6 @@ struct mlx5e_tc_tunnel vxlan_tunnel = {
.generate_ip_tun_hdr = mlx5e_gen_ip_tunnel_header_vxlan,
.parse_udp_ports = mlx5e_tc_tun_parse_udp_ports_vxlan,
.parse_tunnel = mlx5e_tc_tun_parse_vxlan,
- .encap_info_equal = mlx5e_tc_tun_encap_info_equal_generic,
+ .encap_info_equal = mlx5e_tc_tun_encap_info_equal_vxlan,
.get_remote_ifindex = mlx5e_tc_tun_get_remote_ifindex,
};
diff --git a/include/linux/mlx5/device.h b/include/linux/mlx5/device.h
index 71b06ebad402..af4dd536a52c 100644
--- a/include/linux/mlx5/device.h
+++ b/include/linux/mlx5/device.h
@@ -1357,6 +1357,12 @@ enum mlx5_qcam_feature_groups {
#define MLX5_CAP_ESW_INGRESS_ACL_MAX(mdev, cap) \
MLX5_CAP_ESW_FLOWTABLE_MAX(mdev, flow_table_properties_esw_acl_ingress.cap)
+#define MLX5_CAP_ESW_FT_FIELD_SUPPORT_2(mdev, cap) \
+ MLX5_CAP_ESW_FLOWTABLE(mdev, ft_field_support_2_esw_fdb.cap)
+
+#define MLX5_CAP_ESW_FT_FIELD_SUPPORT_2_MAX(mdev, cap) \
+ MLX5_CAP_ESW_FLOWTABLE_MAX(mdev, ft_field_support_2_esw_fdb.cap)
+
#define MLX5_CAP_ESW(mdev, cap) \
MLX5_GET(e_switch_cap, \
mdev->caps.hca[MLX5_CAP_ESWITCH]->cur, cap)
diff --git a/include/linux/mlx5/mlx5_ifc.h b/include/linux/mlx5/mlx5_ifc.h
index 1e530a8a2cf5..caef6aa20454 100644
--- a/include/linux/mlx5/mlx5_ifc.h
+++ b/include/linux/mlx5/mlx5_ifc.h
@@ -399,10 +399,13 @@ struct mlx5_ifc_flow_table_fields_supported_bits {
u8 metadata_reg_c_0[0x1];
};
+/* Table 2170 - Flow Table Fields Supported 2 Format */
struct mlx5_ifc_flow_table_fields_supported_2_bits {
u8 reserved_at_0[0xe];
u8 bth_opcode[0x1];
- u8 reserved_at_f[0x11];
+ u8 reserved_at_f[0x1];
+ u8 tunnel_header_0_1[0x1];
+ u8 reserved_at_11[0xf];
u8 reserved_at_20[0x60];
};
@@ -890,7 +893,13 @@ struct mlx5_ifc_flow_table_eswitch_cap_bits {
struct mlx5_ifc_flow_table_prop_layout_bits flow_table_properties_esw_acl_egress;
- u8 reserved_at_800[0x1000];
+ u8 reserved_at_800[0xC00];
+
+ struct mlx5_ifc_flow_table_fields_supported_2_bits ft_field_support_2_esw_fdb;
+
+ struct mlx5_ifc_flow_table_fields_supported_2_bits ft_field_bitmask_support_2_esw_fdb;
+
+ u8 reserved_at_1500[0x300];
u8 sw_steering_fdb_action_drop_icm_address_rx[0x40];
--
2.31.1
On Fri, Feb 17, 2023 at 05:39:24AM +0200, Gavin Li wrote:
> Constify input argument(i.e. struct ip_tunnel_info *info) of
> ip_tunnel_info_opts( ) so that it wouldn't be needed to W/A it each time
> in each driver.
>
> Signed-off-by: Gavin Li <[email protected]>
> ---
> include/net/ip_tunnels.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
> index fca357679816..32c77f149c6e 100644
> --- a/include/net/ip_tunnels.h
> +++ b/include/net/ip_tunnels.h
> @@ -485,9 +485,9 @@ static inline void iptunnel_xmit_stats(struct net_device *dev, int pkt_len)
> }
> }
>
> -static inline void *ip_tunnel_info_opts(struct ip_tunnel_info *info)
> +static inline void *ip_tunnel_info_opts(const struct ip_tunnel_info *info)
> {
> - return info + 1;
> + return (void *)(info + 1);
I'm unclear on what problem this is trying to solve,
but info being const, and then returning (info +1)
as non-const feels like it is masking rather than fixing a problem.
> }
>
> static inline void ip_tunnel_info_opts_get(void *to,
> --
> 2.31.1
>
On Fri, Feb 17, 2023 at 05:39:23AM +0200, Gavin Li wrote:
> For tunnels with options, eg, geneve and vxlan with gbp, they share the
> same way to compare the headers and options. Extract the code as a common
> function for them.
>
> Signed-off-by: Gavin Li <[email protected]>
> Reviewed-by: Gavi Teitz <[email protected]>
> Reviewed-by: Roi Dayan <[email protected]>
> Reviewed-by: Maor Dickman <[email protected]>
> Acked-by: Saeed Mahameed <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
On Sun, Feb 19, 2023 at 09:29:21PM +0100, Simon Horman wrote:
> On Fri, Feb 17, 2023 at 05:39:24AM +0200, Gavin Li wrote:
> > Constify input argument(i.e. struct ip_tunnel_info *info) of
> > ip_tunnel_info_opts( ) so that it wouldn't be needed to W/A it each time
> > in each driver.
> >
> > Signed-off-by: Gavin Li <[email protected]>
> > ---
> > include/net/ip_tunnels.h | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
> > index fca357679816..32c77f149c6e 100644
> > --- a/include/net/ip_tunnels.h
> > +++ b/include/net/ip_tunnels.h
> > @@ -485,9 +485,9 @@ static inline void iptunnel_xmit_stats(struct net_device *dev, int pkt_len)
> > }
> > }
> >
> > -static inline void *ip_tunnel_info_opts(struct ip_tunnel_info *info)
> > +static inline void *ip_tunnel_info_opts(const struct ip_tunnel_info *info)
> > {
> > - return info + 1;
> > + return (void *)(info + 1);
>
> I'm unclear on what problem this is trying to solve,
> but info being const, and then returning (info +1)
> as non-const feels like it is masking rather than fixing a problem.
I now see that an example of the problem is added by path 5/5.
...
CC [M] drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.o
drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c: In function 'mlx5e_gen_ip_tunnel_header_vxlan':
drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c:103:43: error: passing argument 1 of 'ip_tunnel_info_opts' discards 'const' qualifier from pointer target type [-Werror=discarded-qualifiers]
103 | md = ip_tunnel_info_opts(e->tun_info);
| ~^~~~~~~~~~
In file included from drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c:4:
./include/net/ip_tunnels.h:488:64: note: expected 'struct ip_tunnel_info *' but argument is of type 'const struct ip_tunnel_info *'
488 | static inline void *ip_tunnel_info_opts(struct ip_tunnel_info *info)
| ~~~~~~~~~~~~~~~~~~~~~~~^~~~
...
But I really do wonder if this patch masks rather than fixes the problem.
>
> > }
> >
> > static inline void ip_tunnel_info_opts_get(void *to,
> > --
> > 2.31.1
> >
On 2/20/2023 4:46 AM, Simon Horman wrote:
> External email: Use caution opening links or attachments
>
>
> On Sun, Feb 19, 2023 at 09:29:21PM +0100, Simon Horman wrote:
>> On Fri, Feb 17, 2023 at 05:39:24AM +0200, Gavin Li wrote:
>>> Constify input argument(i.e. struct ip_tunnel_info *info) of
>>> ip_tunnel_info_opts( ) so that it wouldn't be needed to W/A it each time
>>> in each driver.
>>>
>>> Signed-off-by: Gavin Li <[email protected]>
>>> ---
>>> include/net/ip_tunnels.h | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
>>> index fca357679816..32c77f149c6e 100644
>>> --- a/include/net/ip_tunnels.h
>>> +++ b/include/net/ip_tunnels.h
>>> @@ -485,9 +485,9 @@ static inline void iptunnel_xmit_stats(struct net_device *dev, int pkt_len)
>>> }
>>> }
>>>
>>> -static inline void *ip_tunnel_info_opts(struct ip_tunnel_info *info)
>>> +static inline void *ip_tunnel_info_opts(const struct ip_tunnel_info *info)
>>> {
>>> - return info + 1;
>>> + return (void *)(info + 1);
>> I'm unclear on what problem this is trying to solve,
>> but info being const, and then returning (info +1)
>> as non-const feels like it is masking rather than fixing a problem.
> I now see that an example of the problem is added by path 5/5.
>
> ...
> CC [M] drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.o
> drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c: In function 'mlx5e_gen_ip_tunnel_header_vxlan':
> drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c:103:43: error: passing argument 1 of 'ip_tunnel_info_opts' discards 'const' qualifier from pointer target type [-Werror=discarded-qualifiers]
> 103 | md = ip_tunnel_info_opts(e->tun_info);
> | ~^~~~~~~~~~
> In file included from drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c:4:
> ./include/net/ip_tunnels.h:488:64: note: expected 'struct ip_tunnel_info *' but argument is of type 'const struct ip_tunnel_info *'
> 488 | static inline void *ip_tunnel_info_opts(struct ip_tunnel_info *info)
> | ~~~~~~~~~~~~~~~~~~~~~~~^~~~
> ...
>
> But I really do wonder if this patch masks rather than fixes the problem.
Hi Olek, any comment?
>
>>> }
>>>
>>> static inline void ip_tunnel_info_opts_get(void *to,
>>> --
>>> 2.31.1
>>>
On 2/20/2023 4:46 AM, Simon Horman wrote:
> External email: Use caution opening links or attachments
>
>
> On Sun, Feb 19, 2023 at 09:29:21PM +0100, Simon Horman wrote:
>> On Fri, Feb 17, 2023 at 05:39:24AM +0200, Gavin Li wrote:
>>> Constify input argument(i.e. struct ip_tunnel_info *info) of
>>> ip_tunnel_info_opts( ) so that it wouldn't be needed to W/A it each time
>>> in each driver.
>>>
>>> Signed-off-by: Gavin Li <[email protected]>
>>> ---
>>> include/net/ip_tunnels.h | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
>>> index fca357679816..32c77f149c6e 100644
>>> --- a/include/net/ip_tunnels.h
>>> +++ b/include/net/ip_tunnels.h
>>> @@ -485,9 +485,9 @@ static inline void iptunnel_xmit_stats(struct net_device *dev, int pkt_len)
>>> }
>>> }
>>>
>>> -static inline void *ip_tunnel_info_opts(struct ip_tunnel_info *info)
>>> +static inline void *ip_tunnel_info_opts(const struct ip_tunnel_info *info)
>>> {
>>> - return info + 1;
>>> + return (void *)(info + 1);
>> I'm unclear on what problem this is trying to solve,
>> but info being const, and then returning (info +1)
>> as non-const feels like it is masking rather than fixing a problem.
> I now see that an example of the problem is added by path 5/5.
>
> ...
> CC [M] drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.o
> drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c: In function 'mlx5e_gen_ip_tunnel_header_vxlan':
> drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c:103:43: error: passing argument 1 of 'ip_tunnel_info_opts' discards 'const' qualifier from pointer target type [-Werror=discarded-qualifiers]
> 103 | md = ip_tunnel_info_opts(e->tun_info);
> | ~^~~~~~~~~~
> In file included from drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c:4:
> ./include/net/ip_tunnels.h:488:64: note: expected 'struct ip_tunnel_info *' but argument is of type 'const struct ip_tunnel_info *'
> 488 | static inline void *ip_tunnel_info_opts(struct ip_tunnel_info *info)
> | ~~~~~~~~~~~~~~~~~~~~~~~^~~~
> ...
>
> But I really do wonder if this patch masks rather than fixes the problem.
ACK
>
>>> }
>>>
>>> static inline void ip_tunnel_info_opts_get(void *to,
>>> --
>>> 2.31.1
>>>
From: Gavin Li <[email protected]>
Date: Mon, 20 Feb 2023 18:42:14 +0800
>
> On 2/20/2023 4:46 AM, Simon Horman wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On Sun, Feb 19, 2023 at 09:29:21PM +0100, Simon Horman wrote:
>>> On Fri, Feb 17, 2023 at 05:39:24AM +0200, Gavin Li wrote:
>>>> Constify input argument(i.e. struct ip_tunnel_info *info) of
>>>> ip_tunnel_info_opts( ) so that it wouldn't be needed to W/A it each
>>>> time
>>>> in each driver.
>>>>
>>>> Signed-off-by: Gavin Li <[email protected]>
>>>> ---
>>>> include/net/ip_tunnels.h | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
>>>> index fca357679816..32c77f149c6e 100644
>>>> --- a/include/net/ip_tunnels.h
>>>> +++ b/include/net/ip_tunnels.h
>>>> @@ -485,9 +485,9 @@ static inline void iptunnel_xmit_stats(struct
>>>> net_device *dev, int pkt_len)
>>>> }
>>>> }
>>>>
>>>> -static inline void *ip_tunnel_info_opts(struct ip_tunnel_info *info)
>>>> +static inline void *ip_tunnel_info_opts(const struct ip_tunnel_info
>>>> *info)
>>>> {
>>>> - return info + 1;
>>>> + return (void *)(info + 1);
>>> I'm unclear on what problem this is trying to solve,
>>> but info being const, and then returning (info +1)
>>> as non-const feels like it is masking rather than fixing a problem.
>> I now see that an example of the problem is added by path 5/5.
>>
>> ...
>> CC [M] drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.o
>> drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c: In function
>> 'mlx5e_gen_ip_tunnel_header_vxlan':
>> drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c:103:43:
>> error: passing argument 1 of 'ip_tunnel_info_opts' discards 'const'
>> qualifier from pointer target type [-Werror=discarded-qualifiers]
>> 103 | md = ip_tunnel_info_opts(e->tun_info);
>> | ~^~~~~~~~~~
>> In file included from
>> drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c:4:
>> ./include/net/ip_tunnels.h:488:64: note: expected 'struct
>> ip_tunnel_info *' but argument is of type 'const struct ip_tunnel_info *'
>> 488 | static inline void *ip_tunnel_info_opts(struct ip_tunnel_info
>> *info)
>> |
>> ~~~~~~~~~~~~~~~~~~~~~~~^~~~
>> ...
>>
>> But I really do wonder if this patch masks rather than fixes the problem.
> Hi Olek, any comment?
Hi,
Sorry for the late reply, was busy at work ._.
I initially proposed a solution via _Generic or __builtin_choose_expr to
return const or non-const opts basing on the input pointer type. I don't
like simple cast-aways.
See container_of_const() how it dynamically chooses whether to add
`const` or not when returning the result.
>>
>>>> }
>>>>
>>>> static inline void ip_tunnel_info_opts_get(void *to,
>>>> --
>>>> 2.31.1
>>>>
Thanks,
Olek