Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 27 Jun 2022 12:40:29 +0300
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Horatiu Vultur <horatiu.vultur@microchip.com>
Cc:     linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        UNGLinuxDriver@microchip.com, davem@davemloft.net,
        edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
        linux@armlinux.org.uk
Subject: Re: [PATCH net-next 5/8] net: lan966x: Add lag support for lan966x.
Message-ID: <20220627094029.el2m6k6w6ypgtwqg@skbuf>
References: <20220626130451.1079933-1-horatiu.vultur@microchip.com>
 <20220626130451.1079933-6-horatiu.vultur@microchip.com>
 <20220626141139.kbwhpgmwzp7rpxgy@skbuf>
 <20220627064612.vzz2sd7kxpxnprxc@soft-dev3-1.localhost>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220627064612.vzz2sd7kxpxnprxc@soft-dev3-1.localhost>
Precedence: bulk

On Mon, Jun 27, 2022 at 08:46:12AM +0200, Horatiu Vultur wrote:
> > This incorrect logic seems to have been copied from ocelot from before
> > commit a14e6b69f393 ("net: mscc: ocelot: fix incorrect balancing with
> > down LAG ports").
> > 
> > The issue is that you calculate bond_mask with only_active_ports=true.
> > This means the for_each_set_bit() will not iterate through the inactive
> > LAG ports, and won't set the bond_mask as the PGID destination for those
> > ports.
> > 
> > That isn't what is desired; as explained in that commit, inactive LAG
> > ports should be removed via the aggregation PGIDs and not via the
> > destination PGIDs. Otherwise, an FDB entry targeted towards the
> > LAG (effectively towards the "primary" LAG port, whose logical port ID
> > gives the LAG ID) will not egress even the "secondary" LAG port if the
> > primary's link is down.
> 
> Thanks for looking at this.
> That is correct, ocelot was the source of inspiration. The issue that
> you described in the mentioned commit is fixed in the last patch of this
> series.
> I will have a look at your commit and will try to integrated it. Thanks.

I figured that would be the case, although I didn't really understand
the explanation from patch 8/8 (arguably, there it is said that the
switch tries to send on the down port, not that it won't send on the up
port, which is more relevant information). But in any case, it would be
good to introduce code that works from the beginning, rather than fix it
up in a follow-up patch. I believe that the commit I referenced is a
simplification either way, since it removes the "only_active_ports"
argument from the bond mask function.