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

Lukasz Majewski
2025-04-10T08:08:05Z

[-- Attachment #1: Type: text/plain, Size: 3303 bytes --]

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@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]