[PATCH net] net: renesas: rswitch: Fix return value in error path of xmit

[PATCH net] net: renesas: rswitch: Fix return value in error path of xmit

[Yoshihiro Shimoda]

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(-)

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))
-- 
2.25.1

Re: [PATCH net] net: renesas: rswitch: Fix return value in error path of xmit

[Simon Horman]

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.

以上

Re: [PATCH net] net: renesas: rswitch: Fix return value in error path of xmit

[Paolo Abeni]

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

Re: [PATCH net] net: renesas: rswitch: Fix return value in error path of xmit

[Jakub Kicinski]

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..

Re: [PATCH net] net: renesas: rswitch: Fix return value in error path of xmit

[Paolo Abeni]

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

Re: [PATCH net] net: renesas: rswitch: Fix return value in error path of xmit

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html