Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: add RXNFC support

[Andrew Lunn <andrew@lunn.ch>]

> +static int mv88e6xxx_policy_insert(struct mv88e6xxx_chip *chip, int port,
> +				   struct ethtool_rx_flow_spec *fs)
> +{
> +	struct ethhdr *mac_entry = &fs->h_u.ether_spec;
> +	struct ethhdr *mac_mask = &fs->m_u.ether_spec;
> +	enum mv88e6xxx_policy_mapping mapping;
> +	enum mv88e6xxx_policy_action action;
> +	struct mv88e6xxx_policy *policy;
> +	u16 vid = 0;
> +	u8 *addr;
> +	int err;
> +	int id;
> +
> +	if (fs->location != RX_CLS_LOC_ANY)
> +		return -EINVAL;
> +
> +	if (fs->ring_cookie == RX_CLS_FLOW_DISC)
> +		action = MV88E6XXX_POLICY_ACTION_DISCARD;
> +	else
> +		return -EOPNOTSUPP;
> +
> +	switch (fs->flow_type & ~FLOW_EXT) {
> +	case ETHER_FLOW:
> +		if (!is_zero_ether_addr(mac_mask->h_dest) &&
> +		    is_zero_ether_addr(mac_mask->h_source)) {
> +			mapping = MV88E6XXX_POLICY_MAPPING_DA;
> +			addr = mac_entry->h_dest;
> +		} else if (is_zero_ether_addr(mac_mask->h_dest) &&
> +		    !is_zero_ether_addr(mac_mask->h_source)) {
> +			mapping = MV88E6XXX_POLICY_MAPPING_SA;
> +			addr = mac_entry->h_source;
> +		} else {
> +			/* Cannot support DA and SA mapping in the same rule */
> +			return -EOPNOTSUPP;
> +		}
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if ((fs->flow_type & FLOW_EXT) && fs->m_ext.vlan_tci) {
> +		if (fs->m_ext.vlan_tci != 0xffff)
> +			return -EOPNOTSUPP;
> +		vid = be16_to_cpu(fs->h_ext.vlan_tci) & VLAN_VID_MASK;
> +	}
> +
> +	idr_for_each_entry(&chip->policies, policy, id) {
> +		if (policy->port == port && policy->mapping == mapping &&
> +		    policy->action == action && policy->vid == vid &&
> +		    ether_addr_equal(policy->addr, addr))
> +			return -EEXIST;
> +	}
> +
> +	policy = devm_kzalloc(chip->dev, sizeof(*policy), GFP_KERNEL);
> +	if (!policy)
> +		return -ENOMEM;

Hi Vivien

I think this might be the first time we have done dynamic memory
allocation in the mv88e6xxx driver. It might even be a first for a DSA
driver?

I'm not saying it is wrong, but maybe we should discuss it. 

I assume you are doing this because the ATU entry itself is not
sufficient?

How much memory is involved here, worst case? I assume one struct
mv88e6xxx_policy per ATU entry? Which you think is too much to
allocate as part of chip? I guess most users will never use this
feature, so for most users it would be wasted memory. So i do see the
point for dynamically allocating it.

Thanks
	Andrew