| Message ID | 20230529073817.1145208-1-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a60caf039e96d806b1ced893242bae82ba3ccf0d |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: renesas: rswitch: Fix return value in error path of xmit | expand |
On Mon, May 29, 2023 at 04:38:17PM +0900, Yoshihiro Shimoda wrote: > Fix return value in the error path of rswitch_start_xmit(). If TX > queues are full, this function should return NETDEV_TX_BUSY. > > Fixes: 3590918b5d07 ("net: ethernet: renesas: Add support for "Ethernet Switch"") > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> Hi Shimoda-san, I agree that this is the correct return value for this case. But I do wonder if, as per the documentation of ndo_start_xmit, something should be done to avoid getting into such a situation. * netdev_tx_t (*ndo_start_xmit)(struct sk_buff *skb, * struct net_device *dev); * Called when a packet needs to be transmitted. * Returns NETDEV_TX_OK. Can return NETDEV_TX_BUSY, but you should stop * the queue before that can happen; it's for obsolete devices and weird * corner cases, but the stack really does a non-trivial amount * of useless work if you return NETDEV_TX_BUSY. * Required; cannot be NULL. 以上
On Tue, 2023-05-30 at 13:42 +0200, Simon Horman wrote: > On Mon, May 29, 2023 at 04:38:17PM +0900, Yoshihiro Shimoda wrote: > > Fix return value in the error path of rswitch_start_xmit(). If TX > > queues are full, this function should return NETDEV_TX_BUSY. > > > > Fixes: 3590918b5d07 ("net: ethernet: renesas: Add support for "Ethernet Switch"") > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > Hi Shimoda-san, > > I agree that this is the correct return value for this case. > But I do wonder if, as per the documentation of ndo_start_xmit, > something should be done to avoid getting into such a situation. > > * netdev_tx_t (*ndo_start_xmit)(struct sk_buff *skb, > * struct net_device *dev); > * Called when a packet needs to be transmitted. > * Returns NETDEV_TX_OK. Can return NETDEV_TX_BUSY, but you should stop > * the queue before that can happen; it's for obsolete devices and weird > * corner cases, but the stack really does a non-trivial amount > * of useless work if you return NETDEV_TX_BUSY. > * Required; cannot be NULL. I agree with Simon, it looks like the driver usage of netif_stop_subqueue()/netif_wake_subqueue() is a dubious. I think you will be better of using netif_subqueue_maybe_stop()/netif_subqueue_completed_wake() alike what rtl8169 is doing. e.g. netif_subqueue_maybe_stop() should be invoked after the tx buffer enqueue, and netif_subqueue_completed_wake() should be invoked after successful tx ring cleanup. Thanks! Paolo
On Thu, 01 Jun 2023 10:41:34 +0200 Paolo Abeni wrote: > > I agree that this is the correct return value for this case. > > But I do wonder if, as per the documentation of ndo_start_xmit, > > something should be done to avoid getting into such a situation. > > > > * netdev_tx_t (*ndo_start_xmit)(struct sk_buff *skb, > > * struct net_device *dev); > > * Called when a packet needs to be transmitted. > > * Returns NETDEV_TX_OK. Can return NETDEV_TX_BUSY, but you should stop > > * the queue before that can happen; it's for obsolete devices and weird > > * corner cases, but the stack really does a non-trivial amount > > * of useless work if you return NETDEV_TX_BUSY. > > * Required; cannot be NULL. > > I agree with Simon, it looks like the driver usage of > netif_stop_subqueue()/netif_wake_subqueue() is a dubious. > > I think you will be better of using > netif_subqueue_maybe_stop()/netif_subqueue_completed_wake() alike what > rtl8169 is doing. e.g. netif_subqueue_maybe_stop() should be invoked > after the tx buffer enqueue, and netif_subqueue_completed_wake() should > be invoked after successful tx ring cleanup. That's a separate issue, tho, right? The cleanup is lockless and our magic lockless macro scheme does not protect from spurious wakeups. So they still need to check if the queue is full at the top of xmit. And they still need to return the correct error in that case..
On Thu, 2023-06-01 at 09:10 -0700, Jakub Kicinski wrote: > On Thu, 01 Jun 2023 10:41:34 +0200 Paolo Abeni wrote: > > > I agree that this is the correct return value for this case. > > > But I do wonder if, as per the documentation of ndo_start_xmit, > > > something should be done to avoid getting into such a situation. > > > > > > * netdev_tx_t (*ndo_start_xmit)(struct sk_buff *skb, > > > * struct net_device *dev); > > > * Called when a packet needs to be transmitted. > > > * Returns NETDEV_TX_OK. Can return NETDEV_TX_BUSY, but you should stop > > > * the queue before that can happen; it's for obsolete devices and weird > > > * corner cases, but the stack really does a non-trivial amount > > > * of useless work if you return NETDEV_TX_BUSY. > > > * Required; cannot be NULL. > > > > I agree with Simon, it looks like the driver usage of > > netif_stop_subqueue()/netif_wake_subqueue() is a dubious. > > > > I think you will be better of using > > netif_subqueue_maybe_stop()/netif_subqueue_completed_wake() alike what > > rtl8169 is doing. e.g. netif_subqueue_maybe_stop() should be invoked > > after the tx buffer enqueue, and netif_subqueue_completed_wake() should > > be invoked after successful tx ring cleanup. > > That's a separate issue, tho, right? The cleanup is lockless and our > magic lockless macro scheme does not protect from spurious wakeups. > So they still need to check if the queue is full at the top of xmit. > And they still need to return the correct error in that case.. I guess you are right, dubious wakeup could be addresses with a separate patch if needed. Fine by me to apply it as-is. Cheers, Paolo
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Mon, 29 May 2023 16:38:17 +0900 you wrote: > Fix return value in the error path of rswitch_start_xmit(). If TX > queues are full, this function should return NETDEV_TX_BUSY. > > Fixes: 3590918b5d07 ("net: ethernet: renesas: Add support for "Ethernet Switch"") > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > --- > drivers/net/ethernet/renesas/rswitch.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Here is the summary with links: - [net] net: renesas: rswitch: Fix return value in error path of xmit https://git.kernel.org/netdev/net/c/a60caf039e96 You are awesome, thank you!
diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c index 29afaddb598d..aace87139cea 100644 --- a/drivers/net/ethernet/renesas/rswitch.c +++ b/drivers/net/ethernet/renesas/rswitch.c @@ -1485,7 +1485,7 @@ static netdev_tx_t rswitch_start_xmit(struct sk_buff *skb, struct net_device *nd if (rswitch_get_num_cur_queues(gq) >= gq->ring_size - 1) { netif_stop_subqueue(ndev, 0); - return ret; + return NETDEV_TX_BUSY; } if (skb_put_padto(skb, ETH_ZLEN))
Fix return value in the error path of rswitch_start_xmit(). If TX queues are full, this function should return NETDEV_TX_BUSY. Fixes: 3590918b5d07 ("net: ethernet: renesas: Add support for "Ethernet Switch"") Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- drivers/net/ethernet/renesas/rswitch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)