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

Xiaoliang Yang
2021-09-23T02:30:34Z

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