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

[Xiaoliang Yang]

Hi Vladimir,

On Wed, Sep 22, 2021 at 12:47:59 +0000, Vladimir Oltean 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?

Maybe we need to use ocelot_mact_learn() instead of ocelot_mact_write() after setting SFID in StreamData. I think this can avoid writing a wrong entry.

Regards,
Xiaoliang