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

Vladimir Oltean
2021-09-22T12:48:10Z

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?