Message-ID: <20260202102326.vz5qhg6wzzje553v@skbuf>
Date: Mon, 2 Feb 2026 12:23:26 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Daniel Golle <daniel@...rotopia.org>
Cc: Andrew Lunn <andrew@...n.ch>, "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>,
	Simon Horman <horms@...nel.org>, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	Frank Wunderlich <frankwu@....de>, Chad Monroe <chad@...roe.io>,
	Cezary Wilmanski <cezary.wilmanski@...ran.com>,
	Avinash Jayaraman <ajayaraman@...linear.com>,
	Bing tao Xu <bxu@...linear.com>, Liang Xu <lxu@...linear.com>,
	Juraj Povazanec <jpovazanec@...linear.com>,
	"Fanni (Fang-Yi) Chan" <fchan@...linear.com>,
	"Benny (Ying-Tsan) Weng" <yweng@...linear.com>,
	"Livia M. Rosu" <lrosu@...linear.com>,
	John Crispin <john@...ozen.org>
Subject: Re: [PATCH v12 4/4] net: dsa: add basic initial driver for MxL862xx
 switches

On Sun, Feb 01, 2026 at 02:25:13AM +0000, Daniel Golle wrote:
> Add very basic DSA driver for MaxLinear's MxL862xx switches.
> 
> In contrast to previous MaxLinear switches the MxL862xx has a built-in
> processor that runs a sophisticated firmware based on Zephyr RTOS.
> Interaction between the host and the switch hence is organized using a
> software API of that firmware rather than accessing hardware registers
> directly.
> 
> Add descriptions of the most basic firmware API calls to access the
> built-in MDIO bus hosting the 2.5GE PHYs, basic port control as well as
> setting up the CPU port.
> 
> Implement a very basic DSA driver using that API which is sufficient to
> get packets flowing between the user ports and the CPU port.
> 
> The firmware offers all features one would expect from a modern switch
> hardware, they are going to be added one by one in follow-up patch
> series.
> 
> Signed-off-by: Daniel Golle <daniel@...rotopia.org>
> ---
> v12:
>  * list switch variants in Kconfig starting with dash ('-')
>  * remove the __packed attribute on structs which are
>    naturally packed according to C rules (ie. one other struct
>    with only a single byte-aligned member)
>  * log error in mxl862xx_port_disable()
>  * introduce !NULL check for return value of dsa_to_port() in
>    mxl862xx_add_single_port_bridge
>  * check cpu_dp being non-NULL before dereferencing dp->cpu_dp
>  * use non-racy and deterministic name for MII bus
>  * skip ports without cpu_dp assigned in mxl862xx_setup_cpu_bridge()
>    to avoid potential NULL-pointer dereference
>  * call dev_set_drvdata() only after dsa_register_switch() has been
>    successfully completed

I have nothing else to comment except to request you to undo some of the
changes requested by AI review. Sorry I didn't have time to comment on v11.

> +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;
> +
> +	if (!dp)
> +		return -ENODEV;

I'm sorry, I can't accept code that fears its own shadow. Some people,
when they see a defensive NULL check think there is a valid reason
behind it and will try to see what that reason is.

If dsa_to_port() would have possibly returned NULL, mxl862xx_port_setup() ->
dsa_is_cpu_port(), which _is_ implemented using dsa_to_port(), would
have already crashed the kernel and this test here would have been too
little, too late.

But the kernel doesn't crash there, because calling dsa_to_port() is
safe for these arguments, and this test is simply logically inconsistent
with previous unconditional in this call path.

> +
> +	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;
> +	if (dp->cpu_dp)
> +		br_port_cfg.bridge_port_map[0] = cpu_to_le16(BIT(dp->cpu_dp->index));
> +
> +	return MXL862XX_API_WRITE(ds->priv, MXL862XX_BRIDGEPORT_CONFIGSET, br_port_cfg);
> +}
> +
> +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;
> +	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, "%s-mii", dev_name(dev));
> +	bus->read_c45 = mxl862xx_phy_read_c45_mii_bus;
> +	bus->write_c45 = mxl862xx_phy_write_c45_mii_bus;
> +	bus->read = mxl862xx_phy_read_mii_bus;
> +	bus->write = mxl862xx_phy_write_mii_bus;
> +	bus->parent = dev;
> +	bus->phy_mask = ~ds->phys_mii_mask;
> +
> +	mdio_np = of_get_child_by_name(dev->of_node, "mdio");
> +	if (!mdio_np)
> +		return -ENODEV;

As per the dt-bindings patch, the "mdio" child node is not required.
But here it is.

