[PATCH net-next] net: sparx5: Add spinlock for frame transmission from CPU

[PATCH net-next] net: sparx5: Add spinlock for frame transmission from CPU

[Horatiu Vultur]

Both registers used when doing manual injection or fdma injection are
shared between all the net devices of the switch. It was noticed that
when having two process which each of them trying to inject frames on
different ethernet ports, that the HW started to behave strange, by
sending out more frames then expected. When doing fdma injection it is
required to set the frame in the DCB and then make sure that the next
pointer of the last DCB is invalid. But because there is no locks for
this, then easily this pointer between the DCB can be broken and then it
would create a loop of DCBs. And that means that the HW will
continuously transmit these frames in a loop. Until the SW will break
this loop.
Therefore to fix this issue, add a spin lock for when accessing the
registers for manual or fdma injection.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 drivers/net/ethernet/microchip/sparx5/sparx5_main.c   | 1 +
 drivers/net/ethernet/microchip/sparx5/sparx5_main.h   | 1 +
 drivers/net/ethernet/microchip/sparx5/sparx5_packet.c | 2 ++
 3 files changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.c b/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
index d1f7fc8b1b71a..3c066b62e6894 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.c
@@ -757,6 +757,7 @@ static int mchp_sparx5_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, sparx5);
 	sparx5->pdev = pdev;
 	sparx5->dev = &pdev->dev;
+	spin_lock_init(&sparx5->tx_lock);
 
 	/* Do switch core reset if available */
 	reset = devm_reset_control_get_optional_shared(&pdev->dev, "switch");
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
index 6f565c0c0c3dc..316fed5f27355 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
@@ -280,6 +280,7 @@ struct sparx5 {
 	int xtr_irq;
 	/* Frame DMA */
 	int fdma_irq;
+	spinlock_t tx_lock; /* lock for frame transmission */
 	struct sparx5_rx rx;
 	struct sparx5_tx tx;
 	/* PTP */
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c b/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
index 6db6ac6a3bbc2..ac7e1cffbcecf 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
@@ -244,10 +244,12 @@ netdev_tx_t sparx5_port_xmit_impl(struct sk_buff *skb, struct net_device *dev)
 	}
 
 	skb_tx_timestamp(skb);
+	spin_lock(&sparx5->tx_lock);
 	if (sparx5->fdma_irq > 0)
 		ret = sparx5_fdma_xmit(sparx5, ifh, skb);
 	else
 		ret = sparx5_inject(sparx5, ifh, skb, dev);
+	spin_unlock(&sparx5->tx_lock);
 
 	if (ret == -EBUSY)
 		goto busy;
-- 
2.34.1

Re: [PATCH net-next] net: sparx5: Add spinlock for frame transmission from CPU

[Daniel Machon]

> Both registers used when doing manual injection or fdma injection are
> shared between all the net devices of the switch. It was noticed that
> when having two process which each of them trying to inject frames on
> different ethernet ports, that the HW started to behave strange, by
> sending out more frames then expected. When doing fdma injection it is
> required to set the frame in the DCB and then make sure that the next
> pointer of the last DCB is invalid. But because there is no locks for
> this, then easily this pointer between the DCB can be broken and then it
> would create a loop of DCBs. And that means that the HW will
> continuously transmit these frames in a loop. Until the SW will break
> this loop.
> Therefore to fix this issue, add a spin lock for when accessing the
> registers for manual or fdma injection.

Reviewed-by: Daniel Machon <daniel.machon@microchip.com>

Re: [PATCH net-next] net: sparx5: Add spinlock for frame transmission from CPU

[Florian Fainelli]

On 2/13/24 04:17, Horatiu Vultur wrote:
> Both registers used when doing manual injection or fdma injection are
> shared between all the net devices of the switch. It was noticed that
> when having two process which each of them trying to inject frames on
> different ethernet ports, that the HW started to behave strange, by
> sending out more frames then expected. When doing fdma injection it is
> required to set the frame in the DCB and then make sure that the next
> pointer of the last DCB is invalid. But because there is no locks for
> this, then easily this pointer between the DCB can be broken and then it
> would create a loop of DCBs. And that means that the HW will
> continuously transmit these frames in a loop. Until the SW will break
> this loop.
> Therefore to fix this issue, add a spin lock for when accessing the
> registers for manual or fdma injection.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>

Any reason you targeted 'net-next' rather than 'net', as this appears to 
be clearly a bug fix here?
-- 
Florian

Re: [PATCH net-next] net: sparx5: Add spinlock for frame transmission from CPU

[Horatiu Vultur]

The 02/13/2024 09:26, Florian Fainelli wrote:
> 
> On 2/13/24 04:17, Horatiu Vultur wrote:
> > Both registers used when doing manual injection or fdma injection are
> > shared between all the net devices of the switch. It was noticed that
> > when having two process which each of them trying to inject frames on
> > different ethernet ports, that the HW started to behave strange, by
> > sending out more frames then expected. When doing fdma injection it is
> > required to set the frame in the DCB and then make sure that the next
> > pointer of the last DCB is invalid. But because there is no locks for
> > this, then easily this pointer between the DCB can be broken and then it
> > would create a loop of DCBs. And that means that the HW will
> > continuously transmit these frames in a loop. Until the SW will break
> > this loop.
> > Therefore to fix this issue, add a spin lock for when accessing the
> > registers for manual or fdma injection.
> > 
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> 
> Any reason you targeted 'net-next' rather than 'net', as this appears to
> be clearly a bug fix here?

Yes, it is a bug but it is not something that happens all the
time and I thought this fits more into the lines of 'This could be a
problem ...' therefore I had targeted 'net-next'.
But if you consider that I should target 'net' instead of 'net-next' I
can do that.

> --
> Florian
> 

-- 
/Horatiu

Re: [PATCH net-next] net: sparx5: Add spinlock for frame transmission from CPU

[Jakub Kicinski]

On Wed, 14 Feb 2024 09:14:42 +0100 Horatiu Vultur wrote:
> > Any reason you targeted 'net-next' rather than 'net', as this appears to
> > be clearly a bug fix here?  
> 
> Yes, it is a bug but it is not something that happens all the
> time and I thought this fits more into the lines of 'This could be a
> problem ...' therefore I had targeted 'net-next'.
> But if you consider that I should target 'net' instead of 'net-next' I
> can do that.

Definitely a bug fix worthy of net, yes, please.

Re: [PATCH net-next] net: sparx5: Add spinlock for frame transmission from CPU

[Horatiu Vultur]

The 02/14/2024 07:09, Jakub Kicinski wrote:
> 
> On Wed, 14 Feb 2024 09:14:42 +0100 Horatiu Vultur wrote:
> > > Any reason you targeted 'net-next' rather than 'net', as this appears to
> > > be clearly a bug fix here?
> >
> > Yes, it is a bug but it is not something that happens all the
> > time and I thought this fits more into the lines of 'This could be a
> > problem ...' therefore I had targeted 'net-next'.
> > But if you consider that I should target 'net' instead of 'net-next' I
> > can do that.
> 
> Definitely a bug fix worthy of net, yes, please.

Perfect, I will do that.

> 

-- 
/Horatiu