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, pablo@netfilter.org wrote:

[...]

quoted

-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.