> +
> +	ret = devm_of_mdiobus_register(dev, bus, mdio_np);
> +	of_node_put(mdio_np);
> +
> +	return ret;
> +}
> +
> +static int mxl862xx_wait_ready(struct dsa_switch *ds)
> +{
> +	struct mxl862xx_sys_fw_image_version ver = {};
> +	unsigned long start = jiffies, timeout;
> +	struct mxl862xx_priv *priv = ds->priv;
> +	struct mxl862xx_cfg cfg = {};
> +	int ret;
> +
> +	timeout = start + msecs_to_jiffies(MXL862XX_READY_TIMEOUT_MS);
> +	msleep(2000); /* it always takes at least 2 seconds */
> +	do {
> +		ret = MXL862XX_API_READ_QUIET(priv, SYS_MISC_FW_VERSION, ver);
> +		if (ret || !ver.iv_major)
> +			goto not_ready_yet;
> +
> +		/* being able to perform CFGGET indicates that
> +		 * the firmware is ready
> +		 */
> +		ret = MXL862XX_API_READ_QUIET(priv,
> +					      MXL862XX_COMMON_CFGGET,
> +					      cfg);
> +		if (ret)
> +			goto not_ready_yet;
> +
> +		dev_info(ds->dev, "switch ready after %ums, firmware %u.%u.%u (build %u)\n",
> +			 jiffies_to_msecs(jiffies - start),
> +			 ver.iv_major, ver.iv_minor,
> +			 le16_to_cpu(ver.iv_revision),
> +			 le32_to_cpu(ver.iv_build_num));
> +		return 0;
> +
> +not_ready_yet:
> +		msleep(MXL862XX_READY_POLL_MS);
> +	} while (time_before(jiffies, timeout));
> +
> +	dev_err(ds->dev, "switch not responding after reset\n");
> +	return -ETIMEDOUT;
> +}
> +
> +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)
> +			continue;

All user ports are given a valid non-NULL dp->cpu_dp pointer. I strongly
oppose introducing FUD in the code. If there are valid reasons behind
this I'm all ears, but there aren't.

