Re: [PATCH net-next v2 2/3] net: txgbe: support Flow Director perfect filters

[Simon Horman <horms@kernel.org>]

On Wed, Jun 05, 2024 at 10:08:51AM +0800, Jiawen Wu wrote:
> Support the addition and deletion of Flow Director filters.
> 
> Supported fields: src-ip, dst-ip, src-port, dst-port
> Supported flow-types: tcp4, udp4, sctp4, ipv4
> 
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>

...

> +static int txgbe_add_ethtool_fdir_entry(struct txgbe *txgbe,
> +					struct ethtool_rxnfc *cmd)
> +{
> +	struct ethtool_rx_flow_spec *fsp = (struct ethtool_rx_flow_spec *)&cmd->fs;
> +	struct txgbe_fdir_filter *input;
> +	union txgbe_atr_input mask;
> +	struct wx *wx = txgbe->wx;
> +	u16 ptype = 0;
> +	u8 queue;
> +	int err;
> +
> +	if (!(test_bit(WX_FLAG_FDIR_PERFECT, wx->flags)))
> +		return -EOPNOTSUPP;
> +
> +	/* ring_cookie is a masked into a set of queues and txgbe pools or
> +	 * we use drop index
> +	 */
> +	if (fsp->ring_cookie == RX_CLS_FLOW_DISC) {
> +		queue = TXGBE_RDB_FDIR_DROP_QUEUE;
> +	} else {
> +		u32 ring = ethtool_get_flow_spec_ring(fsp->ring_cookie);
> +
> +		if (ring >= wx->num_rx_queues)
> +			return -EINVAL;
> +
> +		/* Map the ring onto the absolute queue index */
> +		queue = wx->rx_ring[ring]->reg_idx;
> +	}
> +
> +	/* Don't allow indexes to exist outside of available space */
> +	if (fsp->location >= ((1024 << TXGBE_FDIR_PBALLOC_64K) - 2)) {
> +		wx_err(wx, "Location out of range\n");
> +		return -EINVAL;
> +	}
> +
> +	input = kzalloc(sizeof(*input), GFP_ATOMIC);
> +	if (!input)
> +		return -ENOMEM;
> +
> +	memset(&mask, 0, sizeof(union txgbe_atr_input));
> +
> +	/* set SW index */
> +	input->sw_idx = fsp->location;
> +
> +	/* record flow type */
> +	if (txgbe_flowspec_to_flow_type(fsp, &input->filter.formatted.flow_type)) {
> +		wx_err(wx, "Unrecognized flow type\n");
> +		goto err_out;
> +	}
> +
> +	mask.formatted.flow_type = TXGBE_ATR_L4TYPE_IPV6_MASK |
> +				   TXGBE_ATR_L4TYPE_MASK;
> +
> +	if (input->filter.formatted.flow_type == TXGBE_ATR_FLOW_TYPE_IPV4)
> +		mask.formatted.flow_type &= TXGBE_ATR_L4TYPE_IPV6_MASK;
> +
> +	/* Copy input into formatted structures */
> +	input->filter.formatted.src_ip[0] = fsp->h_u.tcp_ip4_spec.ip4src;
> +	mask.formatted.src_ip[0] = fsp->m_u.tcp_ip4_spec.ip4src;
> +	input->filter.formatted.dst_ip[0] = fsp->h_u.tcp_ip4_spec.ip4dst;
> +	mask.formatted.dst_ip[0] = fsp->m_u.tcp_ip4_spec.ip4dst;
> +	input->filter.formatted.src_port = fsp->h_u.tcp_ip4_spec.psrc;
> +	mask.formatted.src_port = fsp->m_u.tcp_ip4_spec.psrc;
> +	input->filter.formatted.dst_port = fsp->h_u.tcp_ip4_spec.pdst;
> +	mask.formatted.dst_port = fsp->m_u.tcp_ip4_spec.pdst;
> +
> +	if (fsp->flow_type & FLOW_EXT) {
> +		input->filter.formatted.vm_pool =
> +				(unsigned char)ntohl(fsp->h_ext.data[1]);
> +		mask.formatted.vm_pool =
> +				(unsigned char)ntohl(fsp->m_ext.data[1]);
> +		input->filter.formatted.flex_bytes =
> +						fsp->h_ext.vlan_etype;
> +		mask.formatted.flex_bytes = fsp->m_ext.vlan_etype;
> +	}
> +
> +	switch (input->filter.formatted.flow_type) {
> +	case TXGBE_ATR_FLOW_TYPE_TCPV4:
> +		ptype = WX_PTYPE_L2_IPV4_TCP;
> +		break;
> +	case TXGBE_ATR_FLOW_TYPE_UDPV4:
> +		ptype = WX_PTYPE_L2_IPV4_UDP;
> +		break;
> +	case TXGBE_ATR_FLOW_TYPE_SCTPV4:
> +		ptype = WX_PTYPE_L2_IPV4_SCTP;
> +		break;
> +	case TXGBE_ATR_FLOW_TYPE_IPV4:
> +		ptype = WX_PTYPE_L2_IPV4;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	input->filter.formatted.vlan_id = htons(ptype);
> +	if (mask.formatted.flow_type & TXGBE_ATR_L4TYPE_MASK)
> +		mask.formatted.vlan_id = htons(0xFFFF);
> +	else
> +		mask.formatted.vlan_id = htons(0xFFF8);
> +
> +	/* determine if we need to drop or route the packet */
> +	if (fsp->ring_cookie == RX_CLS_FLOW_DISC)
> +		input->action = TXGBE_RDB_FDIR_DROP_QUEUE;
> +	else
> +		input->action = fsp->ring_cookie;
> +
> +	spin_lock(&txgbe->fdir_perfect_lock);
> +
> +	if (hlist_empty(&txgbe->fdir_filter_list)) {
> +		/* save mask and program input mask into HW */
> +		memcpy(&txgbe->fdir_mask, &mask, sizeof(mask));
> +		err = txgbe_fdir_set_input_mask(wx, &mask);
> +		if (err)
> +			goto err_unlock;
> +	} else if (memcmp(&txgbe->fdir_mask, &mask, sizeof(mask))) {
> +		wx_err(wx, "Hardware only supports one mask per port. To change the mask you must first delete all the rules.\n");
> +		goto err_unlock;
> +	}
> +
> +	/* apply mask and compute/store hash */
> +	txgbe_atr_compute_perfect_hash(&input->filter, &mask);
> +
> +	/* check if new entry does not exist on filter list */
> +	if (txgbe_match_ethtool_fdir_entry(txgbe, input))
> +		goto err_unlock;
> +
> +	/* only program filters to hardware if the net device is running, as
> +	 * we store the filters in the Rx buffer which is not allocated when
> +	 * the device is down
> +	 */
> +	if (netif_running(wx->netdev)) {
> +		err = txgbe_fdir_write_perfect_filter(wx, &input->filter,
> +						      input->sw_idx, queue);
> +		if (err)
> +			goto err_unlock;
> +	}
> +
> +	txgbe_update_ethtool_fdir_entry(txgbe, input, input->sw_idx);
> +
> +	spin_unlock(&txgbe->fdir_perfect_lock);
> +
> +	return err;

Hi Jiawen Wu,

Smatch flags that err may be used uninitialised here.
I'm unsure if that can occur in practice, but perhaps it
would be nicer to simply return 0 here. 

> +err_unlock:
> +	spin_unlock(&txgbe->fdir_perfect_lock);
> +err_out:
> +	kfree(input);
> +	return -EINVAL;

And conversely, perhaps it would be nicer to return err here - ensuring is
it always set.  F.e. this would propagate the error code returned by
txgbe_fdir_write_perfect_filter().

> +}

...