[PATCH v3 net-next] net: ethernet: mtk_ppe_offload: Allow QinQ

[PATCH v3 net-next] net: ethernet: mtk_ppe_offload: Allow QinQ

[Eric Woudstra]

mtk_foe_entry_set_vlan() in mtk_ppe.c already seems to support
double vlan tagging, but mtk_flow_offload_replace() in
mtk_ppe_offload.c only allows for 1 vlan tag, optionally in
combination with pppoe and dsa tags.

This patch adds QinQ support to mtk_flow_offload_replace().

Only PPPoE-in-Q (as before) and Q-in-Q are allowed. A combination
of PPPoE and Q-in-Q is not allowed.

Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
---

Changes in v3:
- Removed unnecessary second check for ETH_P_8021Q.

Changes in v2:
- Unchanged, only RFC to PATCH.

Tested on the BPI-R3(mini), on non-dsa-ports and dsa-ports.

 .../net/ethernet/mediatek/mtk_ppe_offload.c   | 22 +++++++++----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
index f20bb390df3a..c855fb799ce1 100644
--- a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
+++ b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
@@ -34,8 +34,10 @@ struct mtk_flow_data {
 	u16 vlan_in;
 
 	struct {
-		u16 id;
-		__be16 proto;
+		struct {
+			u16 id;
+			__be16 proto;
+		} vlans[2];
 		u8 num;
 	} vlan;
 	struct {
@@ -349,18 +351,19 @@ mtk_flow_offload_replace(struct mtk_eth *eth, struct flow_cls_offload *f,
 		case FLOW_ACTION_CSUM:
 			break;
 		case FLOW_ACTION_VLAN_PUSH:
-			if (data.vlan.num == 1 ||
+			if (data.vlan.num + data.pppoe.num == 2 ||
 			    act->vlan.proto != htons(ETH_P_8021Q))
 				return -EOPNOTSUPP;
 
-			data.vlan.id = act->vlan.vid;
-			data.vlan.proto = act->vlan.proto;
+			data.vlan.vlans[data.vlan.num].id = act->vlan.vid;
+			data.vlan.vlans[data.vlan.num].proto = act->vlan.proto;
 			data.vlan.num++;
 			break;
 		case FLOW_ACTION_VLAN_POP:
 			break;
 		case FLOW_ACTION_PPPOE_PUSH:
-			if (data.pppoe.num == 1)
+			if (data.pppoe.num == 1 ||
+			    data.vlan.num == 2)
 				return -EOPNOTSUPP;
 
 			data.pppoe.sid = act->pppoe.sid;
@@ -450,12 +453,9 @@ mtk_flow_offload_replace(struct mtk_eth *eth, struct flow_cls_offload *f,
 	if (offload_type == MTK_PPE_PKT_TYPE_BRIDGE)
 		foe.bridge.vlan = data.vlan_in;
 
-	if (data.vlan.num == 1) {
-		if (data.vlan.proto != htons(ETH_P_8021Q))
-			return -EOPNOTSUPP;
+	for (i = 0; i < data.vlan.num; i++)
+		mtk_foe_entry_set_vlan(eth, &foe, data.vlan.vlans[i].id);
 
-		mtk_foe_entry_set_vlan(eth, &foe, data.vlan.id);
-	}
 	if (data.pppoe.num == 1)
 		mtk_foe_entry_set_pppoe(eth, &foe, data.pppoe.sid);
 
-- 
2.47.1

Re: [PATCH v3 net-next] net: ethernet: mtk_ppe_offload: Allow QinQ

[Michal Swiatkowski]

On Sun, Feb 09, 2025 at 12:09:36PM +0100, Eric Woudstra wrote:
> mtk_foe_entry_set_vlan() in mtk_ppe.c already seems to support
> double vlan tagging, but mtk_flow_offload_replace() in
> mtk_ppe_offload.c only allows for 1 vlan tag, optionally in
> combination with pppoe and dsa tags.
> 
> This patch adds QinQ support to mtk_flow_offload_replace().
> 
> Only PPPoE-in-Q (as before) and Q-in-Q are allowed. A combination
> of PPPoE and Q-in-Q is not allowed.
> 
> Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
> ---
> 
> Changes in v3:
> - Removed unnecessary second check for ETH_P_8021Q.
> 
> Changes in v2:
> - Unchanged, only RFC to PATCH.
> 
> Tested on the BPI-R3(mini), on non-dsa-ports and dsa-ports.
> 
>  .../net/ethernet/mediatek/mtk_ppe_offload.c   | 22 +++++++++----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
> index f20bb390df3a..c855fb799ce1 100644
> --- a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
> +++ b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
> @@ -34,8 +34,10 @@ struct mtk_flow_data {
>  	u16 vlan_in;
>  
>  	struct {
> -		u16 id;
> -		__be16 proto;
> +		struct {
> +			u16 id;
> +			__be16 proto;
> +		} vlans[2];
>  		u8 num;
>  	} vlan;
>  	struct {
> @@ -349,18 +351,19 @@ mtk_flow_offload_replace(struct mtk_eth *eth, struct flow_cls_offload *f,
>  		case FLOW_ACTION_CSUM:
>  			break;
>  		case FLOW_ACTION_VLAN_PUSH:
> -			if (data.vlan.num == 1 ||
> +			if (data.vlan.num + data.pppoe.num == 2 ||
>  			    act->vlan.proto != htons(ETH_P_8021Q))
>  				return -EOPNOTSUPP;
>  
> -			data.vlan.id = act->vlan.vid;
> -			data.vlan.proto = act->vlan.proto;
> +			data.vlan.vlans[data.vlan.num].id = act->vlan.vid;
> +			data.vlan.vlans[data.vlan.num].proto = act->vlan.proto;
>  			data.vlan.num++;
>  			break;
>  		case FLOW_ACTION_VLAN_POP:
>  			break;
>  		case FLOW_ACTION_PPPOE_PUSH:
> -			if (data.pppoe.num == 1)
> +			if (data.pppoe.num == 1 ||
> +			    data.vlan.num == 2)
>  				return -EOPNOTSUPP;
>  
>  			data.pppoe.sid = act->pppoe.sid;
> @@ -450,12 +453,9 @@ mtk_flow_offload_replace(struct mtk_eth *eth, struct flow_cls_offload *f,
>  	if (offload_type == MTK_PPE_PKT_TYPE_BRIDGE)
>  		foe.bridge.vlan = data.vlan_in;
>  
> -	if (data.vlan.num == 1) {
> -		if (data.vlan.proto != htons(ETH_P_8021Q))
> -			return -EOPNOTSUPP;
> +	for (i = 0; i < data.vlan.num; i++)
> +		mtk_foe_entry_set_vlan(eth, &foe, data.vlan.vlans[i].id);
>  
> -		mtk_foe_entry_set_vlan(eth, &foe, data.vlan.id);
> -	}
>  	if (data.pppoe.num == 1)
>  		mtk_foe_entry_set_pppoe(eth, &foe, data.pppoe.sid);

Thanks for addressing comments.

Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>

>  
> -- 
> 2.47.1

Re: [PATCH v3 net-next] net: ethernet: mtk_ppe_offload: Allow QinQ

[Jakub Kicinski]

On Sun,  9 Feb 2025 12:09:36 +0100 Eric Woudstra wrote:
> This patch adds QinQ support to mtk_flow_offload_replace().
> 
> Only PPPoE-in-Q (as before) and Q-in-Q are allowed. A combination
> of PPPoE and Q-in-Q is not allowed.

AFAIU the standard asks for outer tag in Q-in-Q to be ETH_P_8021AD,
but you still check:

> 			    act->vlan.proto != htons(ETH_P_8021Q))
> 				return -EOPNOTSUPP;

If this is a HW limitation I think you should document that more
clearly in the commit message. If you can fix it, I think you should..
-- 
pw-bot: cr

Re: [PATCH v3 net-next] net: ethernet: mtk_ppe_offload: Allow QinQ

[Eric Woudstra]

On 2/12/25 1:51 AM, Jakub Kicinski wrote:
> On Sun,  9 Feb 2025 12:09:36 +0100 Eric Woudstra wrote:
>> This patch adds QinQ support to mtk_flow_offload_replace().
>>
>> Only PPPoE-in-Q (as before) and Q-in-Q are allowed. A combination
>> of PPPoE and Q-in-Q is not allowed.
> 
> AFAIU the standard asks for outer tag in Q-in-Q to be ETH_P_8021AD,
> but you still check:
> 
>> 			    act->vlan.proto != htons(ETH_P_8021Q))
>> 				return -EOPNOTSUPP;
> 
> If this is a HW limitation I think you should document that more
> clearly in the commit message. If you can fix it, I think you should..

It will be the first case. mtk_foe_entry_set_vlan() is limited to using
only 1 fixed protocol. I'll drop the reviewed-by, amend the commit
message and send v4.

Re: [PATCH v3 net-next] net: ethernet: mtk_ppe_offload: Allow QinQ

[Simon Horman]

On Wed, Feb 12, 2025 at 08:33:52PM +0100, Eric Woudstra wrote:
> 
> 
> On 2/12/25 1:51 AM, Jakub Kicinski wrote:
> > On Sun,  9 Feb 2025 12:09:36 +0100 Eric Woudstra wrote:
> >> This patch adds QinQ support to mtk_flow_offload_replace().
> >>
> >> Only PPPoE-in-Q (as before) and Q-in-Q are allowed. A combination
> >> of PPPoE and Q-in-Q is not allowed.
> > 
> > AFAIU the standard asks for outer tag in Q-in-Q to be ETH_P_8021AD,
> > but you still check:
> > 
> >> 			    act->vlan.proto != htons(ETH_P_8021Q))
> >> 				return -EOPNOTSUPP;
> > 
> > If this is a HW limitation I think you should document that more
> > clearly in the commit message. If you can fix it, I think you should..
> 
> It will be the first case. mtk_foe_entry_set_vlan() is limited to using
> only 1 fixed protocol. I'll drop the reviewed-by, amend the commit
> message and send v4.

I agree with Jakub's comment regarding the spec, and his suggested actions.
But, FWIIW, I also think that situations such as this are not uncommon in
the wild.