diff mbox

[-next,v2,3/4] net: w5100: increase TX timeout period

Message ID 1463205350-7089-4-git-send-email-akinobu.mita@gmail.com
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Akinobu Mita May 14, 2016, 5:55 a.m. UTC
This increases TX timeout period from one second to 5 seconds which is
the default value if the driver doesn't explicitly set
net_device->watchdog_timeo.

The one second timeout is too short for W5100 with SPI interface mode
which doesn't support burst READ/WRITE processing in the SPI transfer.
If the packet is transmitted while RX packets are being received at a
very high rate, the TX transmittion work in the workqueue is delayed
and the watchdog timer is expired.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Mike Sinkovsky <msink@permonline.ru>
Cc: David S. Miller <davem@davemloft.net>
---
* v2
- Remove the watchdong_timeo assignment to set default tx timeout,
  suggested by David Miller.

 drivers/net/ethernet/wiznet/w5100.c | 1 -
 1 file changed, 1 deletion(-)

Comments

David Laight May 16, 2016, 9:33 a.m. UTC | #1
From: netdev-owner@vger.kernel.org Akinobu Mita
> Sent: 14 May 2016 06:56
> This increases TX timeout period from one second to 5 seconds which is
> the default value if the driver doesn't explicitly set
> net_device->watchdog_timeo.
> 
> The one second timeout is too short for W5100 with SPI interface mode
> which doesn't support burst READ/WRITE processing in the SPI transfer.
> If the packet is transmitted while RX packets are being received at a
> very high rate, the TX transmittion work in the workqueue is delayed
> and the watchdog timer is expired.

ISTM that if RX traffic can cause a 1 second timeout to expire I can
see no reason why it shouldn't also cause a 5 second timer to expire.

I'm guessing that something needs to be done to cause the SPI
block to discard receive frames (I guess the hardware will be discarding
them as well) in order to allow frames to be transmitted.

Given the 'damp string' nature of SPI, you might even want to give
transmits priority over receives.

	David
Akinobu Mita May 16, 2016, 1:53 p.m. UTC | #2
2016-05-16 18:33 GMT+09:00 David Laight <David.Laight@aculab.com>:
> From: netdev-owner@vger.kernel.org Akinobu Mita
>> Sent: 14 May 2016 06:56
>> This increases TX timeout period from one second to 5 seconds which is
>> the default value if the driver doesn't explicitly set
>> net_device->watchdog_timeo.
>>
>> The one second timeout is too short for W5100 with SPI interface mode
>> which doesn't support burst READ/WRITE processing in the SPI transfer.
>> If the packet is transmitted while RX packets are being received at a
>> very high rate, the TX transmittion work in the workqueue is delayed
>> and the watchdog timer is expired.
>
> ISTM that if RX traffic can cause a 1 second timeout to expire I can
> see no reason why it shouldn't also cause a 5 second timer to expire.
>
> I'm guessing that something needs to be done to cause the SPI
> block to discard receive frames (I guess the hardware will be discarding
> them as well) in order to allow frames to be transmitted.

You are right.  In SPI interface mode, the interrupt of W5100 is masked
as far as receiving packets in w5100_rx_work(). (Ideally, RECV interrupt
should only be masked as W5200 and W5500 have different interrupt mask
bits for RECV and SENDOK, but W5100 doesn't have)

So this problem should be fixed by limiting the maximum execution time
of w5100_rx_skb() in w5100_rx_work() by maximum loop count or querying
jiffies.  I'll try it out.

> Given the 'damp string' nature of SPI, you might even want to give
> transmits priority over receives.

How can we do this?
diff mbox

Patch

diff --git a/drivers/net/ethernet/wiznet/w5100.c b/drivers/net/ethernet/wiznet/w5100.c
index c80438c..43fdf88 100644
--- a/drivers/net/ethernet/wiznet/w5100.c
+++ b/drivers/net/ethernet/wiznet/w5100.c
@@ -1142,7 +1142,6 @@  int w5100_probe(struct device *dev, const struct w5100_ops *ops,
 
 	ndev->netdev_ops = &w5100_netdev_ops;
 	ndev->ethtool_ops = &w5100_ethtool_ops;
-	ndev->watchdog_timeo = HZ;
 	netif_napi_add(ndev, &priv->napi, w5100_napi_poll, 16);
 
 	/* This chip doesn't support VLAN packets with normal MTU,