Re: [PATCH net-next v3 6/6] net: lan966x: Add switchdev support

From: Horatiu Vultur <horatiu.vultur@microchip.com>
To: Vladimir Oltean <vladimir.oltean@nxp.com>
Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"kuba@kernel.org" <kuba@kernel.org>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"UNGLinuxDriver@microchip.com" <UNGLinuxDriver@microchip.com>,
	"linux@armlinux.org.uk" <linux@armlinux.org.uk>,
	"f.fainelli@gmail.com" <f.fainelli@gmail.com>,
	"vivien.didelot@gmail.com" <vivien.didelot@gmail.com>,
	"andrew@lunn.ch" <andrew@lunn.ch>
Subject: Re: [PATCH net-next v3 6/6] net: lan966x: Add switchdev support
Date: Mon, 13 Dec 2021 14:37:16 +0100	[thread overview]
Message-ID: <20211213133716.sfcgl4zrmynwagbr@soft-dev3-1.localhost> (raw)
In-Reply-To: <20211213114603.jdvv5htw22vd3azj@skbuf>

The 12/13/2021 11:46, Vladimir Oltean wrote:
> 
> On Thu, Dec 09, 2021 at 05:43:11PM +0100, Horatiu Vultur wrote:
> > > My documentation of CPU_SRC_COPY_ENA says:
> > >
> > > If set, all frames received on this port are
> > > copied to the CPU extraction queue given by
> > > CPUQ_CFG.CPUQ_SRC_COPY.
> > >
> > > I think it was established a while ago that this isn't what promiscuous
> > > mode is about? Instead it is about accepting packets on a port
> > > regardless of whether the MAC DA is in their RX filter or not.
> >
> > Yes, I am aware that this change interprets the things differently and I
> > am totally OK to drop this promisc if it is needed.
> 
> I think we just need to agree on the observable behavior. Promiscuous
> means for an interface to receive packets with unknown destination, and
> while in standalone mode you do support that, in bridge mode you're a
> bit on the edge: the port accepts them but will deliver them anywhere
> except to the CPU. I suppose you could try to make an argument that you
> know better than the bridge, and as long as the use cases for that are
> restricted enough, maybe it could work for most scenarios. I don't know.

I think this requires some proper explanations of the intended
behaviour for both the standalone and bridge mode. I will drop this
promisc for now, as other drivers are doing it and at a later point
send some patch series with all the explanations.

> 
> > > Hence the oddity of your change. I understand what it intends to do:
> > > if this is a standalone port you support IFF_UNICAST_FLT, so you drop
> > > frames with unknown MAC DA. But if IFF_PROMISC is set, then why do you
> > > copy all frames to the CPU? Why don't you just put the CPU in the
> > > unknown flooding mask?
> >
> > Because I don't want the CPU to be in the unknown flooding mask. I want
> > to send frames to the CPU only if it is required.
> 
> What is the strategy through which this driver accepts things like
> pinging the bridge device over IPv6, with the Neighbor Discovery
> protocol having the ICMP6 neighbor solicitation messages delivered to
> (according to my knowledge) an unregistered IPv6 multicast address?
> Whose responsibility is it to notify the driver of that address, and
> does the driver copy those packets to the CPU in the right VLAN?

I think in that case the CPU should be part of the multicast flooding
mask. I will need to look more on this because I don't know much about
the IPv6.

> 
> > > How do you handle migration of an FDB entry pointing towards the CPU,
> > > towards one pointing towards a port?
> >
> > Shouldn't I get 2 calls that the entry is removed from CPU and then
> > added to a port?
> 
> Ok.

-- 
/Horatiu