[PATCH net-next 0/3] mtk_ppe_offload fixes

[PATCH net-next 0/3] mtk_ppe_offload fixes

[Pablo Neira Ayuso]

Hi,

A few incremental fixes for the initial flowtable offload support
and this driver:

1) Fix undefined reference to `dsa_port_from_netdev' due to missing
   dependencies in Kconfig, reported by Kbuild robot.

2) Missing mutex to serialize flow events via workqueue to the driver.

3) Handle FLOW_ACTION_VLAN_POP tag action.

Please apply, thanks!

Pablo Neira Ayuso (3):
  net: ethernet: mtk_eth_soc: fix undefined reference to `dsa_port_from_netdev'
  net: ethernet: mtk_eth_soc: missing mutex
  net: ethernet: mtk_eth_soc: handle VLAN pop action

 drivers/net/ethernet/mediatek/Kconfig         |  1 +
 .../net/ethernet/mediatek/mtk_ppe_offload.c   | 21 ++++++++++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)

-- 
2.20.1

[PATCH net-next 1/3] net: ethernet: mtk_eth_soc: fix undefined reference to `dsa_port_from_netdev'

[Pablo Neira Ayuso]

Caused by:

 CONFIG_NET_DSA=m
 CONFIG_NET_MEDIATEK_SOC=y

mtk_ppe_offload.c:undefined reference to `dsa_port_from_netdev'

Fixes: 502e84e2382d ("net: ethernet: mtk_eth_soc: add flow offloading support")
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 drivers/net/ethernet/mediatek/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/mediatek/Kconfig b/drivers/net/ethernet/mediatek/Kconfig
index 3362b148de23..08c2e446d3d5 100644
--- a/drivers/net/ethernet/mediatek/Kconfig
+++ b/drivers/net/ethernet/mediatek/Kconfig
@@ -9,6 +9,7 @@ if NET_VENDOR_MEDIATEK
 
 config NET_MEDIATEK_SOC
 	tristate "MediaTek SoC Gigabit Ethernet support"
+	depends on NET_DSA || !NET_DSA
 	select PHYLINK
 	help
 	  This driver supports the gigabit ethernet MACs in the
-- 
2.20.1

[PATCH net-next 2/3] net: ethernet: mtk_eth_soc: missing mutex

[Pablo Neira Ayuso]

Patch 2ed37183abb7 ("netfilter: flowtable: separate replace, destroy and
stats to different workqueues") splits the workqueue per event type. Add
a mutex to serialize updates.

Fixes: 502e84e2382d ("net: ethernet: mtk_eth_soc: add flow offloading support")
Reported-by: Frank Wunderlich <frank-w@public-files.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 .../net/ethernet/mediatek/mtk_ppe_offload.c   | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
index 4975106fbc42..2759c875c791 100644
--- a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
+++ b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
@@ -391,6 +391,8 @@ mtk_flow_offload_stats(struct mtk_eth *eth, struct flow_cls_offload *f)
 	return 0;
 }
 
