Re: [PATCH net-next 12/12] qede: use ethtool_rx_flow_rule() to remove duplicated parser code

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Mon, Nov 19, 2018 at 05:00:13PM +0100, Jiri Pirko wrote:
> Mon, Nov 19, 2018 at 01:15:19AM CET, pablo@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.