> +
> +		if (dp->cpu_dp->index != port)
> +			continue;
> +
> +		bridge_port_map |= BIT(dp->index);
> +	}
> +	br_port_cfg.bridge_port_map[0] |= cpu_to_le16(bridge_port_map);
> +
> +	return MXL862XX_API_WRITE(priv, MXL862XX_BRIDGEPORT_CONFIGSET, br_port_cfg);
> +}
> +
> +static int mxl862xx_setup(struct dsa_switch *ds)
> +{
> +	struct mxl862xx_priv *priv = ds->priv;
> +	int ret;
> +
> +	ret = mxl862xx_reset(priv);
> +	if (ret)
> +		return ret;
> +
> +	ret = mxl862xx_wait_ready(ds);
> +	if (ret)
> +		return ret;
> +
> +	return mxl862xx_setup_mdio(ds);
> +}
> +
> +static int mxl862xx_port_setup(struct dsa_switch *ds, int port)
> +{
> +	bool is_cpu_port = dsa_is_cpu_port(ds, port);
> +	int ret;
> +
> +	/* disable port and flush MAC entries */
> +	ret = mxl862xx_port_state(ds, port, false);
> +	if (ret)
> +		return ret;
> +
> +	mxl862xx_port_fast_age(ds, port);
> +
> +	/* skip setup for unused and DSA ports */
> +	if (dsa_is_unused_port(ds, port) ||
> +	    dsa_is_dsa_port(ds, port))
> +		return 0;

Each of dsa_is_cpu_port(), dsa_is_unused_port(), dsa_is_dsa_port() are
implemented using dsa_to_port(), and that loops over dst->ports in order
to find "dp" just to return dp->index.

OTOH if you call dsa_to_port() only once, we have dsa_port_is_cpu(),
dsa_port_is_unused(), dsa_port_is_dsa() which can be called on that and
have lower complexity. Please do that.

You do _not_ need to test whether dsa_to_port() returns NULL, if "ds"
and "port" were passed to you by DSA in the context of a DSA callback.
Any exception to this rule will be explicitly noted in the documentation.

> +
> +	/* configure tag protocol */
> +	ret = mxl862xx_configure_sp_tag_proto(ds, port, is_cpu_port);
> +	if (ret)
> +		return ret;
> +
> +	/* assign CTP port IDs */
> +	ret = mxl862xx_configure_ctp_port(ds, port, port,
> +					  is_cpu_port ? 32 - port : 1);
> +	if (ret)
> +		return ret;
> +
> +	if (is_cpu_port)
> +		/* assign user ports to CPU port bridge */
> +		return mxl862xx_setup_cpu_bridge(ds, port);
> +
> +	/* setup single-port bridge for user ports */
> +	return mxl862xx_add_single_port_bridge(ds, port);
> +}
> +
> +static void mxl862xx_phylink_get_caps(struct dsa_switch *ds, int port,
> +				      struct phylink_config *config)
> +{
> +	config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | MAC_10 |
> +				   MAC_100 | MAC_1000 | MAC_2500FD;
> +
> +	__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> +		  config->supported_interfaces);
> +}
> +
> +static const struct dsa_switch_ops mxl862xx_switch_ops = {
> +	.get_tag_protocol = mxl862xx_get_tag_protocol,
> +	.setup = mxl862xx_setup,
> +	.port_setup = mxl862xx_port_setup,
> +	.phylink_get_caps = mxl862xx_phylink_get_caps,
> +	.port_enable = mxl862xx_port_enable,
> +	.port_disable = mxl862xx_port_disable,
> +	.port_fast_age = mxl862xx_port_fast_age,
> +};
> +
> +static void mxl862xx_phylink_mac_config(struct phylink_config *config,
> +					unsigned int mode,
> +					const struct phylink_link_state *state)
> +{
> +}
> +
> +static void mxl862xx_phylink_mac_link_down(struct phylink_config *config,
> +					   unsigned int mode,
> +					   phy_interface_t interface)
> +{
> +}
> +
> +static void mxl862xx_phylink_mac_link_up(struct phylink_config *config,
> +					 struct phy_device *phydev,
> +					 unsigned int mode,
> +					 phy_interface_t interface,
> +					 int speed, int duplex,
> +					 bool tx_pause, bool rx_pause)
> +{
> +}
> +
> +static const struct phylink_mac_ops mxl862xx_phylink_mac_ops = {
> +	.mac_config = mxl862xx_phylink_mac_config,
> +	.mac_link_down = mxl862xx_phylink_mac_link_down,
> +	.mac_link_up = mxl862xx_phylink_mac_link_up,
> +};
> +
> +static int mxl862xx_probe(struct mdio_device *mdiodev)
> +{
> +	struct device *dev = &mdiodev->dev;
> +	struct mxl862xx_priv *priv;
> +	struct dsa_switch *ds;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->mdiodev = mdiodev;
> +
> +	ds = devm_kzalloc(dev, sizeof(*ds), GFP_KERNEL);
> +	if (!ds)
> +		return -ENOMEM;
> +
> +	priv->ds = ds;
> +	ds->dev = dev;
> +	ds->priv = priv;
> +	ds->ops = &mxl862xx_switch_ops;
> +	ds->phylink_mac_ops = &mxl862xx_phylink_mac_ops;
> +	ds->num_ports = MXL862XX_MAX_PORTS;
> +
> +	ret = dsa_register_switch(ds);
> +	if (ret)
> +		return ret;
> +
> +	dev_set_drvdata(dev, ds);

There is no point in doing this, calling dev_set_drvdata() before and
then simply "return dsa_register_switch()" is perfectly fine. You also
won't need the "ret" variable.

> +
> +	return 0;
> +}
> +
> +static void mxl862xx_remove(struct mdio_device *mdiodev)
> +{
> +	struct dsa_switch *ds = dev_get_drvdata(&mdiodev->dev);
> +
> +	if (!ds)
> +		return;
> +
> +	dsa_unregister_switch(ds);
> +}
> +
> +static void mxl862xx_shutdown(struct mdio_device *mdiodev)
> +{
> +	struct dsa_switch *ds = dev_get_drvdata(&mdiodev->dev);
> +
> +	if (!ds)
> +		return;
> +
> +	dsa_switch_shutdown(ds);
> +
> +	dev_set_drvdata(&mdiodev->dev, NULL);
> +}
> +
> +static const struct of_device_id mxl862xx_of_match[] = {
> +	{ .compatible = "maxlinear,mxl86282" },
> +	{ .compatible = "maxlinear,mxl86252" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, mxl862xx_of_match);
> +
> +static struct mdio_driver mxl862xx_driver = {
> +	.probe  = mxl862xx_probe,
> +	.remove = mxl862xx_remove,
> +	.shutdown = mxl862xx_shutdown,
> +	.mdiodrv.driver = {
> +		.name = "mxl862xx",
> +		.of_match_table = mxl862xx_of_match,
> +	},
> +};
> +
> +mdio_module_driver(mxl862xx_driver);
> +
> +MODULE_DESCRIPTION("Driver for MaxLinear MxL862xx switch family");
> +MODULE_LICENSE("GPL");

Powered by blists - more mailing lists