RE: [PATCH net-next 12/12] qede: use ethtool_rx_flow_rule() to remove duplicated parser code
> -Original Message-
> From: netdev-ow...@vger.kernel.org On
> Behalf Of Pablo Neira Ayuso
> Sent: Monday, November 19, 2018 5:45 AM
> To: netdev@vger.kernel.org
> Cc: da...@davemloft.net; thomas.lenda...@amd.com;
> f.faine...@gmail.com; Elior, Ariel ;
> michael.c...@broadcom.com; sant...@chelsio.com;
> madalin.bu...@nxp.com; yisen.zhu...@huawei.com;
> salil.me...@huawei.com; jeffrey.t.kirs...@intel.com; tar...@mellanox.com;
> sae...@mellanox.com; j...@mellanox.com; ido...@mellanox.com;
> jakub.kicin...@netronome.com; peppe.cavall...@st.com;
> grygorii.stras...@ti.com; and...@lunn.ch;
> vivien.dide...@savoirfairelinux.com; alexandre.tor...@st.com;
> joab...@synopsys.com; linux-net-driv...@solarflare.com;
> ganes...@chelsio.com; ogerl...@mellanox.com
> Subject: [PATCH net-next 12/12] qede: use ethtool_rx_flow_rule() to remove
> duplicated parser code
>
> External Email
>
> The qede driver supports for ethtool_rx_flow_spec and flower, both
> codebases look very similar.
>
> This patch uses the ethtool_rx_flow_rule() infrastructure to remove the
> duplicated ethtool_rx_flow_spec parser and consolidate ACL offload support
> around the flow_rule infrastructure.
>
> Furthermore, more code can be consolidated by merging
> qede_add_cls_rule() and qede_add_tc_flower_fltr(), these two functions
> also look very similar.
>
> This driver currently provides simple ACL support, such as 5-tuple matching,
> drop policy and queue to CPU.
>
> Drivers that support more features can benefit from this infrastructure to
> save even more redundant codebase.
>
> Signed-off-by: Pablo Neira Ayuso
> ---
> Note that, after this patch, qede_add_cls_rule() and
> qede_add_tc_flower_fltr() can be also consolidated since their code is
> redundant.
>
> drivers/net/ethernet/qlogic/qede/qede_filter.c | 246
> ++---
> 1 file changed, 53 insertions(+), 193 deletions(-)
>
> diff --git a/drivers/net/ethernet/qlogic/qede/qede_filter.c
> b/drivers/net/ethernet/qlogic/qede/qede_filter.c
> index aca302c3261b..f82b26ba8f80 100644
> --- a/drivers/net/ethernet/qlogic/qede/qede_filter.c
> +++ b/drivers/net/ethernet/qlogic/qede/qede_filter.c
> @@ -1578,30 +1578,6 @@ static void qede_flow_build_ipv6_hdr(struct
> qede_arfs_tuple *t,
> ports[1] = t->dst_port;
> }
>
> -/* Validate fields which are set and not accepted by the driver */ -static
> int
> qede_flow_spec_validate_unused(struct qede_dev *edev,
> - struct ethtool_rx_flow_spec *fs)
> -{
> - if (fs->flow_type & FLOW_MAC_EXT) {
> - DP_INFO(edev, "Don't support MAC extensions\n");
> - return -EOPNOTSUPP;
> - }
> -
> - if ((fs->flow_type & FLOW_EXT) &&
> - (fs->h_ext.vlan_etype || fs->h_ext.vlan_tci)) {
> - DP_INFO(edev, "Don't support vlan-based classification\n");
> - return -EOPNOTSUPP;
> - }
> -
> - if ((fs->flow_type & FLOW_EXT) &&
> - (fs->h_ext.data[0] || fs->h_ext.data[1])) {
> - DP_INFO(edev, "Don't support user defined data\n");
> - return -EOPNOTSUPP;
> - }
> -
> - return 0;
> -}
> -
> static int qede_set_v4_tuple_to_profile(struct qede_dev *edev,
> struct qede_arfs_tuple *t) { @@
> -1665,132 +1641,6
> @@ static int qede_set_v6_tuple_to_profile(struct qede_dev *edev,
> return 0;
> }
>
> -static int qede_flow_spec_to_tuple_ipv4_common(struct qede_dev *edev,
> - struct qede_arfs_tuple *t,
> - struct ethtool_rx_flow_spec
> *fs)
> -{
> - if ((fs->h_u.tcp_ip4_spec.ip4src &
> -fs->m_u.tcp_ip4_spec.ip4src) != fs->h_u.tcp_ip4_spec.ip4src) {
> - DP_INFO(edev, "Don't support IP-masks\n");
> - return -EOPNOTSUPP;
> - }
> -
> - if ((fs->h_u.tcp_ip4_spec.ip4dst &
> -fs->m_u.tcp_ip4_spec.ip4dst) != fs->h_u.tcp_ip4_spec.ip4dst) {
> - DP_INFO(edev, "Don't support IP-masks\n");
> - return -EOPNOTSUPP;
> - }
> -
> - if ((fs->h_u.tcp_ip4_spec.psrc &
> -fs->m_u.tcp_ip4_spec.psrc) != fs->h_u.tcp_ip4_spec.psrc) {
> - DP_INFO(edev, "Don't support port-masks\n");
> - return -EOPNOTSUPP;
> - }
> -
> - if ((fs->h_u.tcp_ip4_spec.pdst &
> -fs->m_u.tcp_ip4_spec.pdst) != fs
Re: [PATCH net-next 12/12] qede: use ethtool_rx_flow_rule() to remove duplicated parser code
On Mon, Nov 19, 2018 at 05:00:13PM +0100, Jiri Pirko wrote:
> Mon, Nov 19, 2018 at 01:15:19AM CET, pa...@netfilter.org wrote:
[...]
> >-static int qede_flow_spec_to_tuple(struct qede_dev *edev,
> >- struct qede_arfs_tuple *t,
> >- struct ethtool_rx_flow_spec *fs)
> >+static int qede_flow_spec_to_rule(struct qede_dev *edev,
> >+ struct qede_arfs_tuple *t,
> >+ struct ethtool_rx_flow_spec *fs)
> > {
> >-memset(t, 0, sizeof(*t));
> >-
> >-if (qede_flow_spec_validate_unused(edev, fs))
> >-return -EOPNOTSUPP;
> >+struct tc_cls_flower_offload f = {};
> >+struct flow_rule *flow_rule;
> >+__be16 proto;
> >+int err = 0;
> >
> > switch ((fs->flow_type & ~FLOW_EXT)) {
> > case TCP_V4_FLOW:
> >-return qede_flow_spec_to_tuple_tcpv4(edev, t, fs);
> > case UDP_V4_FLOW:
> >-return qede_flow_spec_to_tuple_udpv4(edev, t, fs);
> >+proto = htons(ETH_P_IP);
> >+break;
> > case TCP_V6_FLOW:
> >-return qede_flow_spec_to_tuple_tcpv6(edev, t, fs);
> > case UDP_V6_FLOW:
> >-return qede_flow_spec_to_tuple_udpv6(edev, t, fs);
> >+proto = htons(ETH_P_IPV6);
> >+break;
> > default:
> > DP_VERBOSE(edev, NETIF_MSG_IFUP,
> >"Can't support flow of type %08x\n", fs->flow_type);
> > return -EOPNOTSUPP;
> > }
> >
> >-return 0;
> >+flow_rule = ethtool_rx_flow_rule(fs);
> >+if (!flow_rule)
> >+return -ENOMEM;
> >+
> >+f.rule = *flow_rule;
>
> This does not look right. I undersntand that you want to use the same
> driver code to parse same struct coming either from tc-flower or
> ethtool. That is why you introduced flow_rule as a intermediate layer.
> However, here, you use struct that is very tc-flower specific. Common
> parser should work on struct flow_rule.
>
> qede_parse_flower_attr() should accept struct flow_rule * and should be
> renamed to something like qede_parse_flow_rule().
That was intentional, to keep the number of changes in this drivers as
small as possible while introducing the flow_rule infrastructure.
I'll rework the driver as you're asking, so I can pass struct
flow_rule instead to all parser functions.
The patchset is already rather large, and involving many driver, I was
trying to keep changes to minimal but I'll update this driver as you
request, no problem.
Thanks.
Re: [PATCH net-next 12/12] qede: use ethtool_rx_flow_rule() to remove duplicated parser code
Mon, Nov 19, 2018 at 01:15:19AM CET, pa...@netfilter.org wrote:
>The qede driver supports for ethtool_rx_flow_spec and flower, both
>codebases look very similar.
>
>This patch uses the ethtool_rx_flow_rule() infrastructure to remove the
>duplicated ethtool_rx_flow_spec parser and consolidate ACL offload
>support around the flow_rule infrastructure.
>
>Furthermore, more code can be consolidated by merging
>qede_add_cls_rule() and qede_add_tc_flower_fltr(), these two functions
>also look very similar.
>
>This driver currently provides simple ACL support, such as 5-tuple
>matching, drop policy and queue to CPU.
>
>Drivers that support more features can benefit from this infrastructure
>to save even more redundant codebase.
>
>Signed-off-by: Pablo Neira Ayuso
>---
>Note that, after this patch, qede_add_cls_rule() and
>qede_add_tc_flower_fltr() can be also consolidated since their code is
>redundant.
>
> drivers/net/ethernet/qlogic/qede/qede_filter.c | 246 ++---
> 1 file changed, 53 insertions(+), 193 deletions(-)
>
>diff --git a/drivers/net/ethernet/qlogic/qede/qede_filter.c
>b/drivers/net/ethernet/qlogic/qede/qede_filter.c
>index aca302c3261b..f82b26ba8f80 100644
>--- a/drivers/net/ethernet/qlogic/qede/qede_filter.c
>+++ b/drivers/net/ethernet/qlogic/qede/qede_filter.c
>@@ -1578,30 +1578,6 @@ static void qede_flow_build_ipv6_hdr(struct
>qede_arfs_tuple *t,
> ports[1] = t->dst_port;
> }
>
>-/* Validate fields which are set and not accepted by the driver */
>-static int qede_flow_spec_validate_unused(struct qede_dev *edev,
>-struct ethtool_rx_flow_spec *fs)
>-{
>- if (fs->flow_type & FLOW_MAC_EXT) {
>- DP_INFO(edev, "Don't support MAC extensions\n");
>- return -EOPNOTSUPP;
>- }
>-
>- if ((fs->flow_type & FLOW_EXT) &&
>- (fs->h_ext.vlan_etype || fs->h_ext.vlan_tci)) {
>- DP_INFO(edev, "Don't support vlan-based classification\n");
>- return -EOPNOTSUPP;
>- }
>-
>- if ((fs->flow_type & FLOW_EXT) &&
>- (fs->h_ext.data[0] || fs->h_ext.data[1])) {
>- DP_INFO(edev, "Don't support user defined data\n");
>- return -EOPNOTSUPP;
>- }
>-
>- return 0;
>-}
>-
> static int qede_set_v4_tuple_to_profile(struct qede_dev *edev,
> struct qede_arfs_tuple *t)
> {
>@@ -1665,132 +1641,6 @@ static int qede_set_v6_tuple_to_profile(struct
>qede_dev *edev,
> return 0;
> }
>
>-static int qede_flow_spec_to_tuple_ipv4_common(struct qede_dev *edev,
>- struct qede_arfs_tuple *t,
>- struct ethtool_rx_flow_spec *fs)
>-{
>- if ((fs->h_u.tcp_ip4_spec.ip4src &
>- fs->m_u.tcp_ip4_spec.ip4src) != fs->h_u.tcp_ip4_spec.ip4src) {
>- DP_INFO(edev, "Don't support IP-masks\n");
>- return -EOPNOTSUPP;
>- }
>-
>- if ((fs->h_u.tcp_ip4_spec.ip4dst &
>- fs->m_u.tcp_ip4_spec.ip4dst) != fs->h_u.tcp_ip4_spec.ip4dst) {
>- DP_INFO(edev, "Don't support IP-masks\n");
>- return -EOPNOTSUPP;
>- }
>-
>- if ((fs->h_u.tcp_ip4_spec.psrc &
>- fs->m_u.tcp_ip4_spec.psrc) != fs->h_u.tcp_ip4_spec.psrc) {
>- DP_INFO(edev, "Don't support port-masks\n");
>- return -EOPNOTSUPP;
>- }
>-
>- if ((fs->h_u.tcp_ip4_spec.pdst &
>- fs->m_u.tcp_ip4_spec.pdst) != fs->h_u.tcp_ip4_spec.pdst) {
>- DP_INFO(edev, "Don't support port-masks\n");
>- return -EOPNOTSUPP;
>- }
>-
>- if (fs->h_u.tcp_ip4_spec.tos) {
>- DP_INFO(edev, "Don't support tos\n");
>- return -EOPNOTSUPP;
>- }
>-
>- t->eth_proto = htons(ETH_P_IP);
>- t->src_ipv4 = fs->h_u.tcp_ip4_spec.ip4src;
>- t->dst_ipv4 = fs->h_u.tcp_ip4_spec.ip4dst;
>- t->src_port = fs->h_u.tcp_ip4_spec.psrc;
>- t->dst_port = fs->h_u.tcp_ip4_spec.pdst;
>-
>- return qede_set_v4_tuple_to_profile(edev, t);
>-}
>-
>-static int qede_flow_spec_to_tuple_tcpv4(struct qede_dev *edev,
>- struct qede_arfs_tuple *t,
>- struct ethtool_rx_flow_spec *fs)
>-{
>- t->ip_proto = IPPROTO_TCP;
>-
>- if (qede_flow_spec_to_tuple_ipv4_common(edev, t, fs))
>- return -EINVAL;
>-
>- return 0;
>-}
>-
>-static int qede_flow_spec_to_tuple_udpv4(struct qede_dev *edev,
>- struct qede_arfs_tuple *t,
>- struct ethtool_rx_flow_spec *fs)
>-{
>- t->ip_proto = IPPROTO_UDP;
>-
>- if (qede_flow_spec_to_tuple_ipv4_common(edev, t, fs))
>- return -EINVAL;
>-
>- return 0;
>-}
>-
>-static int qede_flow_spec_to_tuple_ipv6_common(struct qede_dev *edev,
>- struct qede_arfs_tuple *t,
>-
[PATCH net-next 12/12] qede: use ethtool_rx_flow_rule() to remove duplicated parser code
The qede driver supports for ethtool_rx_flow_spec and flower, both
codebases look very similar.
This patch uses the ethtool_rx_flow_rule() infrastructure to remove the
duplicated ethtool_rx_flow_spec parser and consolidate ACL offload
support around the flow_rule infrastructure.
Furthermore, more code can be consolidated by merging
qede_add_cls_rule() and qede_add_tc_flower_fltr(), these two functions
also look very similar.
This driver currently provides simple ACL support, such as 5-tuple
matching, drop policy and queue to CPU.
Drivers that support more features can benefit from this infrastructure
to save even more redundant codebase.
Signed-off-by: Pablo Neira Ayuso
---
Note that, after this patch, qede_add_cls_rule() and
qede_add_tc_flower_fltr() can be also consolidated since their code is
redundant.
drivers/net/ethernet/qlogic/qede/qede_filter.c | 246 ++---
1 file changed, 53 insertions(+), 193 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qede/qede_filter.c
b/drivers/net/ethernet/qlogic/qede/qede_filter.c
index aca302c3261b..f82b26ba8f80 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_filter.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_filter.c
@@ -1578,30 +1578,6 @@ static void qede_flow_build_ipv6_hdr(struct
qede_arfs_tuple *t,
ports[1] = t->dst_port;
}
-/* Validate fields which are set and not accepted by the driver */
-static int qede_flow_spec_validate_unused(struct qede_dev *edev,
- struct ethtool_rx_flow_spec *fs)
-{
- if (fs->flow_type & FLOW_MAC_EXT) {
- DP_INFO(edev, "Don't support MAC extensions\n");
- return -EOPNOTSUPP;
- }
-
- if ((fs->flow_type & FLOW_EXT) &&
- (fs->h_ext.vlan_etype || fs->h_ext.vlan_tci)) {
- DP_INFO(edev, "Don't support vlan-based classification\n");
- return -EOPNOTSUPP;
- }
-
- if ((fs->flow_type & FLOW_EXT) &&
- (fs->h_ext.data[0] || fs->h_ext.data[1])) {
- DP_INFO(edev, "Don't support user defined data\n");
- return -EOPNOTSUPP;
- }
-
- return 0;
-}
-
static int qede_set_v4_tuple_to_profile(struct qede_dev *edev,
struct qede_arfs_tuple *t)
{
@@ -1665,132 +1641,6 @@ static int qede_set_v6_tuple_to_profile(struct qede_dev
*edev,
return 0;
}
-static int qede_flow_spec_to_tuple_ipv4_common(struct qede_dev *edev,
- struct qede_arfs_tuple *t,
- struct ethtool_rx_flow_spec *fs)
-{
- if ((fs->h_u.tcp_ip4_spec.ip4src &
-fs->m_u.tcp_ip4_spec.ip4src) != fs->h_u.tcp_ip4_spec.ip4src) {
- DP_INFO(edev, "Don't support IP-masks\n");
- return -EOPNOTSUPP;
- }
-
- if ((fs->h_u.tcp_ip4_spec.ip4dst &
-fs->m_u.tcp_ip4_spec.ip4dst) != fs->h_u.tcp_ip4_spec.ip4dst) {
- DP_INFO(edev, "Don't support IP-masks\n");
- return -EOPNOTSUPP;
- }
-
- if ((fs->h_u.tcp_ip4_spec.psrc &
-fs->m_u.tcp_ip4_spec.psrc) != fs->h_u.tcp_ip4_spec.psrc) {
- DP_INFO(edev, "Don't support port-masks\n");
- return -EOPNOTSUPP;
- }
-
- if ((fs->h_u.tcp_ip4_spec.pdst &
-fs->m_u.tcp_ip4_spec.pdst) != fs->h_u.tcp_ip4_spec.pdst) {
- DP_INFO(edev, "Don't support port-masks\n");
- return -EOPNOTSUPP;
- }
-
- if (fs->h_u.tcp_ip4_spec.tos) {
- DP_INFO(edev, "Don't support tos\n");
- return -EOPNOTSUPP;
- }
-
- t->eth_proto = htons(ETH_P_IP);
- t->src_ipv4 = fs->h_u.tcp_ip4_spec.ip4src;
- t->dst_ipv4 = fs->h_u.tcp_ip4_spec.ip4dst;
- t->src_port = fs->h_u.tcp_ip4_spec.psrc;
- t->dst_port = fs->h_u.tcp_ip4_spec.pdst;
-
- return qede_set_v4_tuple_to_profile(edev, t);
-}
-
-static int qede_flow_spec_to_tuple_tcpv4(struct qede_dev *edev,
-struct qede_arfs_tuple *t,
-struct ethtool_rx_flow_spec *fs)
-{
- t->ip_proto = IPPROTO_TCP;
-
- if (qede_flow_spec_to_tuple_ipv4_common(edev, t, fs))
- return -EINVAL;
-
- return 0;
-}
-
-static int qede_flow_spec_to_tuple_udpv4(struct qede_dev *edev,
-struct qede_arfs_tuple *t,
-struct ethtool_rx_flow_spec *fs)
-{
- t->ip_proto = IPPROTO_UDP;
-
- if (qede_flow_spec_to_tuple_ipv4_common(edev, t, fs))
- return -EINVAL;
-
- return 0;
-}
-
-static int qede_flow_spec_to_tuple_ipv6_common(struct qede_dev *edev,
- struct qede_arfs_tuple *t,
- struct ethtool_rx_flow_spec *fs)
-{
- struct in6_addr zero_addr;
-
-
