Re: [PATCH V2 6/8] net: mediatek: fix TX locking

[Sergei Shtylyov]

Hello.

On 04/07/2016 10:26 PM, John Crispin wrote:

> Inside the TX path there is a lock inside the tx_map function. This is
> however too late. The patch moves the lock to the start of the xmit
> function right before the free count check of the DMA ring happens.
> If we do not do this, the code becomes racy leading to TX stalls and
> dropped packets. This happens as there are 2 netdevs running on the
> same physical DMA ring.
>
> Signed-off-by: John Crispin <blogic@xxxxxxxxxxx>
> ---
>   drivers/net/ethernet/mediatek/mtk_eth_soc.c |   20 ++++++++++----------
>   1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> index 60b66ab..8434355 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
[...]
> @@ -712,14 +702,22 @@ static int mtk_start_xmit(struct sk_buff *skb, struct net_device *dev)
>   	struct mtk_eth *eth = mac->hw;
>   	struct mtk_tx_ring *ring = &eth->tx_ring;
>   	struct net_device_stats *stats = &dev->stats;
> +	unsigned long flags;
>   	bool gso = false;
>   	int tx_num;
>
> +	/* normally we can rely on the stack not calling this more than once,
> +	 * however we have 2 queues running ont he same ring so we need to lock

   s/ont he/ on the/, perhaps a good chance to fix the comment?

> +	 * the ring access
> +	 */
> +	spin_lock_irqsave(&eth->page_lock, flags);
> +
>   	tx_num = mtk_cal_txd_req(skb);
>   	if (unlikely(atomic_read(&ring->free_count) <= tx_num)) {
>   		mtk_stop_queue(eth);
>   		netif_err(eth, tx_queued, dev,
>   			  "Tx Ring full when queue awake!\n");
> +		spin_unlock_irqrestore(&eth->page_lock, flags);
>   		return NETDEV_TX_BUSY;
>   	}
[...]

MBR, Sergei