Re: [net-next PATCH V2] octeontx2-af: Tc flower offload support for inner VLAN

[Simon Horman]

On Thu, Jul 27, 2023 at 10:42:52AM +0530, Suman Ghosh wrote:

...

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> index 1e6fc23eca4f..89836cd299e4 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c
> @@ -439,6 +439,64 @@ static int otx2_tc_parse_actions(struct otx2_nic *nic,
>  	return 0;
>  }
>  
> +static int otx2_tc_process_vlan(struct otx2_nic *nic, struct flow_msg *flow_spec,
> +				struct flow_msg *flow_mask, struct flow_rule *rule,
> +				struct npc_install_flow_req *req, bool is_inner)
> +{

Hi Suman,

Most of the code in this function seems to be moved from elsewhere.
It might make it slightly easier to review if there was a patch
that moved that code, then another patch that modified to
support the inner VLAN feature.

> +	struct flow_match_vlan match;
> +	u16 vlan_tci, vlan_tci_mask;
> +
> +	if (is_inner)
> +		flow_rule_match_cvlan(rule, &match);
> +	else
> +		flow_rule_match_vlan(rule, &match);
> +
> +	if ((ntohs(match.key->vlan_tpid) != ETH_P_8021Q) &&
> +	    (ntohs(match.key->vlan_tpid) != ETH_P_8021AD)) {

I think eth_type_vlan() can be used here.

> +		netdev_err(nic->netdev, "vlan tpid 0x%x not supported\n",
> +			   ntohs(match.key->vlan_tpid));
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (!match.mask->vlan_id) {
> +		struct flow_action_entry *act;
> +		int i;
> +
> +		flow_action_for_each(i, act, &rule->action) {
> +			if (act->id == FLOW_ACTION_DROP) {
> +				netdev_err(nic->netdev,
> +					   "vlan tpid 0x%x with vlan_id %d is not supported for DROP rule.\n",
> +					   ntohs(match.key->vlan_tpid), match.key->vlan_id);
> +				return -EOPNOTSUPP;
> +			}
> +		}
> +	}
> +
> +	if (match.mask->vlan_id ||
> +	    match.mask->vlan_dei ||
> +	    match.mask->vlan_priority) {
> +		vlan_tci = match.key->vlan_id |
> +			   match.key->vlan_dei << 12 |
> +			   match.key->vlan_priority << 13;
> +
> +		vlan_tci_mask = match.mask->vlan_id |
> +				match.mask->vlan_dei << 12 |
> +				match.mask->vlan_priority << 13;
> +
> +		if (is_inner) {
> +			flow_spec->vlan_itci = htons(vlan_tci);
> +			flow_mask->vlan_itci = htons(vlan_tci_mask);
> +			req->features |= BIT_ULL(NPC_INNER_VID);
> +		} else {
> +			flow_spec->vlan_tci = htons(vlan_tci);
> +			flow_mask->vlan_tci = htons(vlan_tci_mask);
> +			req->features |= BIT_ULL(NPC_OUTER_VID);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int otx2_tc_prepare_flow(struct otx2_nic *nic, struct otx2_tc_flow *node,
>  				struct flow_cls_offload *f,
>  				struct npc_install_flow_req *req)
> @@ -458,6 +516,7 @@ static int otx2_tc_prepare_flow(struct otx2_nic *nic, struct otx2_tc_flow *node,
>  	      BIT(FLOW_DISSECTOR_KEY_BASIC) |
>  	      BIT(FLOW_DISSECTOR_KEY_ETH_ADDRS) |
>  	      BIT(FLOW_DISSECTOR_KEY_VLAN) |
> +	      BIT(FLOW_DISSECTOR_KEY_CVLAN) |
>  	      BIT(FLOW_DISSECTOR_KEY_IPV4_ADDRS) |
>  	      BIT(FLOW_DISSECTOR_KEY_IPV6_ADDRS) |
>  	      BIT(FLOW_DISSECTOR_KEY_PORTS) |
> @@ -564,47 +623,19 @@ static int otx2_tc_prepare_flow(struct otx2_nic *nic, struct otx2_tc_flow *node,
>  	}
>  
>  	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_VLAN)) {
> -		struct flow_match_vlan match;
> -		u16 vlan_tci, vlan_tci_mask;
> -
> -		flow_rule_match_vlan(rule, &match);
> -
> -		if (ntohs(match.key->vlan_tpid) != ETH_P_8021Q) {
> -			netdev_err(nic->netdev, "vlan tpid 0x%x not supported\n",
> -				   ntohs(match.key->vlan_tpid));
> -			return -EOPNOTSUPP;
> -		}
> +		int ret;
>  
> -		if (!match.mask->vlan_id) {
> -			struct flow_action_entry *act;
> -			int i;
> -
> -			flow_action_for_each(i, act, &rule->action) {
> -				if (act->id == FLOW_ACTION_DROP) {
> -					netdev_err(nic->netdev,
> -						   "vlan tpid 0x%x with vlan_id %d is not supported for DROP rule.\n",
> -						   ntohs(match.key->vlan_tpid),
> -						   match.key->vlan_id);
> -					return -EOPNOTSUPP;
> -				}
> -			}
> -		}
> -
> -		if (match.mask->vlan_id ||
> -		    match.mask->vlan_dei ||
> -		    match.mask->vlan_priority) {
> -			vlan_tci = match.key->vlan_id |
> -				   match.key->vlan_dei << 12 |
> -				   match.key->vlan_priority << 13;
> +		ret = otx2_tc_process_vlan(nic, flow_spec, flow_mask, rule, req, false);
> +		if (ret)
> +			return ret;
> +	}
>  
> -			vlan_tci_mask = match.mask->vlan_id |
> -					match.mask->vlan_dei << 12 |
> -					match.mask->vlan_priority << 13;
> +	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_CVLAN)) {
> +		int ret;
>  
> -			flow_spec->vlan_tci = htons(vlan_tci);
> -			flow_mask->vlan_tci = htons(vlan_tci_mask);
> -			req->features |= BIT_ULL(NPC_OUTER_VID);
> -		}
> +		ret = otx2_tc_process_vlan(nic, flow_spec, flow_mask, rule, req, true);
> +		if (ret)
> +			return ret;
>  	}
>  
>  	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_IPV4_ADDRS)) {
> -- 
> 2.25.1
> 
>