Re: [PATCH net-next v2 8/9] net: ethernet: ti: am65-cpsw: add network flow classification support

[Paolo Abeni <pabeni@redhat.com>]

On 5/5/25 6:26 PM, Roger Quadros wrote:
[...]
> +/* validate the rxnfc rule and convert it to policer config */
> +static int am65_cpsw_rxnfc_validate(struct am65_cpsw_port *port,
> +				    struct ethtool_rxnfc *rxnfc,
> +				    struct cpsw_ale_policer_cfg *cfg)
> +{
> +	struct ethtool_rx_flow_spec *fs = &rxnfc->fs;
> +	int flow_type = AM65_CPSW_FLOW_TYPE(fs->flow_type);
> +	struct ethhdr *eth_mask;

(Minor nit only mentioned because of more relevant comments on previous
patch) Please respect the reverse christmas tree order above.

> +
> +	memset(cfg, 0, sizeof(*cfg));
> +
> +	if (flow_type & FLOW_RSS)
> +		return -EINVAL;
> +
> +	if (fs->location == RX_CLS_LOC_ANY ||
> +	    fs->location >= port->rxnfc_max)
> +		return -EINVAL;
> +
> +	if (fs->ring_cookie == RX_CLS_FLOW_DISC)
> +		cfg->drop = true;
> +	else if (fs->ring_cookie > AM65_CPSW_MAX_QUEUES)
> +		return -EINVAL;
> +
> +	cfg->port_id = port->port_id;
> +	cfg->thread_id = fs->ring_cookie;
> +
> +	switch (flow_type) {
> +	case ETHER_FLOW:
> +		eth_mask = &fs->m_u.ether_spec;
> +
> +		/* etherType matching is supported by h/w but not yet here */
> +		if (eth_mask->h_proto)
> +			return -EINVAL;
> +
> +		/* Only support source matching addresses by full mask */
> +		if (is_broadcast_ether_addr(eth_mask->h_source)) {
> +			cfg->match_flags |= CPSW_ALE_POLICER_MATCH_MACSRC;
> +			ether_addr_copy(cfg->src_addr,
> +					fs->h_u.ether_spec.h_source);
> +		}
> +
> +		/* Only support destination matching addresses by full mask */
> +		if (is_broadcast_ether_addr(eth_mask->h_dest)) {
> +			cfg->match_flags |= CPSW_ALE_POLICER_MATCH_MACDST;
> +			ether_addr_copy(cfg->dst_addr,
> +					fs->h_u.ether_spec.h_dest);
> +		}
> +
> +		if ((fs->flow_type & FLOW_EXT) && fs->m_ext.vlan_tci) {
> +			/* Don't yet support vlan ethertype */
> +			if (fs->m_ext.vlan_etype)
> +				return -EINVAL;
> +
> +			if (fs->m_ext.vlan_tci != VLAN_TCI_FULL_MASK)
> +				return -EINVAL;
> +
> +			cfg->vid = FIELD_GET(VLAN_VID_MASK,
> +					     ntohs(fs->h_ext.vlan_tci));
> +			cfg->vlan_prio = FIELD_GET(VLAN_PRIO_MASK,
> +						   ntohs(fs->h_ext.vlan_tci));
> +			cfg->match_flags |= CPSW_ALE_POLICER_MATCH_OVLAN;
> +		}
> +
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int am65_cpsw_policer_find_match(struct am65_cpsw_port *port,
> +					struct cpsw_ale_policer_cfg *cfg)
> +{
> +	struct am65_cpsw_rxnfc_rule *rule;
> +	int loc = -EINVAL;
> +
> +	mutex_lock(&port->rxnfc_lock);
> +	list_for_each_entry(rule, &port->rxnfc_rules, list) {
> +		if (!memcmp(&rule->cfg, cfg, sizeof(*cfg))) {
> +			loc = rule->location;
> +			break;
> +		}
> +	}
> +
> +	mutex_unlock(&port->rxnfc_lock);
> +
> +	return loc;
> +}
> +
> +static int am65_cpsw_rxnfc_add_rule(struct am65_cpsw_port *port,
> +				    struct ethtool_rxnfc *rxnfc)
> +{
> +	struct ethtool_rx_flow_spec *fs = &rxnfc->fs;
> +	struct am65_cpsw_rxnfc_rule *rule;
> +	struct cpsw_ale_policer_cfg cfg;
> +	int loc, ret;
> +
> +	if (am65_cpsw_rxnfc_validate(port, rxnfc, &cfg))
> +		return -EINVAL;
> +
> +	/* need to check if similar rule is already present at another location,
> +	 * if yes error out
> +	 */
> +	loc = am65_cpsw_policer_find_match(port, &cfg);
> +	if (loc >= 0 && loc != fs->location) {
> +		netdev_info(port->ndev,
> +			    "rule already exists in location %d. not adding\n",
> +			    loc);
> +		return -EINVAL;
> +	}
> +
> +	/* delete exisiting rule */
> +	if (loc >= 0) {
> +		mutex_lock(&port->rxnfc_lock);

The rxnfc_lock mutex is released and re-aquired after the previous
lookup. Con some other thread delete the matching rule in-between and
add another one at a different location?

/P