linux-kernel - Re: [net-next v4 4/5] net: mtip: The L2 switch driver for imx287

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250410093507.2caeed2e@wsk>
Date: Thu, 10 Apr 2025 09:35:07 +0200
From: Lukasz Majewski <lukma@...x.de>
To: Andrew Lunn <andrew@...n.ch>
Cc: Andrew Lunn <andrew+netdev@...n.ch>, 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>, Shawn Guo
 <shawnguo@...nel.org>, Sascha Hauer <s.hauer@...gutronix.de>, Pengutronix
 Kernel Team <kernel@...gutronix.de>, Fabio Estevam <festevam@...il.com>,
 Richard Cochran <richardcochran@...il.com>, netdev@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org, Stefan Wahren
 <wahrenst@....net>
Subject: Re: [net-next v4 4/5] net: mtip: The L2 switch driver for imx287

Hi Andrew,

> > +static int mtip_ndev_port_link(struct net_device *ndev,
> > +			       struct net_device *br_ndev,
> > +			       struct netlink_ext_ack *extack)
> > +{
> > +	struct mtip_ndev_priv *priv = netdev_priv(ndev),
> > *other_priv;
> > +	struct switch_enet_private *fep = priv->fep;
> > +	struct net_device *other_ndev;
> > +
> > +	/* Check if one port of MTIP switch is already bridged */
> > +	if (fep->br_members && !fep->br_offload) {
> > +		/* Get the second bridge ndev */
> > +		other_ndev = fep->ndev[fep->br_members - 1];
> > +		other_priv = netdev_priv(other_ndev);
> > +		if (other_priv->master_dev != br_ndev) {
> > +			NL_SET_ERR_MSG_MOD(extack,
> > +					   "L2 offloading only
> > possible for the same bridge!");
> > +			return notifier_from_errno(-EOPNOTSUPP);  
> 
> This is not an error condition as such. The switch cannot do it, so
> -EOPNOTSUPP is correct,

Ok, so the:

return notifier_from_errno(-EOPNOTSUPP);

is correct.


> but the Linux software bridge can, and it
> will. So there is no need to use NL_SET_ERR_MSG_MOD().
> 

This NL_SET_ERR_MSG_MOD() only is relevant for two ports managed by
MTIP L2 switch driver.

As a result - other bridges created by other drivers will not follow
this execution path.

Considering the above - I would keep this message - it is informative
for the potential user (similar approach has been taken when HSR driver
was added for KSZ9477 switch).

> 
> > +		}
> > +
> > +		fep->br_offload = 1;
> > +		mtip_switch_dis_port_separation(fep);
> > +		mtip_clear_atable(fep);
> > +	}
> > +
> > +	if (!priv->master_dev)
> > +		priv->master_dev = br_ndev;
> > +
> > +	fep->br_members |= BIT(priv->portnum - 1);
> > +
> > +	dev_dbg(&ndev->dev,
> > +		"%s: ndev: %s br: %s fep: 0x%x members: 0x%x
> > offload: %d\n",
> > +		__func__, ndev->name,  br_ndev->name, (unsigned
> > int)fep,
> > +		fep->br_members, fep->br_offload);
> > +
> > +	return NOTIFY_DONE;
> > +}
> > +
> > +static void mtip_netdevice_port_unlink(struct net_device *ndev)
> > +{
> > +	struct mtip_ndev_priv *priv = netdev_priv(ndev);
> > +	struct switch_enet_private *fep = priv->fep;
> > +
> > +	dev_dbg(&ndev->dev, "%s: ndev: %s members: 0x%x\n",
> > __func__,
> > +		ndev->name, fep->br_members);
> > +
> > +	fep->br_members &= ~BIT(priv->portnum - 1);
> > +	priv->master_dev = NULL;
> > +
> > +	if (!fep->br_members) {
> > +		fep->br_offload = 0;
> > +		mtip_switch_en_port_separation(fep);
> > +		mtip_clear_atable(fep);
> > +	}  
> 
> This does not look quite correct. So you disable port separation once
> both ports are a member of the same bridge. So you should enable port
> separation as soon as one leaves. So if fep->br_members has 0 or 1
> bits set, you need to enable port separation.

Yes, the if (!fep->br_members)

shall be replaced with:

if (fep->br_members && fep->br_offload)

to avoid situation when disabling second port would re-enable
separation and clear atable.

> 
> 	Andrew




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@...x.de

Content of type "application/pgp-signature" skipped