[PATCH net-next v3 0/4] net: lan966x: Enable PTP on bridge interfaces

[PATCH net-next v3 0/4] net: lan966x: Enable PTP on bridge interfaces

[Horatiu Vultur]

Before it was not allowed to run ptp on ports that are part of a bridge
because in case of transparent clock the HW will still forward the frames
so there would be duplicate frames.
Now that there is VCAP support, it is possible to add entries in the VCAP
to trap frames to the CPU and the CPU will forward these frames.
The first part of the patch series, extends the VCAP support to be able to
modify and get the rule, while the last patch uses the VCAP to trap the ptp
frames.

v2->v3:
- rebase on net-next as it didn't apply anymore

v1->v2:
- use PTP_EV_PORT and PTP_GEN_PORT instead of hardcoding the number
- small alignment adjustments

Horatiu Vultur (4):
  net: microchip: vcap: Add vcap_get_rule
  net: microchip: vcap: Add vcap_mod_rule
  net: microchip: vcap: Add vcap_rule_get_key_u32
  net: lan966x: Add ptp trap rules

 .../ethernet/microchip/lan966x/lan966x_main.c |  19 +-
 .../ethernet/microchip/lan966x/lan966x_main.h |  14 +
 .../ethernet/microchip/lan966x/lan966x_ptp.c  | 236 ++++-
 .../microchip/lan966x/lan966x_tc_flower.c     |   8 -
 .../microchip/lan966x/lan966x_vcap_impl.c     |  11 +-
 .../net/ethernet/microchip/vcap/vcap_api.c    | 824 ++++++++++++++++++
 .../ethernet/microchip/vcap/vcap_api_client.h |   8 +
 .../microchip/vcap/vcap_api_debugfs.c         | 498 ++---------
 .../microchip/vcap/vcap_api_private.h         |  14 +
 9 files changed, 1171 insertions(+), 461 deletions(-)

-- 
2.38.0

[PATCH net-next v3 1/4] net: microchip: vcap: Add vcap_get_rule

[Horatiu Vultur]

Add function vcap_get_rule which returns a rule based on the internal
rule id.
The entire functionality of reading and decoding the rule from the VCAP
was inside vcap_api_debugfs file. So move the entire implementation in
vcap_api as this is used also by vcap_get_rule.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 .../net/ethernet/microchip/vcap/vcap_api.c    | 772 ++++++++++++++++++
 .../ethernet/microchip/vcap/vcap_api_client.h |   2 +
 .../microchip/vcap/vcap_api_debugfs.c         | 498 ++---------
 .../microchip/vcap/vcap_api_private.h         |  14 +
 4 files changed, 848 insertions(+), 438 deletions(-)

diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api.c b/drivers/net/ethernet/microchip/vcap/vcap_api.c
index f2435d7ab515f..27128313f15f1 100644
--- a/drivers/net/ethernet/microchip/vcap/vcap_api.c
+++ b/drivers/net/ethernet/microchip/vcap/vcap_api.c
@@ -169,6 +169,227 @@ static void vcap_encode_typegroups(u32 *stream, int sw_width,
 	}
 }
 
