[PATCH net] net: mvneta: do not redirect frames during reconfiguration

[PATCH net] net: mvneta: do not redirect frames during reconfiguration

[Lorenzo Bianconi]

Disable frames injection in mvneta_xdp_xmit routine during hw
re-configuration in order to avoid hardware hangs

Fixes: b0a43db9087a ("net: mvneta: add XDP_TX support")
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 drivers/net/ethernet/marvell/mvneta.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 4cc9abd61c43..946925bbcb2d 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -452,11 +452,17 @@ struct mvneta_pcpu_port {
 	u32			cause_rx_tx;
 };
 
+enum {
+	__MVNETA_DOWN,
+};
+
 struct mvneta_port {
 	u8 id;
 	struct mvneta_pcpu_port __percpu	*ports;
 	struct mvneta_pcpu_stats __percpu	*stats;
 
+	unsigned long state;
+
 	int pkt_size;
 	void __iomem *base;
 	struct mvneta_rx_queue *rxqs;
@@ -2113,6 +2119,9 @@ mvneta_xdp_xmit(struct net_device *dev, int num_frame,
 	struct netdev_queue *nq;
 	u32 ret;
 
+	if (unlikely(test_bit(__MVNETA_DOWN, &pp->state)))
+		return -ENETDOWN;
+
 	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
 		return -EINVAL;
 
@@ -3568,12 +3577,16 @@ static void mvneta_start_dev(struct mvneta_port *pp)
 
 	phylink_start(pp->phylink);
 	netif_tx_start_all_queues(pp->dev);
+
+	clear_bit(__MVNETA_DOWN, &pp->state);
 }
 
 static void mvneta_stop_dev(struct mvneta_port *pp)
 {
 	unsigned int cpu;
 
+	set_bit(__MVNETA_DOWN, &pp->state);
+
 	phylink_stop(pp->phylink);
 
 	if (!pp->neta_armada3700) {
-- 
2.26.2

Re: [PATCH net] net: mvneta: do not redirect frames during reconfiguration

[Andrew Lunn]

On Tue, Jun 09, 2020 at 12:02:39AM +0200, Lorenzo Bianconi wrote:
> Disable frames injection in mvneta_xdp_xmit routine during hw
> re-configuration in order to avoid hardware hangs

Hi Lorenzo

Why does mvneta_tx() also not need the same protection?

    Andrew

Re: [PATCH net] net: mvneta: do not redirect frames during reconfiguration

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 850 bytes --]

> On Tue, Jun 09, 2020 at 12:02:39AM +0200, Lorenzo Bianconi wrote:
> > Disable frames injection in mvneta_xdp_xmit routine during hw
> > re-configuration in order to avoid hardware hangs
> 
> Hi Lorenzo
> 
> Why does mvneta_tx() also not need the same protection?
> 
>     Andrew

Hi Andrew,

So far I have not been able to trigger the issue in the legacy tx path.
I hit the problem adding the capability to attach an eBPF program to CPUMAP
entries [1]. In particular I am redirecting traffic to mvneta and concurrently
attaching/removing a XDP program to/from it.
I am not sure this can occur running mvneta_tx().
Moreover it seems a common pattern for .ndo_xdp_xmit() in other drivers
(e.g ixgbe, bnxt, mlx5)

Regards,
Lorenzo

[1] https://patchwork.ozlabs.org/project/netdev/cover/cover.1590960613.git.lorenzo@kernel.org/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net] net: mvneta: do not redirect frames during reconfiguration

[Andrew Lunn]

On Tue, Jun 09, 2020 at 09:41:10AM +0200, Lorenzo Bianconi wrote:
> > On Tue, Jun 09, 2020 at 12:02:39AM +0200, Lorenzo Bianconi wrote:
> > > Disable frames injection in mvneta_xdp_xmit routine during hw
> > > re-configuration in order to avoid hardware hangs
> > 
> > Hi Lorenzo
> > 
> > Why does mvneta_tx() also not need the same protection?
> > 
> >     Andrew
> 
> Hi Andrew,
> 
> So far I have not been able to trigger the issue in the legacy tx path.

Even if you have not hit the issue, do you still think it is possible?
If it is hard to trigger, maybe it is worth protecting against it,
just in case.

> I hit the problem adding the capability to attach an eBPF program to CPUMAP
> entries [1]. In particular I am redirecting traffic to mvneta and concurrently
> attaching/removing a XDP program to/from it.
> I am not sure this can occur running mvneta_tx().
> Moreover it seems a common pattern for .ndo_xdp_xmit() in other drivers
> (e.g ixgbe, bnxt, mlx5)

I was wondering if this should be solved at a higher level. And if you
say there are more MAC drivers with this issue, maybe it should. Not
sure how though. It seems like MTU change and rx mode change wound
need to be protected, which at a higher level is harder to do. What
exactly do you need to protect, in a generic way?

     Andrew

Re: [PATCH net] net: mvneta: do not redirect frames during reconfiguration

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 2203 bytes --]

> On Tue, Jun 09, 2020 at 09:41:10AM +0200, Lorenzo Bianconi wrote:
> > > On Tue, Jun 09, 2020 at 12:02:39AM +0200, Lorenzo Bianconi wrote:
> > > > Disable frames injection in mvneta_xdp_xmit routine during hw
> > > > re-configuration in order to avoid hardware hangs
> > > 
> > > Hi Lorenzo
> > > 
> > > Why does mvneta_tx() also not need the same protection?
> > > 
> > >     Andrew
> > 
> > Hi Andrew,
> > 
> > So far I have not been able to trigger the issue in the legacy tx path.
> 
> Even if you have not hit the issue, do you still think it is possible?
> If it is hard to trigger, maybe it is worth protecting against it,
> just in case.

The issue occurs putting the device down while it is still transmitting. In
particular mvneta_port_down() fails to stop tx (TIMEOUT for TX stopped status=...)
and the device is not able to recover.
The above pattern can occur with XDP because if we remove the program from a
running interface, we will put the interface down for DMA buffers
reconfiguration while mvneta_xdp_xmit() is concurrently running on a remote
cpu.
Looking at the code I do not think it can occurs in the legacy tx path
(mvneta_tx()) since __dev_close() (trigger by userspace) will run
dev_deactivate_many() before running mvneta_stop().

> 
> > I hit the problem adding the capability to attach an eBPF program to CPUMAP
> > entries [1]. In particular I am redirecting traffic to mvneta and concurrently
> > attaching/removing a XDP program to/from it.
> > I am not sure this can occur running mvneta_tx().
> > Moreover it seems a common pattern for .ndo_xdp_xmit() in other drivers
> > (e.g ixgbe, bnxt, mlx5)
> 
> I was wondering if this should be solved at a higher level. And if you
> say there are more MAC drivers with this issue, maybe it should. Not
> sure how though. It seems like MTU change and rx mode change wound
> need to be protected, which at a higher level is harder to do. What
> exactly do you need to protect, in a generic way?

Yes, we can think about it but I guess we should fix the issue first since it
is already there and it will be easy to backport the fix, agree?

Regards,
Lorenzo

> 
>      Andrew

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net] net: mvneta: do not redirect frames during reconfiguration

