Re: [PATCH net-next v1 2/2] net: dsa: microchip: Add partial ACL support for ksz9477 switches

[Vladimir Oltean <olteanv@gmail.com>]

Hi Oleksij,

I only took a superficial look, and hence, here are some superficial comments.

On Tue, Apr 11, 2023 at 07:24:55PM +0200, Oleksij Rempel wrote:
> The ACL also implements a count function, generating an interrupt
> instead of a forwarding action. It can be used as a watchdog timer or an
> event counter.

Is the interrupt handled here? I didn't see cls_flower_stats().

> The ACL consists of three parts: matching rules, action
> rules, and processing entries. Multiple match conditions can be either
> AND'ed or OR'ed together.
> 
> This patch introduces support for a subset of the available ACL
> functionality, specifically layer 2 matching and prioritization of
> matched packets. For example:
> 
> tc qdisc add dev lan2 clsact
> tc filter add dev lan2 ingress protocol 0x88f7 flower skip_sw hw_tc 7
> 
> tc qdisc add dev lan1 clsact
> tc filter add dev lan1 ingress protocol 0x88f7 flower skip_sw hw_tc 7

Have you considered the "skbedit priority" action as opposed to hw_tc?

> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/dsa/microchip/Makefile      |   2 +-
>  drivers/net/dsa/microchip/ksz9477.c     |   7 +
>  drivers/net/dsa/microchip/ksz9477.h     |   7 +
>  drivers/net/dsa/microchip/ksz9477_acl.c | 950 ++++++++++++++++++++++++
>  drivers/net/dsa/microchip/ksz_common.c  |  40 +
>  drivers/net/dsa/microchip/ksz_common.h  |   1 +
>  6 files changed, 1006 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/net/dsa/microchip/ksz9477_acl.c
> 
> diff --git a/drivers/net/dsa/microchip/Makefile b/drivers/net/dsa/microchip/Makefile
> index 48360cc9fc68..50851519c9a1 100644
> --- a/drivers/net/dsa/microchip/Makefile
> +++ b/drivers/net/dsa/microchip/Makefile
> @@ -1,7 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  obj-$(CONFIG_NET_DSA_MICROCHIP_KSZ_COMMON)	+= ksz_switch.o
>  ksz_switch-objs := ksz_common.o
> -ksz_switch-objs += ksz9477.o
> +ksz_switch-objs += ksz9477.o ksz9477_acl.o
>  ksz_switch-objs += ksz8795.o
>  ksz_switch-objs += lan937x_main.o
>  
> diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> index bf13d47c26cf..f2c34ab3d3eb 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
> @@ -1151,6 +1151,7 @@ int ksz9477_setup(struct dsa_switch *ds)
>  {
>  	struct ksz_device *dev = ds->priv;
>  	int ret = 0;
> +	int i;
>  
>  	ds->mtu_enforcement_ingress = true;
>  
> @@ -1176,6 +1177,12 @@ int ksz9477_setup(struct dsa_switch *ds)
>  	/* enable global MIB counter freeze function */
>  	ksz_cfg(dev, REG_SW_MAC_CTRL_6, SW_MIB_COUNTER_FREEZE, true);
>  
> +	for (i = 0; i < dev->info->port_cnt; i++) {
> +		if (i == dev->cpu_port)
> +			continue;

dsa_switch_for_each_user_port()

> +		ksz9477_port_acl_init(dev, i);

Don't ignore the return code.

> +	}
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/net/dsa/microchip/ksz9477.h b/drivers/net/dsa/microchip/ksz9477.h
> index b6f7e3c46e3f..5201bccda0ed 100644
> --- a/drivers/net/dsa/microchip/ksz9477.h
> +++ b/drivers/net/dsa/microchip/ksz9477.h
> @@ -59,4 +59,11 @@ int ksz9477_switch_init(struct ksz_device *dev);
>  void ksz9477_switch_exit(struct ksz_device *dev);
>  void ksz9477_port_queue_split(struct ksz_device *dev, int port);
>  
> +
> +int ksz9477_port_acl_init(struct ksz_device *dev, int port);
> +int ksz9477_cls_flower_add(struct dsa_switch *ds, int port,
> +			   struct flow_cls_offload *cls, bool ingress);
> +int ksz9477_cls_flower_del(struct dsa_switch *ds, int port,
> +			   struct flow_cls_offload *cls, bool ingress);
> +
>  #endif
> diff --git a/drivers/net/dsa/microchip/ksz9477_acl.c b/drivers/net/dsa/microchip/ksz9477_acl.c
> new file mode 100644
> index 000000000000..319158ebbdd9
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/ksz9477_acl.c
> +#define KSZ9477_ACL_ENTRY_SIZE		18
> +#define KSZ9477_ACL_MAX_ENTRIES		16
> +#define KSZ9477_MAC_TC			7

MAC_TC or MAX_TC?

> +	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
> +		u8 bcast[] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
> +
> +		flow_rule_match_eth_addrs(rule, &ematch);
> +
> +		if (!is_zero_ether_addr(ematch.key->src)) {
> +			if (!ether_addr_equal(ematch.mask->src, bcast))

is_broadcast_ether_addr()

> +				goto not_full_mask_err;
> +
> +			src_mac = ematch.key->src;
> +		}
> +
> +		if (!is_zero_ether_addr(ematch.key->dst)) {
> +			if (!ether_addr_equal(ematch.mask->dst, bcast))
> +				goto not_full_mask_err;
> +
> +			dst_mac = ematch.key->dst;
> +		}
> +	}
> +
> +	acles = &acl->acles;
> +	/* ACL supports only one MAC per entry */
> +	required_entries = src_mac && dst_mac ? 2 : 1;