From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org 7A06D60BF2 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org 76D7560BD7 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=corigine.onmicrosoft.com; s=selector2-corigine-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=FKVGHeb22qzfhzH0WK8BTep6YXmUCvDuCjosBg2TlME=; b=k1g+GkDzccdzfKSIT/dygh80YJRCc31I/6xqX09BwfCf33Deb4MU1wi4kawYFLkmFLeTXEpkkKvUfeyEdlY99Vt3pvZOj3GzgAf9/ZnYo5/Jc+ajG3DJlYI4OvHHnxp2R9qVayXIf2tZpjk2edxhWY6ucJhV73L1qqVDOe88VXE= Date: Mon, 20 Feb 2023 15:11:19 +0100 From: Simon Horman Message-ID: References: <20230130173429.3577450-6-netdev@kapio-technology.com> <9b12275969a204739ccfab972d90f20f@kapio-technology.com> <20230203204422.4wrhyathxfhj6hdt@skbuf> <4abbe32d007240b9c3aea9c8ca936fa3@kapio-technology.com> <87fsb83q5s.fsf@kapio-technology.com> <20230217174431.bkkvfmtno56mfh5a@skbuf> Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230217174431.bkkvfmtno56mfh5a@skbuf> MIME-Version: 1.0 Subject: Re: [Bridge] [PATCH net-next 5/5] net: dsa: mv88e6xxx: implementation of dynamic ATU entries List-Id: Linux Ethernet Bridging List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladimir Oltean Cc: Andrew Lunn , Alexandre Belloni , Nikolay Aleksandrov , Kurt Kanzenbach , Eric Dumazet , Hans Schultz , Ivan Vecera , Florian Fainelli , "moderated list:ETHERNET BRIDGE" , Russell King , Roopa Prabhu , kuba@kernel.org, Paolo Abeni , =?utf-8?B?Q2zDqW1lbnQgTMOpZ2Vy?= , Christian Marangi , Woojung Huh , Landen Chao , Jiri Pirko , Hauke Mehrtens , Sean Wang , DENG Qingfang , Claudiu Manoil , "moderated list:ARM/Mediatek SoC support" , Matthias Brugger , "moderated list:ARM/Mediatek SoC support" , netdev@vger.kernel.org, open list , "maintainer:MICROCHIP KSZ SERIES ETHERNET SWITCH DRIVER" , "open list:RENESAS RZ/N1 A5PSW SWITCH DRIVER" , davem@davemloft.net On Fri, Feb 17, 2023 at 07:44:31PM +0200, Vladimir Oltean wrote: > On Tue, Feb 14, 2023 at 10:14:55PM +0100, Hans Schultz wrote: > > On Mon, Feb 06, 2023 at 17:02, Simon Horman wrote: > > > > > > Just to clarify my suggestion one last time, it would be along the lines > > > of the following (completely untested!). I feel that it robustly covers > > > all cases for fdb_flags. And as a bonus doesn't need to be modified > > > if other (unsupported) flags are added in future. > > > > > > if (fdb_flags & ~(DSA_FDB_FLAG_DYNAMIC)) > > > return -EOPNOTSUPP; > > > > > > is_dynamic = !!(fdb_flags & DSA_FDB_FLAG_DYNAMIC) > > > if (is_dynamic) > > > state = MV88E6XXX_G1_ATU_DATA_STATE_UC_AGE_7_NEWEST; > > > > > > > > > And perhaps for other drivers: > > > > > > if (fdb_flags & ~(DSA_FDB_FLAG_DYNAMIC)) > > > return -EOPNOTSUPP; > > > if (fdb_flags) > > > return 0; > > > > > > Perhaps a helper would be warranted for the above. > > > > How would such a helper look? Inline function is not clean. > > > > > > > > But in writing this I think that, perhaps drivers could return -EOPNOTSUPP > > > for the DSA_FDB_FLAG_DYNAMIC case and the caller can handle, rather tha > > > propagate, -EOPNOTSUPP. > > > > I looked at that, but changing the caller is also a bit ugly. > > Answering on behalf of Simon, and hoping he will agree. Sorry for not responding earlier - I was on vacation last week. TBH my idea was not nearly as well developed as the one you describe below. But yes, I agree this is an interesting approach. > You are missing a big opportunity to make the kernel avoid doing useless work. > The dsa_slave_fdb_event() function runs in atomic switchdev notifier context, > and schedules a deferred workqueue item - dsa_schedule_work() - to get sleepable > context to program hardware. > > Only that scheduling a deferred work item is not exactly cheap, so we try to > avoid doing that unless we know that we'll end up doing something with that > FDB entry once the deferred work does get scheduled: > > /* Check early that we're not doing work in vain. > * Host addresses on LAG ports still require regular FDB ops, > * since the CPU port isn't in a LAG. > */ > if (dp->lag && !host_addr) { > if (!ds->ops->lag_fdb_add || !ds->ops->lag_fdb_del) > return -EOPNOTSUPP; > } else { > if (!ds->ops->port_fdb_add || !ds->ops->port_fdb_del) > return -EOPNOTSUPP; > } > > What you should be doing is you should be using the pahole tool to find > a good place for a new unsigned long field in struct dsa_switch, and add > a new field ds->supported_fdb_flags. You should extend the early checking > from dsa_slave_fdb_event() and exit without doing anything if the > (fdb->flags & ~ds->supported_fdb_flags) expression is non-zero. > > This way you would kill 2 birds with 1 stone, since individual drivers > would no longer need to check the flags; DSA would guarantee not calling > them with unsupported flags. > > It would be also very good to reach an agreement with switchdev > maintainers regarding the naming of the is_static/is_dyn field. > > It would also be excellent if you could rename "fdb_flags" to just > "flags" within DSA.