From: Dalon Westergreen <[email protected]>
The return from tx_buffer call in tse_start_xmit is
inapropriately ignored. tse_buffer calls should return
0 for success or NETDEV_TX_BUSY. tse_start_xmit should
return not report a successful transmit when the tse_buffer
call returns an error condition.
In addition to the above, the msgdma and sgdma do not return
the same value on success or failure. The sgdma_tx_buffer
returned 0 on failure and a positive number of transmitted
packets on success. Given that it only ever sends 1 packet,
this made no sense. The msgdma implementation msgdma_tx_buffer
returns 0 on success.
-> Don't ignore the return from tse_buffer calls
-> Fix sgdma tse_buffer call to return 0 on success
and NETDEV_TX_BUSY on failure.
Signed-off-by: Dalon Westergreen <[email protected]>
Signed-off-by: Joyce Ooi <[email protected]>
---
v2: no change
---
drivers/net/ethernet/altera/altera_sgdma.c | 14 ++++++++------
drivers/net/ethernet/altera/altera_tse_main.c | 4 +++-
2 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/altera/altera_sgdma.c b/drivers/net/ethernet/altera/altera_sgdma.c
index db97170da8c7..77e2c5e3650f 100644
--- a/drivers/net/ethernet/altera/altera_sgdma.c
+++ b/drivers/net/ethernet/altera/altera_sgdma.c
@@ -4,6 +4,7 @@
*/
#include <linux/list.h>
+#include <linux/netdevice.h>
#include "altera_utils.h"
#include "altera_tse.h"
#include "altera_sgdmahw.h"
@@ -159,10 +160,11 @@ void sgdma_clear_txirq(struct altera_tse_private *priv)
SGDMA_CTRLREG_CLRINT);
}
-/* transmits buffer through SGDMA. Returns number of buffers
- * transmitted, 0 if not possible.
- *
- * tx_lock is held by the caller
+/* transmits buffer through SGDMA.
+ * original behavior returned the number of transmitted packets (always 1) &
+ * returned 0 on error. This differs from the msgdma. the calling function
+ * will now actually look at the code, so from now, 0 is good and return
+ * NETDEV_TX_BUSY when busy.
*/
int sgdma_tx_buffer(struct altera_tse_private *priv, struct tse_buffer *buffer)
{
@@ -174,7 +176,7 @@ int sgdma_tx_buffer(struct altera_tse_private *priv, struct tse_buffer *buffer)
/* wait 'til the tx sgdma is ready for the next transmit request */
if (sgdma_txbusy(priv))
- return 0;
+ return NETDEV_TX_BUSY;
sgdma_setup_descrip(cdesc, /* current descriptor */
ndesc, /* next descriptor */
@@ -191,7 +193,7 @@ int sgdma_tx_buffer(struct altera_tse_private *priv, struct tse_buffer *buffer)
/* enqueue the request to the pending transmit queue */
queue_tx(priv, buffer);
- return 1;
+ return 0;
}
diff --git a/drivers/net/ethernet/altera/altera_tse_main.c b/drivers/net/ethernet/altera/altera_tse_main.c
index 1671c1f36691..2a9e6157a8a1 100644
--- a/drivers/net/ethernet/altera/altera_tse_main.c
+++ b/drivers/net/ethernet/altera/altera_tse_main.c
@@ -595,7 +595,9 @@ static int tse_start_xmit(struct sk_buff *skb, struct net_device *dev)
buffer->dma_addr = dma_addr;
buffer->len = nopaged_len;
- priv->dmaops->tx_buffer(priv, buffer);
+ ret = priv->dmaops->tx_buffer(priv, buffer);
+ if (ret)
+ goto out;
skb_tx_timestamp(skb);
--
2.13.0
From: Joyce Ooi <[email protected]>
Date: Mon, 4 May 2020 16:25:49 +0800
> The return from tx_buffer call in tse_start_xmit is
> inapropriately ignored. tse_buffer calls should return
> 0 for success or NETDEV_TX_BUSY. tse_start_xmit should
> return not report a successful transmit when the tse_buffer
> call returns an error condition.
From driver.txt:
====================
1) The ndo_start_xmit method must not return NETDEV_TX_BUSY under
any normal circumstances. It is considered a hard error unless
there is no way your device can tell ahead of time when it's
transmit function will become busy.
====================
The problem is that when you return this error code, something has
to trigger restarting the transmit queue to start sending packets
to your device again. The usual mechanism is waking the transmit
queue, but it's obviously already awake since your transmit routine
is being called. Therefore nothing will reliably restart the queue
when you return this error code.
The best thing to do honestly is to drop the packet and return
NETDEV_TX_OK, meanwhile bumping a statistic counter to record this
event.
> -----Original Message-----
> From: David Miller <[email protected]>
> Sent: Tuesday, May 5, 2020 1:40 AM
> To: Ooi, Joyce <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; Westergreen, Dalon <[email protected]>;
> Tan, Ley Foon <[email protected]>; See, Chin Liang
> <[email protected]>; Nguyen, Dinh <[email protected]>
> Subject: Re: [PATCHv2 01/10] net: eth: altera: tse_start_xmit ignores tx_buffer
> call response
>
> From: Joyce Ooi <[email protected]>
> Date: Mon, 4 May 2020 16:25:49 +0800
>
> > The return from tx_buffer call in tse_start_xmit is inapropriately
> > ignored. tse_buffer calls should return
> > 0 for success or NETDEV_TX_BUSY. tse_start_xmit should return not
> > report a successful transmit when the tse_buffer call returns an error
> > condition.
>
> From driver.txt:
>
> ====================
> 1) The ndo_start_xmit method must not return NETDEV_TX_BUSY under
> any normal circumstances. It is considered a hard error unless
> there is no way your device can tell ahead of time when it's
> transmit function will become busy.
> ====================
>
> The problem is that when you return this error code, something has to trigger
> restarting the transmit queue to start sending packets to your device again. The
> usual mechanism is waking the transmit queue, but it's obviously already awake
> since your transmit routine is being called. Therefore nothing will reliably restart
> the queue when you return this error code.
>
> The best thing to do honestly is to drop the packet and return NETDEV_TX_OK,
> meanwhile bumping a statistic counter to record this event.
My change is similar to this hard error mentioned in drvier.txt:
/* This is a hard error log it. */
if (TX_BUFFS_AVAIL(dp) <= (skb_shinfo(skb)->nr_frags + 1)) {
netif_stop_queue(dev);
unlock_tx(dp);
printk(KERN_ERR PFX "%s: BUG! Tx Ring full when queue awake!\n",
dev->name);
return NETDEV_TX_BUSY;
}
So, before returning NETDEV_TX_BUSY, I can stop the queue first by calling
netif_stop_queue().