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

On Thu, Sep 23, 2021 at 11:23:45AM +0000, Xiaoliang Yang wrote:

Hi Vladimir,

On Thu, Sep 23, 2021 at 15:45:16 +0000, Vladimir Oltean wrote:

quoted

quoted

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.

So you're thinking of introducing a new ocelot_mact_learn_with_streamdata(),
that writes the SFID and SSID of the STREAMDATA too, instead of editing them
in-place for an existing MAC table entry, and then issuing a LEARN MAC Table
command which would hopefully transfer the entire data structure to the MAC
table?

Have you tried that?

Yes, I have tried. I mean writes SFID of STREAMDATA in
vsc9959_mact_stream_set() first, then calls ocelot_mact_learn()
function to write VID, mac and STREAMDATA in MAC table. We don't need
to introduce a new function. Once we call ocelot_mact_learn()
function, STREAMDATA will be stored in the learned entry.

quoted

In the documentation for the LEARN MAC Table command, I see:

Purpose: Insert/learn new entry in MAC table.  Position given by (MAC, VID)

Use: Configure MAC and VID of the new entry in MACHDATA and MACLDATA.
Configure remaining entry fields in MACACCESS.  The location in the MAC
table is calculated based on (MAC, VID).

I just hope it will transfer the STREAMDATA too, it doesn't explicitly say that it
will...

And assuming it does, will the LEARN command overwrite an existing static
FDB entry which has the same MAC DA and VLAN ID, but not SFID?
I haven't tried that either.

I tried the case that when MAC table index has changed, STREAMDATA
will keep move with VID and MAC. The entry { STREAMDATA , VID, MAC}
also can overwrite a static exist entry. I think we can do like this.

Ok, so maybe we should do that?

Even though I must say I don't really like the idea of partially writing
MAC table entry data from the vsc9959 driver, and partially from
ocelot_mact_learn. I also have this patch pending:
https://patchwork.kernel.org/project/netdevbpf/patch/20210824114049.3814660-4-vladimir.oltean@nxp.com/
and concurrency will be an absolute mess. The ocelot->mact_lock will
need to be taken _before_ we start writing the STREAMDATA, so this
variant of ocelot_mact_learn will still have to stay somewhere in the
ocelot library, and be organized something like this:

__ocelot_mact_learn()
{
	do what ocelot_mact_learn() currently does
}

ocelot_mact_learn()
{
	mutex_lock(&ocelot->mact_lock);
	__ocelot_mact_learn();
	mutex_unlock(&ocelot->mact_lock);
}

ocelot_mact_learn_streamdata()
{
	mutex_lock(&ocelot->mact_lock);
	write_streamdata();
	__ocelot_mact_learn();
	mutex_unlock(&ocelot->mact_lock);
}

otherwise I would need to introduce avoidable refactoring in the driver.
In fact, could you please pick up that mact_lock patch? Even if the
rtnl_mutex was not dropped yet, the extra lock should not hurt anyone.