On Thu, Dec 10, 2020 at 00:21, Vladimir Oltean <olte...@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).
