Received-SPF: pass (google.com: domain of linux-kernel+bounces-66436-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Thu, 15 Feb 2024 09:41:01 +0100
From: Horatiu Vultur <horatiu.vultur@microchip.com>
To: <davem@davemloft.net>, <edumazet@google.com>, <kuba@kernel.org>,
	<pabeni@redhat.com>, <lars.povlsen@microchip.com>,
	<Steen.Hegelund@microchip.com>, <daniel.machon@microchip.com>,
	<UNGLinuxDriver@microchip.com>
CC: <u.kleine-koenig@pengutronix.de>, <rmk+kernel@armlinux.org.uk>,
	<jacob.e.keller@intel.com>, <yuehaibing@huawei.com>,
	<vladimir.oltean@nxp.com>, <netdev@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH net v2] net: sparx5: Add spinlock for frame transmission
 from CPU
Message-ID: <20240215084101.bunwttvbsx4ecmad@DEN-DL-M31836.microchip.com>
References: <20240215083333.2139380-1-horatiu.vultur@microchip.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
In-Reply-To: <20240215083333.2139380-1-horatiu.vultur@microchip.com>

The 02/15/2024 09:33, 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>
> Reviewed-by: Daniel Machon <daniel.machon@microchip.com>

Argh... I forgot to add the Fixes tag:
Fixes: f3cad2611a77 ("net: sparx5: add hostmode with phylink support")

> ---
> v1->v2:
> - target net instead of net-next
> ---
>  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
> 

-- 
/Horatiu