From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 6F5BE3EDABA for ; Tue, 31 Mar 2026 09:48:41 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.133.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774950524; cv=none; b=Y5QHx7ZSd0EC4JQXtkVw8JniX2h3esbURJOr0NTE2yH2alHJZa0t6CGtDgMrEK8dMFloZG9C6OJSuMRwtdZ9svNdWv5r9YKC4np8euUwBFalfSjA6v9SVJL2CjKqZSpfRyLW8We1eLdVqv9HQfSivZpNoKBMQ5YDGiJWqVpJYyY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774950524; c=relaxed/simple; bh=OmkQL2rZPo+S3r7wx36gg8PGLqL+K5Qrozwe0KStT10=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=ZKu+CH6MTXB1SoivBmNbHrgWg2D3Phbtz/pVzcb+zLzoN+DY/7BK/P20/BGQZ3qUEPAby++NTPc2+5icReGScDpW6N38r5XwC477Kyr1gTcnE9cdqr6RRHuNm35ljU2CZh/deooy92yJh808yhtpKgO0V3sAb7uNXZMgyY8Wxh4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=ghXP4G0t; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b=UeXrsgPH; arc=none smtp.client-ip=170.10.133.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="ghXP4G0t"; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b="UeXrsgPH" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1774950520; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=WAWTeG+TgfKTJsRPkbNlI/x8xqrSYMpp7Kw/l/cmWps=; b=ghXP4G0tyHs9OlyPaxvcytGDOsRaf1Ejsai/ZylnXqCrhI46ZKDXpPgBP8lZJOyK0vPLh4 o9k/8XZvI1KhvghvqxCa7NLCcKDcV3AFWDBwogkB6E+dPG++HsF8uO6IP+dsr2O/ihDuwP gMjQcgLqoFH8wKPJafITuLtM3F/yc78= Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-86-0qn56nqIMpGPJqMF_IV4oA-1; Tue, 31 Mar 2026 05:48:34 -0400 X-MC-Unique: 0qn56nqIMpGPJqMF_IV4oA-1 X-Mimecast-MFC-AGG-ID: 0qn56nqIMpGPJqMF_IV4oA_1774950513 Received: by mail-wm1-f72.google.com with SMTP id 5b1f17b1804b1-4887e4b338bso3571085e9.1 for ; Tue, 31 Mar 2026 02:48:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=google; t=1774950513; x=1775555313; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=WAWTeG+TgfKTJsRPkbNlI/x8xqrSYMpp7Kw/l/cmWps=; b=UeXrsgPHuiN4fXBscym+goukdHPPb8sAxrAE6ZCXo5k0pX8xERuBfv3yEUOaL32GPf v8TtnMXVek/tHFP7klqR5XZgHSUxJxpPEgAhvbjuVllC3pUkEYGC171GZe6/UJf4m9E0 mCPMO7JKjNQz7vnOi9N4EX2Lqwa98I1+GCm7WWOXZAPXcWup/h9kFlZ+6frvTXubLioR lHVI2WbQ3rQ6SHX3NwodjcmGD61EIVShNlE19+BW5BXW0l5yK9qYuY46bqlsPDJnQsWe rkKkoZh+b8uvmmXtVRnaiTqdIh/VGVZqfyCsO0M45T86eGkJU6nAtkTkdfpaZszzxSba 1QKg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1774950513; x=1775555313; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=WAWTeG+TgfKTJsRPkbNlI/x8xqrSYMpp7Kw/l/cmWps=; b=WaUN1MRXYHxKbCExBJlbx0UBY/nehfV9/cHsntCya3cMYjdBr2FO0oD7Z6aT9B4XKu 38YOlfSuVAF8nL1YPdyiJA0bMAZJHAXTldF/Bpt8e4baKZCNJR6qCKfdoVWUcGJNRg+T xV58qwyMouySKNVTGqvoLw90O2R3ufEMlQPjhkhz6ToWTXhIp/E6+/5Pu6ZeKPSn7YEu C4SQH6x9eqEzIA25na+puHMrNTcVt/4kZULpDpev5K0cAHycoV7O8cA5+P45IkhvxsF4 GfTTVx3ge/JyhjpCxjutUXbL9tKIgo5CKzMeMMwMezRtlblEJexdyXzl29NpO/QoG3Bk KWeg== X-Forwarded-Encrypted: i=1; AJvYcCUCJiFX3TxCkNAlEG63AHWF89GFnnXd8A70ofBquU7R2PscQTkTVq67IJHCCq8fFFmMdRmcGWgQILFutUE=@vger.kernel.org X-Gm-Message-State: AOJu0YwifvOAgJhUXH+UvhqjIZMErq68+nrqcCVIJuA4S2ZFPi7zRxR3 6JR1/+o6O/jnjIR30JmhF3UblIBs0flj2mgXU3Fs8/qgCruMatbKARZZ871JFqMhA6/DLOj6JnN XgsJh0RAgM1vbZYeBAuzr2PG6KZG7uzOLAypVzruWh+g8eBEnG88iV+JH5EvUn6FGzg== X-Gm-Gg: ATEYQzznq/9C2Nx14z8UPVtaAyEAy1hNSWyXCItWCOukd0dOzj43ZdUZUY73ASC6m3v fYAGxLFuBBoYbLKHQtZN/cfrD751X8NMVCr1qL2w98G3dYB2FZiyKyi8Awbnr+79CXbJctmXq29 iYdgdK7m6utesKGkt7e8msACT+JxCLpMdo+0xfn14DTFhmnUPKSNSJvNNo7hsPu7uu/4i94fxWa PmVW5hbdT/qIuRpBGGCWjJgxJxLtgn0BQ8vXMtD9DMMo6fiCK509HFLqSLGSmHHuPI/yaiv7cWW +itT6St/PI7DvoliSsj+loRIwVOT7hPpG6fWWHfqNCAuXaPIvTtzXFY+7TGmSNztar3ucwCzi5Z 0+/QoKU6mUCC3TKlLoUfbrHOshmddMkgIIM5Wy6IZ2lzhphtQZ/mx87kZ X-Received: by 2002:a05:600c:4ecc:b0:46e:59bd:f7e2 with SMTP id 5b1f17b1804b1-4887839a81dmr45299445e9.11.1774950513307; Tue, 31 Mar 2026 02:48:33 -0700 (PDT) X-Received: by 2002:a05:600c:4ecc:b0:46e:59bd:f7e2 with SMTP id 5b1f17b1804b1-4887839a81dmr45299015e9.11.1774950512742; Tue, 31 Mar 2026 02:48:32 -0700 (PDT) Received: from [192.168.88.32] ([212.105.155.58]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4887c536a7fsm25303355e9.1.2026.03.31.02.48.30 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 31 Mar 2026 02:48:31 -0700 (PDT) Message-ID: <3f30c7ba-8053-4443-9fb0-ba628657b385@redhat.com> Date: Tue, 31 Mar 2026 11:48:30 +0200 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH net-next v8 4/4] net: dsa: mxl862xx: implement bridge offloading To: Daniel Golle , Andrew Lunn , Vladimir Oltean , "David S. Miller" , Eric Dumazet , Jakub Kicinski , David Yang , Simon Horman , Russell King , netdev@vger.kernel.org, linux-kernel@vger.kernel.org Cc: Frank Wunderlich , Chad Monroe , Cezary Wilmanski , Liang Xu , "Benny (Ying-Tsan) Weng" , Jose Maria Verdu Munoz , Avinash Jayaraman , John Crispin References: <201af75c81ca093c46ab5c28a9379eeb6e9fa432.1774572749.git.daniel@makrotopia.org> Content-Language: en-US From: Paolo Abeni In-Reply-To: <201af75c81ca093c46ab5c28a9379eeb6e9fa432.1774572749.git.daniel@makrotopia.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 3/27/26 2:01 AM, Daniel Golle wrote: > +static int mxl862xx_set_bridge_port(struct dsa_switch *ds, int port) > +{ > + struct mxl862xx_bridge_port_config br_port_cfg = {}; > + struct dsa_port *dp = dsa_to_port(ds, port); > + struct mxl862xx_priv *priv = ds->priv; > + struct mxl862xx_port *p = &priv->ports[port]; > + u16 bridge_id = dp->bridge ? > + priv->bridges[dp->bridge->num] : p->fid; > + bool enable; > + int i, idx; > + > + br_port_cfg.bridge_port_id = cpu_to_le16(port); > + br_port_cfg.bridge_id = cpu_to_le16(bridge_id); > + br_port_cfg.mask = cpu_to_le32(MXL862XX_BRIDGE_PORT_CONFIG_MASK_BRIDGE_ID | > + MXL862XX_BRIDGE_PORT_CONFIG_MASK_BRIDGE_PORT_MAP | > + MXL862XX_BRIDGE_PORT_CONFIG_MASK_MC_SRC_MAC_LEARNING | > + MXL862XX_BRIDGE_PORT_CONFIG_MASK_EGRESS_SUB_METER); AI reviews say: Does this silently switch the hardware from Independent VLAN Learning (IVL) to Shared VLAN Learning (SVL)? The mask no longer includes MXL862XX_BRIDGE_PORT_CONFIG_MASK_VLAN_BASED_MAC_LEARNING, and the vlan_src_mac_vid_enable and vlan_dst_mac_vid_enable fields are not set. For a VLAN-aware bridge, this appears to break VLAN isolation boundaries, as a MAC address learned on one VLAN could affect forwarding decisions on other VLANs. [ ... ] > +static int mxl862xx_add_single_port_bridge(struct dsa_switch *ds, int port) > +{ > + struct dsa_port *dp = dsa_to_port(ds, port); > + struct mxl862xx_priv *priv = ds->priv; > + int ret; > + > + ret = mxl862xx_allocate_bridge(priv, &priv->ports[port].fid); > + if (ret) { > + dev_err(ds->dev, "failed to allocate a bridge for port %d\n", port); > + return ret; > + } > + > + priv->ports[port].learning = false; > + bitmap_zero(priv->ports[port].portmap, MXL862XX_MAX_BRIDGE_PORTS); > + __set_bit(dp->cpu_dp->index, priv->ports[port].portmap); > + > + ret = mxl862xx_set_bridge_port(ds, port); > + if (ret) > + return ret; AI review says: If mxl862xx_set_bridge_port() fails, does this leak the newly allocated hardware bridge? It looks like there is no call to mxl862xx_free_bridge() on this error path. [...] > +static int mxl862xx_port_bridge_join(struct dsa_switch *ds, int port, > + const struct dsa_bridge bridge, > + bool *tx_fwd_offload, > + struct netlink_ext_ack *extack) > { > - struct mxl862xx_bridge_port_config br_port_cfg = {}; > - struct dsa_port *dp = dsa_to_port(ds, port); > - struct mxl862xx_bridge_alloc br_alloc = {}; > + struct mxl862xx_priv *priv = ds->priv; > + u16 fw_id; > int ret; > > - ret = MXL862XX_API_READ(ds->priv, MXL862XX_BRIDGE_ALLOC, br_alloc); > - if (ret) { > - dev_err(ds->dev, "failed to allocate a bridge for port %d\n", port); > - return ret; > + if (!priv->bridges[bridge.num]) { > + ret = mxl862xx_allocate_bridge(priv, &fw_id); > + if (ret) > + return ret; > + > + priv->bridges[bridge.num] = fw_id; > + > + /* Free bridge here on error, DSA rollback won't. */ > + ret = mxl862xx_sync_bridge_members(ds, &bridge); > + if (ret) { > + mxl862xx_free_bridge(ds, &bridge); > + return ret; AI review says: If mxl862xx_sync_bridge_members() encounters an error partway through updating the ports in its loop, are the already-updated ports left pointing to a bridge ID that is then freed here? It seems the software portmaps and hardware states of the preceding ports are not rolled back, which might leave them corrupted and permanently referencing a freed bridge. > + } > + > + return 0; > } > > - br_port_cfg.bridge_id = br_alloc.bridge_id; > - br_port_cfg.bridge_port_id = cpu_to_le16(port); > - br_port_cfg.mask = cpu_to_le32(MXL862XX_BRIDGE_PORT_CONFIG_MASK_BRIDGE_ID | > - MXL862XX_BRIDGE_PORT_CONFIG_MASK_BRIDGE_PORT_MAP | > - MXL862XX_BRIDGE_PORT_CONFIG_MASK_MC_SRC_MAC_LEARNING | > - MXL862XX_BRIDGE_PORT_CONFIG_MASK_VLAN_BASED_MAC_LEARNING); > - br_port_cfg.src_mac_learning_disable = true; > - br_port_cfg.vlan_src_mac_vid_enable = false; > - br_port_cfg.vlan_dst_mac_vid_enable = false; > - /* As this function is only called for user ports it is safe to rely on > - * cpu_dp being valid > - */ > - br_port_cfg.bridge_port_map[0] = cpu_to_le16(BIT(dp->cpu_dp->index)); > + return mxl862xx_sync_bridge_members(ds, &bridge); > +} > + > +static void mxl862xx_port_bridge_leave(struct dsa_switch *ds, int port, > + const struct dsa_bridge bridge) > +{ > + struct dsa_port *dp = dsa_to_port(ds, port); > + struct mxl862xx_priv *priv = ds->priv; > + struct mxl862xx_port *p = &priv->ports[port]; > + int err; > + > + err = mxl862xx_sync_bridge_members(ds, &bridge); > + if (err) > + dev_err(ds->dev, > + "failed to sync bridge members after port %d left: %pe\n", > + port, ERR_PTR(err)); AI review says: Is the leaving port still present in dp->bridge when this function is called? The DSA core typically clears dp->bridge after the leave callback returns. If so, mxl862xx_sync_bridge_members() would still iterate over the leaving port and incorrectly add it to the hardware portmaps of all remaining bridge members. > > - return MXL862XX_API_WRITE(ds->priv, MXL862XX_BRIDGEPORT_CONFIGSET, br_port_cfg); > + /* Revert leaving port, omitted by the sync above, to its > + * single-port bridge > + */ > + bitmap_zero(p->portmap, MXL862XX_MAX_BRIDGE_PORTS); > + __set_bit(dp->cpu_dp->index, p->portmap); > + p->flood_block = 0; > + err = mxl862xx_set_bridge_port(ds, port); AI review says: Could there be two issues during this revert? First, if dp->bridge is still set as noted above, mxl862xx_set_bridge_port() will evaluate dp->bridge to true and use the shared bridge's hardware FID instead of the port's standalone FID, keeping the port assigned to the shared bridge. Second, if learning was enabled while the port was in the bridge, p->learning remains true here. Should p->learning be set to false when the port reverts to a standalone port to prevent inappropriate MAC learning?