Re: [PATCH v4 net-next 5/8] net: dsa: felix: support psfp filter on vsc9959

[Vladimir Oltean]

Hello Xiaoliang,

On Wed, Sep 22, 2021 at 06:51:59PM +0800, Xiaoliang Yang wrote:
> +static int vsc9959_mact_stream_set(struct ocelot *ocelot,
> +				   struct felix_stream *stream,
> +				   struct netlink_ext_ack *extack)
> +{
> +	struct ocelot_mact_entry entry;
> +	u32 row, col, reg, dst_idx;
> +	u8 type;
> +	int ret;
> +
> +	/* Stream identification desn't support to add a stream with non
> +	 * existent MAC (The MAC entry has not been learned in MAC table).
> +	 */
> +	ret = ocelot_mact_lookup(ocelot, stream->dmac, stream->vid, &row, &col);
> +	if (ret) {
> +		if (extack)
> +			NL_SET_ERR_MSG_MOD(extack, "Stream is not learned in MAC table");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	ocelot_rmw(ocelot,
> +		   (stream->sfid_valid ? ANA_TABLES_STREAMDATA_SFID_VALID : 0) |
> +		   ANA_TABLES_STREAMDATA_SFID(stream->sfid),
> +		   ANA_TABLES_STREAMDATA_SFID_VALID |
> +		   ANA_TABLES_STREAMDATA_SFID_M,
> +		   ANA_TABLES_STREAMDATA);
> +
> +	reg = ocelot_read(ocelot, ANA_TABLES_MACACCESS);
> +	dst_idx = (reg & ANA_TABLES_MACACCESS_DEST_IDX_M) >> 3;
> +	type = ANA_TABLES_MACACCESS_ENTRYTYPE_X(reg);
> +
> +	reg = ocelot_read(ocelot, ANA_TABLES_STREAMDATA);
> +	if ((ANA_TABLES_STREAMDATA_SFID_VALID |
> +	     ANA_TABLES_STREAMDATA_SSID_VALID) & reg) {
> +		entry.type = (type ? type : ENTRYTYPE_LOCKED);
> +		stream->rsv_type = type;
> +	} else {
> +		entry.type = stream->rsv_type;
> +	}
> +
> +	ether_addr_copy(entry.mac, stream->dmac);
> +	entry.vid = stream->vid;
> +
> +	ocelot_mact_write(ocelot, dst_idx, &entry, row, col);
> +
> +	return 0;
> +}

> +static int vsc9959_stream_table_add(struct ocelot *ocelot,
> +				    struct list_head *stream_list,
> +				    struct felix_stream *stream,
> +				    struct netlink_ext_ack *extack)
> +{
> +	struct felix_stream *stream_entry;
> +	int ret;
> +
> +	stream_entry = kzalloc(sizeof(*stream_entry), GFP_KERNEL);
> +	if (!stream_entry)
> +		return -ENOMEM;
> +
> +	memcpy(stream_entry, stream, sizeof(*stream_entry));
> +
> +	ret = vsc9959_mact_stream_set(ocelot, stream_entry, extack);
> +	if (ret) {
> +		kfree(stream_entry);
> +		return ret;
> +	}
> +
> +	list_add_tail(&stream_entry->list, stream_list);
> +
> +	return 0;
> +}

Remember this discussion we had a while ago?

| Let's take the function below.
| 
| static void ocelot_prove_mac_table_entries_can_move(struct ocelot *ocelot)
| {
| 	unsigned char mac1[ETH_ALEN] = {0x00, 0x04, 0x9f, 0x63, 0x35, 0xea};
| 	unsigned char mac2[ETH_ALEN] = {0x00, 0x04, 0x9f, 0x63, 0x35, 0xeb};
| 	int row, bucket, arbitrary_pgid = 4;
| 	int vid1 = 102;
| 	int vid2 = 103;
| 	int err;
| 
| 	err = ocelot_mact_learn(ocelot, arbitrary_pgid, mac1, vid1,
| 				ENTRYTYPE_LOCKED);
| 	if (err)
| 		return;
| 
| 	err = ocelot_mact_lookup(ocelot, mac1, vid1, &row, &bucket);
| 	if (err)
| 		return;
| 
| 	dev_info(ocelot->dev,
| 		 "Address 1 (mac %pM vid %d) is in MAC table row %d bucket %d\n",
| 		 mac1, vid1, row, bucket);
| 
| 	err = ocelot_mact_learn(ocelot, arbitrary_pgid, mac2, vid2,
| 				ENTRYTYPE_LOCKED);
| 	if (err)
| 		return;
| 
| 	err = ocelot_mact_lookup(ocelot, mac2, vid2, &row, &bucket);
| 	if (err)
| 		return;
| 
| 	dev_info(ocelot->dev,
| 		 "Address 2 (mac %pM vid %d) is in MAC table row %d bucket %d\n",
| 		 mac2, vid2, row, bucket);
| 
| 	err = ocelot_mact_lookup(ocelot, mac1, vid1, &row, &bucket);
| 	if (err)
| 		return;
| 
| 	dev_info(ocelot->dev,
| 		 "Address 1 (mac %pM vid %d) is in MAC table row %d bucket %d\n",
| 		 mac1, vid1, row, bucket);
| }
| 
| What will it print?
| 
| Address 1 (mac 00:04:9f:63:35:ea vid 102) is in MAC table row 917 bucket 0
| Address 2 (mac 00:04:9f:63:35:eb vid 103) is in MAC table row 917 bucket 0
| Address 1 (mac 00:04:9f:63:35:ea vid 102) is in MAC table row 917 bucket 1
| 
| What does this mean?
| 
| The ROW portion of a FDB entry's position within the MAC table is
| statically determined using an 11-bit hash derived from the {DMAC, VID}
| key. Within a row, there can be up to 4 buckets, each bucket holding 1
| MAC table entry.
| 
| But when the hashes of 2 addresses collide and they end up in the same
| row (as in the above example, with address 1 = "mac 00:04:9f:63:35:ea
| vid 102" and address 2 = "mac 00:04:9f:63:35:eb vid 103"), things don't
| happen quite as you might expect. Namely, the second address appears to
| be installed by the switch at the same row and bucket as the first
| address. So is the first address overwritten? No, it has been moved by
| the switch, automatically, to bucket 1.

So if the autonomous and concurrent learning of one MAC address might
move existing MAC table entries from a row to the right, then who
guarantees exactly that the {row, col} for which you are setting up the
SFID is the {row, col} that belongs to the {stream->dmac, stream->vid}
you have searched for?

Microchip people, do we need to temporarily disable hardware address
learning on all ports, and take a lock with the FDB add and delete
operations to ensure they are serialized?