netdev - Re: [RFC PATCH v2 net-next] sfc: support offloading TC VLAN push/pop actions to the MAE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y/iMTcvQZ3uW8bgP@corigine.com>
Date:   Fri, 24 Feb 2023 11:07:09 +0100
From:   Simon Horman <simon.horman@...igine.com>
To:     edward.cree@....com
Cc:     netdev@...r.kernel.org, Edward Cree <ecree.xilinx@...il.com>,
        linux-net-drivers@....com, habetsm.xilinx@...il.com,
        leon@...nel.org
Subject: Re: [RFC PATCH v2 net-next] sfc: support offloading TC VLAN push/pop
 actions to the MAE

On Thu, Feb 23, 2023 at 11:50:26PM +0000, edward.cree@....com wrote:
> From: Edward Cree <ecree.xilinx@...il.com>
> 
> EF100 can pop and/or push up to two VLAN tags.
> 
> Signed-off-by: Edward Cree <ecree.xilinx@...il.com>
> ---
> Changed in v2: reworked act->vlan_push/pop to be counts rather than bitmasks,
>  and simplified the corresponding efx_tc_action_order handling.

This looks good to me.

As you'll need to repost as a non-RFC I've added a few nits inline.
But those notwithstanding,

Reviewed-by: Simon Horman <simon.horman@...igine.com>

...

> diff --git a/drivers/net/ethernet/sfc/tc.c b/drivers/net/ethernet/sfc/tc.c
> index deeaab9ee761..12b34320bc81 100644
> --- a/drivers/net/ethernet/sfc/tc.c
> +++ b/drivers/net/ethernet/sfc/tc.c

...

> @@ -494,6 +511,29 @@ static int efx_tc_flower_replace(struct efx_nic *efx,
>  			}
>  			*act = save;
>  			break;
> +		case FLOW_ACTION_VLAN_POP:
> +			if (act->vlan_push) {
> +				act->vlan_push--;
> +			} else if (efx_tc_flower_action_order_ok(act, EFX_TC_AO_VLAN_POP)) {
> +				act->vlan_pop++;
> +			} else {
> +				NL_SET_ERR_MSG_MOD(extack, "More than two VLAN pops, or action order violated");

nit: I'm not sure if there is anything to be done about it,
     but checkpatch complains about ling lines here...

> +				rc = -EINVAL;
> +				goto release;
> +			}
> +			break;
> +		case FLOW_ACTION_VLAN_PUSH:
> +			if (!efx_tc_flower_action_order_ok(act, EFX_TC_AO_VLAN_PUSH)) {
> +				rc = -EINVAL;
> +				NL_SET_ERR_MSG_MOD(extack, "More than two VLAN pushes, or action order violated");

... and here.

> +				goto release;
> +			}
> +			tci = fa->vlan.vid & 0x0fff;
> +			tci |= fa->vlan.prio << 13;

nit: Maybe VLAN_PRIO_SHIFT and VLAN_VID_MASK can be used here.

> +			act->vlan_tci[act->vlan_push] = cpu_to_be16(tci);
> +			act->vlan_proto[act->vlan_push] = fa->vlan.proto;
> +			act->vlan_push++;
> +			break;
>  		default:
>  			NL_SET_ERR_MSG_FMT_MOD(extack, "Unhandled action %u",
>  					       fa->id);

...