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

[Simon Horman]

On Wed, Jul 19, 2023 at 01:19:29PM +0200, Oleksij Rempel wrote:

...

Hi Oleksij,

> +/**
> + * ksz9477_acl_port_enable - Enables ACL functionality on a given port.
> + * @dev: The ksz_device instance.
> + * @port: The port number on which to enable ACL functionality.
> + *
> + * This function enables ACL functionality on the specified port by configuring
> + * the appropriate control registers. It returns 0 if the operation is
> + * successful, or a negative error code if an error occurs.
> + *
> + * 0xn801 - KSZ9477S 5.2.8.2 Port Priority Control Register
> + *        Bit 7 - Highest Priority
> + *        Bit 6 - OR'ed Priority
> + *        Bit 4 - MAC Address Priority Classification
> + *        Bit 3 - VLAN Priority Classification
> + *        Bit 2 - 802.1p Priority Classification
> + *        Bit 1 - Diffserv Priority Classification
> + *        Bit 0 - ACL Priority Classification
> + *
> + * Note: current driver implementation sets 802.1p priority classification
> + * by default. In this function we add ACL priority classification with OR'ed
> + * priority. According to testing, priority set by ACL will supersede the
> + * 802.1p priority.
> + *
> + * 0xn803 - KSZ9477S 5.2.8.4 Port Authentication Control Register
> + *        Bit 2 - Access Control List (ACL) Enable
> + *        Bits 1:0 - Authentication Mode
> + *                00 = Reserved
> + *                01 = Block Mode. Authentication is enabled. When ACL is
> + *                     enabled, all traffic that misses the ACL rules is
> + *                     blocked; otherwise ACL actions apply.
> + *                10 = Pass Mode. Authentication is disabled. When ACL is
> + *                     enabled, all traffic that misses the ACL rules is
> + *                     forwarded; otherwise ACL actions apply.
> + *                11 = Trap Mode. Authentication is enabled. All traffic is
> + *                     forwarded to the host port. When ACL is enabled, all
> + *                     traffic that misses the ACL rules is blocked; otherwise
> + *                     ACL actions apply.
> + *
> + * Note: we are using Pass Mode here.

kernel-doc gcc-12 W=1 and clang-16 W=1 all complains
that there is a duplicate Note section in the above.

...

> +/**
> + * ksz9477_acl_matching_rule_cfg_l2 - Configure an ACL filtering entry to match
> + *				      L2 types of Ethernet frames
> + * @entry: Pointer to ACL entry buffer
> + * @ethertype: Ethertype value
> + * @eth_addr: Pointer to Ethernet address
> + * @is_src: If true, match the source MAC address; if false, match the
> + *	    destination MAC address
> + *
> + * This function configures an Access Control List (ACL) filtering
> + * entry to match Layer 2 types of Ethernet frames based on the provided
> + * ethertype and Ethernet address. Additionally, it can match either the source
> + * or destination MAC address depending on the value of the is_src parameter.
> + *
> + * Register Descriptions for MD = 01 and ENB != 00 (Layer 2 MAC header
> + * filtering)
> + *
> + * 0x01 - Mode and Enable
> + *        Bits 5:4 - MD (Mode)
> + *                01 = Layer 2 MAC header or counter filtering
> + *        Bits 3:2 - ENB (Enable)
> + *                01 = Comparison is performed only on the TYPE value
> + *                10 = Comparison is performed only on the MAC Address value
> + *                11 = Both the MAC Address and TYPE are tested
> + *        Bit  1   - S/D (Source / Destination)
> + *                0 = Destination address
> + *                1 = Source address
> + *        Bit  0   - EQ (Equal / Not Equal)
> + *                0 = Not Equal produces true result
> + *                1 = Equal produces true result
> + *
> + * 0x02-0x07 - MAC Address
> + *        0x02 - MAC Address [47:40]
> + *        0x03 - MAC Address [39:32]
> + *        0x04 - MAC Address [31:24]
> + *        0x05 - MAC Address [23:16]
> + *        0x06 - MAC Address [15:8]
> + *        0x07 - MAC Address [7:0]
> + *
> + * 0x08-0x09 - EtherType
> + *        0x08 - EtherType [15:8]
> + *        0x09 - EtherType [7:0]
> + */
> +static void ksz9477_acl_matching_rule_cfg_l2(u8 *entry, __be16 ethertype,
> +					     u8 *eth_addr, bool is_src)
> +{
> +	u8 enb = 0;
> +	u8 val;
> +
> +	if (ethertype)
> +		enb |= KSZ9477_ACL_ENB_L2_TYPE;
> +	if (eth_addr)
> +		enb |= KSZ9477_ACL_ENB_L2_MAC;
> +
> +	val = FIELD_PREP(KSZ9477_ACL_MD_MASK, KSZ9477_ACL_MD_L2_MAC) |
> +	      FIELD_PREP(KSZ9477_ACL_ENB_MASK, enb) |
> +	      FIELD_PREP(KSZ9477_ACL_SD_SRC, is_src) | KSZ9477_ACL_EQ_EQUAL;
> +	ksz9477_acl_set_reg(entry, KSZ9477_ACL_PORT_ACCESS_1, val);
> +
> +	if (eth_addr) {
> +		int i;
> +
> +		for (i = 0; i < ETH_ALEN; i++) {
> +			ksz9477_acl_set_reg(entry,
> +					    KSZ9477_ACL_PORT_ACCESS_2 + i,
> +					    eth_addr[i]);
> +		}
> +	}
> +
> +	ksz9477_acl_set_reg(entry, KSZ9477_ACL_PORT_ACCESS_8, ethertype & 0xff);
> +	ksz9477_acl_set_reg(entry, KSZ9477_ACL_PORT_ACCESS_9, ethertype >> 8);

ethertype is big endian.
But host byte order math is done on it in the two lines above.
This doesn't seem right.

...

> +/**
> + * ksz9477_flower_parse_action - Parse flow rule actions for a specified port
> + *				 on a ksz_device.
> + * @dev: The ksz_device instance.
> + * @port: The port number to parse the flow rule actions for.
> + * @extack: The netlink extended ACK for reporting errors.
> + * @cls: The flow_cls_offload instance containing the flow rule.
> + * @entry_idx: The index of the ACL entry to store the action.
> + *
> + * This function checks if the actions in the flow rule are supported by
> + * the device. Currently, only actions that change priorities are supported.
> + * If unsupported actions are encountered, an error message is set in the
> + * extended ACK.
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +static int ksz9477_flower_parse_action(struct ksz_device *dev, int port,
> +				       struct netlink_ext_ack *extack,
> +				       struct flow_cls_offload *cls,
> +				       int entry_idx)
> +{
> +	struct flow_rule *rule = flow_cls_offload_flow_rule(cls);
> +	struct ksz9477_acl_priv *acl = dev->ports[port].acl_priv;
> +	const struct flow_action_entry *act;
> +	struct ksz9477_acl_entry *entry;
> +	bool prio_force = false;
> +	u8 prio_val;
> +	int i;
> +
> +	if (TC_H_MIN(cls->classid)) {
> +		NL_SET_ERR_MSG_MOD(extack, "hw_tc is not supported. Use: action skbedit prio");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	flow_action_for_each(i, act, &rule->action) {
> +		switch (act->id) {
> +		case FLOW_ACTION_PRIORITY:
> +			if (act->priority > KSZ9477_MAX_TC) {
> +				NL_SET_ERR_MSG_MOD(extack, "Priority value is too high");
> +				return -EOPNOTSUPP;
> +			}
> +			prio_force = true;
> +			prio_val = act->priority;
> +			break;
> +		default:
> +			NL_SET_ERR_MSG_MOD(extack, "action not supported");
> +			return -EOPNOTSUPP;
> +		}
> +	}
> +
> +	/* pick entry to store action */
> +	entry = &acl->acles.entries[entry_idx];
> +
> +	ksz9477_acl_action_rule_cfg(entry->entry, prio_force, prio_val);

If the flow_action_for_each loop iterates zero times then
prio_val will be uninitialised here. gcc-12 [-Wmaybe-uninitialized]
and Smatch warn about this. But I'm unsure if it can actually happen.

> +	ksz9477_acl_processing_rule_set_action(entry->entry, entry_idx);
> +
> +	return 0;
> +}

...