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

[Roger Quadros <rogerq@kernel.org>]

On 08/05/2025 17:00, Paolo Abeni wrote:
> 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.

Yes, will fix.
> 
>> +
>> +	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?

Good point. I will remove mutex acquiring from am65_cpsw_policer_find_match() and
add acquire it instead in the beginning of am65_cpsw_rxnfc_add_rule()

> 
> /P
> 

-- 
cheers,
-roger