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;
From:   Vladimir Oltean <vladimir.oltean@nxp.com>
To:     Horatiu Vultur <horatiu.vultur@microchip.com>
CC:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "UNGLinuxDriver@microchip.com" <UNGLinuxDriver@microchip.com>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "edumazet@google.com" <edumazet@google.com>,
        "kuba@kernel.org" <kuba@kernel.org>,
        "pabeni@redhat.com" <pabeni@redhat.com>
Subject: Re: [PATCH net-next v3 5/7] net: lan966x: Add lag support for
 lan966x.
Thread-Topic: [PATCH net-next v3 5/7] net: lan966x: Add lag support for
 lan966x.
Thread-Index: AQHYjYwcNm8cypwpn0eaifU5TXdZ+61rIGKA
Date:   Sat, 2 Jul 2022 14:12:20 +0000
Message-ID: <20220702141219.l75f46sx4hyldfyh@skbuf>
References: <20220701205227.1337160-1-horatiu.vultur@microchip.com>
 <20220701205227.1337160-6-horatiu.vultur@microchip.com>
In-Reply-To: <20220701205227.1337160-6-horatiu.vultur@microchip.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?C+Yn2jK3TIPyPlX/c6lZZdY6R005GPIs8l5T6k0LpKeRD4WVkSPOqLzRLHxP?=
 =?us-ascii?Q?BriXswTYK99kjlr5faI+WsegcWJu3PiiY+3UeCD/Yx9+feGQ6wTjp7p2Bv9m?=
 =?us-ascii?Q?5NEZuBw13l5apNDwu/Ps1GIy181zh23Pl9ZDTqiGBht8jg1rMGAOLBFLU/sd?=
 =?us-ascii?Q?oqTTXhAWVAU486Rwm0o3wZ5syvAGv7sDVy27QEn+oJwpoHFqR11BwqrP6Q2d?=
 =?us-ascii?Q?HTA/AK4p9FVzhGxck/bhLxhi1Vz4RNI1qLBQko1Eo23dsOwd5Bru6ChQGE8b?=
 =?us-ascii?Q?vsFViC+kJbnGzJJz8I30NWyKM4ltd0sFQkMjaLp9jX8+Jx/fJ9p+3LIE1vqZ?=
 =?us-ascii?Q?J2HhRYZhBOM+HiS0PmuihnaacU9Z1O81dlEvcFpqr2UXsC4ptz/V7X5D/eUY?=
 =?us-ascii?Q?oTiTTpi3h1296Ggy3mLTFjOSvlp2feTa0Ny7b1evtS4bgvjjPPxcYTq7zVI0?=
 =?us-ascii?Q?WK5slqAAcdR61ywqpYXyvoyDPNe7N6+wLCD7ra4pxV4NrZUoTP8spjOdcWRK?=
 =?us-ascii?Q?HbOrlSM5NlkLpYyR53+IRzPyhmNf00d1J721N1zs3/x90juHfmuVO0JrGMPR?=
 =?us-ascii?Q?EXrnMZJGKk815llu/8KCjCNTTpJJ/YzxqxGeoUNATCCuYygpEyRLD3eDKpnT?=
 =?us-ascii?Q?3U4lNe/9k5P8XoDpHhKnXKQ7oXJSK62LRCtwVWThNG68yJfyKtUeEaqpeMsY?=
 =?us-ascii?Q?XpdLKpvIDTEe+sL+MNd3brJnJFTwSXhpHXGTbpbbkc5/HneevqGKmAh3OI34?=
 =?us-ascii?Q?odmtljLJkiRBqTTTsXURRW3D0tqk/Bb8e13ANq2wuNmDec5Kk6C+QG9SVh8f?=
 =?us-ascii?Q?Htb+jR9WnE27VAKiiB8HcnlsDpLglUEf00XIbQaKaXQM5RH8i4F3ulnkozZ9?=
 =?us-ascii?Q?UxGuacLwVEpxRgaGr7GgQtdDA7PW2twWIyGLjpw1kHaoxCgMC/8c9K9pJLvJ?=
 =?us-ascii?Q?y1cvkdiMQZfueVgI2wiLqvv1W44HwYpyYaexcB3K9/zfJkm3bWygQSnRNJg2?=
 =?us-ascii?Q?3kpai42HjJXb8irH7O0XFosiZG8JP+Ibi+K+5bPjaGzWVqf7UJ+roSbhQSVg?=
 =?us-ascii?Q?lJ8T9OTDYVaiN2UewQod7iCPnNq/NNBGj7g8qHDtNE7N5aVvi/pKgHYbQKA1?=
 =?us-ascii?Q?oPr8IC1UUZv880QbJWnJf+D+kRydNd0889Pevzdf0QbYxH7b2Krtuxlm/sjw?=
 =?us-ascii?Q?2pBx09GHQqLuc9Z4/gawsrweYSuVMmWsEXBk+1bYPm7EDLzwtEmDMRQOiYHj?=
 =?us-ascii?Q?C92KKz/qyKAYZP8cdqmHFaa5rovWIL5ubbPtk526CFeazX4e/g/6u6k2oXbs?=
 =?us-ascii?Q?LsQ+F4o+tb42wAc0ENODr/k7bQ1zWojnCkjjw0jlx0RqUO7PmyKLNhGcTvNn?=
 =?us-ascii?Q?QczstdHGHz0Unr5iuNWeNYKJRLXJQ1dy2qyYyxPNOi2jQ5XdANXDbZCC2hQ6?=
 =?us-ascii?Q?xF68aZOv/VR0a/mvaahJFjbN1JsuoF29zgvFVW6R6eA1LDLP50jdeoZCedlD?=
 =?us-ascii?Q?zooy2wRlrqVxYcsuqIy/wvBN1ok2mdyiqeSqTyTI88QZAhAF3AbOerOLMlLK?=
 =?us-ascii?Q?6bouS5+z5jQJ66xbRzzZMjxXbYySJV11sRnBRa6Qampc/jPDroBBHXC930Z3?=
 =?us-ascii?Q?9w=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <4B5F7B9A5D31EE419E101B7AB312692D@eurprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: VI1PR04MB5136.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 790c317f-1cfb-45a2-2698-08da5c34e0ac
