Date:   Thu, 23 Sep 2021 11:35:28 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Xiaoliang Yang <xiaoliang.yang_1@....com>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "allan.nielsen@...rochip.com" <allan.nielsen@...rochip.com>,
        "joergen.andreasen@...rochip.com" <joergen.andreasen@...rochip.com>,
        "UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>,
        "vinicius.gomes@...el.com" <vinicius.gomes@...el.com>,
        "michael.chan@...adcom.com" <michael.chan@...adcom.com>,
        "saeedm@...lanox.com" <saeedm@...lanox.com>,
        "jiri@...lanox.com" <jiri@...lanox.com>,
        "idosch@...lanox.com" <idosch@...lanox.com>,
        "alexandre.belloni@...tlin.com" <alexandre.belloni@...tlin.com>,
        "kuba@...nel.org" <kuba@...nel.org>, Po Liu <po.liu@....com>,
        Leo Li <leoyang.li@....com>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "vivien.didelot@...il.com" <vivien.didelot@...il.com>,
        Claudiu Manoil <claudiu.manoil@....com>
Subject: 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:
> > > 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.
>
> >
> > 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.

Powered by blists - more mailing lists