From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f67.google.com (mail-wr1-f67.google.com [209.85.221.67]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 7D4AD3382C2 for ; Mon, 2 Feb 2026 09:44:11 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.221.67 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1770025453; cv=none; b=ua00rjTxH/dz1Ek10e6bDDrMidIC+2PGYWSZQ2z03pqcl3feWEI6AIrFfh4dZ5oSX05B3kPj+xWw6g5z9ELlk6Pgi6lIYhr+f15eJB1o0FNpOGVfpJrN1BBpuUSfjTzxIbRZ3zv5WtO9esXjKevb2avxbf/rvKsT0N64nZyem/w= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1770025453; c=relaxed/simple; bh=5EL6AsHcVBdeADGxMcjKBj+/YFrUYYn5U1xjZYVD1R0=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=pVkojTXhtcbM+Tw2hGkEz0ZB2k9ZNg9d0jT0oNHzE6uSNtdQtup6K0TjtVs8nN/KKJqpnhkQibiDAv42vqqwstgz8ZzIyDuBlAnwYCIC0TmbhnyGBeYcTZ4hE5VnjSbyXxI7vBWkxK9q9QAslDeyoVyc8Sv13H2JTDLf4K0FeDg= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=g07vD4Td; arc=none smtp.client-ip=209.85.221.67 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="g07vD4Td" Received: by mail-wr1-f67.google.com with SMTP id ffacd0b85a97d-43595901036so552888f8f.2 for ; Mon, 02 Feb 2026 01:44:11 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1770025450; x=1770630250; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=0rIIYAGyPLAQP7m4TA99kT+nOX8OsDiOc0NkESM+vog=; b=g07vD4TdYLrGyQVgQVplCyFnKiXbeflmRduplqxTWOcj5qyhfYdzdzWagR+HnmQhv4 HOWSI+iuUUN5G6s6mGyCxbwycNAdZD0e6pZiZYyXqNVIuFah0brR3N+/agXoOYYRgNlv 6r21hsieVmtSzuI1uGHasNpKqQEQi5PloS7l4n3iYVbfa3AJ2SrfFIcdtLQbF5gQq093 cGsSuRH6nAXd8W2zwknDLWKaIDvqlWmnfixu3Y/IHcKgU2a+nJ18BpgH82ajMegoxDv3 3Fmz2BpXdocJckeTara/hHaUSZ/44YHLHPjIueOaJPyV1HGegQ4LinDx8R9fwHGdRxIX LGOw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1770025450; x=1770630250; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=0rIIYAGyPLAQP7m4TA99kT+nOX8OsDiOc0NkESM+vog=; b=SAHCAiFHny6KzSd9YALoQUmclUGrXA6pvUqiLyPXDKXXKRuCNfyI9LlAGC7MJWC/lA EoJB9SJLVOuGzGu20FL61SxaHoH1ZpG1/AJt3vbvp7vNmSVTADV4pEl9+u6NXQuqQ1v7 z9dkQifCG8UiI3SymfbO6+lwafM9BN7Y+R39tNWqv8sAMk9ngXtrlOnPk5Tk8w2QiYU/ /Om+z1t20MIa+M49NXTmOzkxjcF3Vmz3he6v3eDyYZF8b0qWW7Bp6OJnh6XKfifD7147 fW24ZdlGHTq4zNr3cyssKI6GJmFjuefhhYAeJFpmZRgQKD9uNjeFv9O5unu1zFvwHpnw nZaA== X-Forwarded-Encrypted: i=1; AJvYcCWnduX+N/QBKDAObVGsMK4/D1lrFsH+yaodkpcDY3iL862cddCTR/1NwmlAgVTHTESB+YyC78TKUGAuZm4=@vger.kernel.org X-Gm-Message-State: AOJu0Yx+KWjwf6FPinMA5G0CUTZpqsqXtgXU7wvJU9G1O8lckKyn6YNs AZbhgM8DJndP+kar4ZY8n0ZVNBTU5A4EMx99XAcgLz6OrrCHkVA+w4BB X-Gm-Gg: AZuq6aJAzno9GOrzlYADt8AlzpLD+qvVWYjs3EUqCgDJe1ToTtMKgbs6uK+V2PGCTnZ g27WXwx3x/WVdPmG9TyxCO7YmrVPKkvd12sjcjQfrcOIsdVhU3BQcV3LtvPZyTvixZS1FX3sx0Y MJt8egPI6NZaOL+bi8uwUCTYtxUxK3BizcSsFM1X148tdPUYbproYDYdxRIOcUhTMtdQCBSbwBs CLCATi2q7Ue2Nitz4WeMiyrufUiBGMV/gQFOT7PZDzptHDC2ETrAFu3Rz0de2x0C82kQSpOstGi 4kq5aj/3VOXxedTmiXyFSjFe54Gubblz8aKoCEqyF7hkTpA9SLjFV1veiNlIJFL4QnTxs1zgFy+ GQFBPEcVRheHVWssl6oDdIq2arFkUbl9t3HVfPPMMKf5t5T4U6iL5CGz1kH9esiw+BigdaBu/u/ KE9yM= X-Received: by 2002:a05:600c:818e:b0:477:9dd9:ac57 with SMTP id 5b1f17b1804b1-482db2022f4mr72271205e9.0.1770025449530; Mon, 02 Feb 2026 01:44:09 -0800 (PST) Received: from skbuf ([2a02:2f04:d501:d900:74c4:3a65:f9a9:6c29]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-435e10e4757sm44205033f8f.5.2026.02.02.01.44.07 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 02 Feb 2026 01:44:08 -0800 (PST) Date: Mon, 2 Feb 2026 11:44:05 +0200 From: Vladimir Oltean To: Jakub Kicinski Cc: daniel@makrotopia.org, lxu@maxlinear.com, hkallweit1@gmail.com, yweng@maxlinear.com, ajayaraman@maxlinear.com, andrew@lunn.ch, netdev@vger.kernel.org, bxu@maxlinear.com, krzk+dt@kernel.org, linux-kernel@vger.kernel.org, lrosu@maxlinear.com, chad@monroe.io, conor+dt@kernel.org, devicetree@vger.kernel.org, robh@kernel.org, edumazet@google.com, pabeni@redhat.com, cezary.wilmanski@adtran.com, davem@davemloft.net, john@phrozen.org, frankwu@gmx.de, jpovazanec@maxlinear.com, linux@armlinux.org.uk, fchan@maxlinear.com, horms@kernel.org Subject: Re: [net-next,v11,4/4] net: dsa: add basic initial driver for MxL862xx switches Message-ID: <20260202094405.rsojaz5xmorubjfz@skbuf> References: <46226b74030f76bd04149ed5c92b3e263abbe6c2.1769817939.git.daniel@makrotopia.org> <20260131175243.1122906-2-kuba@kernel.org> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260131175243.1122906-2-kuba@kernel.org> On Sat, Jan 31, 2026 at 09:52:44AM -0800, Jakub Kicinski wrote: > > +static int mxl862xx_add_single_port_bridge(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_bridge_alloc br_alloc = {}; > > + 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; > > + } > > + > > + 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; > > + br_port_cfg.bridge_port_map[0] = cpu_to_le16(BIT(dp->cpu_dp->index)); > > Can this dereference NULL pointers? No. DSA will not make calls to ds->ops->port_setup() with "port" values for which dsa_to_port(ds, port) will not find a struct dsa_port *. > The dsa_to_port() function can return NULL if the port is not found in > the switch tree. The code stores the result in dp but doesn't check if > dp is NULL before dereferencing dp->cpu_dp->index. > > Looking at include/net/dsa.h, dsa_to_port() returns NULL when the port > is not found: > > static inline struct dsa_port *dsa_to_port(struct dsa_switch *ds, int p) > { > struct dsa_switch_tree *dst = ds->dst; > struct dsa_port *dp; > > list_for_each_entry(dp, &dst->ports, list) > if (dp->ds == ds && dp->index == p) > return dp; > > return NULL; > } > > Additionally, even if dp is non-NULL, dp->cpu_dp could also be NULL if > the CPU port hasn't been properly assigned during initialization. mxl862xx_add_single_port_bridge() has been called when all other port types except user ports have been excluded. All user and DSA ports have a non-NULL dp->cpu_dp pointer after dsa_tree_setup_cpu_ports() runs, i.e. also at the time of ds->ops->port_setup(). > > +static int mxl862xx_setup_mdio(struct dsa_switch *ds) > > +{ > > + struct mxl862xx_priv *priv = ds->priv; > > + struct device *dev = ds->dev; > > + struct device_node *mdio_np; > > + struct mii_bus *bus; > > + static int idx; > > + int ret; > > + > > + bus = devm_mdiobus_alloc(dev); > > + if (!bus) > > + return -ENOMEM; > > + > > + bus->priv = priv; > > + ds->user_mii_bus = bus; > > + bus->name = KBUILD_MODNAME "-mii"; > > + snprintf(bus->id, MII_BUS_ID_SIZE, KBUILD_MODNAME "-%d", idx++); > > Can concurrent probes race on the idx variable? > > The static int idx is shared across all driver instances and accessed > without synchronization. If multiple instances of this driver are probed > concurrently on different CPU cores, they could race on the idx++ > operation potentially leading to duplicate MII bus IDs. MII bus IDs must > be unique system-wide. > > [ ... ] Technically ds->ops->setup() runs under dsa2_mutex, but the "static int idx" is still not ideal due to the ever-increasing index upon unbinding and rebinding. > > +static int mxl862xx_setup_cpu_bridge(struct dsa_switch *ds, int port) > > +{ > > + struct mxl862xx_bridge_port_config br_port_cfg = {}; > > + struct mxl862xx_priv *priv = ds->priv; > > + u16 bridge_port_map = 0; > > + struct dsa_port *dp; > > + > > + /* CPU port bridge setup */ > > + br_port_cfg.mask = cpu_to_le32(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.bridge_port_id = cpu_to_le16(port); > > + br_port_cfg.src_mac_learning_disable = false; > > + br_port_cfg.vlan_src_mac_vid_enable = true; > > + br_port_cfg.vlan_dst_mac_vid_enable = true; > > + > > + /* include all assigned user ports in the CPU portmap */ > > + dsa_switch_for_each_user_port(dp, ds) { > > + if (dp->cpu_dp->index != port) > > + continue; > > Can dp->cpu_dp be dereferenced safely here? > > The code dereferences dp->cpu_dp->index without checking if cpu_dp is > NULL. While the DSA framework should ensure user ports have a CPU port > assigned during initialization, there is no explicit NULL check. If > cpu_dp is NULL for any reason such as incomplete initialization or > configuration error, this will cause a NULL pointer dereference and > kernel crash. No incompletely initialized ports are published to dst->ports. > > + ret = dsa_register_switch(ds); > > + if (ret) > > + return ret; > > Is dev_set_drvdata() called too early? > > The function calls dev_set_drvdata(dev, ds) before calling > dsa_register_switch(ds). If dsa_register_switch() fails, the function > returns the error but leaves the driver data pointing to an unregistered > switch. > > This creates a problem for cleanup paths. The mxl862xx_remove() function > retrieves the switch with dev_get_drvdata() and calls > dsa_unregister_switch(ds). If probe failed, ds is not NULL so the check > passes but the switch was never successfully registered. Similarly, > mxl862xx_shutdown() could be called and would also operate on an > unregistered switch. While dsa_switch_shutdown() has a check for > ds->setup flag that provides some protection, this is fragile and could > lead to inconsistent state or crashes in error scenarios. > > [Jakub] AI is a bit pedantic about leaving the drvdata pointer in place > but I guess it's not a bad thing to clean up Can somebody gently explain what is the point? It is easily testable that if mdio_driver :: probe() fails, .remove() or .shutdown() will not be called. It really does not matter whether dev_set_drvdata() was called or not.