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

From: Tobias Waldekranz <tobias@waldekranz.com>
To: Vladimir Oltean <olteanv@gmail.com>
Cc: davem@davemloft.net, kuba@kernel.org, andrew@lunn.ch,
	vivien.didelot@gmail.com, f.fainelli@gmail.com,
	j.vosburgh@gmail.com, vfalico@gmail.com, andy@greyhouse.net,
	netdev@vger.kernel.org
Subject: Re: [PATCH v3 net-next 2/4] net: dsa: Link aggregation support
Date: Wed, 09 Dec 2020 15:11:41 +0100	[thread overview]
Message-ID: <87eejz5asi.fsf@waldekranz.com> (raw)
In-Reply-To: <20201209105326.boulnhj5hoaooppz@skbuf>

On Wed, Dec 09, 2020 at 12:53, Vladimir Oltean <olteanv@gmail.com> wrote:
> On Wed, Dec 09, 2020 at 09:37:39AM +0100, Tobias Waldekranz wrote:
>> I will remove `struct dsa_lag` in v4.
>
> Ok, thanks.
> It would be nice if you could also make .port_lag_leave return an int code.

Sure.

> And I think that .port_lag_change passes more arguments than needed to
> the driver.

You mean the `struct netdev_lag_lower_state_info`? Fine by me, it was
mostly to avoid hiding state from the driver if anyone needed it.

>> > I don't think the DSA switch tree is private to anyone.
>>
>> Well I need somewhere to store the association from LAG netdev to LAG
>> ID. These IDs are shared by all chips in the tree. It could be
>> replicated on each ds of course, but that does not feel quite right.
>
> The LAG ID does not have significance beyond the mv88e6xxx driver, does
> it? And even there, it's just a number. You could recalculate all IDs
> dynamically upon every join/leave, and they would be consistent by
> virtue of the fact that you use a formula which ensures consistency
> without storing the LAG ID anywhere. Like, say, the LAG ID is to be
> determined by the first struct dsa_port in the DSA switch tree that has
> dp->bond == lag_dev. The ID itself can be equal to (dp->ds->index *
> MAX_NUM_PORTS + dp->index). All switches will agree on what is the first
> dp in dst, since they iterate in the same way, and any LAG join/leave
> will notify all of them. It has to be like this anyway.

This will not work for mv88e6xxx. The ID is not just an internal number
used by the driver. If that was the case we could just as well use the
LAG netdev pointer for this purpose. This ID is configured in hardware,
and it is shared between blocks in the switch, we can not just
dynamically change them. Neither can we use your formula since this is a
4-bit field.

Another issue is how we are going to handle this in the tagger now,
since we can no longer call dsa_lag_dev_by_id. I.e. with `struct
dsa_lag` we could resolve the LAG ID (which is the only source
information we have in the tag) to the corresponding netdev. This
information is now only available in mv88e6xxx driver. I am not sure how
I am supposed to conjure it up. Ideas?