Message-ID: <20260206022640.3197138-1-kuba@kernel.org>
Date: Thu,  5 Feb 2026 18:26:40 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: lukasz.majewski@...lbox.org
Cc: Jakub Kicinski <kuba@...nel.org>,
	imx@...ts.linux.dev,
	andrew+netdev@...n.ch,
	linux-kernel@...r.kernel.org,
	krzk+dt@...nel.org,
	davem@...emloft.net,
	kernel@...gutronix.de,
	devicetree@...r.kernel.org,
	horms@...nel.org,
	festevam@...il.com,
	andrew@...n.ch,
	conor+dt@...nel.org,
	pabeni@...hat.com,
	shawnguo@...nel.org,
	linux-arm-kernel@...ts.infradead.org,
	wahrenst@....net,
	richardcochran@...il.com,
	s.hauer@...gutronix.de,
	netdev@...r.kernel.org,
	edumazet@...gle.com,
	robh@...nel.org
Subject: Re: [net-next,v23,2/7] net: mtip: The L2 switch driver for imx287

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: mtip: The L2 switch driver for imx287

This patch provides support for More Than IP L2 switch embedded in the
imx287 SoC, a two port switch placed between uDMA[01] and MAC-NET[01]
for offloading network traffic.

> diff --git a/drivers/net/ethernet/freescale/mtipsw/Kconfig b/drivers/net/ethernet/freescale/mtipsw/Kconfig
> new file mode 100644
> index 000000000000..7767bd7c707b
> --- /dev/null
> +++ b/drivers/net/ethernet/freescale/mtipsw/Kconfig
> @@ -0,0 +1,13 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config FEC_MTIP_L2SW
> +	tristate "MoreThanIP L2 switch support to FEC driver"

This isn't a bug, but the vendor name formatting is inconsistent between
the description line ("MoreThanIP") and the help text below ("MoreThan
IP"). The vendor should be consistently named throughout the file.

> +	depends on OF
> +	depends on NET_SWITCHDEV
> +	depends on BRIDGE
> +	depends on SOC_IMX28 || COMPILE_TEST
> +	help
> +	  This enables support for the MoreThan IP L2 switch on i.MX
> +	  SoCs (e.g. iMX287). It offloads bridging to this IP block's

The SoC name "iMX287" should be "i.MX287" to match NXP's standard naming
convention used throughout the kernel for i.MX SoCs.

> diff --git a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c
> new file mode 100644
> index 000000000000..0a193a12e160
> --- /dev/null
> +++ b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c

[ ... ]

> +static void mtip_mgnt_timer(struct timer_list *t)
> +{
> +	struct switch_enet_private *fep = timer_container_of(fep, t,
> +							     timer_mgnt);

Does this code cause undefined behavior? The timer_container_of macro
expands to container_of(t, typeof(*fep), timer_mgnt), which dereferences
the uninitialized 'fep' variable in the typeof() expression. This is the
first statement in the function where 'fep' is being declared and assigned
simultaneously.

Looking at how other timer callbacks use this macro, shouldn't this be:

	struct switch_enet_private *fep = container_of(t,
		struct switch_enet_private, timer_mgnt);

This will cause a crash when the timer fires at the mod_timer() call site
in mtip_config_switch().

[ ... ]

> +static void mtip_switch_tx(struct switch_enet_private *fep)
> +{
> +}
> +
> +static int mtip_switch_rx(struct net_device *dev, int budget)
> +{
> +	return -ENOMEM;
> +}

Can the driver actually send or receive packets with these implementations?

Looking at the call chain:
  mtip_interrupt() schedules NAPI on RX interrupt ->
  mtip_rx_napi() calls mtip_switch_rx() ->
  mtip_switch_rx() always returns -ENOMEM

This means all receive interrupts are treated as out-of-memory conditions
and no packets are ever delivered to the network stack. Similarly,
mtip_switch_tx() is an empty stub that does nothing.

The commit message states "It can be used interchangeably with current
FEC driver" which suggests a functional driver, but the network interface
cannot actually send or receive packets. Should the commit message clarify
this is an incomplete implementation, or are these functions missing their
implementation?

[ ... ]

> +static int mtip_parse_of(struct switch_enet_private *fep,
> +			 struct device_node *np)
> +{
> +	struct device_node *p;
> +	unsigned int port_num;
> +	int ret = 0;
> +
> +	p = of_get_child_by_name(np, "ethernet-ports");
> +
> +	for_each_available_child_of_node_scoped(p, port) {
> +		if (of_property_read_u32(port, "reg", &port_num))
> +			continue;
> +
> +		if (!(port_num == 1 || port_num == 2)) {
> +			dev_err(&fep->pdev->dev,
> +				"%s: The switch supports up to %d ports!\n",
> +				__func__, SWITCH_EPORT_NUMBER);
> +			goto of_get_err;
> +		}
> +
> +		fep->n_ports = port_num;
> +		ret = of_get_mac_address(port, &fep->mac[port_num - 1][0]);
> +		if (ret)
> +			dev_dbg(&fep->pdev->dev,
> +				"of_get_mac_address(%pOF) failed (%d)!\n",
> +				port, ret);
> +
> +		ret = of_property_read_string(port, "label",
> +					      &fep->ndev_name[port_num - 1]);
> +		if (ret < 0) {
> +			dev_err(&fep->pdev->dev,
> +				"%s: Cannot get ethernet port name (%d)!\n",
> +				__func__, ret);
> +			goto of_get_err;
> +		}
> +
> +		ret = of_get_phy_mode(port, &fep->phy_interface[port_num - 1]);
> +		if (ret < 0) {
> +			dev_err(&fep->pdev->dev,
> +				"%s: Cannot get PHY mode (%d)!\n", __func__,
> +				ret);
> +			goto of_get_err;
> +		}
> +
> +		fep->phy_np[port_num - 1] = of_parse_phandle(port,
> +							     "phy-handle", 0);

Can this code leak OF node references?

If the first port (port_num=1) successfully obtains its PHY handle via
of_parse_phandle() and stores the reference in phy_np[0], but a
subsequent port fails during of_property_read_string() for the 'label'
property or of_get_phy_mode(), the error path jumps to of_get_err which
only releases the parent node 'p'. The PHY handle reference obtained for
the first port is never released.

The error path returns to mtip_sw_probe() which returns error without
calling mtip_parse_of_cleanup(), leaking the phy_np[0] reference. Note
that line 1191 correctly calls mtip_parse_of_cleanup(fep) before goto,
showing the cleanup is needed for these earlier error paths too.

> +		if (!fep->phy_np[port_num - 1]) {
> +			mtip_parse_of_cleanup(fep);
> +			dev_err(&fep->pdev->dev,
> +				"%s: PHY handle not defined!\n", __func__);
> +			ret = -ENODEV;
> +			goto of_get_err;
> +		}
> +	}
> +
> + of_get_err:
> +	of_node_put(p);
-- 
pw-bot: cr

Powered by blists - more mailing lists