+static bool vcap_bitarray_zero(int width, u8 *value)
+{
+	int bytes = DIV_ROUND_UP(width, BITS_PER_BYTE);
+	u8 total = 0, bmask = 0xff;
+	int rwidth = width;
+	int idx;
+
+	for (idx = 0; idx < bytes; ++idx, rwidth -= BITS_PER_BYTE) {
+		if (rwidth && rwidth < BITS_PER_BYTE)
+			bmask = (1 << rwidth) - 1;
+		total += value[idx] & bmask;
+	}
+	return total == 0;
+}
+
+static bool vcap_get_bit(u32 *stream, struct vcap_stream_iter *itr)
+{
+	u32 mask = BIT(itr->reg_bitpos);
+	u32 *p = &stream[itr->reg_idx];
+
+	return !!(*p & mask);
+}
+
+static void vcap_decode_field(u32 *stream, struct vcap_stream_iter *itr,
+			      int width, u8 *value)
+{
+	int idx;
+
+	/* Loop over the field value bits and get the field bits and
+	 * set them in the output value byte array
+	 */
+	for (idx = 0; idx < width; idx++) {
+		u8 bidx = idx & 0x7;
+
+		/* Decode one field value bit */
+		if (vcap_get_bit(stream, itr))
+			*value |= 1 << bidx;
+		vcap_iter_next(itr);
+		if (bidx == 7)
+			value++;
+	}
+}
+
+/* Verify that the type id in the stream matches the type id of the keyset */
+static bool vcap_verify_keystream_keyset(struct vcap_control *vctrl,
+					 enum vcap_type vt,
+					 u32 *keystream,
+					 u32 *mskstream,
+					 enum vcap_keyfield_set keyset)
+{
+	const struct vcap_info *vcap = &vctrl->vcaps[vt];
+	const struct vcap_field *typefld;
+	const struct vcap_typegroup *tgt;
+	const struct vcap_field *fields;
+	struct vcap_stream_iter iter;
+	const struct vcap_set *info;
+	u32 value = 0;
+	u32 mask = 0;
+
+	if (vcap_keyfield_count(vctrl, vt, keyset) == 0)
+		return false;
+
+	info = vcap_keyfieldset(vctrl, vt, keyset);
+	/* Check that the keyset is valid */
+	if (!info)
+		return false;
+
+	/* a type_id of value -1 means that there is no type field */
+	if (info->type_id == (u8)-1)
+		return true;
+
+	/* Get a valid typegroup for the specific keyset */
+	tgt = vcap_keyfield_typegroup(vctrl, vt, keyset);
+	if (!tgt)
+		return false;
+
+	fields = vcap_keyfields(vctrl, vt, keyset);
+	if (!fields)
+		return false;
+
+	typefld = &fields[VCAP_KF_TYPE];
+	vcap_iter_init(&iter, vcap->sw_width, tgt, typefld->offset);
+	vcap_decode_field(mskstream, &iter, typefld->width, (u8 *)&mask);
+	/* no type info if there are no mask bits */
+	if (vcap_bitarray_zero(typefld->width, (u8 *)&mask))
+		return false;
+
+	/* Get the value of the type field in the stream and compare to the
+	 * one define in the vcap keyset
+	 */
+	vcap_iter_init(&iter, vcap->sw_width, tgt, typefld->offset);
+	vcap_decode_field(keystream, &iter, typefld->width, (u8 *)&value);
+
+	return (value & mask) == (info->type_id & mask);
+}
+
+/* Verify that the typegroup bits have the correct values */
+static int vcap_verify_typegroups(u32 *stream, int sw_width,
+				  const struct vcap_typegroup *tgt, bool mask,
+				  int sw_max)
+{
+	struct vcap_stream_iter iter;
+	int sw_cnt, idx;
+
+	vcap_iter_set(&iter, sw_width, tgt, 0);
+	sw_cnt = 0;
+	while (iter.tg->width) {
+		u32 value = 0;
+		u32 tg_value = iter.tg->value;
+
+		if (mask)
+			tg_value = (1 << iter.tg->width) - 1;
+		/* Set position to current typegroup bit */
+		iter.offset = iter.tg->offset;
+		vcap_iter_update(&iter);
+		for (idx = 0; idx < iter.tg->width; idx++) {
+			/* Decode one typegroup bit */
+			if (vcap_get_bit(stream, &iter))
+				value |= 1 << idx;
+			iter.offset++;
+			vcap_iter_update(&iter);
+		}
+		if (value != tg_value)
+			return -EINVAL;
+		iter.tg++; /* next typegroup */
+		sw_cnt++;
+		/* Stop checking more typegroups */
+		if (sw_max && sw_cnt >= sw_max)
+			break;
+	}
+	return 0;
+}
+
+/* Find the subword width of the key typegroup that matches the stream data */
+static int vcap_find_keystream_typegroup_sw(struct vcap_control *vctrl,
+					    enum vcap_type vt, u32 *stream,
+					    bool mask, int sw_max)
+{
+	const struct vcap_typegroup **tgt;
+	int sw_idx, res;
+
+	tgt = vctrl->vcaps[vt].keyfield_set_typegroups;
+	/* Try the longest subword match first */
+	for (sw_idx = vctrl->vcaps[vt].sw_count; sw_idx >= 0; sw_idx--) {
+		if (!tgt[sw_idx])
+			continue;
+
+		res = vcap_verify_typegroups(stream, vctrl->vcaps[vt].sw_width,
+					     tgt[sw_idx], mask, sw_max);
+		if (res == 0)
+			return sw_idx;
+	}
+	return -EINVAL;
+}
+
+/* Verify that the typegroup information, subword count, keyset and type id
+ * are in sync and correct, return the list of matchin keysets
+ */
+int
+vcap_find_keystream_keysets(struct vcap_control *vctrl,
+			    enum vcap_type vt,
+			    u32 *keystream,
+			    u32 *mskstream,
+			    bool mask, int sw_max,
+			    struct vcap_keyset_list *kslist)
+{
+	const struct vcap_set *keyfield_set;
+	int sw_count, idx;
+
+	sw_count = vcap_find_keystream_typegroup_sw(vctrl, vt, keystream, mask,
+						    sw_max);
+	if (sw_count < 0)
+		return sw_count;
+
+	keyfield_set = vctrl->vcaps[vt].keyfield_set;
+	for (idx = 0; idx < vctrl->vcaps[vt].keyfield_set_size; ++idx) {
+		if (keyfield_set[idx].sw_per_item != sw_count)
+			continue;
+
+		if (vcap_verify_keystream_keyset(vctrl, vt, keystream,
+						 mskstream, idx))
+			vcap_keyset_list_add(kslist, idx);
+	}
+	if (kslist->cnt > 0)
+		return 0;
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(vcap_find_keystream_keysets);
+
+/* Read key data from a VCAP address and discover if there are any rule keysets
+ * here
+ */
+int vcap_addr_keysets(struct vcap_control *vctrl,
+		      struct net_device *ndev,
+		      struct vcap_admin *admin,
+		      int addr,
+		      struct vcap_keyset_list *kslist)
+{
+	enum vcap_type vt = admin->vtype;
+	int keyset_sw_regs, idx;
+	u32 key = 0, mask = 0;
+
+	/* Read the cache at the specified address */
+	keyset_sw_regs = DIV_ROUND_UP(vctrl->vcaps[vt].sw_width, 32);
+	vctrl->ops->update(ndev, admin, VCAP_CMD_READ, VCAP_SEL_ALL, addr);
+	vctrl->ops->cache_read(ndev, admin, VCAP_SEL_ENTRY, 0,
+			       keyset_sw_regs);
+	/* Skip uninitialized key/mask entries */
+	for (idx = 0; idx < keyset_sw_regs; ++idx) {
+		key |= ~admin->cache.keystream[idx];
+		mask |= admin->cache.maskstream[idx];
+	}
+	if (key == 0 && mask == 0)
+		return -EINVAL;
+	/* Decode and locate the keysets */
+	return vcap_find_keystream_keysets(vctrl, vt, admin->cache.keystream,
+					   admin->cache.maskstream, false, 0,
+					   kslist);
+}
+EXPORT_SYMBOL_GPL(vcap_addr_keysets);
+
 /* Return the list of keyfields for the keyset */
 const struct vcap_field *vcap_keyfields(struct vcap_control *vctrl,
 					enum vcap_type vt,
@@ -618,6 +839,517 @@ struct vcap_rule_internal *vcap_dup_rule(struct vcap_rule_internal *ri)
 	return duprule;
 }
 
+static void vcap_apply_width(u8 *dst, int width, int bytes)
+{
+	u8 bmask;
+	int idx;
+
+	for (idx = 0; idx < bytes; idx++) {
+		if (width > 0)
+			if (width < 8)
+				bmask = (1 << width) - 1;
+			else
+				bmask = ~0;
+		else
+			bmask = 0;
+		dst[idx] &= bmask;
+		width -= 8;
+	}
+}
+
+static void vcap_copy_from_w32be(u8 *dst, u8 *src, int size, int width)
+{
+	int idx, ridx, wstart, nidx;
+	int tail_bytes = (((size + 4) >> 2) << 2) - size;
+
+	for (idx = 0, ridx = size - 1; idx < size; ++idx, --ridx) {
+		wstart = (idx >> 2) << 2;
+		nidx = wstart + 3 - (idx & 0x3);
+		if (nidx >= size)
+			nidx -= tail_bytes;
+		dst[nidx] = src[ridx];
+	}
+
+	vcap_apply_width(dst, width, size);
+}
+
+static void vcap_copy_action_bit_field(struct vcap_u1_action *field, u8 *value)
+{
+	field->value = (*value) & 0x1;
+}
+
+static void vcap_copy_limited_actionfield(u8 *dstvalue, u8 *srcvalue,
+					  int width, int bytes)
+{
+	memcpy(dstvalue, srcvalue, bytes);
+	vcap_apply_width(dstvalue, width, bytes);
+}
+
+static void vcap_copy_to_client_actionfield(struct vcap_rule_internal *ri,
+					    struct vcap_client_actionfield *field,
+					    u8 *value, u16 width)
+{
+	int field_size = actionfield_size_table[field->ctrl.type];
+
+	if (ri->admin->w32be) {
+		switch (field->ctrl.type) {
+		case VCAP_FIELD_BIT:
+			vcap_copy_action_bit_field(&field->data.u1, value);
+			break;
+		case VCAP_FIELD_U32:
+			vcap_copy_limited_actionfield((u8 *)&field->data.u32.value,
+						      value,
+						      width, field_size);
+			break;
+		case VCAP_FIELD_U48:
+			vcap_copy_from_w32be(field->data.u48.value, value,
+					     field_size, width);
+			break;
+		case VCAP_FIELD_U56:
+			vcap_copy_from_w32be(field->data.u56.value, value,
+					     field_size, width);
+			break;
+		case VCAP_FIELD_U64:
+			vcap_copy_from_w32be(field->data.u64.value, value,
+					     field_size, width);
+			break;
+		case VCAP_FIELD_U72:
+			vcap_copy_from_w32be(field->data.u72.value, value,
+					     field_size, width);
+			break;
+		case VCAP_FIELD_U112:
+			vcap_copy_from_w32be(field->data.u112.value, value,
+					     field_size, width);
+			break;
+		case VCAP_FIELD_U128:
+			vcap_copy_from_w32be(field->data.u128.value, value,
+					     field_size, width);
+			break;
+		};
+	} else {
+		switch (field->ctrl.type) {
+		case VCAP_FIELD_BIT:
+			vcap_copy_action_bit_field(&field->data.u1, value);
+			break;
+		case VCAP_FIELD_U32:
+			vcap_copy_limited_actionfield((u8 *)&field->data.u32.value,
+						      value,
+						      width, field_size);
+			break;
+		case VCAP_FIELD_U48:
+			vcap_copy_limited_actionfield(field->data.u48.value,
+						      value,
+						      width, field_size);
+			break;
+		case VCAP_FIELD_U56:
+			vcap_copy_limited_actionfield(field->data.u56.value,
+						      value,
+						      width, field_size);
+			break;
+		case VCAP_FIELD_U64:
+			vcap_copy_limited_actionfield(field->data.u64.value,
+						      value,
+						      width, field_size);
+			break;
+		case VCAP_FIELD_U72:
+			vcap_copy_limited_actionfield(field->data.u72.value,
+						      value,
+						      width, field_size);
+			break;
+		case VCAP_FIELD_U112:
+			vcap_copy_limited_actionfield(field->data.u112.value,
+						      value,
+						      width, field_size);
+			break;
+		case VCAP_FIELD_U128:
+			vcap_copy_limited_actionfield(field->data.u128.value,
+						      value,
+						      width, field_size);
+			break;
+		};
+	}
+}
+
+static void vcap_copy_key_bit_field(struct vcap_u1_key *field,
+				    u8 *value, u8 *mask)
+{
+	field->value = (*value) & 0x1;
+	field->mask = (*mask) & 0x1;
+}
+
+static void vcap_copy_limited_keyfield(u8 *dstvalue, u8 *dstmask,
+				       u8 *srcvalue, u8 *srcmask,
+				       int width, int bytes)
+{
+	memcpy(dstvalue, srcvalue, bytes);
+	vcap_apply_width(dstvalue, width, bytes);
+	memcpy(dstmask, srcmask, bytes);
+	vcap_apply_width(dstmask, width, bytes);
+}
+
+static void vcap_copy_to_client_keyfield(struct vcap_rule_internal *ri,
+					 struct vcap_client_keyfield *field,
+					 u8 *value, u8 *mask, u16 width)
+{
+	int field_size = keyfield_size_table[field->ctrl.type] / 2;
+
+	if (ri->admin->w32be) {
+		switch (field->ctrl.type) {
+		case VCAP_FIELD_BIT:
+			vcap_copy_key_bit_field(&field->data.u1, value, mask);
+			break;
+		case VCAP_FIELD_U32:
+			vcap_copy_limited_keyfield((u8 *)&field->data.u32.value,
+						   (u8 *)&field->data.u32.mask,
+						   value, mask,
+						   width, field_size);
+			break;
+		case VCAP_FIELD_U48:
+			vcap_copy_from_w32be(field->data.u48.value, value,
+					     field_size, width);
+			vcap_copy_from_w32be(field->data.u48.mask,  mask,
+					     field_size, width);
+			break;
+		case VCAP_FIELD_U56:
+			vcap_copy_from_w32be(field->data.u56.value, value,
+					     field_size, width);
+			vcap_copy_from_w32be(field->data.u56.mask,  mask,
+					     field_size, width);
+			break;
+		case VCAP_FIELD_U64:
+			vcap_copy_from_w32be(field->data.u64.value, value,
+					     field_size, width);
+			vcap_copy_from_w32be(field->data.u64.mask,  mask,
+					     field_size, width);
+			break;
+		case VCAP_FIELD_U72:
+			vcap_copy_from_w32be(field->data.u72.value, value,
+					     field_size, width);
+			vcap_copy_from_w32be(field->data.u72.mask,  mask,
+					     field_size, width);
+			break;
+		case VCAP_FIELD_U112:
+			vcap_copy_from_w32be(field->data.u112.value, value,
+					     field_size, width);
+			vcap_copy_from_w32be(field->data.u112.mask,  mask,
+					     field_size, width);
+			break;
+		case VCAP_FIELD_U128:
+			vcap_copy_from_w32be(field->data.u128.value, value,
+					     field_size, width);
+			vcap_copy_from_w32be(field->data.u128.mask,  mask,
+					     field_size, width);
+			break;
+		};
+	} else {
+		switch (field->ctrl.type) {
+		case VCAP_FIELD_BIT:
+			vcap_copy_key_bit_field(&field->data.u1, value, mask);
+			break;
+		case VCAP_FIELD_U32:
+			vcap_copy_limited_keyfield((u8 *)&field->data.u32.value,
+						   (u8 *)&field->data.u32.mask,
+						   value, mask,
+						   width, field_size);
+			break;
+		case VCAP_FIELD_U48:
+			vcap_copy_limited_keyfield(field->data.u48.value,
+						   field->data.u48.mask,
+						   value, mask,
+						   width, field_size);
+			break;
+		case VCAP_FIELD_U56:
+			vcap_copy_limited_keyfield(field->data.u56.value,
+						   field->data.u56.mask,
+						   value, mask,
+						   width, field_size);
+			break;
+		case VCAP_FIELD_U64:
+			vcap_copy_limited_keyfield(field->data.u64.value,
+						   field->data.u64.mask,
+						   value, mask,
+						   width, field_size);
+			break;
+		case VCAP_FIELD_U72:
+			vcap_copy_limited_keyfield(field->data.u72.value,
+						   field->data.u72.mask,
+						   value, mask,
+						   width, field_size);
+			break;
+		case VCAP_FIELD_U112:
+			vcap_copy_limited_keyfield(field->data.u112.value,
+						   field->data.u112.mask,
+						   value, mask,
+						   width, field_size);
+			break;
+		case VCAP_FIELD_U128:
+			vcap_copy_limited_keyfield(field->data.u128.value,
+						   field->data.u128.mask,
+						   value, mask,
+						   width, field_size);
+			break;
+		};
+	}
+}
+
+static void vcap_rule_alloc_keyfield(struct vcap_rule_internal *ri,
+				     const struct vcap_field *keyfield,
+				     enum vcap_key_field key,
+				     u8 *value, u8 *mask)
+{
+	struct vcap_client_keyfield *field;
+
+	field = kzalloc(sizeof(*field), GFP_KERNEL);
+	if (!field)
+		return;
+	INIT_LIST_HEAD(&field->ctrl.list);
+	field->ctrl.key = key;
+	field->ctrl.type = keyfield->type;
+	vcap_copy_to_client_keyfield(ri, field, value, mask, keyfield->width);
+	list_add_tail(&field->ctrl.list, &ri->data.keyfields);
+}
+
+/* Read key data from a VCAP address and discover if there is a rule keyset
+ * here
+ */
+static bool
+vcap_verify_actionstream_actionset(struct vcap_control *vctrl,
+				   enum vcap_type vt,
+				   u32 *actionstream,
+				   enum vcap_actionfield_set actionset)
+{
+	const struct vcap_typegroup *tgt;
+	const struct vcap_field *fields;
+	const struct vcap_set *info;
+
+	if (vcap_actionfield_count(vctrl, vt, actionset) == 0)
+		return false;
+
+	info = vcap_actionfieldset(vctrl, vt, actionset);
+	/* Check that the actionset is valid */
+	if (!info)
+		return false;
+
+	/* a type_id of value -1 means that there is no type field */
+	if (info->type_id == (u8)-1)
+		return true;
+
+	/* Get a valid typegroup for the specific actionset */
+	tgt = vcap_actionfield_typegroup(vctrl, vt, actionset);
+	if (!tgt)
+		return false;
+
+	fields = vcap_actionfields(vctrl, vt, actionset);
+	if (!fields)
+		return false;
+
+	/* Later this will be expanded with a check of the type id */
+	return true;
+}
+
+/* Find the subword width of the action typegroup that matches the stream data
+ */
+static int vcap_find_actionstream_typegroup_sw(struct vcap_control *vctrl,
+					       enum vcap_type vt, u32 *stream,
+					       int sw_max)
+{
+	const struct vcap_typegroup **tgt;
+	int sw_idx, res;
+
+	tgt = vctrl->vcaps[vt].actionfield_set_typegroups;
+	/* Try the longest subword match first */
+	for (sw_idx = vctrl->vcaps[vt].sw_count; sw_idx >= 0; sw_idx--) {
+		if (!tgt[sw_idx])
+			continue;
+		res = vcap_verify_typegroups(stream, vctrl->vcaps[vt].act_width,
+					     tgt[sw_idx], false, sw_max);
+		if (res == 0)
+			return sw_idx;
+	}
+	return -EINVAL;
+}
+
+/* Verify that the typegroup information, subword count, actionset and type id
+ * are in sync and correct, return the actionset
+ */
+static enum vcap_actionfield_set
+vcap_find_actionstream_actionset(struct vcap_control *vctrl,
+				 enum vcap_type vt,
+				 u32 *stream,
+				 int sw_max)
+{
+	const struct vcap_set *actionfield_set;
+	int sw_count, idx;
+	bool res;
+
+	sw_count = vcap_find_actionstream_typegroup_sw(vctrl, vt, stream,
+						       sw_max);
+	if (sw_count < 0)
+		return sw_count;
+
+	actionfield_set = vctrl->vcaps[vt].actionfield_set;
+	for (idx = 0; idx < vctrl->vcaps[vt].actionfield_set_size; ++idx) {
+		if (actionfield_set[idx].sw_per_item != sw_count)
+			continue;
+
+		res = vcap_verify_actionstream_actionset(vctrl, vt,
+							 stream, idx);
+		if (res)
+			return idx;
+	}
+	return -EINVAL;
+}
+
+/* Store action value in an element in a list for the client */
+static void vcap_rule_alloc_actionfield(struct vcap_rule_internal *ri,
+					const struct vcap_field *actionfield,
+					enum vcap_action_field action,
+					u8 *value)
+{
+	struct vcap_client_actionfield *field;
+
+	field = kzalloc(sizeof(*field), GFP_KERNEL);
+	if (!field)
+		return;
+	INIT_LIST_HEAD(&field->ctrl.list);
+	field->ctrl.action = action;
+	field->ctrl.type = actionfield->type;
+	vcap_copy_to_client_actionfield(ri, field, value, actionfield->width);
+	list_add_tail(&field->ctrl.list, &ri->data.actionfields);
+}
+
+static int vcap_decode_actionset(struct vcap_rule_internal *ri)
+{
+	struct vcap_control *vctrl = ri->vctrl;
+	struct vcap_admin *admin = ri->admin;
+	const struct vcap_field *actionfield;
+	enum vcap_actionfield_set actionset;
+	enum vcap_type vt = admin->vtype;
+	const struct vcap_typegroup *tgt;
+	struct vcap_stream_iter iter;
+	int idx, res, actfield_count;
+	u32 *actstream;
+	u8 value[16];
+
+	actstream = admin->cache.actionstream;
+	res = vcap_find_actionstream_actionset(vctrl, vt, actstream, 0);
+	if (res < 0) {
+		pr_err("%s:%d: could not find valid actionset: %d\n",
+		       __func__, __LINE__, res);
+		return -EINVAL;
+	}
+	actionset = res;
+	actfield_count = vcap_actionfield_count(vctrl, vt, actionset);
+	actionfield = vcap_actionfields(vctrl, vt, actionset);
+	tgt = vcap_actionfield_typegroup(vctrl, vt, actionset);
+	/* Start decoding the stream */
+	for (idx = 0; idx < actfield_count; ++idx) {
+		if (actionfield[idx].width <= 0)
+			continue;
+		/* Get the action */
+		memset(value, 0, DIV_ROUND_UP(actionfield[idx].width, 8));
+		vcap_iter_init(&iter, vctrl->vcaps[vt].act_width, tgt,
+			       actionfield[idx].offset);
+		vcap_decode_field(actstream, &iter, actionfield[idx].width,
+				  value);
+		/* Skip if no bits are set */
+		if (vcap_bitarray_zero(actionfield[idx].width, value))
+			continue;
+		vcap_rule_alloc_actionfield(ri, &actionfield[idx], idx, value);
+		/* Later the action id will also be checked */
+	}
+	return vcap_set_rule_set_actionset((struct vcap_rule *)ri, actionset);
+}
+
+static int vcap_decode_keyset(struct vcap_rule_internal *ri)
+{
+	struct vcap_control *vctrl = ri->vctrl;
+	struct vcap_stream_iter kiter, miter;
+	struct vcap_admin *admin = ri->admin;
+	enum vcap_keyfield_set keysets[10];
+	const struct vcap_field *keyfield;
+	enum vcap_type vt = admin->vtype;
+	const struct vcap_typegroup *tgt;
+	struct vcap_keyset_list matches;
+	enum vcap_keyfield_set keyset;
+	int idx, res, keyfield_count;
+	u32 *maskstream;
+	u32 *keystream;
+	u8 value[16];
+	u8 mask[16];
+
+	keystream = admin->cache.keystream;
+	maskstream = admin->cache.maskstream;
+	matches.keysets = keysets;
+	matches.cnt = 0;
+	matches.max = ARRAY_SIZE(keysets);
+	res = vcap_find_keystream_keysets(vctrl, vt, keystream, maskstream,
+					  false, 0, &matches);
+	if (res < 0) {
+		pr_err("%s:%d: could not find valid keysets: %d\n",
+		       __func__, __LINE__, res);
+		return -EINVAL;
+	}
+	keyset = matches.keysets[0];
+	keyfield_count = vcap_keyfield_count(vctrl, vt, keyset);
+	keyfield = vcap_keyfields(vctrl, vt, keyset);
+	tgt = vcap_keyfield_typegroup(vctrl, vt, keyset);
+	/* Start decoding the streams */
+	for (idx = 0; idx < keyfield_count; ++idx) {
+		if (keyfield[idx].width <= 0)
+			continue;
+		/* First get the mask */
+		memset(mask, 0, DIV_ROUND_UP(keyfield[idx].width, 8));
+		vcap_iter_init(&miter, vctrl->vcaps[vt].sw_width, tgt,
+			       keyfield[idx].offset);
+		vcap_decode_field(maskstream, &miter, keyfield[idx].width,
+				  mask);
+		/* Skip if no mask bits are set */
+		if (vcap_bitarray_zero(keyfield[idx].width, mask))
+			continue;
+		/* Get the key */
+		memset(value, 0, DIV_ROUND_UP(keyfield[idx].width, 8));
+		vcap_iter_init(&kiter, vctrl->vcaps[vt].sw_width, tgt,
+			       keyfield[idx].offset);
+		vcap_decode_field(keystream, &kiter, keyfield[idx].width,
+				  value);
+		vcap_rule_alloc_keyfield(ri, &keyfield[idx], idx, value, mask);
+	}
+	return vcap_set_rule_set_keyset((struct vcap_rule *)ri, keyset);
+}
+
+/* Read VCAP content into the VCAP cache */
+static int vcap_read_rule(struct vcap_rule_internal *ri)
+{
+	struct vcap_admin *admin = ri->admin;
+	int sw_idx, ent_idx = 0, act_idx = 0;
+	u32 addr = ri->addr;
+
+	if (!ri->size || !ri->keyset_sw_regs || !ri->actionset_sw_regs) {
+		pr_err("%s:%d: rule is empty\n", __func__, __LINE__);
+		return -EINVAL;
+	}
+	vcap_erase_cache(ri);
+	/* Use the values in the streams to read the VCAP cache */
+	for (sw_idx = 0; sw_idx < ri->size; sw_idx++, addr++) {
+		ri->vctrl->ops->update(ri->ndev, admin, VCAP_CMD_READ,
+				       VCAP_SEL_ALL, addr);
+		ri->vctrl->ops->cache_read(ri->ndev, admin,
+					   VCAP_SEL_ENTRY, ent_idx,
+					   ri->keyset_sw_regs);
+		ri->vctrl->ops->cache_read(ri->ndev, admin,
+					   VCAP_SEL_ACTION, act_idx,
+					   ri->actionset_sw_regs);
+		if (sw_idx == 0)
+			ri->vctrl->ops->cache_read(ri->ndev, admin,
+						   VCAP_SEL_COUNTER,
+						   ri->counter_id, 0);
+		ent_idx += ri->keyset_sw_regs;
+		act_idx += ri->actionset_sw_regs;
+	}
+	return 0;
+}
+
 /* Write VCAP cache content to the VCAP HW instance */
 static int vcap_write_rule(struct vcap_rule_internal *ri)
 {
@@ -1183,6 +1915,46 @@ void vcap_free_rule(struct vcap_rule *rule)
 }
 EXPORT_SYMBOL_GPL(vcap_free_rule);
 
+struct vcap_rule *vcap_get_rule(struct vcap_control *vctrl, u32 id)
+{
+	struct vcap_rule_internal *elem;
+	struct vcap_rule_internal *ri;
+	int err;
+
+	ri = NULL;
+
+	err = vcap_api_check(vctrl);
+	if (err)
+		return ERR_PTR(err);
+	elem = vcap_lookup_rule(vctrl, id);
+	if (!elem)
+		return NULL;
+	mutex_lock(&elem->admin->lock);
+	ri = vcap_dup_rule(elem);
+	if (IS_ERR(ri))
+		goto unlock;
+	err = vcap_read_rule(ri);
+	if (err) {
+		ri = ERR_PTR(err);
+		goto unlock;
+	}
+	err = vcap_decode_keyset(ri);
+	if (err) {
+		ri = ERR_PTR(err);
+		goto unlock;
+	}
+	err = vcap_decode_actionset(ri);
+	if (err) {
+		ri = ERR_PTR(err);
+		goto unlock;
+	}
+
+unlock:
+	mutex_unlock(&elem->admin->lock);
+	return (struct vcap_rule *)ri;
+}
+EXPORT_SYMBOL_GPL(vcap_get_rule);
+
 /* Return the alignment offset for a new rule address */
 static int vcap_valid_rule_move(struct vcap_rule_internal *el, int offset)
 {
diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api_client.h b/drivers/net/ethernet/microchip/vcap/vcap_api_client.h
index 93a0fcb12a819..a354dcd741e22 100644
--- a/drivers/net/ethernet/microchip/vcap/vcap_api_client.h
+++ b/drivers/net/ethernet/microchip/vcap/vcap_api_client.h
@@ -170,6 +170,8 @@ int vcap_add_rule(struct vcap_rule *rule);
 int vcap_del_rule(struct vcap_control *vctrl, struct net_device *ndev, u32 id);
 /* Make a full copy of an existing rule with a new rule id */
 struct vcap_rule *vcap_copy_rule(struct vcap_rule *rule);
+/* Get rule from a VCAP instance */
+struct vcap_rule *vcap_get_rule(struct vcap_control *vctrl, u32 id);
 
 /* Update the keyset for the rule */
 int vcap_set_rule_set_keyset(struct vcap_rule *rule,
diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api_debugfs.c b/drivers/net/ethernet/microchip/vcap/vcap_api_debugfs.c
index 3b8d165dc8322..895bfff550d23 100644
--- a/drivers/net/ethernet/microchip/vcap/vcap_api_debugfs.c
+++ b/drivers/net/ethernet/microchip/vcap/vcap_api_debugfs.c
@@ -18,355 +18,15 @@ struct vcap_port_debugfs_info {
 	struct net_device *ndev;
 };
 
-static bool vcap_bitarray_zero(int width, u8 *value)
-{
-	int bytes = DIV_ROUND_UP(width, BITS_PER_BYTE);
-	u8 total = 0, bmask = 0xff;
-	int rwidth = width;
-	int idx;
-
-	for (idx = 0; idx < bytes; ++idx, rwidth -= BITS_PER_BYTE) {
-		if (rwidth && rwidth < BITS_PER_BYTE)
-			bmask = (1 << rwidth) - 1;
-		total += value[idx] & bmask;
-	}
-	return total == 0;
-}
-
-static bool vcap_get_bit(u32 *stream, struct vcap_stream_iter *itr)
-{
-	u32 mask = BIT(itr->reg_bitpos);
-	u32 *p = &stream[itr->reg_idx];
-
-	return !!(*p & mask);
-}
-
-static void vcap_decode_field(u32 *stream, struct vcap_stream_iter *itr,
-			      int width, u8 *value)
-{
-	int idx;
-
-	/* Loop over the field value bits and get the field bits and
-	 * set them in the output value byte array
-	 */
-	for (idx = 0; idx < width; idx++) {
-		u8 bidx = idx & 0x7;
-
-		/* Decode one field value bit */
-		if (vcap_get_bit(stream, itr))
-			*value |= 1 << bidx;
-		vcap_iter_next(itr);
-		if (bidx == 7)
-			value++;
-	}
-}
-
-/* Verify that the typegroup bits have the correct values */
-static int vcap_verify_typegroups(u32 *stream, int sw_width,
-				  const struct vcap_typegroup *tgt, bool mask,
-				  int sw_max)
-{
-	struct vcap_stream_iter iter;
-	int sw_cnt, idx;
-
-	vcap_iter_set(&iter, sw_width, tgt, 0);
-	sw_cnt = 0;
-	while (iter.tg->width) {
-		u32 value = 0;
-		u32 tg_value = iter.tg->value;
-
-		if (mask)
-			tg_value = (1 << iter.tg->width) - 1;
-		/* Set position to current typegroup bit */
-		iter.offset = iter.tg->offset;
-		vcap_iter_update(&iter);
-		for (idx = 0; idx < iter.tg->width; idx++) {
-			/* Decode one typegroup bit */
-			if (vcap_get_bit(stream, &iter))
-				value |= 1 << idx;
-			iter.offset++;
-			vcap_iter_update(&iter);
-		}
-		if (value != tg_value)
-			return -EINVAL;
-		iter.tg++; /* next typegroup */
-		sw_cnt++;
-		/* Stop checking more typegroups */
-		if (sw_max && sw_cnt >= sw_max)
-			break;
-	}
-	return 0;
-}
-
-/* Find the subword width of the key typegroup that matches the stream data */
-static int vcap_find_keystream_typegroup_sw(struct vcap_control *vctrl,
-					    enum vcap_type vt, u32 *stream,
-					    bool mask, int sw_max)
-{
-	const struct vcap_typegroup **tgt;
-	int sw_idx, res;
-
-	tgt = vctrl->vcaps[vt].keyfield_set_typegroups;
-	/* Try the longest subword match first */
-	for (sw_idx = vctrl->vcaps[vt].sw_count; sw_idx >= 0; sw_idx--) {
-		if (!tgt[sw_idx])
-			continue;
-
-		res = vcap_verify_typegroups(stream, vctrl->vcaps[vt].sw_width,
-					     tgt[sw_idx], mask, sw_max);
-		if (res == 0)
-			return sw_idx;
-	}
-	return -EINVAL;
-}
-
-/* Find the subword width of the action typegroup that matches the stream data
- */
-static int vcap_find_actionstream_typegroup_sw(struct vcap_control *vctrl,
-					       enum vcap_type vt, u32 *stream,
-					       int sw_max)
-{
-	const struct vcap_typegroup **tgt;
-	int sw_idx, res;
-
-	tgt = vctrl->vcaps[vt].actionfield_set_typegroups;
-	/* Try the longest subword match first */
-	for (sw_idx = vctrl->vcaps[vt].sw_count; sw_idx >= 0; sw_idx--) {
-		if (!tgt[sw_idx])
-			continue;
-		res = vcap_verify_typegroups(stream, vctrl->vcaps[vt].act_width,
-					     tgt[sw_idx], false, sw_max);
-		if (res == 0)
-			return sw_idx;
-	}
-	return -EINVAL;
-}
-
-/* Verify that the type id in the stream matches the type id of the keyset */
-static bool vcap_verify_keystream_keyset(struct vcap_control *vctrl,
-					 enum vcap_type vt,
-					 u32 *keystream,
-					 u32 *mskstream,
-					 enum vcap_keyfield_set keyset)
-{
-	const struct vcap_info *vcap = &vctrl->vcaps[vt];
-	const struct vcap_field *typefld;
-	const struct vcap_typegroup *tgt;
-	const struct vcap_field *fields;
-	struct vcap_stream_iter iter;
-	const struct vcap_set *info;
-	u32 value = 0;
-	u32 mask = 0;
-
-	if (vcap_keyfield_count(vctrl, vt, keyset) == 0)
-		return false;
-
-	info = vcap_keyfieldset(vctrl, vt, keyset);
-	/* Check that the keyset is valid */
-	if (!info)
-		return false;
-
-	/* a type_id of value -1 means that there is no type field */
-	if (info->type_id == (u8)-1)
-		return true;
-
-	/* Get a valid typegroup for the specific keyset */
-	tgt = vcap_keyfield_typegroup(vctrl, vt, keyset);
-	if (!tgt)
-		return false;
-
-	fields = vcap_keyfields(vctrl, vt, keyset);
-	if (!fields)
-		return false;
-
-	typefld = &fields[VCAP_KF_TYPE];
-	vcap_iter_init(&iter, vcap->sw_width, tgt, typefld->offset);
-	vcap_decode_field(mskstream, &iter, typefld->width, (u8 *)&mask);
-	/* no type info if there are no mask bits */
-	if (vcap_bitarray_zero(typefld->width, (u8 *)&mask))
-		return false;
-
-	/* Get the value of the type field in the stream and compare to the
-	 * one define in the vcap keyset
-	 */
-	vcap_iter_init(&iter, vcap->sw_width, tgt, typefld->offset);
-	vcap_decode_field(keystream, &iter, typefld->width, (u8 *)&value);
-
-	return (value & mask) == (info->type_id & mask);
-}
-
-/* Verify that the typegroup information, subword count, keyset and type id
- * are in sync and correct, return the list of matching keysets
- */
-static int
-vcap_find_keystream_keysets(struct vcap_control *vctrl,
-			    enum vcap_type vt,
-			    u32 *keystream,
-			    u32 *mskstream,
-			    bool mask, int sw_max,
-			    struct vcap_keyset_list *kslist)
-{
-	const struct vcap_set *keyfield_set;
-	int sw_count, idx;
-
-	sw_count = vcap_find_keystream_typegroup_sw(vctrl, vt, keystream, mask,
-						    sw_max);
-	if (sw_count < 0)
-		return sw_count;
-
-	keyfield_set = vctrl->vcaps[vt].keyfield_set;
-	for (idx = 0; idx < vctrl->vcaps[vt].keyfield_set_size; ++idx) {
-		if (keyfield_set[idx].sw_per_item != sw_count)
-			continue;
-
-		if (vcap_verify_keystream_keyset(vctrl, vt, keystream,
-						 mskstream, idx))
-			vcap_keyset_list_add(kslist, idx);
-	}
-	if (kslist->cnt > 0)
-		return 0;
-	return -EINVAL;
-}
-
-/* Read key data from a VCAP address and discover if there is a rule keyset
- * here
- */
-static bool
-vcap_verify_actionstream_actionset(struct vcap_control *vctrl,
-				   enum vcap_type vt,
-				   u32 *actionstream,
-				   enum vcap_actionfield_set actionset)
-{
-	const struct vcap_typegroup *tgt;
-	const struct vcap_field *fields;
-	const struct vcap_set *info;
-
-	if (vcap_actionfield_count(vctrl, vt, actionset) == 0)
-		return false;
-
-	info = vcap_actionfieldset(vctrl, vt, actionset);
-	/* Check that the actionset is valid */
-	if (!info)
-		return false;
-
-	/* a type_id of value -1 means that there is no type field */
-	if (info->type_id == (u8)-1)
-		return true;
-
-	/* Get a valid typegroup for the specific actionset */
-	tgt = vcap_actionfield_typegroup(vctrl, vt, actionset);
-	if (!tgt)
-		return false;
-
-	fields = vcap_actionfields(vctrl, vt, actionset);
-	if (!fields)
-		return false;
-
-	/* Later this will be expanded with a check of the type id */
-	return true;
-}
-
-/* Verify that the typegroup information, subword count, actionset and type id
- * are in sync and correct, return the actionset
- */
-static enum vcap_actionfield_set
-vcap_find_actionstream_actionset(struct vcap_control *vctrl,
-				 enum vcap_type vt,
-				 u32 *stream,
-				 int sw_max)
-{
-	const struct vcap_set *actionfield_set;
-	int sw_count, idx;
-	bool res;
-
-	sw_count = vcap_find_actionstream_typegroup_sw(vctrl, vt, stream,
-						       sw_max);
-	if (sw_count < 0)
-		return sw_count;
-
-	actionfield_set = vctrl->vcaps[vt].actionfield_set;
-	for (idx = 0; idx < vctrl->vcaps[vt].actionfield_set_size; ++idx) {
-		if (actionfield_set[idx].sw_per_item != sw_count)
-			continue;
-
-		res = vcap_verify_actionstream_actionset(vctrl, vt,
-							 stream, idx);
-		if (res)
-			return idx;
-	}
-	return -EINVAL;
-}
-
-/* Read key data from a VCAP address and discover if there are any rule keysets
- * here
- */
-static int vcap_addr_keysets(struct vcap_control *vctrl,
-			     struct net_device *ndev,
-			     struct vcap_admin *admin,
-			     int addr,
-			     struct vcap_keyset_list *kslist)
-{
-	enum vcap_type vt = admin->vtype;
-	int keyset_sw_regs, idx;
-	u32 key = 0, mask = 0;
-
-	/* Read the cache at the specified address */
-	keyset_sw_regs = DIV_ROUND_UP(vctrl->vcaps[vt].sw_width, 32);
-	vctrl->ops->update(ndev, admin, VCAP_CMD_READ, VCAP_SEL_ALL, addr);
-	vctrl->ops->cache_read(ndev, admin, VCAP_SEL_ENTRY, 0,
-			       keyset_sw_regs);
-	/* Skip uninitialized key/mask entries */
-	for (idx = 0; idx < keyset_sw_regs; ++idx) {
-		key |= ~admin->cache.keystream[idx];
-		mask |= admin->cache.maskstream[idx];
-	}
-	if (key == 0 && mask == 0)
-		return -EINVAL;
-	/* Decode and locate the keysets */
-	return vcap_find_keystream_keysets(vctrl, vt, admin->cache.keystream,
-					   admin->cache.maskstream, false, 0,
-					   kslist);
-}
-
-static int vcap_read_rule(struct vcap_rule_internal *ri)
-{
-	struct vcap_admin *admin = ri->admin;
-	int sw_idx, ent_idx = 0, act_idx = 0;
-	u32 addr = ri->addr;
-
-	if (!ri->size || !ri->keyset_sw_regs || !ri->actionset_sw_regs) {
-		pr_err("%s:%d: rule is empty\n", __func__, __LINE__);
-		return -EINVAL;
-	}
-	vcap_erase_cache(ri);
-	/* Use the values in the streams to read the VCAP cache */
-	for (sw_idx = 0; sw_idx < ri->size; sw_idx++, addr++) {
-		ri->vctrl->ops->update(ri->ndev, admin, VCAP_CMD_READ,
-				       VCAP_SEL_ALL, addr);
-		ri->vctrl->ops->cache_read(ri->ndev, admin,
-					   VCAP_SEL_ENTRY, ent_idx,
-					   ri->keyset_sw_regs);
-		ri->vctrl->ops->cache_read(ri->ndev, admin,
-					   VCAP_SEL_ACTION, act_idx,
-					   ri->actionset_sw_regs);
-		if (sw_idx == 0)
-			ri->vctrl->ops->cache_read(ri->ndev, admin,
-						   VCAP_SEL_COUNTER,
-						   ri->counter_id, 0);
-		ent_idx += ri->keyset_sw_regs;
-		act_idx += ri->actionset_sw_regs;
-	}
-	return 0;
-}
-
 /* Dump the keyfields value and mask values */
 static void vcap_debugfs_show_rule_keyfield(struct vcap_control *vctrl,
 					    struct vcap_output_print *out,
 					    enum vcap_key_field key,
 					    const struct vcap_field *keyfield,
-					    u8 *value, u8 *mask)
+					    struct vcap_client_keyfield_data *data)
 {
 	bool hex = false;
+	u8 *value, *mask;
 	int idx, bytes;
 
 	out->prf(out->dst, "    %s: W%d: ", vcap_keyfield_name(vctrl, key),
@@ -374,40 +34,62 @@ static void vcap_debugfs_show_rule_keyfield(struct vcap_control *vctrl,
 
 	switch (keyfield[key].type) {
 	case VCAP_FIELD_BIT:
-		out->prf(out->dst, "%d/%d", value[0], mask[0]);
+		out->prf(out->dst, "%d/%d", data->u1.value, data->u1.mask);
 		break;
 	case VCAP_FIELD_U32:
+		value = (u8 *)(&data->u32.value);
+		mask = (u8 *)(&data->u32.mask);
+
 		if (key == VCAP_KF_L3_IP4_SIP || key == VCAP_KF_L3_IP4_DIP) {
-			out->prf(out->dst, "%pI4h/%pI4h", value, mask);
+			out->prf(out->dst, "%pI4h/%pI4h", &data->u32.value,
+				 &data->u32.mask);
 		} else if (key == VCAP_KF_ETYPE ||
 			   key == VCAP_KF_IF_IGR_PORT_MASK) {
 			hex = true;
 		} else {
 			u32 fmsk = (1 << keyfield[key].width) - 1;
-			u32 val = *(u32 *)value;
-			u32 msk = *(u32 *)mask;
 
-			out->prf(out->dst, "%u/%u", val & fmsk, msk & fmsk);
+			out->prf(out->dst, "%u/%u", data->u32.value & fmsk,
+				 data->u32.mask & fmsk);
 		}
 		break;
 	case VCAP_FIELD_U48:
+		value = data->u48.value;
+		mask = data->u48.mask;
 		if (key == VCAP_KF_L2_SMAC || key == VCAP_KF_L2_DMAC)
-			out->prf(out->dst, "%pMR/%pMR", value, mask);
+			out->prf(out->dst, "%pMR/%pMR", data->u48.value,
+				 data->u48.mask);
 		else
 			hex = true;
 		break;
 	case VCAP_FIELD_U56:
+		value = data->u56.value;
+		mask = data->u56.mask;
+		hex = true;
+		break;
 	case VCAP_FIELD_U64:
+		value = data->u64.value;
+		mask = data->u64.mask;
+		hex = true;
+		break;
 	case VCAP_FIELD_U72:
+		value = data->u72.value;
+		mask = data->u72.mask;
+		hex = true;
+		break;
 	case VCAP_FIELD_U112:
+		value = data->u112.value;
+		mask = data->u112.mask;
 		hex = true;
 		break;
 	case VCAP_FIELD_U128:
 		if (key == VCAP_KF_L3_IP6_SIP || key == VCAP_KF_L3_IP6_DIP) {
 			u8 nvalue[16], nmask[16];
 
-			vcap_netbytes_copy(nvalue, value, sizeof(nvalue));
-			vcap_netbytes_copy(nmask, mask, sizeof(nmask));
+			vcap_netbytes_copy(nvalue, data->u128.value,
+					   sizeof(nvalue));
+			vcap_netbytes_copy(nmask, data->u128.mask,
+					   sizeof(nmask));
 			out->prf(out->dst, "%pI6/%pI6", nvalue, nmask);
 		} else {
 			hex = true;
@@ -472,19 +154,15 @@ static int vcap_debugfs_show_rule_keyset(struct vcap_rule_internal *ri,
 					 struct vcap_output_print *out)
 {
 	struct vcap_control *vctrl = ri->vctrl;
-	struct vcap_stream_iter kiter, miter;
 	struct vcap_admin *admin = ri->admin;
 	enum vcap_keyfield_set keysets[10];
 	const struct vcap_field *keyfield;
 	enum vcap_type vt = admin->vtype;
-	const struct vcap_typegroup *tgt;
+	struct vcap_client_keyfield *ckf;
 	struct vcap_keyset_list matches;
-	enum vcap_keyfield_set keyset;
-	int idx, res, keyfield_count;
 	u32 *maskstream;
 	u32 *keystream;
-	u8 value[16];
-	u8 mask[16];
+	int res;
 
 	keystream = admin->cache.keystream;
 	maskstream = admin->cache.maskstream;
@@ -498,39 +176,20 @@ static int vcap_debugfs_show_rule_keyset(struct vcap_rule_internal *ri,
 		       __func__, __LINE__, res);
 		return -EINVAL;
 	}
-	keyset = matches.keysets[0];
 	out->prf(out->dst, "  keysets:");
-	for (idx = 0; idx < matches.cnt; ++idx)
+	for (int idx = 0; idx < matches.cnt; ++idx)
 		out->prf(out->dst, " %s",
 			 vcap_keyset_name(vctrl, matches.keysets[idx]));
 	out->prf(out->dst, "\n");
 	out->prf(out->dst, "  keyset_sw: %d\n", ri->keyset_sw);
 	out->prf(out->dst, "  keyset_sw_regs: %d\n", ri->keyset_sw_regs);
-	keyfield_count = vcap_keyfield_count(vctrl, vt, keyset);
-	keyfield = vcap_keyfields(vctrl, vt, keyset);
-	tgt = vcap_keyfield_typegroup(vctrl, vt, keyset);
-	/* Start decoding the streams */
-	for (idx = 0; idx < keyfield_count; ++idx) {
-		if (keyfield[idx].width <= 0)
-			continue;
-		/* First get the mask */
-		memset(mask, 0, DIV_ROUND_UP(keyfield[idx].width, 8));
-		vcap_iter_init(&miter, vctrl->vcaps[vt].sw_width, tgt,
-			       keyfield[idx].offset);
-		vcap_decode_field(maskstream, &miter, keyfield[idx].width,
-				  mask);
-		/* Skip if no mask bits are set */
-		if (vcap_bitarray_zero(keyfield[idx].width, mask))
-			continue;
-		/* Get the key */
-		memset(value, 0, DIV_ROUND_UP(keyfield[idx].width, 8));
-		vcap_iter_init(&kiter, vctrl->vcaps[vt].sw_width, tgt,
-			       keyfield[idx].offset);
-		vcap_decode_field(keystream, &kiter, keyfield[idx].width,
-				  value);
-		vcap_debugfs_show_rule_keyfield(vctrl, out, idx, keyfield,
-						value, mask);
+
+	list_for_each_entry(ckf, &ri->data.keyfields, ctrl.list) {
+		keyfield = vcap_keyfields(vctrl, admin->vtype, ri->data.keyset);
+		vcap_debugfs_show_rule_keyfield(vctrl, out, ckf->ctrl.key,
+						keyfield, &ckf->data);
 	}
+
 	return 0;
 }
 
@@ -540,48 +199,21 @@ static int vcap_debugfs_show_rule_actionset(struct vcap_rule_internal *ri,
 	struct vcap_control *vctrl = ri->vctrl;
 	struct vcap_admin *admin = ri->admin;
 	const struct vcap_field *actionfield;
-	enum vcap_actionfield_set actionset;
-	enum vcap_type vt = admin->vtype;
-	const struct vcap_typegroup *tgt;
-	struct vcap_stream_iter iter;
-	int idx, res, actfield_count;
-	u32 *actstream;
-	u8 value[16];
-	bool no_bits;
-
-	actstream = admin->cache.actionstream;
-	res = vcap_find_actionstream_actionset(vctrl, vt, actstream, 0);
-	if (res < 0) {
-		pr_err("%s:%d: could not find valid actionset: %d\n",
-		       __func__, __LINE__, res);
-		return -EINVAL;
-	}
-	actionset = res;
+	struct vcap_client_actionfield *caf;
+
 	out->prf(out->dst, "  actionset: %s\n",
 		 vcap_actionset_name(vctrl, ri->data.actionset));
 	out->prf(out->dst, "  actionset_sw: %d\n", ri->actionset_sw);
 	out->prf(out->dst, "  actionset_sw_regs: %d\n", ri->actionset_sw_regs);
-	actfield_count = vcap_actionfield_count(vctrl, vt, actionset);
-	actionfield = vcap_actionfields(vctrl, vt, actionset);
-	tgt = vcap_actionfield_typegroup(vctrl, vt, actionset);
-	/* Start decoding the stream */
-	for (idx = 0; idx < actfield_count; ++idx) {
-		if (actionfield[idx].width <= 0)
-			continue;
-		/* Get the action */
-		memset(value, 0, DIV_ROUND_UP(actionfield[idx].width, 8));
-		vcap_iter_init(&iter, vctrl->vcaps[vt].act_width, tgt,
-			       actionfield[idx].offset);
-		vcap_decode_field(actstream, &iter, actionfield[idx].width,
-				  value);
-		/* Skip if no bits are set */
-		no_bits = vcap_bitarray_zero(actionfield[idx].width, value);
-		if (no_bits)
-			continue;
-		/* Later the action id will also be checked */
-		vcap_debugfs_show_rule_actionfield(vctrl, out, idx, actionfield,
-						   value);
+
+	list_for_each_entry(caf, &ri->data.actionfields, ctrl.list) {
+		actionfield = vcap_actionfields(vctrl, admin->vtype,
+						ri->data.actionset);
+		vcap_debugfs_show_rule_actionfield(vctrl, out, caf->ctrl.action,
+						   actionfield,
+						   &caf->data.u1.value);
 	}
+
 	return 0;
 }
 
@@ -632,32 +264,22 @@ static int vcap_show_admin(struct vcap_control *vctrl,
 			   struct vcap_admin *admin,
 			   struct vcap_output_print *out)
 {
-	struct vcap_rule_internal *elem, *ri;
+	struct vcap_rule_internal *elem;
+	struct vcap_rule *vrule;
 	int ret = 0;
 
 	vcap_show_admin_info(vctrl, admin, out);
-	mutex_lock(&admin->lock);
 	list_for_each_entry(elem, &admin->rules, list) {
-		ri = vcap_dup_rule(elem);
-		if (IS_ERR(ri)) {
-			ret = PTR_ERR(ri);
-			goto err_unlock;
+		vrule = vcap_get_rule(vctrl, elem->data.id);
+		if (IS_ERR_OR_NULL(vrule)) {
+			ret = PTR_ERR(vrule);
+			break;
 		}
-		/* Read data from VCAP */
-		ret = vcap_read_rule(ri);
-		if (ret)
-			goto err_free_rule;
+
 		out->prf(out->dst, "\n");
-		vcap_show_admin_rule(vctrl, admin, out, ri);
-		vcap_free_rule((struct vcap_rule *)ri);
+		vcap_show_admin_rule(vctrl, admin, out, to_intrule(vrule));
+		vcap_free_rule(vrule);
 	}
-	mutex_unlock(&admin->lock);
-	return 0;
-
-err_free_rule:
-	vcap_free_rule((struct vcap_rule *)ri);
-err_unlock:
-	mutex_unlock(&admin->lock);
 	return ret;
 }
 
diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api_private.h b/drivers/net/ethernet/microchip/vcap/vcap_api_private.h
index 9ac1b1d55f22e..4fd21da976799 100644
--- a/drivers/net/ethernet/microchip/vcap/vcap_api_private.h
+++ b/drivers/net/ethernet/microchip/vcap/vcap_api_private.h
@@ -96,4 +96,18 @@ const char *vcap_actionset_name(struct vcap_control *vctrl,
 const char *vcap_actionfield_name(struct vcap_control *vctrl,
 				  enum vcap_action_field action);
 
+/* Read key data from a VCAP address and discover if there are any rule keysets
+ * here
+ */
+int vcap_addr_keysets(struct vcap_control *vctrl, struct net_device *ndev,
+		      struct vcap_admin *admin, int addr,
+		      struct vcap_keyset_list *kslist);
+
+/* Verify that the typegroup information, subword count, keyset and type id
+ * are in sync and correct, return the list of matchin keysets
+ */
+int vcap_find_keystream_keysets(struct vcap_control *vctrl, enum vcap_type vt,
+				u32 *keystream, u32 *mskstream, bool mask,
+				int sw_max, struct vcap_keyset_list *kslist);
+
 #endif /* __VCAP_API_PRIVATE__ */
-- 
2.38.0

Re: [PATCH net-next v3 1/4] net: microchip: vcap: Add vcap_get_rule

[Paolo Abeni]

Hello,

On Sat, 2022-12-03 at 11:43 +0100, Horatiu Vultur wrote:
> @@ -632,32 +264,22 @@ static int vcap_show_admin(struct vcap_control *vctrl,
>  			   struct vcap_admin *admin,
>  			   struct vcap_output_print *out)
>  {
> -	struct vcap_rule_internal *elem, *ri;
> +	struct vcap_rule_internal *elem;
> +	struct vcap_rule *vrule;
>  	int ret = 0;
>  
>  	vcap_show_admin_info(vctrl, admin, out);
> -	mutex_lock(&admin->lock);
>  	list_for_each_entry(elem, &admin->rules, list) {

Not strictly related to this patch, as the patter is AFAICS already
there in other places, but I'd like to understand the admin->rules
locking schema.

It looks like addition/removal are protected by admin->lock, but
traversal is usually lockless, which in turn looks buggy ?!?

Note: as this patch does not introduce the mentioned behavior, I'm not
going to block the series for the above.

Thanks,

Paolo
> -		ri = vcap_dup_rule(elem);
> -		if (IS_ERR(ri)) {
> -			ret = PTR_ERR(ri);
> -			goto err_unlock;
> +		vrule = vcap_get_rule(vctrl, elem->data.id);
> +		if (IS_ERR_OR_NULL(vrule)) {
> +			ret = PTR_ERR(vrule);
> +			break;
>  		}
> -		/* Read data from VCAP */
> -		ret = vcap_read_rule(ri);
> -		if (ret)
> -			goto err_free_rule;
> +
>  		out->prf(out->dst, "\n");
> -		vcap_show_admin_rule(vctrl, admin, out, ri);
> -		vcap_free_rule((struct vcap_rule *)ri);
> +		vcap_show_admin_rule(vctrl, admin, out, to_intrule(vrule));
> +		vcap_free_rule(vrule);
>  	}
> -	mutex_unlock(&admin->lock);
> -	return 0;
> -
> -err_free_rule:
> -	vcap_free_rule((struct vcap_rule *)ri);
> -err_unlock:
> -	mutex_unlock(&admin->lock);
>  	return ret;
>  }

Re: [PATCH net-next v3 1/4] net: microchip: vcap: Add vcap_get_rule

[Horatiu Vultur]

The 12/06/2022 13:31, Paolo Abeni wrote:
> 
> Hello,

Hi Paolo,

> 
> On Sat, 2022-12-03 at 11:43 +0100, Horatiu Vultur wrote:
> > @@ -632,32 +264,22 @@ static int vcap_show_admin(struct vcap_control *vctrl,
> >                          struct vcap_admin *admin,
> >                          struct vcap_output_print *out)
> >  {
> > -     struct vcap_rule_internal *elem, *ri;
> > +     struct vcap_rule_internal *elem;
> > +     struct vcap_rule *vrule;
> >       int ret = 0;
> >
> >       vcap_show_admin_info(vctrl, admin, out);
> > -     mutex_lock(&admin->lock);
> >       list_for_each_entry(elem, &admin->rules, list) {
> 
> Not strictly related to this patch, as the patter is AFAICS already
> there in other places, but I'd like to understand the admin->rules
> locking schema.

According to the commit message that introduced this lock [0] and the
comment to the lock, this lock is used to protect the access to the
admin->rules, admin->enabled and the caches which means the access to
the HW (to read/write the rules).
> 
> It looks like addition/removal are protected by admin->lock, but
> traversal is usually lockless, which in turn looks buggy ?!?

Thanks for the observation! You are correct, there seems to be some bugs
regarding the usage of this lock. We will look over this and will send a
patch to fix this.

> 
> Note: as this patch does not introduce the mentioned behavior, I'm not
> going to block the series for the above.

[0] 71c9de995260 ("net: microchip: sparx5: Add VCAP locking to protect rules")
> 
> Thanks,
> 
> Paolo
> > -             ri = vcap_dup_rule(elem);
> > -             if (IS_ERR(ri)) {
> > -                     ret = PTR_ERR(ri);
> > -                     goto err_unlock;
> > +             vrule = vcap_get_rule(vctrl, elem->data.id);
> > +             if (IS_ERR_OR_NULL(vrule)) {
> > +                     ret = PTR_ERR(vrule);
> > +                     break;
> >               }
> > -             /* Read data from VCAP */
> > -             ret = vcap_read_rule(ri);
> > -             if (ret)
> > -                     goto err_free_rule;
> > +
> >               out->prf(out->dst, "\n");
> > -             vcap_show_admin_rule(vctrl, admin, out, ri);
> > -             vcap_free_rule((struct vcap_rule *)ri);
> > +             vcap_show_admin_rule(vctrl, admin, out, to_intrule(vrule));
> > +             vcap_free_rule(vrule);
> >       }
> > -     mutex_unlock(&admin->lock);
> > -     return 0;
> > -
> > -err_free_rule:
> > -     vcap_free_rule((struct vcap_rule *)ri);
> > -err_unlock:
> > -     mutex_unlock(&admin->lock);
> >       return ret;
> >  }
> 

-- 
/Horatiu

[PATCH net-next v3 2/4] net: microchip: vcap: Add vcap_mod_rule

[Horatiu Vultur]

Add the function vcap_mod_rule which allows to update an existing rule
in the vcap. It is required for the rule to exist in the vcap to be able
to modify it.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 .../net/ethernet/microchip/vcap/vcap_api.c    | 36 +++++++++++++++++++
 .../ethernet/microchip/vcap/vcap_api_client.h |  2 ++
 2 files changed, 38 insertions(+)

diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api.c b/drivers/net/ethernet/microchip/vcap/vcap_api.c
index 27128313f15f1..eae4e9fe0e147 100644
--- a/drivers/net/ethernet/microchip/vcap/vcap_api.c
+++ b/drivers/net/ethernet/microchip/vcap/vcap_api.c
@@ -1955,6 +1955,42 @@ struct vcap_rule *vcap_get_rule(struct vcap_control *vctrl, u32 id)
 }
 EXPORT_SYMBOL_GPL(vcap_get_rule);
 
+/* Update existing rule */
+int vcap_mod_rule(struct vcap_rule *rule)
+{
+	struct vcap_rule_internal *ri = to_intrule(rule);
+	struct vcap_counter ctr;
+	int err;
+
+	err = vcap_api_check(ri->vctrl);
+	if (err)
+		return err;
+
+	if (!vcap_lookup_rule(ri->vctrl, ri->data.id))
+		return -ENOENT;
+
+	mutex_lock(&ri->admin->lock);
+	/* Encode the bitstreams to the VCAP cache */
+	vcap_erase_cache(ri);
+	err = vcap_encode_rule(ri);
+	if (err)
+		goto out;
+
+	err = vcap_write_rule(ri);
+	if (err)
+		goto out;
+
+	memset(&ctr, 0, sizeof(ctr));
+	err =  vcap_write_counter(ri, &ctr);
+	if (err)
+		goto out;
+
+out:
+	mutex_unlock(&ri->admin->lock);
+	return err;
+}
+EXPORT_SYMBOL_GPL(vcap_mod_rule);
+
 /* Return the alignment offset for a new rule address */
 static int vcap_valid_rule_move(struct vcap_rule_internal *el, int offset)
 {
diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api_client.h b/drivers/net/ethernet/microchip/vcap/vcap_api_client.h
index a354dcd741e22..fdfc5d58813bb 100644
--- a/drivers/net/ethernet/microchip/vcap/vcap_api_client.h
+++ b/drivers/net/ethernet/microchip/vcap/vcap_api_client.h
@@ -172,6 +172,8 @@ int vcap_del_rule(struct vcap_control *vctrl, struct net_device *ndev, u32 id);
 struct vcap_rule *vcap_copy_rule(struct vcap_rule *rule);
 /* Get rule from a VCAP instance */
 struct vcap_rule *vcap_get_rule(struct vcap_control *vctrl, u32 id);
+/* Update existing rule */
+int vcap_mod_rule(struct vcap_rule *rule);
 
 /* Update the keyset for the rule */
 int vcap_set_rule_set_keyset(struct vcap_rule *rule,
-- 
2.38.0

[PATCH net-next v3 3/4] net: microchip: vcap: Add vcap_rule_get_key_u32

[Horatiu Vultur]

Add the function vcap_rule_get_key_u32 which allows to get the value and
the mask of a key that exist on the rule. If the key doesn't exist,
it would return error.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 drivers/net/ethernet/microchip/vcap/vcap_api.c   | 16 ++++++++++++++++
 .../ethernet/microchip/vcap/vcap_api_client.h    |  4 ++++
 2 files changed, 20 insertions(+)

diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api.c b/drivers/net/ethernet/microchip/vcap/vcap_api.c
index eae4e9fe0e147..05e915ea858d6 100644
--- a/drivers/net/ethernet/microchip/vcap/vcap_api.c
+++ b/drivers/net/ethernet/microchip/vcap/vcap_api.c
@@ -2338,6 +2338,22 @@ int vcap_rule_add_key_u128(struct vcap_rule *rule, enum vcap_key_field key,
 }
 EXPORT_SYMBOL_GPL(vcap_rule_add_key_u128);
 
+int vcap_rule_get_key_u32(struct vcap_rule *rule, enum vcap_key_field key,
+			  u32 *value, u32 *mask)
+{
+	struct vcap_client_keyfield *ckf;
+
+	ckf = vcap_find_keyfield(rule, key);
+	if (!ckf)
+		return -ENOENT;
+
+	*value = ckf->data.u32.value;
+	*mask = ckf->data.u32.mask;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(vcap_rule_get_key_u32);
+
 /* Find a client action field in a rule */
 static struct vcap_client_actionfield *
 vcap_find_actionfield(struct vcap_rule *rule, enum vcap_action_field act)
diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api_client.h b/drivers/net/ethernet/microchip/vcap/vcap_api_client.h
index fdfc5d58813bb..0319866f9c94d 100644
--- a/drivers/net/ethernet/microchip/vcap/vcap_api_client.h
+++ b/drivers/net/ethernet/microchip/vcap/vcap_api_client.h
@@ -258,4 +258,8 @@ int vcap_rule_mod_action_u32(struct vcap_rule *rule,
 			     enum vcap_action_field action,
 			     u32 value);
 
+/* Get a 32 bit key field value and mask from the rule */
+int vcap_rule_get_key_u32(struct vcap_rule *rule, enum vcap_key_field key,
+			  u32 *value, u32 *mask);
+
 #endif /* __VCAP_API_CLIENT__ */
-- 
2.38.0

[PATCH net-next v3 4/4] net: lan966x: Add ptp trap rules

[Horatiu Vultur]

Currently lan966x, doesn't allow to run PTP over interfaces that are
part of the bridge. The reason is when the lan966x was receiving a
PTP frame (regardless if L2/IPv4/IPv6) the HW it would flood this
frame.
Now that it is possible to add VCAP rules to the HW, such to trap these
frames to the CPU, it is possible to run PTP also over interfaces that
are part of the bridge.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 .../ethernet/microchip/lan966x/lan966x_main.c |  19 +-
 .../ethernet/microchip/lan966x/lan966x_main.h |  14 ++
 .../ethernet/microchip/lan966x/lan966x_ptp.c  | 236 +++++++++++++++++-
 .../microchip/lan966x/lan966x_tc_flower.c     |   8 -
 .../microchip/lan966x/lan966x_vcap_impl.c     |  11 +-
 5 files changed, 265 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
index f6092983d0281..cadde20505ba0 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
@@ -443,11 +443,22 @@ static int lan966x_port_ioctl(struct net_device *dev, struct ifreq *ifr,
 			      int cmd)
 {
 	struct lan966x_port *port = netdev_priv(dev);
+	int err;
+
+	if (cmd == SIOCSHWTSTAMP) {
+		err = lan966x_ptp_setup_traps(port, ifr);
+		if (err)
+			return err;
+	}
 
 	if (!phy_has_hwtstamp(dev->phydev) && port->lan966x->ptp) {
 		switch (cmd) {
 		case SIOCSHWTSTAMP:
-			return lan966x_ptp_hwtstamp_set(port, ifr);
+			err = lan966x_ptp_hwtstamp_set(port, ifr);
+			if (err)
+				lan966x_ptp_del_traps(port);
+
+			return err;
 		case SIOCGHWTSTAMP:
 			return lan966x_ptp_hwtstamp_get(port, ifr);
 		}
@@ -456,7 +467,11 @@ static int lan966x_port_ioctl(struct net_device *dev, struct ifreq *ifr,
 	if (!dev->phydev)
 		return -ENODEV;
 
-	return phy_mii_ioctl(dev->phydev, ifr, cmd);
+	err = phy_mii_ioctl(dev->phydev, ifr, cmd);
+	if (err && cmd == SIOCSHWTSTAMP)
+		lan966x_ptp_del_traps(port);
+
+	return err;
 }
 
 static const struct net_device_ops lan966x_port_netdev_ops = {
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
index f2e45da7ffd4f..3491f19618358 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
@@ -88,6 +88,10 @@
 #define SE_IDX_QUEUE			0  /* 0-79 : Queue scheduler elements */
 #define SE_IDX_PORT			80 /* 80-89 : Port schedular elements */
 
+#define LAN966X_VCAP_CID_IS2_L0 VCAP_CID_INGRESS_STAGE2_L0 /* IS2 lookup 0 */
+#define LAN966X_VCAP_CID_IS2_L1 VCAP_CID_INGRESS_STAGE2_L1 /* IS2 lookup 1 */
+#define LAN966X_VCAP_CID_IS2_MAX (VCAP_CID_INGRESS_STAGE2_L2 - 1) /* IS2 Max */
+
 /* MAC table entry types.
  * ENTRYTYPE_NORMAL is subject to aging.
  * ENTRYTYPE_LOCKED is not subject to aging.
@@ -116,6 +120,14 @@ enum lan966x_fdma_action {
 	FDMA_REDIRECT,
 };
 
+/* Controls how PORT_MASK is applied */
+enum LAN966X_PORT_MASK_MODE {
+	LAN966X_PMM_NO_ACTION,
+	LAN966X_PMM_REPLACE,
+	LAN966X_PMM_FORWARDING,
+	LAN966X_PMM_REDIRECT,
+};
+
 struct lan966x_port;
 
 struct lan966x_db {
@@ -473,6 +485,8 @@ irqreturn_t lan966x_ptp_irq_handler(int irq, void *args);
 irqreturn_t lan966x_ptp_ext_irq_handler(int irq, void *args);
 u32 lan966x_ptp_get_period_ps(void);
 int lan966x_ptp_gettime64(struct ptp_clock_info *ptp, struct timespec64 *ts);
+int lan966x_ptp_setup_traps(struct lan966x_port *port, struct ifreq *ifr);
+int lan966x_ptp_del_traps(struct lan966x_port *port);
 
 int lan966x_fdma_xmit(struct sk_buff *skb, __be32 *ifh, struct net_device *dev);
 int lan966x_fdma_xmit_xdpf(struct lan966x_port *port,
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c b/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c
index e5a2bbe064f8f..300fe40059191 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c
@@ -3,6 +3,8 @@
 #include <linux/ptp_classify.h>
 
 #include "lan966x_main.h"
+#include "vcap_api.h"
+#include "vcap_api_client.h"
 
 #define LAN966X_MAX_PTP_ID	512
 
@@ -18,6 +20,17 @@
 
 #define TOD_ACC_PIN		0x7
 
+/* This represents the base rule ID for the PTP rules that are added in the
+ * VCAP to trap frames to CPU. This number needs to be bigger than the maximum
+ * number of entries that can exist in the VCAP.
+ */
+#define LAN966X_VCAP_PTP_RULE_ID	1000000
+#define LAN966X_VCAP_L2_PTP_TRAP	(LAN966X_VCAP_PTP_RULE_ID + 0)
+#define LAN966X_VCAP_IPV4_EV_PTP_TRAP	(LAN966X_VCAP_PTP_RULE_ID + 1)
+#define LAN966X_VCAP_IPV4_GEN_PTP_TRAP	(LAN966X_VCAP_PTP_RULE_ID + 2)
+#define LAN966X_VCAP_IPV6_EV_PTP_TRAP	(LAN966X_VCAP_PTP_RULE_ID + 3)
+#define LAN966X_VCAP_IPV6_GEN_PTP_TRAP	(LAN966X_VCAP_PTP_RULE_ID + 4)
+
 enum {
 	PTP_PIN_ACTION_IDLE = 0,
 	PTP_PIN_ACTION_LOAD,
@@ -35,19 +48,228 @@ static u64 lan966x_ptp_get_nominal_value(void)
 	return 0x304d4873ecade305;
 }
 
+static int lan966x_ptp_add_trap(struct lan966x_port *port,
+				int (*add_ptp_key)(struct vcap_rule *vrule,
+						   struct lan966x_port*),
+				u32 rule_id,
+				u16 proto)
+{
+	struct lan966x *lan966x = port->lan966x;
+	struct vcap_rule *vrule;
+	int err;
+
+	vrule = vcap_get_rule(lan966x->vcap_ctrl, rule_id);
+	if (vrule) {
+		u32 value, mask;
+
+		/* Just modify the ingress port mask and exit */
+		vcap_rule_get_key_u32(vrule, VCAP_KF_IF_IGR_PORT_MASK,
+				      &value, &mask);
+		mask &= ~BIT(port->chip_port);
+		vcap_rule_mod_key_u32(vrule, VCAP_KF_IF_IGR_PORT_MASK,
+				      value, mask);
+
+		err = vcap_mod_rule(vrule);
+		goto free_rule;
+	}
+
+	vrule = vcap_alloc_rule(lan966x->vcap_ctrl, port->dev,
+				LAN966X_VCAP_CID_IS2_L0,
+				VCAP_USER_PTP, 0, rule_id);
+	if (!vrule)
+		return -ENOMEM;
+	if (IS_ERR(vrule))
+		return PTR_ERR(vrule);
+
+	err = add_ptp_key(vrule, port);
+	if (err)
+		goto free_rule;
+
+	err = vcap_set_rule_set_actionset(vrule, VCAP_AFS_BASE_TYPE);
+	err |= vcap_rule_add_action_bit(vrule, VCAP_AF_CPU_COPY_ENA, VCAP_BIT_1);
+	err |= vcap_rule_add_action_u32(vrule, VCAP_AF_MASK_MODE, LAN966X_PMM_REPLACE);
+	err |= vcap_val_rule(vrule, proto);
+	if (err)
+		goto free_rule;
+
+	err = vcap_add_rule(vrule);
+
+free_rule:
+	/* Free the local copy of the rule */
+	vcap_free_rule(vrule);
+	return err;
+}
+
+static int lan966x_ptp_del_trap(struct lan966x_port *port,
+				u32 rule_id)
+{
+	struct lan966x *lan966x = port->lan966x;
+	struct vcap_rule *vrule;
+	u32 value, mask;
+	int err;
+
+	vrule = vcap_get_rule(lan966x->vcap_ctrl, rule_id);
+	if (!vrule)
+		return -EEXIST;
+
+	vcap_rule_get_key_u32(vrule, VCAP_KF_IF_IGR_PORT_MASK, &value, &mask);
+	mask |= BIT(port->chip_port);
+
+	/* No other port requires this trap, so it is safe to remove it */
+	if (mask == GENMASK(lan966x->num_phys_ports, 0)) {
+		err = vcap_del_rule(lan966x->vcap_ctrl, port->dev, rule_id);
+		goto free_rule;
+	}
+
+	vcap_rule_mod_key_u32(vrule, VCAP_KF_IF_IGR_PORT_MASK, value, mask);
+	err = vcap_mod_rule(vrule);
+
+free_rule:
+	vcap_free_rule(vrule);
+	return err;
+}
+
+static int lan966x_ptp_add_l2_key(struct vcap_rule *vrule,
+				  struct lan966x_port *port)
+{
+	return vcap_rule_add_key_u32(vrule, VCAP_KF_ETYPE, ETH_P_1588, ~0);
+}
+
+static int lan966x_ptp_add_ip_event_key(struct vcap_rule *vrule,
+					struct lan966x_port *port)
+{
+	return vcap_rule_add_key_u32(vrule, VCAP_KF_L4_DPORT, PTP_EV_PORT, ~0) ||
+	       vcap_rule_add_key_bit(vrule, VCAP_KF_TCP_IS, VCAP_BIT_0);
+}
+
+static int lan966x_ptp_add_ip_general_key(struct vcap_rule *vrule,
+					  struct lan966x_port *port)
+{
+	return vcap_rule_add_key_u32(vrule, VCAP_KF_L4_DPORT, PTP_GEN_PORT, ~0) ||
+	       vcap_rule_add_key_bit(vrule, VCAP_KF_TCP_IS, VCAP_BIT_0);
+}
+
+static int lan966x_ptp_add_l2_rule(struct lan966x_port *port)
+{
+	return lan966x_ptp_add_trap(port, lan966x_ptp_add_l2_key,
+				    LAN966X_VCAP_L2_PTP_TRAP, ETH_P_ALL);
+}
+
+static int lan966x_ptp_add_ipv4_rules(struct lan966x_port *port)
+{
+	int err;
+
+	err = lan966x_ptp_add_trap(port, lan966x_ptp_add_ip_event_key,
+				   LAN966X_VCAP_IPV4_EV_PTP_TRAP, ETH_P_IP);
+	if (err)
+		return err;
+
+	err = lan966x_ptp_add_trap(port, lan966x_ptp_add_ip_general_key,
+				   LAN966X_VCAP_IPV4_GEN_PTP_TRAP, ETH_P_IP);
+	if (err)
+		lan966x_ptp_del_trap(port, LAN966X_VCAP_IPV4_EV_PTP_TRAP);
+
+	return err;
+}
+
+static int lan966x_ptp_add_ipv6_rules(struct lan966x_port *port)
+{
+	int err;
+
+	err = lan966x_ptp_add_trap(port, lan966x_ptp_add_ip_event_key,
+				   LAN966X_VCAP_IPV6_EV_PTP_TRAP, ETH_P_IPV6);
+	if (err)
+		return err;
+
+	err = lan966x_ptp_add_trap(port, lan966x_ptp_add_ip_general_key,
+				   LAN966X_VCAP_IPV6_GEN_PTP_TRAP, ETH_P_IPV6);
+	if (err)
+		lan966x_ptp_del_trap(port, LAN966X_VCAP_IPV6_EV_PTP_TRAP);
+
+	return err;
+}
+
+static int lan966x_ptp_del_l2_rule(struct lan966x_port *port)
+{
+	return lan966x_ptp_del_trap(port, LAN966X_VCAP_L2_PTP_TRAP);
+}
+
+static int lan966x_ptp_del_ipv4_rules(struct lan966x_port *port)
+{
+	int err;
+
+	err = lan966x_ptp_del_trap(port, LAN966X_VCAP_IPV4_EV_PTP_TRAP);
+	err |= lan966x_ptp_del_trap(port, LAN966X_VCAP_IPV4_GEN_PTP_TRAP);
+
+	return err;
+}
+
+static int lan966x_ptp_del_ipv6_rules(struct lan966x_port *port)
+{
+	int err;
+
+	err = lan966x_ptp_del_trap(port, LAN966X_VCAP_IPV6_EV_PTP_TRAP);
+	err |= lan966x_ptp_del_trap(port, LAN966X_VCAP_IPV6_GEN_PTP_TRAP);
+
+	return err;
+}
+
+static int lan966x_ptp_add_traps(struct lan966x_port *port)
+{
+	int err;
+
+	err = lan966x_ptp_add_l2_rule(port);
+	if (err)
+		goto err_l2;
+
+	err = lan966x_ptp_add_ipv4_rules(port);
+	if (err)
+		goto err_ipv4;
+
+	err = lan966x_ptp_add_ipv6_rules(port);
+	if (err)
+		goto err_ipv6;
+
+	return err;
+
+err_ipv6:
+	lan966x_ptp_del_ipv4_rules(port);
+err_ipv4:
+	lan966x_ptp_del_l2_rule(port);
+err_l2:
+	return err;
+}
+
+int lan966x_ptp_del_traps(struct lan966x_port *port)
+{
+	int err;
+
+	err = lan966x_ptp_del_l2_rule(port);
+	err |= lan966x_ptp_del_ipv4_rules(port);
+	err |= lan966x_ptp_del_ipv6_rules(port);
+
+	return err;
+}
+
+int lan966x_ptp_setup_traps(struct lan966x_port *port, struct ifreq *ifr)
+{
+	struct hwtstamp_config cfg;
+
+	if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
+		return -EFAULT;
+
+	if (cfg.rx_filter == HWTSTAMP_FILTER_NONE)
+		return lan966x_ptp_del_traps(port);
+	else
+		return lan966x_ptp_add_traps(port);
+}
+
 int lan966x_ptp_hwtstamp_set(struct lan966x_port *port, struct ifreq *ifr)
 {
 	struct lan966x *lan966x = port->lan966x;
 	struct hwtstamp_config cfg;
 	struct lan966x_phc *phc;
 
-	/* For now don't allow to run ptp on ports that are part of a bridge,
-	 * because in case of transparent clock the HW will still forward the
-	 * frames, so there would be duplicate frames
-	 */
-	if (lan966x->bridge_mask & BIT(port->chip_port))
-		return -EINVAL;
-
 	if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
 		return -EFAULT;
 
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.c b/drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.c
index 04a2afd683cca..ba3fa917d6b78 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.c
@@ -4,14 +4,6 @@
 #include "vcap_api.h"
 #include "vcap_api_client.h"
 
-/* Controls how PORT_MASK is applied */
-enum LAN966X_PORT_MASK_MODE {
-	LAN966X_PMM_NO_ACTION,
-	LAN966X_PMM_REPLACE,
-	LAN966X_PMM_FORWARDING,
-	LAN966X_PMM_REDIRECT,
-};
-
 struct lan966x_tc_flower_parse_usage {
 	struct flow_cls_offload *f;
 	struct flow_rule *frule;
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c b/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c
index 44f40d9149470..d8dc9fbb81e1a 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c
@@ -5,10 +5,6 @@
 #include "vcap_api.h"
 #include "vcap_api_client.h"
 
-#define LAN966X_VCAP_CID_IS2_L0 VCAP_CID_INGRESS_STAGE2_L0 /* IS2 lookup 0 */
-#define LAN966X_VCAP_CID_IS2_L1 VCAP_CID_INGRESS_STAGE2_L1 /* IS2 lookup 1 */
-#define LAN966X_VCAP_CID_IS2_MAX (VCAP_CID_INGRESS_STAGE2_L2 - 1) /* IS2 Max */
-
 #define STREAMSIZE (64 * 4)
 
 #define LAN966X_IS2_LOOKUPS 2
@@ -219,9 +215,12 @@ static void lan966x_vcap_add_default_fields(struct net_device *dev,
 					    struct vcap_rule *rule)
 {
 	struct lan966x_port *port = netdev_priv(dev);
+	u32 value, mask;
 
-	vcap_rule_add_key_u32(rule, VCAP_KF_IF_IGR_PORT_MASK, 0,
-			      ~BIT(port->chip_port));
+	if (vcap_rule_get_key_u32(rule, VCAP_KF_IF_IGR_PORT_MASK,
+				  &value, &mask))
+		vcap_rule_add_key_u32(rule, VCAP_KF_IF_IGR_PORT_MASK, 0,
+				      ~BIT(port->chip_port));
 
 	if (lan966x_vcap_is_first_chain(rule))
 		vcap_rule_add_key_bit(rule, VCAP_KF_LOOKUP_FIRST_IS,
-- 
2.38.0

Re: [PATCH net-next v3 4/4] net: lan966x: Add ptp trap rules

[Michael Walle]

Hi Horatiu,

> Currently lan966x, doesn't allow to run PTP over interfaces that are
> part of the bridge. The reason is when the lan966x was receiving a
> PTP frame (regardless if L2/IPv4/IPv6) the HW it would flood this
> frame.
> Now that it is possible to add VCAP rules to the HW, such to trap these
> frames to the CPU, it is possible to run PTP also over interfaces that
> are part of the bridge.

This gives me:

# /etc/init.d/S65ptp4l start
Starting linuxptp daemon: OK
[   44.136870] vcap_val_rule:1678: keyset was not updated: -22
[   44.140196] vcap_val_rule:1678: keyset was not updated: -22
#

# ptp4l -v
3.1.1
# uname -a
Linux buildroot 6.1.0-rc8-next-20221208+ #924 SMP Thu Dec  8 10:08:58 CET 2022 armv7l GNU/Linux

I don't know whats going on, but I'm happy to help with debugging with some
guidance.

-michael

Re: [PATCH net-next v3 4/4] net: lan966x: Add ptp trap rules

[Michael Walle]

Am 2022-12-08 10:25, schrieb Michael Walle:
> Hi Horatiu,
> 
>> Currently lan966x, doesn't allow to run PTP over interfaces that are
>> part of the bridge. The reason is when the lan966x was receiving a
>> PTP frame (regardless if L2/IPv4/IPv6) the HW it would flood this
>> frame.
>> Now that it is possible to add VCAP rules to the HW, such to trap 
>> these
>> frames to the CPU, it is possible to run PTP also over interfaces that
>> are part of the bridge.
> 
> This gives me:
> 
> # /etc/init.d/S65ptp4l start
> Starting linuxptp daemon: OK
> [   44.136870] vcap_val_rule:1678: keyset was not updated: -22
> [   44.140196] vcap_val_rule:1678: keyset was not updated: -22
> #
> 
> # ptp4l -v
> 3.1.1
> # uname -a
> Linux buildroot 6.1.0-rc8-next-20221208+ #924 SMP Thu Dec  8 10:08:58
> CET 2022 armv7l GNU/Linux
> 
> I don't know whats going on, but I'm happy to help with debugging with 
> some
> guidance.

Oh, and linuxptp is running on eth0, no bridges are set up. linuxptp
is started with "/usr/sbin/ptp4l -f /etc/linuxptp.cfg"

# cat /etc/linuxptp.cfg
# LinuxPTP configuration file for synchronizing the system clock to
# a remote PTP master in slave-only mode.
#
# By default synchronize time in slave-only mode using UDP and hardware 
time
# stamps on eth0. If the difference to master is >1.0 second correct by
# stepping the clock instead of adjusting the frequency.
#
# If you change the configuration don't forget to update the phc2sys
# parameters accordingly in linuxptp-system-clock.service (systemd)
# or the linuxptp SysV init script.

[global]
slaveOnly		1
delay_mechanism		Auto
network_transport	UDPv4
time_stamping		hardware
step_threshold		1.0

[eth0]

-michael

Re: [PATCH net-next v3 4/4] net: lan966x: Add ptp trap rules

[Horatiu Vultur]

The 12/08/2022 10:27, Michael Walle wrote:
> 
> Am 2022-12-08 10:25, schrieb Michael Walle:
> > Hi Horatiu,

Hi Michael,

> > 
> > > Currently lan966x, doesn't allow to run PTP over interfaces that are
> > > part of the bridge. The reason is when the lan966x was receiving a
> > > PTP frame (regardless if L2/IPv4/IPv6) the HW it would flood this
> > > frame.
> > > Now that it is possible to add VCAP rules to the HW, such to trap
> > > these
> > > frames to the CPU, it is possible to run PTP also over interfaces that
> > > are part of the bridge.
> > 
> > This gives me:
> > 
> > # /etc/init.d/S65ptp4l start
> > Starting linuxptp daemon: OK
> > [   44.136870] vcap_val_rule:1678: keyset was not updated: -22
> > [   44.140196] vcap_val_rule:1678: keyset was not updated: -22
> > #
> > 
> > # ptp4l -v
> > 3.1.1
> > # uname -a
> > Linux buildroot 6.1.0-rc8-next-20221208+ #924 SMP Thu Dec  8 10:08:58
> > CET 2022 armv7l GNU/Linux
> > 
> > I don't know whats going on, but I'm happy to help with debugging with
> > some
> > guidance.
> 
> Oh, and linuxptp is running on eth0, no bridges are set up. linuxptp
> is started with "/usr/sbin/ptp4l -f /etc/linuxptp.cfg"
> 
> # cat /etc/linuxptp.cfg
> # LinuxPTP configuration file for synchronizing the system clock to
> # a remote PTP master in slave-only mode.
> #
> # By default synchronize time in slave-only mode using UDP and hardware
> time
> # stamps on eth0. If the difference to master is >1.0 second correct by
> # stepping the clock instead of adjusting the frequency.
> #
> # If you change the configuration don't forget to update the phc2sys
> # parameters accordingly in linuxptp-system-clock.service (systemd)
> # or the linuxptp SysV init script.
> 
> [global]
> slaveOnly               1
> delay_mechanism         Auto
> network_transport       UDPv4
> time_stamping           hardware
> step_threshold          1.0
> 
> [eth0]

Thanks for trying this!

The issue is because you have not enabled the TCAM lookups per
port. They can be enabled using this commands:

tc qdisc add dev eth0 clsact
tc filter add dev eth0 ingress prio 5 handle 5 matchall skip_sw action goto chain 8000000

This will enable the lookup and then you should be able to start again
the ptp4l. Sorry for not mention this, at least I should have written it
somewhere that this is required.

I was not sure if lan966x should or not enable tcam lookups
automatically when a ptp trap action is added. I am open to suggestion
here.

> 
> -michael

-- 
/Horatiu

Re: [PATCH net-next v3 4/4] net: lan966x: Add ptp trap rules

[Michael Walle]

Hi Horatiu,

Am 2022-12-08 14:04, schrieb Horatiu Vultur:
>> > > Currently lan966x, doesn't allow to run PTP over interfaces that are
>> > > part of the bridge. The reason is when the lan966x was receiving a
>> > > PTP frame (regardless if L2/IPv4/IPv6) the HW it would flood this
>> > > frame.
>> > > Now that it is possible to add VCAP rules to the HW, such to trap
>> > > these
>> > > frames to the CPU, it is possible to run PTP also over interfaces that
>> > > are part of the bridge.
>> >
>> > This gives me:
>> >
>> > # /etc/init.d/S65ptp4l start
>> > Starting linuxptp daemon: OK
>> > [   44.136870] vcap_val_rule:1678: keyset was not updated: -22
>> > [   44.140196] vcap_val_rule:1678: keyset was not updated: -22
>> > #
>> >
>> > # ptp4l -v
>> > 3.1.1
>> > # uname -a
>> > Linux buildroot 6.1.0-rc8-next-20221208+ #924 SMP Thu Dec  8 10:08:58
>> > CET 2022 armv7l GNU/Linux
>> >
>> > I don't know whats going on, but I'm happy to help with debugging with
>> > some
>> > guidance.
>> 
>> Oh, and linuxptp is running on eth0, no bridges are set up. linuxptp
>> is started with "/usr/sbin/ptp4l -f /etc/linuxptp.cfg"
>> 
>> # cat /etc/linuxptp.cfg
>> # LinuxPTP configuration file for synchronizing the system clock to
>> # a remote PTP master in slave-only mode.
>> #
>> # By default synchronize time in slave-only mode using UDP and 
>> hardware
>> time
>> # stamps on eth0. If the difference to master is >1.0 second correct 
>> by
>> # stepping the clock instead of adjusting the frequency.
>> #
>> # If you change the configuration don't forget to update the phc2sys
>> # parameters accordingly in linuxptp-system-clock.service (systemd)
>> # or the linuxptp SysV init script.
>> 
>> [global]
>> slaveOnly               1
>> delay_mechanism         Auto
>> network_transport       UDPv4
>> time_stamping           hardware
>> step_threshold          1.0
>> 
>> [eth0]
> 
> Thanks for trying this!

Actually I was just booting my board which happens to have linuxptp
started by default. And the error messages were new. But I'm not so
sure anymore if PTP was really working. I'm still puzzled by reading
your commit message. Was it already working for interfaces which aren't
part of a bridge and this commit will make it work even for interfaces
which are part of a bridge?

> The issue is because you have not enabled the TCAM lookups per
> port. They can be enabled using this commands:
> 
> tc qdisc add dev eth0 clsact

This gives me the following error, might be a missing kconfig option:

# tc qdisc add dev eth0 clsact
RTNETLINK answers: Operation not supported

> tc filter add dev eth0 ingress prio 5 handle 5 matchall skip_sw action
> goto chain 8000000
> 
> This will enable the lookup and then you should be able to start again
> the ptp4l. Sorry for not mention this, at least I should have written 
> it
> somewhere that this is required.
> 
> I was not sure if lan966x should or not enable tcam lookups
> automatically when a ptp trap action is added. I am open to suggestion
> here.

IMHO, from a user point of view this should just work. For a user
there is no connection between running linuxptp and some filtering
stuff with 'tc'.

Also, if the answer to my question above is yes, and ptp should
have worked on eth0 before, this is a regression then.

-michael

Re: [PATCH net-next v3 4/4] net: lan966x: Add ptp trap rules

[Horatiu Vultur]

The 12/08/2022 14:18, Michael Walle wrote:
> 
> Hi Horatiu,

Hi Michael,

> 
> Am 2022-12-08 14:04, schrieb Horatiu Vultur:
> > > > > Currently lan966x, doesn't allow to run PTP over interfaces that are
> > > > > part of the bridge. The reason is when the lan966x was receiving a
> > > > > PTP frame (regardless if L2/IPv4/IPv6) the HW it would flood this
> > > > > frame.
> > > > > Now that it is possible to add VCAP rules to the HW, such to trap
> > > > > these
> > > > > frames to the CPU, it is possible to run PTP also over interfaces that
> > > > > are part of the bridge.
> > > >
> > > > This gives me:
> > > >
> > > > # /etc/init.d/S65ptp4l start
> > > > Starting linuxptp daemon: OK
> > > > [   44.136870] vcap_val_rule:1678: keyset was not updated: -22
> > > > [   44.140196] vcap_val_rule:1678: keyset was not updated: -22
> > > > #
> > > >
> > > > # ptp4l -v
> > > > 3.1.1
> > > > # uname -a
> > > > Linux buildroot 6.1.0-rc8-next-20221208+ #924 SMP Thu Dec  8 10:08:58
> > > > CET 2022 armv7l GNU/Linux
> > > >
> > > > I don't know whats going on, but I'm happy to help with debugging with
> > > > some
> > > > guidance.
> > > 
> > > Oh, and linuxptp is running on eth0, no bridges are set up. linuxptp
> > > is started with "/usr/sbin/ptp4l -f /etc/linuxptp.cfg"
> > > 
> > > # cat /etc/linuxptp.cfg
> > > # LinuxPTP configuration file for synchronizing the system clock to
> > > # a remote PTP master in slave-only mode.
> > > #
> > > # By default synchronize time in slave-only mode using UDP and
> > > hardware
> > > time
> > > # stamps on eth0. If the difference to master is >1.0 second correct
> > > by
> > > # stepping the clock instead of adjusting the frequency.
> > > #
> > > # If you change the configuration don't forget to update the phc2sys
> > > # parameters accordingly in linuxptp-system-clock.service (systemd)
> > > # or the linuxptp SysV init script.
> > > 
> > > [global]
> > > slaveOnly               1
> > > delay_mechanism         Auto
> > > network_transport       UDPv4
> > > time_stamping           hardware
> > > step_threshold          1.0
> > > 
> > > [eth0]
> > 
> > Thanks for trying this!
> 
> Actually I was just booting my board which happens to have linuxptp
> started by default. And the error messages were new. But I'm not so
> sure anymore if PTP was really working. I'm still puzzled by reading
> your commit message. Was it already working for interfaces which aren't
> part of a bridge and this commit will make it work even for interfaces
> which are part of a bridge?

Exactly!
This worked on interfaces that were not part of the bridge. And with
this commit will make it work even on interfaces that are part of the
bridge.

> 
> > The issue is because you have not enabled the TCAM lookups per
> > port. They can be enabled using this commands:
> > 
> > tc qdisc add dev eth0 clsact
> 
> This gives me the following error, might be a missing kconfig option:
> 
> # tc qdisc add dev eth0 clsact
> RTNETLINK answers: Operation not supported

Yes that should be the case, I think you are missing:
CONFIG_NET_SCHED
But may be others when you try to add the next rule.

> 
> > tc filter add dev eth0 ingress prio 5 handle 5 matchall skip_sw action
> > goto chain 8000000
> > 
> > This will enable the lookup and then you should be able to start again
> > the ptp4l. Sorry for not mention this, at least I should have written
> > it
> > somewhere that this is required.
> > 
> > I was not sure if lan966x should or not enable tcam lookups
> > automatically when a ptp trap action is added. I am open to suggestion
> > here.
> 
> IMHO, from a user point of view this should just work. For a user
> there is no connection between running linuxptp and some filtering
> stuff with 'tc'.
> 
> Also, if the answer to my question above is yes, and ptp should
> have worked on eth0 before, this is a regression then.

OK, I can see your point.
With the following diff, you should see the same behaviour as before:
---
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c b/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c
index 904f5a3f636d3..538f4b76cf97a 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c
@@ -91,8 +91,6 @@ lan966x_vcap_is2_get_port_keysets(struct net_device *dev, int lookup,

        /* Check if the port keyset selection is enabled */
        val = lan_rd(lan966x, ANA_VCAP_S2_CFG(port->chip_port));
-       if (!ANA_VCAP_S2_CFG_ENA_GET(val))
-               return -ENOENT;

        /* Collect all keysets for the port in a list */
        if (l3_proto == ETH_P_ALL)
---

> 
> -michael

-- 
/Horatiu

Re: [PATCH net-next v3 4/4] net: lan966x: Add ptp trap rules

[Michael Walle]

Hi,

Am 2022-12-09 10:29, schrieb Horatiu Vultur:
> The 12/08/2022 14:18, Michael Walle wrote:
>> Am 2022-12-08 14:04, schrieb Horatiu Vultur:
>> > > > > Currently lan966x, doesn't allow to run PTP over interfaces that are
>> > > > > part of the bridge. The reason is when the lan966x was receiving a
>> > > > > PTP frame (regardless if L2/IPv4/IPv6) the HW it would flood this
>> > > > > frame.
>> > > > > Now that it is possible to add VCAP rules to the HW, such to trap
>> > > > > these
>> > > > > frames to the CPU, it is possible to run PTP also over interfaces that
>> > > > > are part of the bridge.
>> > > >
>> > > > This gives me:
>> > > >
>> > > > # /etc/init.d/S65ptp4l start
>> > > > Starting linuxptp daemon: OK
>> > > > [   44.136870] vcap_val_rule:1678: keyset was not updated: -22
>> > > > [   44.140196] vcap_val_rule:1678: keyset was not updated: -22
>> > > > #
>> > > >
>> > > > # ptp4l -v
>> > > > 3.1.1
>> > > > # uname -a
>> > > > Linux buildroot 6.1.0-rc8-next-20221208+ #924 SMP Thu Dec  8 10:08:58
>> > > > CET 2022 armv7l GNU/Linux
>> > > >
>> > > > I don't know whats going on, but I'm happy to help with debugging with
>> > > > some
>> > > > guidance.
>> > >
>> > > Oh, and linuxptp is running on eth0, no bridges are set up. linuxptp
>> > > is started with "/usr/sbin/ptp4l -f /etc/linuxptp.cfg"
>> > >
>> > > # cat /etc/linuxptp.cfg
>> > > # LinuxPTP configuration file for synchronizing the system clock to
>> > > # a remote PTP master in slave-only mode.
>> > > #
>> > > # By default synchronize time in slave-only mode using UDP and
>> > > hardware
>> > > time
>> > > # stamps on eth0. If the difference to master is >1.0 second correct
>> > > by
>> > > # stepping the clock instead of adjusting the frequency.
>> > > #
>> > > # If you change the configuration don't forget to update the phc2sys
>> > > # parameters accordingly in linuxptp-system-clock.service (systemd)
>> > > # or the linuxptp SysV init script.
>> > >
>> > > [global]
>> > > slaveOnly               1
>> > > delay_mechanism         Auto
>> > > network_transport       UDPv4
>> > > time_stamping           hardware
>> > > step_threshold          1.0
>> > >
>> > > [eth0]
>> >
>> > Thanks for trying this!
>> 
>> Actually I was just booting my board which happens to have linuxptp
>> started by default. And the error messages were new. But I'm not so
>> sure anymore if PTP was really working. I'm still puzzled by reading
>> your commit message. Was it already working for interfaces which 
>> aren't
>> part of a bridge and this commit will make it work even for interfaces
>> which are part of a bridge?
> 
> Exactly!
> This worked on interfaces that were not part of the bridge. And with
> this commit will make it work even on interfaces that are part of the
> bridge.
> 
>> 
>> > The issue is because you have not enabled the TCAM lookups per
>> > port. They can be enabled using this commands:
>> >
>> > tc qdisc add dev eth0 clsact
>> 
>> This gives me the following error, might be a missing kconfig option:
>> 
>> # tc qdisc add dev eth0 clsact
>> RTNETLINK answers: Operation not supported
> 
> Yes that should be the case, I think you are missing:
> CONFIG_NET_SCHED
> But may be others when you try to add the next rule.

I guess I'd need to update my kernel config sometime. At the
moment I just have a basic one, as there is still so much stuff
missing for the lan9668. So I haven't come around testing anything
else. As I said, I just noticed because my rootfs happens to have
linuxptp started by default.

>> > tc filter add dev eth0 ingress prio 5 handle 5 matchall skip_sw action
>> > goto chain 8000000
>> >
>> > This will enable the lookup and then you should be able to start again
>> > the ptp4l. Sorry for not mention this, at least I should have written
>> > it
>> > somewhere that this is required.
>> >
>> > I was not sure if lan966x should or not enable tcam lookups
>> > automatically when a ptp trap action is added. I am open to suggestion
>> > here.
>> 
>> IMHO, from a user point of view this should just work. For a user
>> there is no connection between running linuxptp and some filtering
>> stuff with 'tc'.
>> 
>> Also, if the answer to my question above is yes, and ptp should
>> have worked on eth0 before, this is a regression then.
> 
> OK, I can see your point.
> With the following diff, you should see the same behaviour as before:

Ok, I can say, I don't see the error message anymore. Haven't tested
PTP though. I'd need to setup it up first.

Does it also work out of the box with the following patch if
the interface is part of a bridge or do you still have to do
the tc magic from above?

-michael

> ---
> diff --git
> a/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c
> b/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c
> index 904f5a3f636d3..538f4b76cf97a 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c
> @@ -91,8 +91,6 @@ lan966x_vcap_is2_get_port_keysets(struct net_device
> *dev, int lookup,
> 
>         /* Check if the port keyset selection is enabled */
>         val = lan_rd(lan966x, ANA_VCAP_S2_CFG(port->chip_port));
> -       if (!ANA_VCAP_S2_CFG_ENA_GET(val))
> -               return -ENOENT;
> 
>         /* Collect all keysets for the port in a list */
>         if (l3_proto == ETH_P_ALL)
> ---
> 
>> 
>> -michael

Re: [PATCH net-next v3 4/4] net: lan966x: Add ptp trap rules

[Horatiu Vultur]

The 12/09/2022 13:10, Michael Walle wrote:
> 
> Hi,

Hi Michael,

> 
> > > > The issue is because you have not enabled the TCAM lookups per
> > > > port. They can be enabled using this commands:
> > > >
> > > > tc qdisc add dev eth0 clsact
> > > 
> > > This gives me the following error, might be a missing kconfig option:
> > > 
> > > # tc qdisc add dev eth0 clsact
> > > RTNETLINK answers: Operation not supported
> > 
> > Yes that should be the case, I think you are missing:
> > CONFIG_NET_SCHED
> > But may be others when you try to add the next rule.
> 
> I guess I'd need to update my kernel config sometime. At the
> moment I just have a basic one, as there is still so much stuff
> missing for the lan9668. So I haven't come around testing anything
> else. As I said, I just noticed because my rootfs happens to have
> linuxptp started by default.

I understand.

> 
> > > > tc filter add dev eth0 ingress prio 5 handle 5 matchall skip_sw action
> > > > goto chain 8000000
> > > >
> > > > This will enable the lookup and then you should be able to start again
> > > > the ptp4l. Sorry for not mention this, at least I should have written
> > > > it
> > > > somewhere that this is required.
> > > >
> > > > I was not sure if lan966x should or not enable tcam lookups
> > > > automatically when a ptp trap action is added. I am open to suggestion
> > > > here.
> > > 
> > > IMHO, from a user point of view this should just work. For a user
> > > there is no connection between running linuxptp and some filtering
> > > stuff with 'tc'.
> > > 
> > > Also, if the answer to my question above is yes, and ptp should
> > > have worked on eth0 before, this is a regression then.
> > 
> > OK, I can see your point.
> > With the following diff, you should see the same behaviour as before:
> 
> Ok, I can say, I don't see the error message anymore. Haven't tested
> PTP though. I'd need to setup it up first.

Good, at least no more warnings and should not be any regression there.

> 
> Does it also work out of the box with the following patch if
> the interface is part of a bridge or do you still have to do
> the tc magic from above?

You will still need to enable the TCAM using the tc command to have it
working when the interface is part of the bridge.

> 
> -michael
> 
> > ---
> > diff --git
> > a/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c
> > b/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c
> > index 904f5a3f636d3..538f4b76cf97a 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c
> > @@ -91,8 +91,6 @@ lan966x_vcap_is2_get_port_keysets(struct net_device
> > *dev, int lookup,
> > 
> >         /* Check if the port keyset selection is enabled */
> >         val = lan_rd(lan966x, ANA_VCAP_S2_CFG(port->chip_port));
> > -       if (!ANA_VCAP_S2_CFG_ENA_GET(val))
> > -               return -ENOENT;
> > 
> >         /* Collect all keysets for the port in a list */
> >         if (l3_proto == ETH_P_ALL)
> > ---
> > 
> > > 
> > > -michael

-- 
/Horatiu

Re: [PATCH net-next v3 4/4] net: lan966x: Add ptp trap rules

[Vladimir Oltean]

On Fri, Dec 09, 2022 at 01:58:57PM +0100, Horatiu Vultur wrote:
> > Does it also work out of the box with the following patch if
> > the interface is part of a bridge or do you still have to do
> > the tc magic from above?
> 
> You will still need to enable the TCAM using the tc command to have it
> working when the interface is part of the bridge.

FWIW, with ocelot (same VCAP mechanism), PTP traps work out of the box,
no need to use tc. Same goes for ocelot-8021q, which also uses the VCAP.
I wouldn't consider forcing the user to add any tc command in order for
packet timestamping to work properly.

Re: [PATCH net-next v3 4/4] net: lan966x: Add ptp trap rules

[Michael Walle]

Am 2022-12-09 13:56, schrieb Vladimir Oltean:
> On Fri, Dec 09, 2022 at 01:58:57PM +0100, Horatiu Vultur wrote:
>> > Does it also work out of the box with the following patch if
>> > the interface is part of a bridge or do you still have to do
>> > the tc magic from above?
>> 
>> You will still need to enable the TCAM using the tc command to have it
>> working when the interface is part of the bridge.
> 
> FWIW, with ocelot (same VCAP mechanism), PTP traps work out of the box,
> no need to use tc. Same goes for ocelot-8021q, which also uses the 
> VCAP.
> I wouldn't consider forcing the user to add any tc command in order for
> packet timestamping to work properly.

+1
Esp. because there is no warning. I.e. I tried this patch while
the interface was added on a bridge and there was no error
whatsoever. Also, you'd force the user to have that Kconfig option
set.

-michael

Re: [PATCH net-next v3 4/4] net: lan966x: Add ptp trap rules

[Vladimir Oltean]

On Fri, Dec 09, 2022 at 03:05:01PM +0100, Michael Walle wrote:
> Am 2022-12-09 13:56, schrieb Vladimir Oltean:
> > On Fri, Dec 09, 2022 at 01:58:57PM +0100, Horatiu Vultur wrote:
> > > > Does it also work out of the box with the following patch if
> > > > the interface is part of a bridge or do you still have to do
> > > > the tc magic from above?
> > > 
> > > You will still need to enable the TCAM using the tc command to have it
> > > working when the interface is part of the bridge.
> > 
> > FWIW, with ocelot (same VCAP mechanism), PTP traps work out of the box,
> > no need to use tc. Same goes for ocelot-8021q, which also uses the VCAP.
> > I wouldn't consider forcing the user to add any tc command in order for
> > packet timestamping to work properly.
> 
> +1
> Esp. because there is no warning. I.e. I tried this patch while
> the interface was added on a bridge and there was no error
> whatsoever. Also, you'd force the user to have that Kconfig option
> set.

Yup. What I said about Ocelot is also applicable to MRP traps, which
Horatiu added. No tc necessary there, either.

Re: [PATCH net-next v3 4/4] net: lan966x: Add ptp trap rules

[Horatiu Vultur]

The 12/09/2022 15:05, Michael Walle wrote:
> 
> Am 2022-12-09 13:56, schrieb Vladimir Oltean:
> > On Fri, Dec 09, 2022 at 01:58:57PM +0100, Horatiu Vultur wrote:
> > > > Does it also work out of the box with the following patch if
> > > > the interface is part of a bridge or do you still have to do
> > > > the tc magic from above?
> > > 
> > > You will still need to enable the TCAM using the tc command to have it
> > > working when the interface is part of the bridge.
> > 
> > FWIW, with ocelot (same VCAP mechanism), PTP traps work out of the box,
> > no need to use tc. Same goes for ocelot-8021q, which also uses the
> > VCAP.
> > I wouldn't consider forcing the user to add any tc command in order for
> > packet timestamping to work properly.

On ocelot, the vcap is enabled at port initialization, while on other
platforms(lan966x and sparx5) you have the option to enable or disable.

> 
> +1
> Esp. because there is no warning. I.e. I tried this patch while
> the interface was added on a bridge and there was no error
> whatsoever.

What error/warning were you expecting to see here?

> Also, you'd force the user to have that Kconfig option
> set.



> 
> -michael
> 

-- 
/Horatiu

Re: [PATCH net-next v3 4/4] net: lan966x: Add ptp trap rules

[Michael Walle]

Am 2022-12-09 15:20, schrieb Horatiu Vultur:
> The 12/09/2022 15:05, Michael Walle wrote:
>> 
>> Am 2022-12-09 13:56, schrieb Vladimir Oltean:
>> > On Fri, Dec 09, 2022 at 01:58:57PM +0100, Horatiu Vultur wrote:
>> > > > Does it also work out of the box with the following patch if
>> > > > the interface is part of a bridge or do you still have to do
>> > > > the tc magic from above?
>> > >
>> > > You will still need to enable the TCAM using the tc command to have it
>> > > working when the interface is part of the bridge.
>> >
>> > FWIW, with ocelot (same VCAP mechanism), PTP traps work out of the box,
>> > no need to use tc. Same goes for ocelot-8021q, which also uses the
>> > VCAP.
>> > I wouldn't consider forcing the user to add any tc command in order for
>> > packet timestamping to work properly.
> 
> On ocelot, the vcap is enabled at port initialization, while on other
> platforms(lan966x and sparx5) you have the option to enable or disable.
> 
>> 
>> +1
>> Esp. because there is no warning. I.e. I tried this patch while
>> the interface was added on a bridge and there was no error
>> whatsoever.
> 
> What error/warning were you expecting to see here?

Scrap that. ptp4l is reporting an error in case the device is part
of a bridge:
Jan  1 02:33:04 buildroot user.info syslog: [9184.261] driver rejected 
most general HWTSTAMP filter

Nevertheless, from a users POV I'd just expect it to work. How
would I know what I need to do here?

-michael

Re: [PATCH net-next v3 4/4] net: lan966x: Add ptp trap rules

[Horatiu Vultur]

The 12/09/2022 15:23, Michael Walle wrote:
> 
> Am 2022-12-09 15:20, schrieb Horatiu Vultur:
> > The 12/09/2022 15:05, Michael Walle wrote:
> > > 
> > > Am 2022-12-09 13:56, schrieb Vladimir Oltean:
> > > > On Fri, Dec 09, 2022 at 01:58:57PM +0100, Horatiu Vultur wrote:
> > > > > > Does it also work out of the box with the following patch if
> > > > > > the interface is part of a bridge or do you still have to do
> > > > > > the tc magic from above?
> > > > >
> > > > > You will still need to enable the TCAM using the tc command to have it
> > > > > working when the interface is part of the bridge.
> > > >
> > > > FWIW, with ocelot (same VCAP mechanism), PTP traps work out of the box,
> > > > no need to use tc. Same goes for ocelot-8021q, which also uses the
> > > > VCAP.
> > > > I wouldn't consider forcing the user to add any tc command in order for
> > > > packet timestamping to work properly.
> > 
> > On ocelot, the vcap is enabled at port initialization, while on other
> > platforms(lan966x and sparx5) you have the option to enable or disable.
> > 
> > > 
> > > +1
> > > Esp. because there is no warning. I.e. I tried this patch while
> > > the interface was added on a bridge and there was no error
> > > whatsoever.
> > 
> > What error/warning were you expecting to see here?
> 
> Scrap that. ptp4l is reporting an error in case the device is part
> of a bridge:
> Jan  1 02:33:04 buildroot user.info syslog: [9184.261] driver rejected
> most general HWTSTAMP filter
> 
> Nevertheless, from a users POV I'd just expect it to work. How
> would I know what I need to do here?

What about a warning in the driver? Say that the vcap needs to be
enabled.

> 
> -michael

-- 
/Horatiu

Re: [PATCH net-next v3 4/4] net: lan966x: Add ptp trap rules

[Vladimir Oltean]

On Fri, Dec 09, 2022 at 03:20:58PM +0100, Horatiu Vultur wrote:
> On ocelot, the vcap is enabled at port initialization, while on other
> platforms(lan966x and sparx5) you have the option to enable or disable.

Even if that wasn't the case, I'd still consider enabling/disabling VCAP
lookups privately in the ocelot driver when there are non-tc users of
traps, instead of requiring users to do anything with tc. It's simply
too unfriendly for somebody who just wants PTP. You can use devlink
traps to show which non-tc traps are active.

Re: [PATCH net-next v3 4/4] net: lan966x: Add ptp trap rules

[Vladimir Oltean]

On Fri, Dec 09, 2022 at 04:43:28PM +0200, Vladimir Oltean wrote:
> On Fri, Dec 09, 2022 at 03:20:58PM +0100, Horatiu Vultur wrote:
> > On ocelot, the vcap is enabled at port initialization, while on other
> > platforms(lan966x and sparx5) you have the option to enable or disable.
> 
> Even if that wasn't the case, I'd still consider enabling/disabling VCAP
> lookups privately in the ocelot driver when there are non-tc users of
> traps, instead of requiring users to do anything with tc. It's simply
> too unfriendly for somebody who just wants PTP. You can use devlink
> traps to show which non-tc traps are active.

To put it differently. Why do you even bother to make the driver
auto-install PTP traps, and not let the user do that? It's the same
argument as asking the user enable the VCAP lookups.

Re: [PATCH net-next v3 4/4] net: lan966x: Add ptp trap rules

[Horatiu Vultur]

The 12/09/2022 16:43, Vladimir Oltean wrote:
> 
> On Fri, Dec 09, 2022 at 03:20:58PM +0100, Horatiu Vultur wrote:
> > On ocelot, the vcap is enabled at port initialization, while on other
> > platforms(lan966x and sparx5) you have the option to enable or disable.
> 
> Even if that wasn't the case, I'd still consider enabling/disabling VCAP
> lookups privately in the ocelot driver when there are non-tc users of
> traps, instead of requiring users to do anything with tc.

I was thinking also about this, such the ptp to enable the VCAP
privately. But then the issue would be if a user adds entries using tc
and then start ptp, then suddently the rules that were added using tc
could be hit. That is the reason why expected the user to enable the
tcam manually.

> It's simply too unfriendly for somebody who just wants PTP. You can use
> devlink traps to show which non-tc traps are active.

-- 
/Horatiu

Re: [PATCH net-next v3 4/4] net: lan966x: Add ptp trap rules

[Vladimir Oltean]

On Fri, Dec 09, 2022 at 03:57:20PM +0100, Horatiu Vultur wrote:
> The 12/09/2022 16:43, Vladimir Oltean wrote:
> > 
> > On Fri, Dec 09, 2022 at 03:20:58PM +0100, Horatiu Vultur wrote:
> > > On ocelot, the vcap is enabled at port initialization, while on other
> > > platforms(lan966x and sparx5) you have the option to enable or disable.
> > 
> > Even if that wasn't the case, I'd still consider enabling/disabling VCAP
> > lookups privately in the ocelot driver when there are non-tc users of
> > traps, instead of requiring users to do anything with tc.
> 
> I was thinking also about this, such the ptp to enable the VCAP
> privately. But then the issue would be if a user adds entries using tc
> and then start ptp, then suddently the rules that were added using tc
> could be hit. That is the reason why expected the user to enable the
> tcam manually.

I don't understand, tc rules which do what? Why would those rules only
be hit after PTP is enabled and not before?

Re: [PATCH net-next v3 4/4] net: lan966x: Add ptp trap rules

[Horatiu Vultur]

The 12/09/2022 16:56, Vladimir Oltean wrote:
> 
> On Fri, Dec 09, 2022 at 03:57:20PM +0100, Horatiu Vultur wrote:
> > The 12/09/2022 16:43, Vladimir Oltean wrote:
> > >
> > > On Fri, Dec 09, 2022 at 03:20:58PM +0100, Horatiu Vultur wrote:
> > > > On ocelot, the vcap is enabled at port initialization, while on other
> > > > platforms(lan966x and sparx5) you have the option to enable or disable.
> > >
> > > Even if that wasn't the case, I'd still consider enabling/disabling VCAP
> > > lookups privately in the ocelot driver when there are non-tc users of
> > > traps, instead of requiring users to do anything with tc.
> >
> > I was thinking also about this, such the ptp to enable the VCAP
> > privately. But then the issue would be if a user adds entries using tc
> > and then start ptp, then suddently the rules that were added using tc
> > could be hit. That is the reason why expected the user to enable the
> > tcam manually.
> 
> I don't understand, tc rules which do what? Why would those rules only
> be hit after PTP is enabled and not before?

Because you have not enabled the vcap.

For example this rule:
tc filter add dev eth0 ingress chain 8000000 prio 1 handle 1 protocol all
flower skip_sw dst_mac 00:11:22:33:44:55/ff:ff:ff:ff:ff:ff action trap
action goto chain 8100000

This will not be hit until you add this rule:
tc filter add dev eth0 ingress prio 1 handle 2 matchall skip_sw action goto chain 8000000

Because this rule will enable the HW. Just to aligned to a SW
implementation of the tc, we don't enable the vcap until there is a rule
in chain 0 that has an action to go to chain 8000000 were it resides
IS2 rules.

So for example, on a fresh started lan966x the user will add the following
rule:
tc filter add dev eth0 ingress chain 8000000 prio 1 handle 1 protocol
all flower skip_sw dst_mac 00:11:22:33:44:55/ff:ff:ff:ff:ff:ff action
trap action goto chain 8100000

He expects this rule not to be hit as there is no rule in chain 0. Now if
PTP is started and it would enable vcap, then suddenly this rule may be
hit.

I hope this helps a little bit.

-- 
/Horatiu

Re: [PATCH net-next v3 4/4] net: lan966x: Add ptp trap rules

[Vladimir Oltean]

On Fri, Dec 09, 2022 at 04:30:10PM +0100, Horatiu Vultur wrote:
> For example this rule:
> tc filter add dev eth0 ingress chain 8000000 prio 1 handle 1 protocol all
> flower skip_sw dst_mac 00:11:22:33:44:55/ff:ff:ff:ff:ff:ff action trap
> action goto chain 8100000
> 
> This will not be hit until you add this rule:
> tc filter add dev eth0 ingress prio 1 handle 2 matchall skip_sw action goto chain 8000000
> 
> Because this rule will enable the HW. Just to aligned to a SW
> implementation of the tc, we don't enable the vcap until there is a rule
> in chain 0 that has an action to go to chain 8000000 were it resides
> IS2 rules.
> 
> So for example, on a fresh started lan966x the user will add the following
> rule:
> tc filter add dev eth0 ingress chain 8000000 prio 1 handle 1 protocol
> all flower skip_sw dst_mac 00:11:22:33:44:55/ff:ff:ff:ff:ff:ff action
> trap action goto chain 8100000
> 
> He expects this rule not to be hit as there is no rule in chain 0. Now if
> PTP is started and it would enable vcap, then suddenly this rule may be
> hit.

Is it too restrictive to only allow adding offloaded filters to a chain
that has a valid goto towards it, coming (perhaps indirectly) from chain 0?

Re: [PATCH net-next v3 4/4] net: lan966x: Add ptp trap rules

[Jakub Kicinski]

On Fri, 9 Dec 2022 17:27:13 +0200 Vladimir Oltean wrote:
> > So for example, on a fresh started lan966x the user will add the following
> > rule:
> > tc filter add dev eth0 ingress chain 8000000 prio 1 handle 1 protocol
> > all flower skip_sw dst_mac 00:11:22:33:44:55/ff:ff:ff:ff:ff:ff action
> > trap action goto chain 8100000
> > 
> > He expects this rule not to be hit as there is no rule in chain 0. Now if
> > PTP is started and it would enable vcap, then suddenly this rule may be
> > hit.  
> 
> Is it too restrictive to only allow adding offloaded filters to a chain
> that has a valid goto towards it, coming (perhaps indirectly) from chain 0?

Right, we fumbled the review and let the chain oddness in. 
Until recently the driver worked without any rules in chain 0 :(

Maybe adding and offload of the rules can be separated?
Only actually add the rules to the HW once the goto chain rule 
has been added? 

Re: [PATCH net-next v3 4/4] net: lan966x: Add ptp trap rules

[Horatiu Vultur]

The 12/09/2022 15:03, Jakub Kicinski wrote:
> 
> On Fri, 9 Dec 2022 17:27:13 +0200 Vladimir Oltean wrote:
> > > So for example, on a fresh started lan966x the user will add the following
> > > rule:
> > > tc filter add dev eth0 ingress chain 8000000 prio 1 handle 1 protocol
> > > all flower skip_sw dst_mac 00:11:22:33:44:55/ff:ff:ff:ff:ff:ff action
> > > trap action goto chain 8100000
> > >
> > > He expects this rule not to be hit as there is no rule in chain 0. Now if
> > > PTP is started and it would enable vcap, then suddenly this rule may be
> > > hit.
> >
> > Is it too restrictive to only allow adding offloaded filters to a chain
> > that has a valid goto towards it, coming (perhaps indirectly) from chain 0?
> 
> Right, we fumbled the review and let the chain oddness in.
> Until recently the driver worked without any rules in chain 0 :(
> 
> Maybe adding and offload of the rules can be separated?
> Only actually add the rules to the HW once the goto chain rule
> has been added?

Yes, we would like to do something like this.

-- 
/Horatiu

Re: [PATCH net-next v3 4/4] net: lan966x: Add ptp trap rules

[Horatiu Vultur]

The 12/09/2022 17:27, Vladimir Oltean wrote:
> 
Sorry for late reply!

> On Fri, Dec 09, 2022 at 04:30:10PM +0100, Horatiu Vultur wrote:
> > For example this rule:
> > tc filter add dev eth0 ingress chain 8000000 prio 1 handle 1 protocol all
> > flower skip_sw dst_mac 00:11:22:33:44:55/ff:ff:ff:ff:ff:ff action trap
> > action goto chain 8100000
> >
> > This will not be hit until you add this rule:
> > tc filter add dev eth0 ingress prio 1 handle 2 matchall skip_sw action goto chain 8000000
> >
> > Because this rule will enable the HW. Just to aligned to a SW
> > implementation of the tc, we don't enable the vcap until there is a rule
> > in chain 0 that has an action to go to chain 8000000 were it resides
> > IS2 rules.
> >
> > So for example, on a fresh started lan966x the user will add the following
> > rule:
> > tc filter add dev eth0 ingress chain 8000000 prio 1 handle 1 protocol
> > all flower skip_sw dst_mac 00:11:22:33:44:55/ff:ff:ff:ff:ff:ff action
> > trap action goto chain 8100000
> >
> > He expects this rule not to be hit as there is no rule in chain 0. Now if
> > PTP is started and it would enable vcap, then suddenly this rule may be
> > hit.
> 
> Is it too restrictive to only allow adding offloaded filters to a chain
> that has a valid goto towards it, coming (perhaps indirectly) from chain 0?

We were thinking to do something like this. With a small difference, to
allow the user to add filters to a chain, but offload them in HW only when
there is a valid goto towards. Instead of checking if there is a goto to
that chain. But I think we need to spend a little bit more time on this.
Any suggestion is more than welcome.

-- 
/Horatiu

Re: [PATCH net-next v3 4/4] net: lan966x: Add ptp trap rules

[Michael Walle]

Hi,

>> Also, if the answer to my question above is yes, and ptp should
>> have worked on eth0 before, this is a regression then.
> 
> OK, I can see your point.
> With the following diff, you should see the same behaviour as before:
> ---
> diff --git
> a/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c
> b/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c
> index 904f5a3f636d3..538f4b76cf97a 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c
> @@ -91,8 +91,6 @@ lan966x_vcap_is2_get_port_keysets(struct net_device
> *dev, int lookup,
> 
>         /* Check if the port keyset selection is enabled */
>         val = lan_rd(lan966x, ANA_VCAP_S2_CFG(port->chip_port));
> -       if (!ANA_VCAP_S2_CFG_ENA_GET(val))
> -               return -ENOENT;
> 
>         /* Collect all keysets for the port in a list */
>         if (l3_proto == ETH_P_ALL)

Any news on this? Apart from the patches which would change the
need to use some tc magic, this should be a separate fixes patch,
right?

-michael

Re: [PATCH net-next v3 4/4] net: lan966x: Add ptp trap rules

[Horatiu Vultur]

The 01/05/2023 16:09, Michael Walle wrote:
> 
> Hi,

Hi Michael,

> 
> > > Also, if the answer to my question above is yes, and ptp should
> > > have worked on eth0 before, this is a regression then.
> > 
> > OK, I can see your point.
> > With the following diff, you should see the same behaviour as before:
> > ---
> > diff --git
> > a/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c
> > b/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c
> > index 904f5a3f636d3..538f4b76cf97a 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c
> > @@ -91,8 +91,6 @@ lan966x_vcap_is2_get_port_keysets(struct net_device
> > *dev, int lookup,
> > 
> >         /* Check if the port keyset selection is enabled */
> >         val = lan_rd(lan966x, ANA_VCAP_S2_CFG(port->chip_port));
> > -       if (!ANA_VCAP_S2_CFG_ENA_GET(val))
> > -               return -ENOENT;
> > 
> >         /* Collect all keysets for the port in a list */
> >         if (l3_proto == ETH_P_ALL)
> 
> Any news on this? Apart from the patches which would change the
> need to use some tc magic, this should be a separate fixes patch,
> right?

My colleague Steen, has sent a patch series here [1].
This allows to PTP rules in HW without any tc commands.

[1] https://lore.kernel.org/lkml/20230105081335.1261636-1-steen.hegelund@microchip.com/T/

> 
> -michael

-- 
/Horatiu

Re: [PATCH net-next v3 0/4] net: lan966x: Enable PTP on bridge interfaces

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (master)
by Paolo Abeni <pabeni@redhat.com>:

On Sat, 3 Dec 2022 11:43:44 +0100 you wrote:
> Before it was not allowed to run ptp on ports that are part of a bridge
> because in case of transparent clock the HW will still forward the frames
> so there would be duplicate frames.
> Now that there is VCAP support, it is possible to add entries in the VCAP
> to trap frames to the CPU and the CPU will forward these frames.
> The first part of the patch series, extends the VCAP support to be able to
> modify and get the rule, while the last patch uses the VCAP to trap the ptp
> frames.
> 
> [...]

Here is the summary with links:
  - [net-next,v3,1/4] net: microchip: vcap: Add vcap_get_rule
    https://git.kernel.org/netdev/net-next/c/610c32b2ce66
  - [net-next,v3,2/4] net: microchip: vcap: Add vcap_mod_rule
    https://git.kernel.org/netdev/net-next/c/2662b3f93d26
  - [net-next,v3,3/4] net: microchip: vcap: Add vcap_rule_get_key_u32
    https://git.kernel.org/netdev/net-next/c/6009b61f80e0
  - [net-next,v3,4/4] net: lan966x: Add ptp trap rules
    https://git.kernel.org/netdev/net-next/c/72df3489fb10

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html