Re: [PATCH RFC net-next v2] net: dsa: mv88e6xxx: Add partial support for TCAM entries

[Andrew Lunn]

> +static int mv88e6xxx_tcam_read_page(struct mv88e6xxx_chip *chip, u8 page, u8 entry)
> +
> +{
> +	u16 val = MV88E6XXX_TCAM_OP_BUSY | MV88E6XXX_TCAM_OP_OP_READ | (page & 0x3) << 10 | entry;
> +	int rc;

This driver pretty consistently uses err for error codes. There are a
small number of cases of using ret, but not many. rc is not used at
all.

This driver also uses 80 character line lengths, which is still the
default for netdev.

> +/* insert tcam entry in ordered list and move existing entries if necessary */
> +static int mv88e6xxx_tcam_insert_entry(struct mv88e6xxx_chip *chip,
> +				       struct mv88e6xxx_tcam_entry *entry)
> +{
> +	struct mv88e6xxx_tcam_entry *elem;
> +	struct list_head *hpos;
> +	int rc;
> +
> +	list_for_each_prev(hpos, &chip->tcam.entries) {
> +		elem = list_entry(hpos, struct mv88e6xxx_tcam_entry, list);
> +		if (entry->prio >= elem->prio)
> +			break;
> +
> +		u8 move_idx = elem->hw_idx + 1;

Please don't create variables mid block.

> +
> +		mv88e6xxx_reg_lock(chip);
> +		rc = mv88e6xxx_tcam_flush_entry(chip, move_idx);
> +		mv88e6xxx_reg_unlock(chip);
> +		if (rc)
> +			return rc;
> +		mv88e6xxx_reg_lock(chip);

and it would be normal to have a blank like after a return.


> +		rc = chip->info->ops->tcam_ops->entry_add(chip, elem, move_idx);
> +		mv88e6xxx_reg_unlock(chip);

These mv88e6xxx_reg_lock()/mv88e6xxx_reg_unlock() are also
suspicious. Generally, the top level functions of the driver do this
locking. So the tc entry points into the driver. The lower level code
just assumes the lock is held, and the lowest level then has a debug
test to ensure it is held. The exception is interrupt handling, which
is a bit messy due to re-entrance.

There is also the advantage of the top level locking that your list
manipulation is then thread safe.

> +#define mv88e6xxx_tcam_match_set(key, _offset, data, mask) \
> +	do { \
> +		typeof(_offset) (offset) = (_offset); \
> +		BUILD_BUG_ON((offset) + sizeof((data)) > TCAM_MATCH_SIZE); \
> +		__mv88e6xxx_tcam_match_set(key, offset, sizeof(data), \
> +					   (u8 *)&(data), (u8 *)&(mask)); \
> +	} while (0)
> +
> +static inline void __mv88e6xxx_tcam_match_set(struct mv88e6xxx_tcam_key *key, unsigned int offset,
> +					      size_t size, u8 *data, u8 *mask)
> +{
> +	memcpy(&key->frame_data[offset], data, size);
> +	memcpy(&key->frame_mask[offset], mask, size);
> +}

I want to look at these two in more detail, i don't like macros like
this, nor inline functions in header files. But maybe they are needed.

> +static int mv88e6xx_flower_parse_key(struct mv88e6xxx_chip *chip,
> +				     struct netlink_ext_ack *extack,
> +				     struct flow_cls_offload *cls,
> +				     struct mv88e6xxx_tcam_key *key)
> +{
> +	struct flow_rule *rule = flow_cls_offload_flow_rule(cls);
> +	struct flow_dissector *dissector = rule->match.dissector;
> +	u16 addr_type = 0;
> +
> +	if (dissector->used_keys &
> +	    ~(BIT_ULL(FLOW_DISSECTOR_KEY_BASIC) |
> +	      BIT_ULL(FLOW_DISSECTOR_KEY_CONTROL) |
> +	      BIT_ULL(FLOW_DISSECTOR_KEY_IPV4_ADDRS) |
> +	      BIT_ULL(FLOW_DISSECTOR_KEY_IPV6_ADDRS))) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Unsupported keys used");
> +		dev_warn(chip->dev, "used_keys: 0x%llx", dissector->used_keys);

This dev_warn seems like debug?

> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_CONTROL)) {
> +		struct flow_match_control match;
> +
> +		flow_rule_match_control(rule, &match);
> +		addr_type = match.key->addr_type;
> +
> +		if (flow_rule_has_control_flags(match.mask->flags,
> +						cls->common.extack))
> +			return -EOPNOTSUPP;
> +	}
> +	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_BASIC)) {

Missing blank line have }

> +		struct flow_match_basic match;
> +
> +		flow_rule_match_basic(rule, &match);
> +		// ethertype
> +		mv88e6xxx_tcam_match_set(key, 16, match.key->n_proto, match.mask->n_proto);
> +		mv88e6xxx_tcam_match_set(key, 27, match.key->ip_proto, match.mask->ip_proto);

What are these magic numbers?

> +int mv88e6xxx_cls_flower_add(struct dsa_switch *ds, int port,
> +			     struct flow_cls_offload *cls, bool ingress)
> +{
> +	struct mv88e6xxx_chip *chip = ds->priv;
> +	struct flow_rule *rule = flow_cls_offload_flow_rule(cls);
> +	struct netlink_ext_ack *extack = cls->common.extack;
> +	unsigned long cookie = cls->cookie;
> +	const struct flow_action_entry *act;
> +	struct mv88e6xxx_tcam_key key = { 0 };
> +	struct mv88e6xxx_tcam_entry *entry;
> +	int rc, i;

Reverse Christmass tree. This applies everywhere in the patch.

> +	flow_action_for_each(i, act, &rule->action) {
> +		switch (act->id) {
> +		case FLOW_ACTION_TRAP: {
> +			int cpu = dsa_upstream_port(ds, port);
> +
> +			entry->action.dpv_mode = DPV_MODE_REPLACE;
> +			entry->action.dpv = BIT(cpu);
> +			break;
> +		}
> +		default: {
> +			NL_SET_ERR_MSG_MOD(extack, "action not supported");
> +			rc = -EOPNOTSUPP;
> +			goto err_free_entry;
> +		}
> +		}

Two } on the same level suggests the indentation is broken.

	Andrew