Re: [PATCH net-next 8/8] net: switchdev: merge switchdev_handle_fdb_{add,del}_to_device

From: Vladimir Oltean <vladimir.oltean@nxp.com>
To: Ido Schimmel <idosch@idosch.org>
Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	Ido Schimmel <idosch@nvidia.com>,
	Jakub Kicinski <kuba@kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	Roopa Prabhu <roopa@nvidia.com>,
	Nikolay Aleksandrov <nikolay@nvidia.com>,
	Andrew Lunn <andrew@lunn.ch>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Vivien Didelot <vivien.didelot@gmail.com>,
	Vladimir Oltean <olteanv@gmail.com>, Jiri Pirko <jiri@nvidia.com>
Subject: Re: [PATCH net-next 8/8] net: switchdev: merge switchdev_handle_fdb_{add,del}_to_device
Date: Wed, 27 Oct 2021 11:28:40 +0000	[thread overview]
Message-ID: <20211027112839.6izp7or7ivlt5bzn@skbuf> (raw)
In-Reply-To: <YXkR0NCj1OyEwycZ@shredder>

On Wed, Oct 27, 2021 at 11:46:08AM +0300, Ido Schimmel wrote:
> On Tue, Oct 26, 2021 at 05:27:43PM +0300, Vladimir Oltean wrote:
> > To reduce code churn, the same patch makes multiple changes, since they
> > all touch the same lines:
> > 
> > 1. The implementations for these two are identical, just with different
> >    function pointers. Reduce duplications and name the function pointers
> >    "mod_cb" instead of "add_cb" and "del_cb". Pass the event as argument.
> > 
> > 2. Drop the "const" attribute from "orig_dev". If the driver needs to
> >    check whether orig_dev belongs to itself and then
> >    call_switchdev_notifiers(orig_dev, SWITCHDEV_FDB_OFFLOADED), it
> >    can't, because call_switchdev_notifiers takes a non-const struct
> >    net_device *.
> 
> Regarding 2, I don't mind about the code change itself, but can you
> expand on the motivation? Is this required for a subsequent patchset you
> plan to submit?

Yes, I have a change that calls SWITCHDEV_FDB_OFFLOADED on the orig_dev
(which is always a bridge port, or the bridge itself) instead of the DSA
interface that may be beneath it.

If I understand correctly, the purpose of BR_FDB_OFFLOADED is simply to
show the user that it is offloaded, right?

Things get a little bit interesting when we sniff an FDB event on a
foreign interface that's in a bridge with us, which we trap to the CPU.
There we would be basically telling the user that the FDB entry towards,
say, an Intel card is offloaded, which is not wrong, because it kind of
is, being programmed to hardware, but it's also not "offloaded" in the
sense that there is a data path towards that port which bypasses the
CPU. But that is never the case anyway. When you have a bridge with
mixed hardware domains, then an "offloaded" FDB entry might not always
be offloaded, depending on the hardware domain of the source port.

> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> Reviewed-by: Ido Schimmel <idosch@nvidia.com>