Re: [PATCH v3 net-next 2/4] net: dsa: Link aggregation support

[Tobias Waldekranz <tobias@waldekranz.com>]

On Thu, Dec 10, 2020 at 00:21, Vladimir Oltean <olteanv@gmail.com> wrote:
> On Wed, Dec 09, 2020 at 11:01:25PM +0100, Tobias Waldekranz wrote:
>> It is not the Fibonacci sequence or anything, it is an integer in the
>> range 0..num_lags-1. I realize that some hardware probably allocate IDs
>> from some shared (and thus possibly non-contiguous) pool. Maybe ocelot
>> works like that. But it seems reasonable to think that at least some
>> other drivers could make use of a linear range.
>
> In the ocelot RFC patches that I've sent to the list yesterday, you
> could see that the ports within the same bond must have the same logical
> port ID (as opposed to regular mode, when each port has a logical ID
> equal to its physical ID, i.e. swp0 -> 0, swp1 -> 1, etc). We can't use
> the contiguous LAG ID assignment that you do in DSA, because maybe we
> have swp1 and swp2 in a bond, and the LAG ID you give that bond is 0.
> But if we assign logical port ID 0 to physical ports 1 and 2, then we
> end up also bonding with swp0... So what is done in ocelot is that the
> LAG ID is derived from the index of the first port that is part of the
> bond, and the logical port IDs are all assigned to that value. It's
> really simple when you think about it. It would have probably been the
> same for Marvell too if it weren't for that cross-chip thing.
>
> If I were to take a look at Florian's b53-bond branch, I do see that
> Broadcom switches also expect a contiguous range of LAG IDs:
> https://github.com/ffainelli/linux/tree/b53-bond
>
> So ok, maybe ocelot is in the minority. Not an issue. If you add that
> lookup table in the DSA layer, then you could get your linear "LAG ID"
> by searching through it using the struct net_device *bond as key.
> Drivers which don't need this linear array will just not use it.

Great, I can work with that.

>> > I think that there is a low practical risk that the assumption will not
>> > hold true basically forever. But I also see why you might like your
>> > approach more. Maybe Vivien, Andrew, Florian could also chime in and we
>> > can see if struct dsa_lag "bothers" anybody else except me (bothers in
>> > the sense that it's an unnecessary complication to hold in DSA). We
>> > could, of course, also take the middle ground, which would be to keep
>> > the 16-entry array of bonding net_device pointers in DSA, and you could
>> > still call your dsa_lag_dev_by_id() and pretend it's generic, and that
>> > would just look up that table. Even with this middle ground, we are
>> > getting rid of the port lists and of the reference counting, which is
>> > still a welcome simplification in my book.
>>
>> Yeah I agree that we can trim it down to just the array. Going beyond
>> that point, i.e. doing something like how sja1105 works, is more painful
>> but possible if Andrew can live with it.
>
> I did not get the reference to sja1105 here. That does not support
> bonding offload, but does work properly with software bridging thanks to
> your patches.

Yeah sorry, I should have explained that better.

I meant in the sense that it shares information between the tagger and
the driver (struct sja1105_tagger_data).