Re: [PATCH net-next v3 1/3] net: dsa: add support for bridge flags

[Vivien Didelot <vivien.didelot@gmail.com>]

Hi Russell,

On Wed, 20 Feb 2019 12:36:48 +0000, Russell King <rmk+kernel@armlinux.org.uk> wrote:
> The Linux bridge implementation allows various properties of the bridge
> to be controlled, such as flooding unknown unicast and multicast frames.
> This patch adds the necessary DSA infrastructure to allow the Linux
> bridge support to control these properties for DSA switches.
> 
> We implement this by providing two new methods: one to get the switch-
> wide support bitmask, and another to set the properties.

This is not true anymore ;-)

> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  include/net/dsa.h  |  2 ++
>  net/dsa/dsa_priv.h |  2 ++
>  net/dsa/port.c     | 16 ++++++++++++++++
>  net/dsa/slave.c    |  6 ++++++
>  4 files changed, 26 insertions(+)
> 
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 7f2a668ef2cc..2c2c10812814 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -400,6 +400,8 @@ struct dsa_switch_ops {
>  	void	(*port_stp_state_set)(struct dsa_switch *ds, int port,
>  				      u8 state);
>  	void	(*port_fast_age)(struct dsa_switch *ds, int port);
> +	int	(*port_egress_floods)(struct dsa_switch *ds, int port,
> +				      bool unicast, bool multicast);
>  
>  	/*
>  	 * VLAN support
> diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
> index 1f4972dab9f2..f4f99ec29f5d 100644
> --- a/net/dsa/dsa_priv.h
> +++ b/net/dsa/dsa_priv.h
> @@ -160,6 +160,8 @@ int dsa_port_mdb_add(const struct dsa_port *dp,
>  		     struct switchdev_trans *trans);
>  int dsa_port_mdb_del(const struct dsa_port *dp,
>  		     const struct switchdev_obj_port_mdb *mdb);
> +int dsa_port_bridge_flags(const struct dsa_port *dp, unsigned long flags,
> +			  struct switchdev_trans *trans);
>  int dsa_port_vlan_add(struct dsa_port *dp,
>  		      const struct switchdev_obj_port_vlan *vlan,
>  		      struct switchdev_trans *trans);
> diff --git a/net/dsa/port.c b/net/dsa/port.c
> index 2d7e01b23572..b84d010fb165 100644
> --- a/net/dsa/port.c
> +++ b/net/dsa/port.c
> @@ -177,6 +177,22 @@ int dsa_port_ageing_time(struct dsa_port *dp, clock_t ageing_clock,
>  	return dsa_port_notify(dp, DSA_NOTIFIER_AGEING_TIME, &info);
>  }
>  
> +int dsa_port_bridge_flags(const struct dsa_port *dp, unsigned long flags,
> +			  struct switchdev_trans *trans)
> +{
> +	struct dsa_switch *ds = dp->ds;
> +	int port = dp->index;
> +
> +	if (switchdev_trans_ph_prepare(trans))
> +		return 0;
> +
> +	if (ds->ops->port_egress_floods)
> +		ds->ops->port_egress_floods(ds, port, flags & BR_FLOOD,
> +					    flags & BR_MCAST_FLOOD);

Even though we're not supposed to fail in the switchdev commit phase, I'd
prefer not to ignore the return code.

> +
> +	return 0;
> +}
> +
>  int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr,
>  		     u16 vid)
>  {
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 2e5e7c04821b..f99161c3b1ea 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -295,6 +295,9 @@ static int dsa_slave_port_attr_set(struct net_device *dev,
>  	case SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME:
>  		ret = dsa_port_ageing_time(dp, attr->u.ageing_time, trans);
>  		break;
> +	case SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS:
> +		ret = dsa_port_bridge_flags(dp, attr->u.brport_flags, trans);
> +		break;
>  	default:
>  		ret = -EOPNOTSUPP;
>  		break;
> @@ -384,6 +387,9 @@ static int dsa_slave_port_attr_get(struct net_device *dev,
>  	switch (attr->id) {
>  	case SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT:
>  		attr->u.brport_flags_support = 0;
> +		if (ds->ops->port_egress_floods)
> +			attr->u.brport_flags_support |= BR_FLOOD |
> +							BR_MCAST_FLOOD;
>  		break;
>  	default:
>  		return -EOPNOTSUPP;

Otherwise, LGTM:

Reviewed-by: Vivien Didelot <vivien.didelot@gmail.com>