X-MS-Exchange-CrossTenant-originalarrivaltime: 02 Jul 2022 14:12:20.3154
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: ja9f3kEwVnKDywT16js3mKq9WxeDN4LPudBwoAroZCHHMZ0TyuHZQRmVSPxKkzlIVv7e/MTEhouwhtd1mYrbww==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM6PR04MB6088
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, Jul 01, 2022 at 10:52:25PM +0200, Horatiu Vultur wrote:
> Add link aggregation hardware offload support for lan966x
>=20
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---

Superficially the code looks OK (maybe if you could only remove the
trailing "." from the commit message). A comment below.

> +int lan966x_lag_port_prechangeupper(struct net_device *dev,
> +				    struct netdev_notifier_changeupper_info *info)
> +{
> +	struct lan966x_port *port =3D netdev_priv(dev);
> +	struct lan966x *lan966x =3D port->lan966x;
> +	struct netdev_lag_upper_info *lui;
> +	struct netlink_ext_ack *extack;
> +
> +	extack =3D netdev_notifier_info_to_extack(&info->info);
> +	lui =3D info->upper_info;
> +	if (!lui)
> +		return NOTIFY_DONE;
> +
> +	if (lui->tx_type !=3D NETDEV_LAG_TX_TYPE_HASH) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "LAG device using unsupported Tx type");
> +		return notifier_from_errno(-EINVAL);
> +	}
> +
> +	switch (lui->hash_type) {
> +	case NETDEV_LAG_HASH_L2:
> +		lan_wr(ANA_AGGR_CFG_AC_DMAC_ENA_SET(1) |
> +		       ANA_AGGR_CFG_AC_SMAC_ENA_SET(1),
> +		       lan966x, ANA_AGGR_CFG);
> +		break;
> +	case NETDEV_LAG_HASH_L34:
> +		lan_wr(ANA_AGGR_CFG_AC_IP6_TCPUDP_ENA_SET(1) |
> +		       ANA_AGGR_CFG_AC_IP4_TCPUDP_ENA_SET(1) |
> +		       ANA_AGGR_CFG_AC_IP4_SIPDIP_ENA_SET(1),
> +		       lan966x, ANA_AGGR_CFG);
> +		break;
> +	case NETDEV_LAG_HASH_L23:
> +		lan_wr(ANA_AGGR_CFG_AC_DMAC_ENA_SET(1) |
> +		       ANA_AGGR_CFG_AC_SMAC_ENA_SET(1) |
> +		       ANA_AGGR_CFG_AC_IP6_TCPUDP_ENA_SET(1) |
> +		       ANA_AGGR_CFG_AC_IP4_TCPUDP_ENA_SET(1),
> +		       lan966x, ANA_AGGR_CFG);

ANA_AGGR_CFG is global to the switch, yet it constantly changes with the
hash_type of each new offloaded LAG. So a LAG that hashes on L3/L4 will
break the hash mode of an existing L2 LAG. I think you should implement
a system that disallows changes to ANA_AGGR_CFG as long as there are
users of a certain mode.

> +		break;
> +	default:
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "LAG device using unsupported hash type");
> +		return notifier_from_errno(-EINVAL);
> +	}
> +
> +	return NOTIFY_OK;
> +}=