Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Vladimir Oltean <vladimir.oltean@nxp.com>
To:     Xiaoliang Yang <xiaoliang.yang_1@nxp.com>
CC:     "davem@davemloft.net" <davem@davemloft.net>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "allan.nielsen@microchip.com" <allan.nielsen@microchip.com>,
        "joergen.andreasen@microchip.com" <joergen.andreasen@microchip.com>,
        "UNGLinuxDriver@microchip.com" <UNGLinuxDriver@microchip.com>,
        "vinicius.gomes@intel.com" <vinicius.gomes@intel.com>,
        "michael.chan@broadcom.com" <michael.chan@broadcom.com>,
        "saeedm@mellanox.com" <saeedm@mellanox.com>,
        "jiri@mellanox.com" <jiri@mellanox.com>,
        "idosch@mellanox.com" <idosch@mellanox.com>,
        "alexandre.belloni@bootlin.com" <alexandre.belloni@bootlin.com>,
        "kuba@kernel.org" <kuba@kernel.org>, Po Liu <po.liu@nxp.com>,
        Leo Li <leoyang.li@nxp.com>,
        "f.fainelli@gmail.com" <f.fainelli@gmail.com>,
        "andrew@lunn.ch" <andrew@lunn.ch>,
        "vivien.didelot@gmail.com" <vivien.didelot@gmail.com>,
        Claudiu Manoil <claudiu.manoil@nxp.com>
Subject: Re: [PATCH v4 net-next 5/8] net: dsa: felix: support psfp filter on
 vsc9959
Thread-Topic: [PATCH v4 net-next 5/8] net: dsa: felix: support psfp filter on
 vsc9959
Thread-Index: AQHXr56GWZAJ2atrv0ecdycHDF0T3KuwAOYAgADlwACAAHlYgIAAG7WAgAADRYA=
Date:   Thu, 23 Sep 2021 11:35:28 +0000
Message-ID: <20210923113527.jd3buthrh2rx6ulz@skbuf>
References: <20210922105202.12134-1-xiaoliang.yang_1@nxp.com>
 <20210922105202.12134-6-xiaoliang.yang_1@nxp.com>
 <20210922124758.3n2yrjgb6ijrq6ls@skbuf>
 <DB8PR04MB578547CBED62C7EEA9F8F534F0A39@DB8PR04MB5785.eurprd04.prod.outlook.com>
 <20210923094435.3jrpwd63fnmwhx7i@skbuf>
 <DB8PR04MB578574BC7F055D55725374CCF0A39@DB8PR04MB5785.eurprd04.prod.outlook.com>
In-Reply-To: <DB8PR04MB578574BC7F055D55725374CCF0A39@DB8PR04MB5785.eurprd04.prod.outlook.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?QS5XNJQPPZWTf7sCLKCOXYCqoFzxB0TlrkiR20Iu+qpVFNZv09Gh5Hr1u6rX?=
 =?us-ascii?Q?PJz9o3+vgnEFPCae+AKOnDBRER6+sYr8qGARZv1Kykn2V64OI15a+esAUEV4?=
 =?us-ascii?Q?DcTRhavpD7olo0uexODRcwPJ0lHL0dxufL4MQtHpbN0QI+agKQInnsx24E3V?=
 =?us-ascii?Q?HCfwfvQTf3Sn1THY2FhpxpQMrzv2CJtrm5GzvrzQ5xtUii5mVMgt7DW9Hz+7?=
 =?us-ascii?Q?nb+HGRqBX3+7ECX9bJCkmAgZPIDYIqbeSg4NWpjYNQyhTN0pmCXT/6AAbr5O?=
 =?us-ascii?Q?9f0+Vndnb5cnVxhHjzGRdZZBeLRnPdzAoP48+n0Z5bnXyfqo3IeJzT/Lvm/B?=
 =?us-ascii?Q?yC9tAn9GJ6PE+jVMqASDt0zmyYPycFobUx5Ub841d47hp1XeuJBxwEbWV5EZ?=
 =?us-ascii?Q?pupbhmZRM4gzgW2xmOV4yX22D/YfDX4YoTXziySxolMGQvckObv9Ry1GFnJf?=
 =?us-ascii?Q?E+jQ7iO9Q9cvKVolzkLVsyLbHfws5Zy3luXX0cNqaAqy5cfxni0itdIp8nRc?=
 =?us-ascii?Q?O3jT9EM9fAyggHIDX9ui92EV+ksl932iAVBDuJ2Bwcav9gkRHk+qEoRUpmtV?=
 =?us-ascii?Q?ooDNiCj9BQmoAskEsuQPxy4LTnO7zDy1dzSa+rk7AL4BPL8fX/4P2/MFHHlz?=
 =?us-ascii?Q?M0f8kFyEDEqbdoEqtVgWMZ01stFYxixutz3aRMCF56bTeGbqPkfRsLfNJUzy?=
 =?us-ascii?Q?Gurth4PTdI82wd9n0H1m/WPhwiCBNi7NnJWT2ddeHat09O3g8/q5neQfVtNf?=
 =?us-ascii?Q?ePGN4p6Q7fGbx+/ItSdsVakNvMkvAsfggFYZbgUTusYuraKDSMseJnCN9gP+?=
 =?us-ascii?Q?UGJ7mjylC9J80g5575tXYO5yWG60IfiA0UeHozeoftPhw5DKIMqEyNEQidLN?=
 =?us-ascii?Q?A82gEa0A/vpFI1iT1ZKUkdU9cXoL+n+88tq/3SO8m6h2z+duO2B/Fz+7mLzV?=
 =?us-ascii?Q?VEfu3a/FRvjSUoi8BfOs2NTIgTEiMrSeXGWzWjzaDFVB2JUQPRIqFiIA3vj1?=
 =?us-ascii?Q?XaXvVOn78RMq0749w4Z9+ZuhFa6eGWtywgKczcNG+VZMRdQrB/GUP79SKCNq?=
 =?us-ascii?Q?wR7KxdPCLI+dwzCbX0zK9QB1FwPnoaLM02d8+GFPKRJsonCNtSrl2w/gsn2X?=
 =?us-ascii?Q?wCmpI7QkEwR/Lry6lkNgMols1qa7bhHftzPabYe58USGhyd3an/8gxFjzj4l?=
 =?us-ascii?Q?XoL9LqZQ7yK0zWj+OROp49xX+aeUYav9VwtGW7UE1aVIu6G6qr49qZxvZVYU?=
 =?us-ascii?Q?xbb2EP5/ojbj68B+NEwU00PIZInfuq/83gHPSrTziNLIOy4YKsyw5qAYu6g4?=
 =?us-ascii?Q?usUSYZodbz4oLlz4hJKdmYEZWLcWPPanfdbSgZHxiNbPog=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <81BC30E2C9097542B69D82816DAB543E@eurprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: VI1PR04MB5136.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 4f0e8e86-a0ef-40f5-5379-08d97e863e56
X-MS-Exchange-CrossTenant-originalarrivaltime: 23 Sep 2021 11:35:28.4479
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 6pu8rbEaY8kQ/pdjDa+nz6agbOJu34BLhQTkGkk9runPFQsAHVS7zmPesYDHw0eOGK11wZn5iraxfrobdxA40A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR0402MB3406
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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 ca=
n
> > > avoid writing a wrong entry.
> >
> > So you're thinking of introducing a new ocelot_mact_learn_with_streamda=
ta(),
> > 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 stat=
ic
> > 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.=