netdev - Re: [PATCH v4 net-next 4/5] net: dsa: mv88e6xxx: Link aggregation support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87h7olbiy3.fsf@waldekranz.com>
Date:   Wed, 16 Dec 2020 21:21:40 +0100
From:   Tobias Waldekranz <tobias@...dekranz.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     davem@...emloft.net, kuba@...nel.org, andrew@...n.ch,
        vivien.didelot@...il.com, f.fainelli@...il.com,
        j.vosburgh@...il.com, vfalico@...il.com, andy@...yhouse.net,
        netdev@...r.kernel.org
Subject: Re: [PATCH v4 net-next 4/5] net: dsa: mv88e6xxx: Link aggregation support

On Wed, Dec 16, 2020 at 21:04, Vladimir Oltean <olteanv@...il.com> wrote:
> On Wed, Dec 16, 2020 at 05:00:55PM +0100, Tobias Waldekranz wrote:
>> Support offloading of LAGs to hardware. LAGs may be attached to a
>> bridge in which case VLANs, multicast groups, etc. are also offloaded
>> as usual.
>> 
>> Signed-off-by: Tobias Waldekranz <tobias@...dekranz.com>
>> ---
>>  drivers/net/dsa/mv88e6xxx/chip.c    | 298 +++++++++++++++++++++++++++-
>>  drivers/net/dsa/mv88e6xxx/global2.c |   8 +-
>>  drivers/net/dsa/mv88e6xxx/global2.h |   5 +
>>  drivers/net/dsa/mv88e6xxx/port.c    |  21 ++
>>  drivers/net/dsa/mv88e6xxx/port.h    |   5 +
>>  5 files changed, 329 insertions(+), 8 deletions(-)
>> 
>> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
>> index eafe6bedc692..bd80b3939157 100644
>> --- a/drivers/net/dsa/mv88e6xxx/chip.c
>> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
>> @@ -1189,7 +1189,8 @@ static int mv88e6xxx_set_mac_eee(struct dsa_switch *ds, int port,
>>  }
>>  
>>  /* Mask of the local ports allowed to receive frames from a given fabric port */
>> -static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port)
>> +static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port,
>> +			       struct net_device **lag)
>>  {
>>  	struct dsa_switch *ds = chip->ds;
>>  	struct dsa_switch_tree *dst = ds->dst;
>> @@ -1201,6 +1202,9 @@ static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port)
>>  	list_for_each_entry(dp, &dst->ports, list) {
>>  		if (dp->ds->index == dev && dp->index == port) {
>>  			found = true;
>> +
>> +			if (dp->lag_dev && lag)
>> +				*lag = dp->lag_dev;
>>  			break;
>>  		}
>>  	}
>
> I'll let Andrew and Vivien have the decisive word, who are vastly more
> familiar with mv88e6xxx than I am, but to me it looks like a bit of a
> hack to put this logic here, especially since one of the two callers
> (i.e. half) doesn't even care about the LAG.
>
>> @@ -1396,14 +1402,21 @@ static int mv88e6xxx_mac_setup(struct mv88e6xxx_chip *chip)
>>  
>>  static int mv88e6xxx_pvt_map(struct mv88e6xxx_chip *chip, int dev, int port)
>>  {
>> +	struct net_device *lag = NULL;
>>  	u16 pvlan = 0;
>>  
>>  	if (!mv88e6xxx_has_pvt(chip))
>>  		return 0;
>>  
>>  	/* Skip the local source device, which uses in-chip port VLAN */
>> -	if (dev != chip->ds->index)
>> -		pvlan = mv88e6xxx_port_vlan(chip, dev, port);
>> +	if (dev != chip->ds->index) {
>> +		pvlan = mv88e6xxx_port_vlan(chip, dev, port, &lag);
>> +
>> +		if (lag) {
>> +			dev = MV88E6XXX_G2_PVT_ADRR_DEV_TRUNK;
>> +			port = dsa_lag_id(chip->ds->dst, lag);
>> +		}
>> +	}
>
> What about the following, which should remove the need of modifying mv88e6xxx_port_vlan:
>
> static int mv88e6xxx_pvt_map(struct mv88e6xxx_chip *chip, int dev, int port)
> {
> 	struct dsa_switch *ds = chip->ds;
> 	struct net_device *lag = NULL;
> 	u16 pvlan = 0;
>
> 	if (!mv88e6xxx_has_pvt(chip))
> 		return 0;
>
> 	/* Skip the local source device, which uses in-chip port VLAN */
> 	if (dev != ds->index) {
> 		pvlan = mv88e6xxx_port_vlan(chip, dev, port);
> 		struct dsa_switch *other_ds;
> 		struct dsa_port *other_dp;
>
> 		other_ds = dsa_switch_find(ds->dst->index, dev);
> 		other_dp = dsa_to_port(other_ds, port);
>
> 		/* XXX needs an explanation for the reinterpreted values of
> 		 * dev and port
> 		 */
> 		if (other_dp->lag_dev) {
> 			dev = MV88E6XXX_G2_PVT_ADRR_DEV_TRUNK;
> 			port = dsa_lag_id(ds->dst, other_dp->lag_dev);
> 		}
> 	}
>
> 	return mv88e6xxx_g2_pvt_write(chip, dev, port, pvlan);
> }

Yeah I agree. This is really a left-over from the RFC that I meant to
clean-up. I will change it for v5.

>>  
>>  	return mv88e6xxx_g2_pvt_write(chip, dev, port, pvlan);
>>  }
>> @@ -5375,6 +5388,271 @@ static int mv88e6xxx_port_egress_floods(struct dsa_switch *ds, int port,
>>  	return err;
>>  }
>>