+static DEFINE_MUTEX(mtk_flow_offload_mutex);
+
 static int
 mtk_eth_setup_tc_block_cb(enum tc_setup_type type, void *type_data, void *cb_priv)
 {
@@ -398,6 +400,7 @@ mtk_eth_setup_tc_block_cb(enum tc_setup_type type, void *type_data, void *cb_pri
 	struct net_device *dev = cb_priv;
 	struct mtk_mac *mac = netdev_priv(dev);
 	struct mtk_eth *eth = mac->hw;
+	int err;
 
 	if (!tc_can_offload(dev))
 		return -EOPNOTSUPP;
@@ -405,18 +408,24 @@ mtk_eth_setup_tc_block_cb(enum tc_setup_type type, void *type_data, void *cb_pri
 	if (type != TC_SETUP_CLSFLOWER)
 		return -EOPNOTSUPP;
 
+	mutex_lock(&mtk_flow_offload_mutex);
 	switch (cls->command) {
 	case FLOW_CLS_REPLACE:
-		return mtk_flow_offload_replace(eth, cls);
+		err = mtk_flow_offload_replace(eth, cls);
+		break;
 	case FLOW_CLS_DESTROY:
-		return mtk_flow_offload_destroy(eth, cls);
+		err = mtk_flow_offload_destroy(eth, cls);
+		break;
 	case FLOW_CLS_STATS:
-		return mtk_flow_offload_stats(eth, cls);
+		err = mtk_flow_offload_stats(eth, cls);
+		break;
 	default:
-		return -EOPNOTSUPP;
+		err = -EOPNOTSUPP;
+		break;
 	}
+	mutex_unlock(&mtk_flow_offload_mutex);
 
-	return 0;
+	return err;
 }
 
 static int
-- 
2.20.1

[PATCH net-next 3/3] net: ethernet: mtk_eth_soc: handle VLAN pop action

[Pablo Neira Ayuso]

Do not hit EOPNOTSUPP when flowtable offload provides a VLAN pop action.

Fixes: efce49dfe6a8 ("netfilter: flowtable: add vlan pop action offload support")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
efce49dfe6a8 is coming in the nf-next PR for net-next.

 drivers/net/ethernet/mediatek/mtk_ppe_offload.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
index 2759c875c791..ae16299f7fe8 100644
--- a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
+++ b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
@@ -232,6 +232,8 @@ mtk_flow_offload_replace(struct mtk_eth *eth, struct flow_cls_offload *f)
 			data.vlan.proto = act->vlan.proto;
 			data.vlan.num++;
 			break;
+		case FLOW_ACTION_VLAN_POP:
+			break;
 		case FLOW_ACTION_PPPOE_PUSH:
 			if (data.pppoe.num == 1)
 				return -EOPNOTSUPP;
-- 
2.20.1

Re: [PATCH net-next 2/3] net: ethernet: mtk_eth_soc: missing mutex

[Jakub Kicinski]

On Sun, 18 Apr 2021 23:11:44 +0200 Pablo Neira Ayuso wrote:
> Patch 2ed37183abb7 ("netfilter: flowtable: separate replace, destroy and
> stats to different workqueues") splits the workqueue per event type. Add
> a mutex to serialize updates.
> 
> Fixes: 502e84e2382d ("net: ethernet: mtk_eth_soc: add flow offloading support")
> Reported-by: Frank Wunderlich <frank-w@public-files.de>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

This driver doesn't set unlocked_driver_cb, why is it expected to take
any locks? I thought the contract is that caller should hold rtnl.

Re: [PATCH net-next 2/3] net: ethernet: mtk_eth_soc: missing mutex

[Pablo Neira Ayuso]

On Mon, Apr 19, 2021 at 02:16:01PM -0700, Jakub Kicinski wrote:
> On Sun, 18 Apr 2021 23:11:44 +0200 Pablo Neira Ayuso wrote:
> > Patch 2ed37183abb7 ("netfilter: flowtable: separate replace, destroy and
> > stats to different workqueues") splits the workqueue per event type. Add
> > a mutex to serialize updates.
> > 
> > Fixes: 502e84e2382d ("net: ethernet: mtk_eth_soc: add flow offloading support")
> > Reported-by: Frank Wunderlich <frank-w@public-files.de>
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> 
> This driver doesn't set unlocked_driver_cb, why is it expected to take
> any locks? I thought the contract is that caller should hold rtnl.

No rtnl lock is held from the netfilter side, see:

42f1c2712090 ("netfilter: nftables: comment indirect serialization of
commit_mutex with rtnl_mutex")

Re: [PATCH net-next 2/3] net: ethernet: mtk_eth_soc: missing mutex

[Jakub Kicinski]

On Mon, 19 Apr 2021 23:40:19 +0200 Pablo Neira Ayuso wrote:
> On Mon, Apr 19, 2021 at 02:16:01PM -0700, Jakub Kicinski wrote:
> > On Sun, 18 Apr 2021 23:11:44 +0200 Pablo Neira Ayuso wrote:  
> > > Patch 2ed37183abb7 ("netfilter: flowtable: separate replace, destroy and
> > > stats to different workqueues") splits the workqueue per event type. Add
> > > a mutex to serialize updates.
> > > 
> > > Fixes: 502e84e2382d ("net: ethernet: mtk_eth_soc: add flow offloading support")
> > > Reported-by: Frank Wunderlich <frank-w@public-files.de>
> > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>  
> > 
> > This driver doesn't set unlocked_driver_cb, why is it expected to take
> > any locks? I thought the contract is that caller should hold rtnl.  
> 
> No rtnl lock is held from the netfilter side, see:
> 
> 42f1c2712090 ("netfilter: nftables: comment indirect serialization of
> commit_mutex with rtnl_mutex")

All the tc-centric drivers but mlx5 depend on rtnl_lock, is there
something preventing them from binding to netfilter blocks?

Re: [PATCH net-next 2/3] net: ethernet: mtk_eth_soc: missing mutex

[Pablo Neira Ayuso]

On Mon, Apr 19, 2021 at 02:43:41PM -0700, Jakub Kicinski wrote:
> On Mon, 19 Apr 2021 23:40:19 +0200 Pablo Neira Ayuso wrote:
> > On Mon, Apr 19, 2021 at 02:16:01PM -0700, Jakub Kicinski wrote:
> > > On Sun, 18 Apr 2021 23:11:44 +0200 Pablo Neira Ayuso wrote:  
> > > > Patch 2ed37183abb7 ("netfilter: flowtable: separate replace, destroy and
> > > > stats to different workqueues") splits the workqueue per event type. Add
> > > > a mutex to serialize updates.
> > > > 
> > > > Fixes: 502e84e2382d ("net: ethernet: mtk_eth_soc: add flow offloading support")
> > > > Reported-by: Frank Wunderlich <frank-w@public-files.de>
> > > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>  
> > > 
> > > This driver doesn't set unlocked_driver_cb, why is it expected to take
> > > any locks? I thought the contract is that caller should hold rtnl.  
> > 
> > No rtnl lock is held from the netfilter side, see:
> > 
> > 42f1c2712090 ("netfilter: nftables: comment indirect serialization of
> > commit_mutex with rtnl_mutex")
> 
> All the tc-centric drivers but mlx5 depend on rtnl_lock, is there
> something preventing them from binding to netfilter blocks?

Only mlx5 and this driver support for TC_SETUP_FT.

This fix is targetting at the TC_SETUP_FT flow block type.

Re: [PATCH net-next 2/3] net: ethernet: mtk_eth_soc: missing mutex

[Jakub Kicinski]

On Mon, 19 Apr 2021 23:54:32 +0200 Pablo Neira Ayuso wrote:
> On Mon, Apr 19, 2021 at 02:43:41PM -0700, Jakub Kicinski wrote:
> > On Mon, 19 Apr 2021 23:40:19 +0200 Pablo Neira Ayuso wrote:  
> > > No rtnl lock is held from the netfilter side, see:
> > > 
> > > 42f1c2712090 ("netfilter: nftables: comment indirect serialization of
> > > commit_mutex with rtnl_mutex")  
> > 
> > All the tc-centric drivers but mlx5 depend on rtnl_lock, is there
> > something preventing them from binding to netfilter blocks?  
> 
> Only mlx5 and this driver support for TC_SETUP_FT.
> 
> This fix is targetting at the TC_SETUP_FT flow block type.

Ah, there is a separate block type. Thanks.

Re: [PATCH net-next 0/3] mtk_ppe_offload fixes

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

On Sun, 18 Apr 2021 23:11:42 +0200 you wrote:
> Hi,
> 
> A few incremental fixes for the initial flowtable offload support
> and this driver:
> 
> 1) Fix undefined reference to `dsa_port_from_netdev' due to missing
>    dependencies in Kconfig, reported by Kbuild robot.
> 
> [...]

Here is the summary with links:
  - [net-next,1/3] net: ethernet: mtk_eth_soc: fix undefined reference to `dsa_port_from_netdev'
    https://git.kernel.org/netdev/net-next/c/0e389028ad75
  - [net-next,2/3] net: ethernet: mtk_eth_soc: missing mutex
    https://git.kernel.org/netdev/net-next/c/014d029876b2
  - [net-next,3/3] net: ethernet: mtk_eth_soc: handle VLAN pop action
    https://git.kernel.org/netdev/net-next/c/f5c2cb583abe

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next 2/3] net: ethernet: mtk_eth_soc: missing mutex

[Frank Wunderlich]

Am 18. April 2021 23:11:44 MESZ schrieb Pablo Neira Ayuso <pablo@netfilter.org>:
>Patch 2ed37183abb7 ("netfilter: flowtable: separate replace, destroy
>and
>stats to different workqueues") splits the workqueue per event type.
>Add
>a mutex to serialize updates.
>
>Fixes: 502e84e2382d ("net: ethernet: mtk_eth_soc: add flow offloading
>support")
>Reported-by: Frank Wunderlich <frank-w@public-files.de>
>Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Hi Pablo,

As far we tested it, the mutex does not avoid the hang. It looks a bit better,but at the end it was fixed by this Patch

https://patchwork.kernel.org/project/linux-mediatek/patch/20210417072905.207032-1-dqfext@gmail.com/

Alex did some tests without the lock here and it still looks stable. So it looks like it is not needed
regards Frank

Re: [PATCH net-next 2/3] net: ethernet: mtk_eth_soc: missing mutex

[Pablo Neira Ayuso]

On Tue, Apr 20, 2021 at 01:51:07PM +0200, Frank Wunderlich wrote:
> Am 18. April 2021 23:11:44 MESZ schrieb Pablo Neira Ayuso <pablo@netfilter.org>:
> >Patch 2ed37183abb7 ("netfilter: flowtable: separate replace, destroy
> >and
> >stats to different workqueues") splits the workqueue per event type.
> >Add
> >a mutex to serialize updates.
> >
> >Fixes: 502e84e2382d ("net: ethernet: mtk_eth_soc: add flow offloading
> >support")
> >Reported-by: Frank Wunderlich <frank-w@public-files.de>
> >Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> 
> Hi Pablo,
> 
> As far we tested it, the mutex does not avoid the hang. It looks a bit better,but at the end it was fixed by this Patch
> 
> https://patchwork.kernel.org/project/linux-mediatek/patch/20210417072905.207032-1-dqfext@gmail.com/
> 
> Alex did some tests without the lock here and it still looks stable.
> So it looks like it is not needed

It might be hard to trigger the race, but it's needed. There are
several workqueues racing to add and delete entries from the driver
flowtable representation which has no locks.