Re: [net-next PATCH v3 8/8] net: dsa: realtek: rtl8365mb: add bridge port flags

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: dsa: realtek: rtl8365mb: add bridge port flags

This patch adds support for bridge port flags (BR_LEARNING, BR_FLOOD,
BR_MCAST_FLOOD, BR_BCAST_FLOOD) for rtl8365mb, adds new ops to the shared
realtek_ops structure, and introduces rtl83xx_port_bridge_flags and
rtl83xx_setup_port_flood_control helpers in the shared rtl83xx module.
All flooding types are enabled by default at port setup time.

> diff --git a/drivers/net/dsa/realtek/rtl8365mb_main.c b/drivers/net/dsa/realtek/rtl8365mb_main.c
> index 1b8034311b17..7c296f6a0a05 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb_main.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_main.c
> @@ -304,6 +304,21 @@
>  #define   RTL8365MB_MSTI_CTRL_PORT_STATE_MASK(_physport) \
>  		(0x3 << RTL8365MB_MSTI_CTRL_PORT_STATE_OFFSET((_physport)))
>  
> +/* Unknown unicast DA flooding port mask */
> +#define RTL8365MB_UNKNOWN_UNICAST_FLOODING_PMASK_REG		0x0890
> +#define   RTL8365MB_UNKNOWN_UNICAST_FLOODING_PMASK_MASK		0x07FF
> +
> +/* Unknown multicast DA flooding port mask */
> +#define RTL8365MB_UNKNOWN_MULTICAST_FLOODING_PMASK_REG		0x0891
> +#define   RTL8365MB_UNKNOWN_MULTICAST_FLOODING_PMASK_MASK	0x07FF
> +
> +/* Broadcast flooding port mask */
> +#define RTL8365MB_UNKNOWN_BROADCAST_FLOODING_PMASK_REG		0x0892
> +#define   RTL8365MB_UNKNOWN_BROADCAST_FLOODING_PMASK_MASK	0x07FF

[Low]
The three _PMASK_MASK defines (all 0x07FF) describing the 11-bit valid
port-mask field are introduced here, but the helpers below pass BIT(port)
directly as the regmap_update_bits mask and never reference these macros.
There is also no BUILD_BUG_ON or bounds guard relating BIT(port) to the
hardware field width.

Should these _MASK macros be dropped, or used in something like a
BUILD_BUG_ON asserting BIT(RTL8365MB_MAX_NUM_PORTS - 1) fits within the
valid field?

> +
> +#define RTL8365MB_SUPPORTED_BRIDGE_FLAGS \
> +	    (BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD | BR_BCAST_FLOOD)
> +
>  /* Miscellaneous port configuration register, incl. VLAN egress mode */
>  #define RTL8365MB_PORT_MISC_CFG_REG_BASE			0x000E
>  #define RTL8365MB_PORT_MISC_CFG_REG(_p) \
> @@ -1450,6 +1465,49 @@ static int rtl8365mb_port_set_learning(struct realtek_priv *priv, int port,
>  			    enable ? RTL8365MB_LEARN_LIMIT_MAX : 0);
>  }
>  
> +static int rtl8365mb_port_set_ucast_flood(struct realtek_priv *priv, int port,
> +					  bool enable)
> +{
> +	/* Frames with unknown unicast DA will be flooded to a programmable
> +	 * port mask that by default includes all ports. Add or remove
> +	 * the specified port from this port mask accordingly.
> +	 */
> +	return regmap_update_bits(priv->map,
> +				  RTL8365MB_UNKNOWN_UNICAST_FLOODING_PMASK_REG,
> +				  BIT(port), enable ? BIT(port) : 0);
> +}

[ ... ]

> +static int rtl8365mb_port_pre_bridge_flags(struct dsa_switch *ds, int port,
> +					   struct switchdev_brport_flags flags,
> +					   struct netlink_ext_ack *extack)
> +{
> +	struct realtek_priv *priv = ds->priv;
> +
> +	dev_dbg(priv->dev, "pre_bridge_flags port:%d flags:%lx supported:%lx",
> +		port, flags.mask, RTL8365MB_SUPPORTED_BRIDGE_FLAGS);
> +
> +	if (flags.mask & ~RTL8365MB_SUPPORTED_BRIDGE_FLAGS)
> +		return -EINVAL;
> +
> +	return 0;
> +}

[Low]
Is there a reason the supported-mask lives in the per-chip driver while the
apply path in the shared module independently decides support by NULL-checking
each ops pointer at runtime?

The pre_bridge_flags contract in DSA/switchdev is that pre_bridge_flags must
reject any flag that bridge_flags cannot later apply. Today rtl8365mb
populates all four port_set_* ops and the hardcoded mask agrees, so there is
no user-visible bug.

However, a future sibling driver reusing the exported
rtl83xx_port_bridge_flags with a mismatched per-chip pre_bridge_flags could
violate that contract, since rtl83xx_port_bridge_flags still returns
-EOPNOTSUPP for a missing ops pointer:

drivers/net/dsa/realtek/rtl83xx.c:rtl83xx_port_bridge_flags() {
    ...
    if (flags.mask & BR_LEARNING) {
        if (!priv->ops->port_set_learning)
            return -EOPNOTSUPP;
        ...
    }
    ...
}

Would it be cleaner to have the shared module own a common pre_bridge_flags
(or a helper that computes the supported mask from the ops vector) so the
two paths cannot drift?

> +
>  static int rtl8365mb_port_set_efid(struct realtek_priv *priv, int port,
>  				   u32 efid)
>  {

[ ... ]