Re: [PATCH v3 net-next 2/2] net: dsa: Implement tc offloading for drop target.

[Vladimir Oltean <olteanv@gmail.com>]

Again, please use an adequate commit prefix. In this case that is
"net: dsa: mv88e6xxx: ".

On Mon, Apr 04, 2022 at 12:48:26PM +0200, Mattias Forsblad wrote:
> Add the ability to handle tc matchall drop HW offloading for Marvell
> switches.
> 
> Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com>
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 64f4fdd02902..84e319520d36 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -1436,7 +1436,7 @@ static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port)
>  	 * bridge group.
>  	 */
>  	dsa_switch_for_each_port(other_dp, ds)
> -		if (other_dp->type == DSA_PORT_TYPE_CPU ||
> +		if ((other_dp->type == DSA_PORT_TYPE_CPU && dp->bridge->local_rcv_effective) ||

In the light of the idea that we should keep dsa_bridge :: have_foreign
an independent variable, maybe there should be a static inline
dsa_bridge_local_rcv(const struct dsa_bridge *bridge) helper which
returns bridge->have_foreign || bridge->local_rcv. Then you could use
that here.

Also note that said dsa_bridge_local_rcv() function returns a loop
invariant, so you should consider caching the result before using it in
dsa_switch_for_each_port().

>  		    other_dp->type == DSA_PORT_TYPE_DSA ||
>  		    dsa_port_bridge_same(dp, other_dp))
>  			pvlan |= BIT(other_dp->index);
> @@ -6439,6 +6439,26 @@ static void mv88e6xxx_port_mirror_del(struct dsa_switch *ds, int port,
>  	mutex_unlock(&chip->reg_lock);
>  }
>  
> +static int mv88e6xxx_bridge_local_rcv(struct dsa_switch *ds, int port,
> +				      struct dsa_mall_drop_tc_entry *drop)

I think you should ask yourself some questions about passing the "drop"
argument to ->bridge_local_rcv then never using it...

> +{
> +	struct mv88e6xxx_chip *chip = ds->priv;
> +	struct dsa_port *dp;
> +	int err;
> +
> +	dp = dsa_to_port(ds, port);
> +	if (!dp)
> +		return -EINVAL;
> +
> +	mutex_lock(&chip->reg_lock);
> +
> +	err = mv88e6xxx_bridge_map(chip, *dp->bridge);
> +
> +	mutex_unlock(&chip->reg_lock);
> +
> +	return err;
> +}
> +
>  static int mv88e6xxx_port_pre_bridge_flags(struct dsa_switch *ds, int port,
>  					   struct switchdev_brport_flags flags,
>  					   struct netlink_ext_ack *extack)
> @@ -6837,6 +6857,7 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
>  	.port_mdb_del           = mv88e6xxx_port_mdb_del,
>  	.port_mirror_add	= mv88e6xxx_port_mirror_add,
>  	.port_mirror_del	= mv88e6xxx_port_mirror_del,
> +	.bridge_local_rcv	= mv88e6xxx_bridge_local_rcv,
>  	.crosschip_bridge_join	= mv88e6xxx_crosschip_bridge_join,
>  	.crosschip_bridge_leave	= mv88e6xxx_crosschip_bridge_leave,
>  	.port_hwtstamp_set	= mv88e6xxx_port_hwtstamp_set,
> -- 
> 2.25.1
>