[David Miller]

From: Lorenzo Bianconi <lorenzo@kernel.org>
Date: Tue,  9 Jun 2020 00:02:39 +0200

> Disable frames injection in mvneta_xdp_xmit routine during hw
> re-configuration in order to avoid hardware hangs
> 
> Fixes: b0a43db9087a ("net: mvneta: add XDP_TX support")
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>

Looking around, I wonder if the fundamental difference from the normal
TX path is that the XDP path doesn't use the TXQ enable/disable
machinery and checks like the normal ndo_start_xmit() paths do.

And that's why only the XDP path has this issue.

I'll apply this, so that the bug is fixed, but note that I consider
this kind of change adding a new flags mask and one state bit to solve
a problem to be ultimately inelegant and ususally pointing out a more
fundamental issue.

Thank you.

Re: [PATCH net] net: mvneta: do not redirect frames during reconfiguration

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1146 bytes --]

> From: Lorenzo Bianconi <lorenzo@kernel.org>
> Date: Tue,  9 Jun 2020 00:02:39 +0200
> 
> > Disable frames injection in mvneta_xdp_xmit routine during hw
> > re-configuration in order to avoid hardware hangs
> > 
> > Fixes: b0a43db9087a ("net: mvneta: add XDP_TX support")
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> 
> Looking around, I wonder if the fundamental difference from the normal
> TX path is that the XDP path doesn't use the TXQ enable/disable
> machinery and checks like the normal ndo_start_xmit() paths do.
> 
> And that's why only the XDP path has this issue.

yes, I agree

> 
> I'll apply this, so that the bug is fixed, but note that I consider
> this kind of change adding a new flags mask and one state bit to solve
> a problem to be ultimately inelegant and ususally pointing out a more
> fundamental issue.

I am completely fine to find a common solution since it seems a pattern used
even in other drivers (e.g. bnxt). Reviewing the code probably we need some
checks in __xdp_enqueue() since xdp_ok_fwd_dev() checks just IFF_UP flag.

Regards,
Lorenzo

> 
> Thank you.
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]