From: Russell King - ARM Linux <hidden> Date: 2014-06-20 12:11:18
This is my initial round of patches (roughly half of my total patch
set) for the Freescale FEC driver.
I'm sending this set out initially for comments and testing. One of
my motivations for only sending half is to get this half into a state
where DaveM is happy to merge it before I sort out the remainder.
There's quite a lot here, so bear with me on this.
I've tried to sort the fixes before the cleanups as best I can, but
reordering the series is a full-time job due to it's size - it's taken
from Monday until now to get this far with it, so I'm hoping that there
won't be any "you should rearrange the patches as X" comments.
While the original series was well tested during it's original
development, including with performance tests on each patch, that
testing and validation has been lost due to the changes during the
last merge window, and subsequent rebasing and updating of the patches.
The series is based on v3.16-rc1, and as such, I have added Andy's
checksum fix to the start of the series. This patch is not strictly
part of the RFC, but is included because my complete patch series needs
to account for that change.
Some of these patches are to fix theoretical problems in the driver
(ones which have been found via a review of the code) others address
real observable problems (such as poor half-duplex performance.)
Towards the end of this series, I have included a patch which was
initially at the beginning of the series for dumping out the state of
the transmit ring, which is very useful to debug transmit problems.
This was acceptable when the transmit ring was between 16 and 128
descriptors, but during the recent merge window, this was increased to
512 descriptors, so it will now print around 512 lines to the kernel
message log on transmit timeout. I'm not entirely convinced this is a
good idea - maybe it should become optional, or maybe the timed-out ring
status should be available via debugfs, but that's a problem to retrieve
if you're running NFS rootfs and the timeout doesn't recover properly.
This series of patches is also available from the following *unstable*
git branch - unstable as it's provided for convenience so you don't
have to apply all these patches individually, unstable as the patches
need to have attributations added, unstable because DaveM will probably
want to apply them as patches to his tree:
git://ftp.arm.linux.org.uk/~rmk/linux-arm.git fec-testing
drivers/net/ethernet/freescale/fec.h | 10 +-
drivers/net/ethernet/freescale/fec_main.c | 383 +++++++++++++++++-------------
2 files changed, 225 insertions(+), 168 deletions(-)
--
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
From: Russell King <hidden> Date: 2014-06-20 12:11:48
From: Fugang Duan <redacted>
To: linux-arm-kernel@lists.infradead.org
The commit 96c50caa5148 (net: fec: Enable IP header hardware checksum)
enable HW IP header checksum for IPV4 and IPV6, which causes IPV6 TCP/UDP
cannot work. (The issue is reported by Russell King)
For FEC IP header checksum function: Insert IP header checksum. This "IINS"
bit is written by the user. If set, IP accelerator calculates the IP header
checksum and overwrites the IINS corresponding header field with the calculated
value. The checksum field must be cleared by user, otherwise the checksum
always is 0xFFFF.
So the previous patch clear IP header checksum field regardless of IP frame
type.
In fact, IP HW detect the packet as IPV6 type, even if the "IINS" bit is set,
the IP accelerator is not triggered to calculates IPV6 header checksum because
IPV6 frame format don't have checksum.
So this results in the IPV6 frame being corrupted.
The patch just add software detect the current packet type, if it is IPV6
frame, it don't clear IP header checksum field.
Reported-and-tested-by: Russell King <redacted>
Signed-off-by: Fugang Duan <redacted>
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec_main.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
From: Russell King <hidden> Date: 2014-06-20 12:11:53
The iMX6 gigabit FEC does not support half-duplex gigabit operation.
Phys attacked to the FEC may support this, and we currently do nothing
to disable this feature. This may result in an invalid configuration.
Mask out phy support for gigabit half-duplex operation.
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec_main.c | 1 +
1 file changed, 1 insertion(+)
@@ -1667,6 +1667,7 @@ static int fec_enet_mii_probe(struct net_device *ndev)/* mask with MAC supported features */if(id_entry->driver_data&FEC_QUIRK_HAS_GBIT){phy_dev->supported&=PHY_GBIT_FEATURES;+phy_dev->supported&=~SUPPORTED_1000baseT_Half;#if !defined(CONFIG_M5272)phy_dev->supported|=SUPPORTED_Pause;#endif
From: Russell King <hidden> Date: 2014-06-20 12:11:58
Setting the pause parameters causes a running network interface to be
restarted. However, the restart forces the FEC into half-duplex mode,
whether or not the remote end is in half-duplex mode. Misconfigured
duplex mode is a known source of problems on a link.
Fix this by always preserving the duplex mode on configuration changes.
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Russell King <hidden> Date: 2014-06-20 12:12:03
While running: while :; do iperf -c <HOST> -P 4; done, transmit timeouts
are regularly reported. With the tx ring dumping in place, we can see
that all entries are in use, and the hardware has finished transmitting
these packets. However, the driver has not reclaimed these ring
entries.
This can occur if the interrupt handler is invoked at the wrong moment -
eg:
CPU0 CPU1
fec_enet_tx()
interrupt, IEVENT = FEC_ENET_TXF
FEC_ENET_TXF cleared
napi_schedule_prep()
napi_complete()
The result is that we clear the transmit interrupt, but we don't trigger
any cleaning of the transmit ring. Instead, use a different strategy:
- When receiving a transmit or receive interrupt, disable both tx and rx
interrupts, but do not acknowledge them. Schedule a napi poll. Don't
loop.
- When we are polled, read IEVENT, acknowledging the pending transmit
and receive interrupts, before then going on to process the
appropriate rings.
This allows us to avoid the race, and has a number of other advantages:
- we cut down on the number of transmit interrupts we have to process.
- we only look at the rings which have pending events.
- we gain additional throughput: the iperf total bandwidth increases
from about 180Mbps to 240Mbps:
[ 3] 0.0-10.0 sec 68.1 MBytes 57.0 Mbits/sec
[ 5] 0.0-10.0 sec 72.4 MBytes 60.5 Mbits/sec
[ 4] 0.0-10.1 sec 76.1 MBytes 63.5 Mbits/sec
[ 6] 0.0-10.1 sec 71.9 MBytes 59.9 Mbits/sec
[SUM] 0.0-10.1 sec 288 MBytes 241 Mbits/sec
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec_main.c | 40 +++++++++++++++++--------------
1 file changed, 22 insertions(+), 18 deletions(-)
From: Russell King <hidden> Date: 2014-06-20 12:12:08
We use netif_stop_queue() in several places where we want to ensure that
the start_xmit function is not running. netif_stop_queue() is not
sufficient to achieve that - it merely sets a flag to indicate that the
transmit queue(s) should not be run.
netif_tx_disable() gives this guarantee, since it takes the transmit
queue lock while marking the queue stopped. This will wait for the
transmit function to complete before returning.
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec_main.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
@@ -834,7 +834,7 @@ fec_restart(struct net_device *ndev, int duplex)if(netif_running(ndev)){netif_device_detach(ndev);napi_disable(&fep->napi);-netif_stop_queue(ndev);+netif_tx_disable(ndev);netif_tx_lock_bh(ndev);}
@@ -2181,7 +2181,7 @@ fec_enet_close(struct net_device *ndev)/* Don't know what to do yet. */napi_disable(&fep->napi);fep->opened=0;-netif_stop_queue(ndev);+netif_tx_disable(ndev);fec_stop(ndev);if(fep->phy_dev){
From: Russell King <hidden> Date: 2014-06-20 12:12:13
fep->phy_dev can not be NULL here for two reasons:
- fec_enet_open() will have successfully connected the phy, or will have
failed.
- fec_enet_open() will have called phy_start(fep->phy_dev), which
unconditionally dereferences this pointer.
If it were to be NULL here, then fec_enet_open() will have already
oopsed.
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec_main.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
From: Russell King <hidden> Date: 2014-06-20 12:12:18
When we disconnect from a phy, we should forget our pointer to it so we
don't accidentally try to configure it. We handle a NULL phy pointer
correctly in most places, except fec_enet_set_pauseparam(). Fix this
too.
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec_main.c | 4 ++++
1 file changed, 4 insertions(+)
@@ -1875,6 +1875,9 @@ static int fec_enet_set_pauseparam(struct net_device *ndev,{structfec_enet_private*fep=netdev_priv(ndev);+if(!fep->phy_dev)+return-ENODEV;+if(pause->tx_pause!=pause->rx_pause){netdev_info(ndev,"hardware only support enable/disable both tx and rx");
From: Russell King <hidden> Date: 2014-06-20 12:12:24
When the network interface goes down, stop the phy to prevent further
link up status changes before taking the MAC or netif sections down.
This prevents further reception of link up events which could
potentially call fec_restart().
Since phy_stop() takes the mutex which adjust_link() runs under, we
also ensure that adjust_link() will not already be processing a link
up event.
We also need to do this when suspending as well - we don't want a
mis-timed phy state change to restart the MAC after we have stopped
it for suspend, and thus need to restart the phy when resuming.
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec_main.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
@@ -2181,13 +2181,14 @@ fec_enet_close(struct net_device *ndev){structfec_enet_private*fep=netdev_priv(ndev);+phy_stop(fep->phy_dev);+/* Don't know what to do yet. */napi_disable(&fep->napi);fep->opened=0;netif_tx_disable(ndev);fec_stop(ndev);-phy_stop(fep->phy_dev);phy_disconnect(fep->phy_dev);fep->phy_dev=NULL;
From: Russell King <hidden> Date: 2014-06-20 12:12:29
napi_disable() waits until the NAPI processing has completed, and then
prevents any further polls. At this point, the driver then clears
fep->opened. The NAPI poll function uses this to stop processing in
the receive path. Hence, it will never see this variable cleared,
because the NAPI poll has to complete before it will be cleared.
Therefore, this variable serves no purpose, so let's remove it.
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec.h | 1 -
drivers/net/ethernet/freescale/fec_main.c | 5 -----
2 files changed, 6 deletions(-)
@@ -1215,9 +1215,6 @@ fec_enet_rx(struct net_device *ndev, int budget)if((status&BD_ENET_RX_LAST)==0)netdev_err(ndev,"rcv is not +last\n");-if(!fep->opened)-gotorx_processing_done;-/* Check for errors. */if(status&(BD_ENET_RX_LG|BD_ENET_RX_SH|BD_ENET_RX_NO|BD_ENET_RX_CR|BD_ENET_RX_OV)){
@@ -2185,7 +2181,6 @@ fec_enet_close(struct net_device *ndev)/* Don't know what to do yet. */napi_disable(&fep->napi);-fep->opened=0;netif_tx_disable(ndev);fec_stop(ndev);
From: Russell King <hidden> Date: 2014-06-20 12:12:34
Allocate, and then map the receive skb before writing any data to the
ring descriptor or storing the skb. When freeing the receive ring
entries, unmap and free the skb, and then clear the stored skb pointer.
This means we have ring data and skb pointer in one of two states:
either both fully setup, or nothing setup.
This simplifies the cleanup, as we can use just the skb pointer to
indicate whether the descriptor is setup, and thus avoids potentially
calling dma_unmap_single() on a DMA error value.
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec_main.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
From: Russell King <hidden> Date: 2014-06-20 12:12:39
Avoid writing any state until we're certain we can proceed with the
transmission: this avoids writing mapping error address values to the
descriptors, or setting the skbuff pointer until we have successfully
mapped the skb.
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec_main.c | 33 +++++++++++++++++--------------
1 file changed, 18 insertions(+), 15 deletions(-)
@@ -490,12 +493,9 @@ static int fec_enet_txq_submit_skb(struct sk_buff *skb, struct net_device *ndev)swap_buffer(bufaddr,buflen);}-/* Push the data cache so the CPM does not get stale memory-*data.-*/-bdp->cbd_bufaddr=dma_map_single(&fep->pdev->dev,bufaddr,-buflen,DMA_TO_DEVICE);-if(dma_mapping_error(&fep->pdev->dev,bdp->cbd_bufaddr)){+/* Push the data cache so the CPM does not get stale memory data. */+addr=dma_map_single(&fep->pdev->dev,bufaddr,buflen,DMA_TO_DEVICE);+if(dma_mapping_error(&fep->pdev->dev,addr)){dev_kfree_skb_any(skb);if(net_ratelimit())netdev_err(ndev,"Tx DMA memory map failed\n");
@@ -537,6 +537,7 @@ static int fec_enet_txq_submit_skb(struct sk_buff *skb, struct net_device *ndev)fep->tx_skbuff[index]=skb;bdp->cbd_datlen=buflen;+bdp->cbd_bufaddr=addr;/* Send it on its way. Tell FEC it's ready, interrupt when done,*it'sthelastBDoftheframe,andtoputtheCRContheend.
@@ -801,7 +804,7 @@ static void fec_enet_bd_init(struct net_device *dev)/* Initialize the BD for every fragment in the page. */bdp->cbd_sc=0;-if(bdp->cbd_bufaddr&&fep->tx_skbuff[i]){+if(fep->tx_skbuff[i]){dev_kfree_skb_any(fep->tx_skbuff[i]);fep->tx_skbuff[i]=NULL;}
@@ -1154,7 +1158,6 @@ fec_enet_tx(struct net_device *ndev)/* Free the sk buffer associated with this last transmit */dev_kfree_skb_any(skb);-fep->tx_skbuff[index]=NULL;fep->dirty_tx=bdp;
From: Russell King <hidden> Date: 2014-06-20 12:12:44
Ensure that we do not double-free any allocations, and that any transmit
skbuffs are properly freed when we clean up the rings.
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec_main.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
From: Russell King <hidden> Date: 2014-06-20 12:12:49
fec_enet_alloc_buffers() assumes that kmalloc() will never fail, which
is an invalid assumption. Fix this by implementing a common error
cleanup path, and use it to also clean up after failed bounce buffer
allocation.
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec_main.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
From: Russell King <hidden> Date: 2014-06-20 12:12:54
We should hold the rtnl lock while suspending, resuming or processing
the transmit timeout to ensure that nothing will interfere while we
bring up, take down or restart the hardware. The transmit timeout
could run if we're preempted during suspend.
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec_main.c | 7 +++++++
1 file changed, 7 insertions(+)
From: Russell King <hidden> Date: 2014-06-20 12:12:59
When the FEC is suspended, the device is detached. Upon resume failure,
the device is left in detached mode, possibly with some of the required
clocks not running. We don't want to be poking the device in that state
because as it may cause bus errors.
If the device is marked detached, avoid calling fec_stop().
This depends upon: "net:fec: improve safety of suspend/resume paths"
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec_main.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
@@ -2198,10 +2198,10 @@ fec_enet_close(struct net_device *ndev)phy_stop(fep->phy_dev);-/* Don't know what to do yet. */napi_disable(&fep->napi);netif_tx_disable(ndev);-fec_stop(ndev);+if(netif_device_present(ndev))+fec_stop(ndev);phy_disconnect(fep->phy_dev);fep->phy_dev=NULL;
From: Russell King <hidden> Date: 2014-06-20 12:13:04
Avoid calling fec_restart() or fec_stop() while the device is down
or not present (iow suspended.)
Although the ndo_timeout method will only be called if the device is
present and running, we defer this to a work queue. The work queue
can run independently, and so needs to repeat these checks to ensure
that a restart doesn't occur after the device has been taken down or
detached for suspend. In this case, we call fec_restart() in the
resume path, so nothing is lost.
For fec_set_features, we add a call to fec_restart() in fec_enet_open()
to ensure that the hardware is appropriate programmed when the interface
is opened. fec_set_features() call should not occur while we're
suspended, so we don't have to worry about that case.
The adjust_link needs similar treatment - this also is called from a
work queue, which may be run independently after we have taken the
device down and detached it. In this case, we just mark the link
down and take no further action. We will reset things appropriately
once the device is up and running again, at which point we will receive
another adjust_link callback.
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec_main.c | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)
From: Russell King <hidden> Date: 2014-06-20 12:13:10
Move the calls to quiesce and resume packet processing out of
fec_restart() to its call sites. This is the first step in a two stage
clean up of this code, where we just move the calls out of fec_restart()
without changing them. Not everywhere needs to issue these calls, and
not everywhere needs all of these calls to be issued.
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec_main.c | 64 ++++++++++++++++++++++---------
1 file changed, 45 insertions(+), 19 deletions(-)
@@ -818,9 +818,10 @@ static void fec_enet_bd_init(struct net_device *dev)fep->dirty_tx=bdp;}-/* This function is called to start or restart the FEC during a link-*change.Thisonlyhappenswhenswitchingbetweenhalfandfull-*duplex.+/*+*ThisfunctioniscalledtostartorrestarttheFECduringalink+*change,transmittimeout,ortoreconfiguretheFEC.Thenetwork+*packetprocessingforthisdevicemustbestoppedbeforethiscall.*/staticvoidfec_restart(structnet_device*ndev,intduplex)
@@ -834,13 +835,6 @@ fec_restart(struct net_device *ndev, int duplex)u32rcntl=OPT_FRAME_SIZE|0x04;u32ecntl=0x2;/* ETHEREN */-if(netif_running(ndev)){-netif_device_detach(ndev);-napi_disable(&fep->napi);-netif_tx_disable(ndev);-netif_tx_lock_bh(ndev);-}-/* Whack a reset. We should wait for this. */writel(1,fep->hwp+FEC_ECNTRL);udelay(10);
@@ -1009,13 +1003,6 @@ fec_restart(struct net_device *ndev, int duplex)/* Enable interrupts we wish to service */writel(FEC_DEFAULT_IMASK,fep->hwp+FEC_IMASK);--if(netif_running(ndev)){-netif_tx_unlock_bh(ndev);-netif_wake_queue(ndev);-napi_enable(&fep->napi);-netif_device_attach(ndev);-}}staticvoid
@@ -1529,8 +1523,17 @@ static void fec_enet_adjust_link(struct net_device *ndev)}/* if any of the above changed restart the FEC */-if(status_change)+if(status_change){+netif_device_detach(ndev);+napi_disable(&fep->napi);+netif_tx_disable(ndev);+netif_tx_lock_bh(ndev);fec_restart(ndev,phy_dev->duplex);+netif_tx_unlock_bh(ndev);+netif_wake_queue(ndev);+napi_enable(&fep->napi);+netif_device_attach(ndev);+}}else{if(fep->link){fec_stop(ndev);
@@ -1915,8 +1918,17 @@ static int fec_enet_set_pauseparam(struct net_device *ndev,fec_stop(ndev);phy_start_aneg(fep->phy_dev);}-if(netif_running(ndev))+if(netif_running(ndev)){+netif_device_detach(ndev);+napi_disable(&fep->napi);+netif_tx_disable(ndev);+netif_tx_lock_bh(ndev);fec_restart(ndev,fep->full_duplex);+netif_tx_unlock_bh(ndev);+netif_wake_queue(ndev);+napi_enable(&fep->napi);+netif_device_attach(ndev);+}return0;}
@@ -2360,8 +2372,15 @@ static int fec_set_features(struct net_device *netdev,if(netif_running(netdev)){fec_stop(netdev);+netif_device_detach(netdev);+napi_disable(&fep->napi);+netif_tx_disable(netdev);+netif_tx_lock_bh(netdev);fec_restart(netdev,fep->phy_dev->duplex);+netif_tx_unlock_bh(netdev);netif_wake_queue(netdev);+napi_enable(&fep->napi);+netif_device_attach(netdev);}}
From: Russell King <hidden> Date: 2014-06-20 12:13:15
This is the second stage to "move calls to quiesce/resume packet
processing out of fec_restart()", where we remove calls which are not
appropriate to the call site.
In the majority of cases, there is no need to detach and reattach the
interface as we are holding the queue xmit lock across the reset. The
exception to that is in fec_resume(), where we are already detached by
the suspend function. Here, we can remove the call to detach the
interface.
We also do not need to stop the transmit queue. Holding the xmit lock
is enough to ensure that the transmit packet processing is not running
while we perform our task. However, since fec_restart() always cleans
the rings, we call netif_wake_queue() (or netif_device_attach() in the
case of resume) just before dropping the xmit lock. This prevents the
watchdog firing.
Lastly, always call napi_enable() after the device has been reattached
in the resume path so that we know that the transmit packet processing
is already in an enabled state, so we don't call netif_wake_queue()
while detached.
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec_main.c | 26 ++++++--------------------
1 file changed, 6 insertions(+), 20 deletions(-)
@@ -1524,15 +1521,12 @@ static void fec_enet_adjust_link(struct net_device *ndev)/* if any of the above changed restart the FEC */if(status_change){-netif_device_detach(ndev);napi_disable(&fep->napi);-netif_tx_disable(ndev);netif_tx_lock_bh(ndev);fec_restart(ndev,phy_dev->duplex);-netif_tx_unlock_bh(ndev);netif_wake_queue(ndev);+netif_tx_unlock_bh(ndev);napi_enable(&fep->napi);-netif_device_attach(ndev);}}else{if(fep->link){
@@ -1919,15 +1913,12 @@ static int fec_enet_set_pauseparam(struct net_device *ndev,phy_start_aneg(fep->phy_dev);}if(netif_running(ndev)){-netif_device_detach(ndev);napi_disable(&fep->napi);-netif_tx_disable(ndev);netif_tx_lock_bh(ndev);fec_restart(ndev,fep->full_duplex);-netif_tx_unlock_bh(ndev);netif_wake_queue(ndev);+netif_tx_unlock_bh(ndev);napi_enable(&fep->napi);-netif_device_attach(ndev);}return0;
@@ -2372,15 +2363,12 @@ static int fec_set_features(struct net_device *netdev,if(netif_running(netdev)){fec_stop(netdev);-netif_device_detach(netdev);napi_disable(&fep->napi);-netif_tx_disable(netdev);netif_tx_lock_bh(netdev);fec_restart(netdev,fep->phy_dev->duplex);-netif_tx_unlock_bh(netdev);netif_wake_queue(netdev);+netif_tx_unlock_bh(netdev);napi_enable(&fep->napi);-netif_device_attach(netdev);}}
From: Russell King <hidden> Date: 2014-06-20 12:13:20
fec_suspend() calls fec_stop() to stop the transmit ring while the
transmit packet processing is still active. This can lead to the
transmit queue being restarted by an intervening packet queued for
transmission, or by the tx quirk timer expiring.
Fix this by disabling NAPI first, which will ensure that the NAPI
handlers are not running. Then, take the transmit lock before
detaching the netif device. This ensures that there are no races
with the transmit path - and also ensures that the watchdog won't
fire.
We can then safely stop the ethernet device itself, knowing that the
rest of the driver is safely shut down.
On resume, we bring the device back up in reverse order - we restart
the device, reattach the device (under the tx lock), and then enable
the NAPI handlers.
We also need to adjust the close function to cope with this new
sequence, so that it's possible to cleanly close down the driver
after the hardware fails to resume (eg, due to the regulator_enable()
or pinctrl calls in the resume path returning an error.)
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec_main.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
From: Russell King <hidden> Date: 2014-06-20 12:13:25
fec_set_features() calls fec_stop() to stop the transmit ring while the
transmit queue is still active. This can lead to the transmit ring
being restarted by an intervening packet queued for transmission, or
by the tx quirk timer expiring.
Fix this by disabling NAPI (which ensures that the NAPI handlers are
not running), and then take the transmit lock while we stop and
restart the adapter (which prevents new packets being queued).
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Russell King <hidden> Date: 2014-06-20 12:13:30
Changing the features (receive checksumming) requires the hardware to be
reprogrammed, and also changes the checks in the receive packet
processing.
The current implementation has a race - fec_set_features() changes the
flags which alter the receive packet processing while the adapter is
active, and potentially receiving frames. Only after we've modified
the software flag do we shutdown and reconfigure the hardware.
This can lead to packets being received and marked with a valid checksum
(via CHECKSUM_UNNECESSARY) when the hardware checksum validation has not
yet been enabled.
We must quiesce the device, then change the software configuration for
this feature, and then resume the device if it was previously running.
The resulting code structure also allows us to add other configuration
features in this path without having to quiesce and resume the network
interface and device.
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec_main.c | 25 ++++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
@@ -2347,12 +2347,21 @@ static void fec_poll_controller(struct net_device *dev)}#endif+#define FEATURES_NEED_QUIESCE NETIF_F_RXCSUM+staticintfec_set_features(structnet_device*netdev,netdev_features_tfeatures){structfec_enet_private*fep=netdev_priv(netdev);netdev_features_tchanged=features^netdev->features;+/* Quiesce the device if necessary */+if(netif_running(netdev)&&changed&FEATURES_NEED_QUIESCE){+napi_disable(&fep->napi);+netif_tx_lock_bh(netdev);+fec_stop(netdev);+}+netdev->features=features;/* Receive checksum has been changed */
@@ -2361,16 +2370,14 @@ static int fec_set_features(struct net_device *netdev,fep->csum_flags|=FLAG_RX_CSUM_ENABLED;elsefep->csum_flags&=~FLAG_RX_CSUM_ENABLED;+}-if(netif_running(netdev)){-napi_disable(&fep->napi);-netif_tx_lock_bh(netdev);-fec_stop(netdev);-fec_restart(netdev,fep->phy_dev->duplex);-netif_wake_queue(netdev);-netif_tx_unlock_bh(netdev);-napi_enable(&fep->napi);-}+/* Resume the device after updates */+if(netif_running(netdev)&&changed&FEATURES_NEED_QUIESCE){+fec_restart(netdev,fep->phy_dev->duplex);+netif_wake_queue(netdev);+netif_tx_unlock_bh(netdev);+napi_enable(&fep->napi);}return0;
From: Russell King <hidden> Date: 2014-06-20 12:13:35
When the link goes down, the adjust_link method will be called, but
there is no synchronisation to ensure that we won't be processing some
last remaining packets via the NAPI handlers while performing a reset of
the device.
Add the necessary synchronisation to ensure that packet processing
is complete before we stop and reset the FEC.
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec_main.c | 4 ++++
1 file changed, 4 insertions(+)
From: Russell King <hidden> Date: 2014-06-20 12:13:40
Many places call fec_restart() with the second parameter being some kind
of previously saved duplex value, but only two places call it with some
other setting. This is at odds with how the other link settings are
handled, and used to be racy before the rtnl locks were added to
fec_restart()'s various call paths.
Clean this up so all link capabilities are handled in the same way -
saved into the fec_enet_private structure, and then fec_restart() acts
on those settings.
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec_main.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
@@ -875,7 +875,7 @@ fec_restart(struct net_device *ndev, int duplex)}/* Enable MII mode */-if(duplex){+if(fep->full_duplex==DUPLEX_FULL){/* FD enable */writel(0x04,fep->hwp+FEC_X_CNTRL);}else{
@@ -884,8 +884,6 @@ fec_restart(struct net_device *ndev, int duplex)writel(0x0,fep->hwp+FEC_X_CNTRL);}-fep->full_duplex=duplex;-/* Set MII speed */writel(fep->phy_speed,fep->hwp+FEC_MII_SPEED);
@@ -2378,7 +2378,7 @@ static int fec_set_features(struct net_device *netdev,/* Resume the device after updates */if(netif_running(netdev)&&changed&FEATURES_NEED_QUIESCE){-fec_restart(netdev,fep->phy_dev->duplex);+fec_restart(netdev);netif_wake_queue(netdev);netif_tx_unlock_bh(netdev);napi_enable(&fep->napi);
@@ -2482,7 +2482,7 @@ static int fec_enet_init(struct net_device *ndev)ndev->hw_features=ndev->features;-fec_restart(ndev,0);+fec_restart(ndev);return0;}
From: Russell King <hidden> Date: 2014-06-20 12:13:45
Using a (delayed) workqueue for ERR006358 is not correct - a work queue
is a single-trigger device. Once the work queue has been scheduled, it
can't be re-scheduled until it has been run. This can cause problems -
with an appropriate packet timing, we can end up with packets queued,
but not sent by the hardware, resulting in the transmit timeout firing.
Re-implement this as per the workaround detailed in the ERR006358
documentation - if there are packets waiting to be sent when we service
the transmit ring, and we see that the transmitter is not running,
kick the transmitter to run the pending entries in the ring.
Testing here with a 10Mbit half duplex link sees the resulting iperf
TCP bandwidth increase from between 1 to 2Mbps to between 8 to 9Mbps.
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec.h | 1 -
drivers/net/ethernet/freescale/fec_main.c | 31 +++++--------------------------
2 files changed, 5 insertions(+), 27 deletions(-)
@@ -259,7 +259,6 @@ struct bufdesc_ex {structfec_enet_delayed_work{structdelayed_workdelay_work;booltimeout;-booltrig_tx;};/* The FEC buffer descriptors track the ring buffers. The rx_bd_base and
@@ -545,8 +529,6 @@ static int fec_enet_txq_submit_skb(struct sk_buff *skb, struct net_device *ndev)status|=(BD_ENET_TX_READY|BD_ENET_TX_TC);bdp->cbd_sc=status;-fec_enet_submit_work(bdp,fep);-/* If this was the last BD in the ring, start at the beginning again. */bdp=fec_enet_get_nextdesc(last_bdp,fep);
@@ -735,8 +717,6 @@ static int fec_enet_txq_submit_tso(struct sk_buff *skb, struct net_device *ndev)/* Save skb pointer */fep->tx_skbuff[index]=skb;-fec_enet_submit_work(bdp,fep);-skb_tx_timestamp(skb);fep->cur_tx=bdp;
@@ -1166,7 +1141,11 @@ fec_enet_tx(struct net_device *ndev)netif_wake_queue(ndev);}}-return;++/* ERR006538: Keep the transmitter going */+if(fep->dirty_tx!=fep->cur_tx&&+readl(fep->hwp+FEC_X_DES_ACTIVE)==0)+writel(0,fep->hwp+FEC_X_DES_ACTIVE);}/* During a receive, the cur_rx points to the current incoming buffer.
From: Russell King <hidden> Date: 2014-06-20 12:13:50
As of "better implementation of iMX6 ERR006358 quirk", we no longer have
a requirement for a delayed work. Moreover, the work is now only used
for timeout purposes, so the timeout flag is also pointless - we set it
each time we queue the work, and the work clears it.
Replace the fec_enet_delayed_work struct with a standard work_struct,
resulting in simplified timeout handling code.
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec.h | 8 ++------
drivers/net/ethernet/freescale/fec_main.c | 34 +++++++++++++------------------
2 files changed, 16 insertions(+), 26 deletions(-)
From: Russell King <hidden> Date: 2014-06-20 12:13:56
Clear any pending receive interrupt before we process a pending packet.
This helps to avoid any spurious interrupts being raised after we have
fully cleaned the receive ring, while still allowing an interrupt to be
raised if we receive another packet.
The position of this is critical: we must do this prior to reading the
next packet status to avoid potentially dropping an interrupt when a
packet is still pending.
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec_main.c | 2 ++
1 file changed, 2 insertions(+)
@@ -1185,6 +1185,8 @@ fec_enet_rx(struct net_device *ndev, int budget)if((status&BD_ENET_RX_LAST)==0)netdev_err(ndev,"rcv is not +last\n");+writel(FEC_ENET_RXF,fep->hwp+FEC_IEVENT);+/* Check for errors. */if(status&(BD_ENET_RX_LG|BD_ENET_RX_SH|BD_ENET_RX_NO|BD_ENET_RX_CR|BD_ENET_RX_OV)){
From: Russell King <hidden> Date: 2014-06-20 12:14:01
This allows us to merge two separate preprocessor conditionals together.
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec_main.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
From: Russell King <hidden> Date: 2014-06-20 12:14:06
When we timeout on transmit, it would be useful to dump the transmit
ring, so we can see the ring state. This can be helpful to diagnose
the cause of transmit timeouts.
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec_main.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
From: Russell King <hidden> Date: 2014-06-20 12:14:11
Remove a useless status check in the transmit reap path - we have
already checked that the BD_ENET_TX_READY bit is clear, and as the
hardware only ever clears this bit, there is no way this test can ever
be true.
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec_main.c | 3 ---
1 file changed, 3 deletions(-)
@@ -1133,9 +1133,6 @@ fec_enet_tx(struct net_device *ndev)skb_tstamp_tx(skb,&shhwtstamps);}-if(status&BD_ENET_TX_READY)-netdev_err(ndev,"HEY! Enet xmit interrupt and TX_READY\n");-/* Deferred means some collisions occurred during transmit,*butweeventuallysentthepacketOK.*/
From: Russell King <hidden> Date: 2014-06-20 12:14:16
Both transmit and receive use the same infrastructure for calculating
the packet timestamp. Rather than duplicating the code, provide a
function to do this common work. Model this function in the Intel
e1000e version which avoids calling ns_to_ktime() within the spinlock;
the spinlock is critical for timecounter_cyc2time() but not
ns_to_ktime().
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec_main.c | 37 ++++++++++++++++---------------
1 file changed, 19 insertions(+), 18 deletions(-)
@@ -1289,18 +1299,9 @@ fec_enet_rx(struct net_device *ndev, int budget)skb->protocol=eth_type_trans(skb,ndev);/* Get receive timestamp from the skb */-if(fep->hwts_rx_en&&fep->bufdesc_ex){-structskb_shared_hwtstamps*shhwtstamps=-skb_hwtstamps(skb);-unsignedlongflags;--memset(shhwtstamps,0,sizeof(*shhwtstamps));--spin_lock_irqsave(&fep->tmreg_lock,flags);-shhwtstamps->hwtstamp=ns_to_ktime(-timecounter_cyc2time(&fep->tc,ebdp->ts));-spin_unlock_irqrestore(&fep->tmreg_lock,flags);-}+if(fep->hwts_rx_en&&fep->bufdesc_ex)+fec_enet_hwtstamp(fep,ebdp->ts,+skb_hwtstamps(skb));if(fep->bufdesc_ex&&(fep->csum_flags&FLAG_RX_CSUM_ENABLED)){
From: David Laight <hidden> Date: 2014-06-20 12:39:57
From: Russell King
While running: while :; do iperf -c <HOST> -P 4; done, transmit timeouts
are regularly reported. With the tx ring dumping in place, we can see
that all entries are in use, and the hardware has finished transmitting
these packets. However, the driver has not reclaimed these ring
entries.
This can occur if the interrupt handler is invoked at the wrong moment -
eg:
CPU0 CPU1
fec_enet_tx()
interrupt, IEVENT = FEC_ENET_TXF
FEC_ENET_TXF cleared
napi_schedule_prep()
napi_complete()
The result is that we clear the transmit interrupt, but we don't trigger
any cleaning of the transmit ring. Instead, use a different strategy:
- When receiving a transmit or receive interrupt, disable both tx and rx
interrupts, but do not acknowledge them. Schedule a napi poll. Don't
loop.
- When we are polled, read IEVENT, acknowledging the pending transmit
and receive interrupts, before then going on to process the
appropriate rings.
This allows us to avoid the race, and has a number of other advantages:
- we cut down on the number of transmit interrupts we have to process.
- we only look at the rings which have pending events.
- we gain additional throughput: the iperf total bandwidth increases
from about 180Mbps to 240Mbps:
...
It is usually possible to reduce the interrupt count even further by:
1) processing the rings.
2) clear the IRQ.
3) check the rings for new entries, if any start again.
This means that you don't take the interrupt for anything that completes
while processing the earlier ring entries.
The slight downside is that it is easy to exit the ISR before the IRQ
line actually drops - resulting in a spurious interrupt.
(patch 24 seems to be similar).
David
From: Russell King - ARM Linux <hidden> Date: 2014-06-20 15:36:14
On Fri, Jun 20, 2014 at 12:39:57PM +0000, David Laight wrote:
It is usually possible to reduce the interrupt count even further by:
1) processing the rings.
2) clear the IRQ.
3) check the rings for new entries, if any start again.
This means that you don't take the interrupt for anything that completes
while processing the earlier ring entries.
The slight downside is that it is easy to exit the ISR before the IRQ
line actually drops - resulting in a spurious interrupt.
(patch 24 seems to be similar).
ITYM patch 26, as patch 24 is an update to the errata workaround.
Patch 4 is what I regard to be the bare minimum fix, patch 26 is an
improvement over it.
Remember though that the real interrupt processing happens in the NAPI
handler, not the main interrupt handler - upon seeing a transmit and/or
receive complete interrupt, the main interrupt handler masks both of
these interrupts at the chip and triggers the NAPI handler to run.
Nevertheless, your suggestion is equally valid for a NAPI handler:
- process the rings
- clear the interrupt(s)
- if more work is allowed (according to the NAPI weight), repeat
This is simple enough for the RX ring, where each packet received is
equal one one NAPI unit of work. The TX side is less obvious as
the NAPI documentation suggests that cleaning the ring partially
equates to no NAPI units of work but a clean of the entire ring
equates to the full allowance. Maybe I've mis-understood something
there (which would mean there's another bug which needs fixing in the
tx reaping code...)
--
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
From: David Laight <hidden> Date: 2014-06-20 16:00:45
From: Russell King - ARM Linux
On Fri, Jun 20, 2014 at 12:39:57PM +0000, David Laight wrote:
quoted
It is usually possible to reduce the interrupt count even further by:
1) processing the rings.
2) clear the IRQ.
3) check the rings for new entries, if any start again.
This means that you don't take the interrupt for anything that completes
while processing the earlier ring entries.
The slight downside is that it is easy to exit the ISR before the IRQ
line actually drops - resulting in a spurious interrupt.
(patch 24 seems to be similar).
ITYM patch 26, as patch 24 is an update to the errata workaround.
Misread my jottings.
Patch 4 is what I regard to be the bare minimum fix, patch 26 is an
improvement over it.
Remember though that the real interrupt processing happens in the NAPI
handler, not the main interrupt handler - upon seeing a transmit and/or
receive complete interrupt, the main interrupt handler masks both of
these interrupts at the chip and triggers the NAPI handler to run.
Nevertheless, your suggestion is equally valid for a NAPI handler:
- process the rings
- clear the interrupt(s)
- if more work is allowed (according to the NAPI weight), repeat
Not quite, you only want to clear the hardware IRQ and unmask the ISR
when there are no ring entries to process.
(There is no point doing the slow bus cycles until then.)
If you clear the hardware IRQ at the start of processing the ring you'll
get another interrupt when you finally enable the interrupt for a rx
packet that arrives while processing the earlier packets.
However you must check the ring AFTER clearing the IRQ - otherwise
you miss the interrupt for a packet the arrives at that time.
So the sequence is:
1) Find that there are no ring entries to process.
(a packet can arrive here)
2) Clear the IRQ request.
3) Check there are still no ring entries to process.
4) Unmask the interrupt.
This is simple enough for the RX ring, where each packet received is
equal one one NAPI unit of work. The TX side is less obvious as
the NAPI documentation suggests that cleaning the ring partially
equates to no NAPI units of work but a clean of the entire ring
equates to the full allowance. Maybe I've mis-understood something
there (which would mean there's another bug which needs fixing in the
tx reaping code...)
I've never tried to understand NAPI work limits.
I suspect that once the NAPI callbacks for other devices (etc) have
had a chance to run, you get called again.
So you'd leave the hardware interrupt masked until then.
David
On Fri, Jun 20, 2014 at 01:14:16PM +0100, Russell King wrote:
Both transmit and receive use the same infrastructure for calculating
the packet timestamp. Rather than duplicating the code, provide a
function to do this common work. Model this function in the Intel
e1000e version which avoids calling ns_to_ktime() within the spinlock;
the spinlock is critical for timecounter_cyc2time() but not
ns_to_ktime().
Thanks for cleaning this up.
Acked-by: Richard Cochran <richardcochran@gmail.com>
From: fugang.duan at freescale.com <hidden> Date: 2014-06-22 06:38:59
From: Russell King <redacted> Data: Friday, June 20, 2014 8:12 PM
quoted hunk
To: linux-arm-kernel@lists.infradead.org
Cc: Duan Fugang-B38611; netdev@vger.kernel.org
Subject: [PATCH RFC 04/30] net: fec: fix interrupt handling races
While running: while :; do iperf -c <HOST> -P 4; done, transmit timeouts
are regularly reported. With the tx ring dumping in place, we can see
that all entries are in use, and the hardware has finished transmitting
these packets. However, the driver has not reclaimed these ring entries.
This can occur if the interrupt handler is invoked at the wrong moment -
eg:
CPU0 CPU1
fec_enet_tx()
interrupt, IEVENT = FEC_ENET_TXF
FEC_ENET_TXF cleared
napi_schedule_prep()
napi_complete()
The result is that we clear the transmit interrupt, but we don't trigger
any cleaning of the transmit ring. Instead, use a different strategy:
- When receiving a transmit or receive interrupt, disable both tx and rx
interrupts, but do not acknowledge them. Schedule a napi poll. Don't
loop.
- When we are polled, read IEVENT, acknowledging the pending transmit
and receive interrupts, before then going on to process the
appropriate rings.
This allows us to avoid the race, and has a number of other advantages:
- we cut down on the number of transmit interrupts we have to process.
- we only look at the rings which have pending events.
- we gain additional throughput: the iperf total bandwidth increases
from about 180Mbps to 240Mbps:
[ 3] 0.0-10.0 sec 68.1 MBytes 57.0 Mbits/sec [ 5] 0.0-10.0 sec 72.4
MBytes 60.5 Mbits/sec [ 4] 0.0-10.1 sec 76.1 MBytes 63.5 Mbits/sec
[ 6] 0.0-10.1 sec 71.9 MBytes 59.9 Mbits/sec
[SUM] 0.0-10.1 sec 288 MBytes 241 Mbits/sec
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec_main.c | 40 +++++++++++++++++---------
-----
1 file changed, 22 insertions(+), 18 deletions(-)
int fec_enet_rx_napi(struct napi_struct *napi, int budget) {
struct net_device *ndev = napi->dev;
- int pkts = fec_enet_rx(ndev, budget);
struct fec_enet_private *fep = netdev_priv(ndev);
+ int pkts;
+
+ /*
+ * Clear any pending transmit or receive interrupts before
+ * processing the rings to avoid racing with the hardware.
+ */
+ writel(FEC_ENET_RXF | FEC_ENET_TXF, fep->hwp + FEC_IEVENT);
+
+ pkts = fec_enet_rx(ndev, budget);
fec_enet_tx(ndev);
Can you add the interrupt check like below. If there have no tx interrupt, it is most likely no tx rings reclaim.
Events = readl(fep->hwp + FEC_IEVENT);
If (events & FEC_ENET_RXF)
pkts = fec_enet_rx(ndev, budget);
if (events & FEC_ENET_TXF)
fec_enet_tx(ndev);
Thanks,
Andy
From: fugang.duan at freescale.com <hidden> Date: 2014-06-22 06:54:28
From: Russell King <redacted> Data: Friday, June 20, 2014 8:13 PM
quoted hunk
To: linux-arm-kernel@lists.infradead.org
Cc: Duan Fugang-B38611; netdev@vger.kernel.org
Subject: [PATCH RFC 18/30] net: fec: remove inappropriate calls around
fec_restart()
This is the second stage to "move calls to quiesce/resume packet
processing out of fec_restart()", where we remove calls which are not
appropriate to the call site.
In the majority of cases, there is no need to detach and reattach the
interface as we are holding the queue xmit lock across the reset. The
exception to that is in fec_resume(), where we are already detached by the
suspend function. Here, we can remove the call to detach the interface.
We also do not need to stop the transmit queue. Holding the xmit lock is
enough to ensure that the transmit packet processing is not running while
we perform our task. However, since fec_restart() always cleans the rings,
we call netif_wake_queue() (or netif_device_attach() in the case of resume)
just before dropping the xmit lock. This prevents the watchdog firing.
Lastly, always call napi_enable() after the device has been reattached in
the resume path so that we know that the transmit packet processing is
already in an enabled state, so we don't call netif_wake_queue() while
detached.
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec_main.c | 26 ++++++--------------------
1 file changed, 6 insertions(+), 20 deletions(-)
From: fugang.duan at freescale.com <hidden> Date: 2014-06-22 07:49:11
From: Russell King <redacted> Data: Friday, June 20, 2014 8:14 PM
quoted hunk
To: linux-arm-kernel@lists.infradead.org
Cc: Duan Fugang-B38611; netdev@vger.kernel.org
Subject: [PATCH RFC 24/30] net: fec: better implementation of iMX6
ERR006358 quirk
Using a (delayed) workqueue for ERR006358 is not correct - a work queue is
a single-trigger device. Once the work queue has been scheduled, it can't
be re-scheduled until it has been run. This can cause problems - with an
appropriate packet timing, we can end up with packets queued, but not sent
by the hardware, resulting in the transmit timeout firing.
Re-implement this as per the workaround detailed in the ERR006358
documentation - if there are packets waiting to be sent when we service
the transmit ring, and we see that the transmitter is not running, kick
the transmitter to run the pending entries in the ring.
Testing here with a 10Mbit half duplex link sees the resulting iperf TCP
bandwidth increase from between 1 to 2Mbps to between 8 to 9Mbps.
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec.h | 1 -
drivers/net/ethernet/freescale/fec_main.c | 31 +++++---------------------
-----
2 files changed, 5 insertions(+), 27 deletions(-)
diff --git a/drivers/net/ethernet/freescale/fec.h
b/drivers/net/ethernet/freescale/fec.h
index 96d2a18f1b99..17e294970207 100644
struct fec_enet_delayed_work {
struct delayed_work delay_work;
bool timeout;
- bool trig_tx;
};
/* The FEC buffer descriptors track the ring buffers. The rx_bd_base and
If fep->dirty_tx == fep->cur_tx, which means tx bd entry is empty, it doesn't need to trigger TDAR.
The issue is the last tx frame cannot be trasmited by uDMA since uDMA had cleared the TDAR after user write TDAR.
User uDMA
Process the previous frame, detect the next BD ready bit,
The next BD ready bit is not set
Set the next BD ready bit
Set TDAR
Since the next BD ready bit detected as not set, start to clear TDAR.
=> So, the next frame cannot be tramsmit until the next next frame is filled by user.
The issue has happened customer product, and was fixed by previous workaround.
The issue only happened at little packets transmission. Special for command communicate protocol to the other end.
If you move the extra trigger to tx clean function, you must check:
bdp_pre = fec_enet_get_prevdesc(fep->cur_tx, fep);
bdp_next = fec_enet_get_nextdesc(fep->dirty_tx, fep);
if (bdp_next == bdp_pre &&
bdp_pre->cbd_sc & BD_ENET_TX_READY &&
readl(fep->hwp + FEC_X_DES_ACTIVE) == 0)
writel(0, fep->hwp + FEC_X_DES_ACTIVE);
Thanks,
Andy
From: Russell King - ARM Linux <hidden> Date: 2014-06-22 07:55:14
On Sun, Jun 22, 2014 at 06:54:28AM +0000, fugang.duan@freescale.com wrote:
From: Russell King <redacted> Data: Friday, June 20, 2014 8:13 PM
quoted
To: linux-arm-kernel@lists.infradead.org
Cc: Duan Fugang-B38611; netdev@vger.kernel.org
Subject: [PATCH RFC 18/30] net: fec: remove inappropriate calls around
fec_restart()
This is the second stage to "move calls to quiesce/resume packet
processing out of fec_restart()", where we remove calls which are not
appropriate to the call site.
In the majority of cases, there is no need to detach and reattach the
interface as we are holding the queue xmit lock across the reset. The
exception to that is in fec_resume(), where we are already detached by the
suspend function. Here, we can remove the call to detach the interface.
We also do not need to stop the transmit queue. Holding the xmit lock is
enough to ensure that the transmit packet processing is not running while
we perform our task. However, since fec_restart() always cleans the rings,
we call netif_wake_queue() (or netif_device_attach() in the case of resume)
just before dropping the xmit lock. This prevents the watchdog firing.
Lastly, always call napi_enable() after the device has been reattached in
the resume path so that we know that the transmit packet processing is
already in an enabled state, so we don't call netif_wake_queue() while
detached.
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec_main.c | 26 ++++++--------------------
1 file changed, 6 insertions(+), 20 deletions(-)
While you can merge various patches together into a single patch, the
question is whether it's the right thing to do.
I kept 17 and 18 separate as 17 is merely moving the calls out without
modification, whereas 18 is changing the functionality.
That makes 17 easy to review - from the reviewer perspective, it's a case
of ensuring that the calls are all placed at the fec_restart() callsite.
Once that's done, then each callsite can be considered on its own merit
for the changes in patch 18.
--
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
From: fugang.duan at freescale.com <hidden> Date: 2014-06-22 08:07:33
From: Russell King - ARM Linux <redacted> Data: Sunday, June 22, 2014 3:55 PM
To: Duan Fugang-B38611
Cc: linux-arm-kernel@lists.infradead.org; netdev@vger.kernel.org
Subject: Re: [PATCH RFC 18/30] net: fec: remove inappropriate calls around
fec_restart()
On Sun, Jun 22, 2014 at 06:54:28AM +0000, fugang.duan@freescale.com wrote:
quoted
From: Russell King <redacted> Data: Friday, June 20, 2014
8:13 PM
quoted
To: linux-arm-kernel@lists.infradead.org
Cc: Duan Fugang-B38611; netdev@vger.kernel.org
Subject: [PATCH RFC 18/30] net: fec: remove inappropriate calls
around
fec_restart()
This is the second stage to "move calls to quiesce/resume packet
processing out of fec_restart()", where we remove calls which are not
appropriate to the call site.
In the majority of cases, there is no need to detach and reattach the
interface as we are holding the queue xmit lock across the reset.
The exception to that is in fec_resume(), where we are already
detached by the suspend function. Here, we can remove the call to
detach the interface.
quoted
quoted
We also do not need to stop the transmit queue. Holding the xmit
lock is enough to ensure that the transmit packet processing is not
running while we perform our task. However, since fec_restart()
always cleans the rings, we call netif_wake_queue() (or
netif_device_attach() in the case of resume) just before dropping the
xmit lock. This prevents the watchdog firing.
quoted
quoted
Lastly, always call napi_enable() after the device has been
reattached in the resume path so that we know that the transmit
packet processing is already in an enabled state, so we don't call
netif_wake_queue() while detached.
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec_main.c | 26
++++++--------------------
1 file changed, 6 insertions(+), 20 deletions(-)
While you can merge various patches together into a single patch, the
question is whether it's the right thing to do.
I kept 17 and 18 separate as 17 is merely moving the calls out without
modification, whereas 18 is changing the functionality.
That makes 17 easy to review - from the reviewer perspective, it's a case
of ensuring that the calls are all placed at the fec_restart() callsite.
Once that's done, then each callsite can be considered on its own merit
for the changes in patch 18.
From: Russell King - ARM Linux <hidden> Date: 2014-06-22 08:12:35
On Sun, Jun 22, 2014 at 07:49:11AM +0000, fugang.duan@freescale.com wrote:
From: Russell King <redacted> Data: Friday, June 20, 2014 8:14 PM
quoted
To: linux-arm-kernel@lists.infradead.org
Cc: Duan Fugang-B38611; netdev@vger.kernel.org
Subject: [PATCH RFC 24/30] net: fec: better implementation of iMX6
ERR006358 quirk
Using a (delayed) workqueue for ERR006358 is not correct - a work queue is
a single-trigger device. Once the work queue has been scheduled, it can't
be re-scheduled until it has been run. This can cause problems - with an
appropriate packet timing, we can end up with packets queued, but not sent
by the hardware, resulting in the transmit timeout firing.
Re-implement this as per the workaround detailed in the ERR006358
documentation - if there are packets waiting to be sent when we service
the transmit ring, and we see that the transmitter is not running, kick
the transmitter to run the pending entries in the ring.
Testing here with a 10Mbit half duplex link sees the resulting iperf TCP
bandwidth increase from between 1 to 2Mbps to between 8 to 9Mbps.
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec.h | 1 -
drivers/net/ethernet/freescale/fec_main.c | 31 +++++---------------------
-----
2 files changed, 5 insertions(+), 27 deletions(-)
diff --git a/drivers/net/ethernet/freescale/fec.h
b/drivers/net/ethernet/freescale/fec.h
index 96d2a18f1b99..17e294970207 100644
struct fec_enet_delayed_work {
struct delayed_work delay_work;
bool timeout;
- bool trig_tx;
};
/* The FEC buffer descriptors track the ring buffers. The rx_bd_base and
If fep->dirty_tx == fep->cur_tx, which means tx bd entry is empty, it
doesn't need to trigger TDAR.
I think you're wrong. Upon initialisation (which is the empty case
because at initialisation time, the tx ring will be empty), fep->cur_tx
is set to the first tx bd entry, and fep->dirty_tx is set to the
/preceding/ entry.
When a packet is loaded to be transmitted, it is placed in the fep->cur_tx
slot. In other words, fep->cur_tx points at the tx bd entry which takes
the _next_ packet to be loaded into the ring.
When packets are reaped, the first thing the reaping function does after
reading fep->dirty_tx is increment to the next ring entry.
So, the "ring full" case is fep->dirty_tx == fep->cur_tx, and the "ring
empty" case is next(fep->dirty_tx) == fep->cur_tx.
You've made this mistake in the SG patch, where you miscalculate the ring
free size when fep->dirty_tx == fep->cur_tx, indicating that the ring is
entirely free, when in fact it has no free entries.
The issue is the last tx frame cannot be trasmited by uDMA since uDMA
had cleared the TDAR after user write TDAR.
User uDMA
Process the previous frame, detect the next BD ready bit,
The next BD ready bit is not set
Set the next BD ready bit
Set TDAR
Since the next BD ready bit detected as not set, start to clear TDAR.
Yes, I'm well aware of the mechanism that causes this problem.
The issue has happened customer product, and was fixed by previous
workaround. The issue only happened at little packets transmission.
Special for command communicate protocol to the other end.
So, please explain to me why, with the current solution, I get:
1. half duplex performance is abismal, to the point that the ethernet
only achieves about 12-25% of the bandwidth, and jumps massively
using the solution in this patch (which is as per the workaround
documented in the errata document.)
2. transmit timeouts when operating in half-duplex mode.
If you move the extra trigger to tx clean function, you must check:
bdp_pre = fec_enet_get_prevdesc(fep->cur_tx, fep);
bdp_next = fec_enet_get_nextdesc(fep->dirty_tx, fep);
if (bdp_next == bdp_pre &&
bdp_pre->cbd_sc & BD_ENET_TX_READY &&
readl(fep->hwp + FEC_X_DES_ACTIVE) == 0)
writel(0, fep->hwp + FEC_X_DES_ACTIVE);
While I agree that we can read back to check whether the device indicates
that transmit is active, there's no point to the other tests. If there
are entries in the transmit ring but the transmitter indicates that it is
not active, then it is obvious that the bug has been hit. This is exactly
what my implementation above does.
--
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
From: Russell King - ARM Linux <hidden> Date: 2014-06-22 08:24:00
On Sun, Jun 22, 2014 at 09:12:35AM +0100, Russell King - ARM Linux wrote:
On Sun, Jun 22, 2014 at 07:49:11AM +0000, fugang.duan@freescale.com wrote:
quoted
The issue has happened customer product, and was fixed by previous
workaround. The issue only happened at little packets transmission.
Special for command communicate protocol to the other end.
So, please explain to me why, with the current solution, I get:
1. half duplex performance is abismal, to the point that the ethernet
only achieves about 12-25% of the bandwidth, and jumps massively
using the solution in this patch (which is as per the workaround
documented in the errata document.)
2. transmit timeouts when operating in half-duplex mode.
I also forgot to point out that in half duplex mode with the current
solution, I get /lots/ of collisions (which is the case of (1)),
including "late" collisions which are never supposed to happen.
Late collisions occur because another ethernet station is found to be
transmitting onto the shared media after a certain point in the packet.
This point is set in the standards according to the maximum cable length
and the packet propagation delays, and the occurance of a late collision
indicates that either the cables are too long, or the hardware is not
detecting the presence of a receive carrier before it starts blurting
out on the shared media - over an existing transmission.
It seems that when regularly writing to FEC_X_DES_ACTIVE, this breaks
the half-duplex hold-off mechanism, and the FEC just blurts out the next
packet without respecting the media access rules.
In my case, I can assure you that it is not a faulty hub, or an
excessively long cable (which was less than 1m) since the only hardware
which exhibits this behaviour is iMX6, and this behaviour occurs with
the two hubs I have here (one a 10bT hub, the other a 10/100bT hub).
I did a lot of experiments with various setups of switches and hubs
which conclusively prove that the iMX6 is the broken party.
--
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
From: fugang.duan at freescale.com <hidden> Date: 2014-06-22 08:38:49
From: Russell King - ARM Linux <redacted> Data: Sunday, June 22, 2014 4:13 PM
To: Duan Fugang-B38611
Cc: linux-arm-kernel@lists.infradead.org; netdev@vger.kernel.org
Subject: Re: [PATCH RFC 24/30] net: fec: better implementation of iMX6
ERR006358 quirk
On Sun, Jun 22, 2014 at 07:49:11AM +0000, fugang.duan@freescale.com wrote:
quoted
From: Russell King <redacted> Data: Friday, June 20, 2014
8:14 PM
quoted
To: linux-arm-kernel@lists.infradead.org
Cc: Duan Fugang-B38611; netdev@vger.kernel.org
Subject: [PATCH RFC 24/30] net: fec: better implementation of iMX6
ERR006358 quirk
Using a (delayed) workqueue for ERR006358 is not correct - a work
queue is a single-trigger device. Once the work queue has been
scheduled, it can't be re-scheduled until it has been run. This can
cause problems - with an appropriate packet timing, we can end up
with packets queued, but not sent by the hardware, resulting in the
transmit timeout firing.
quoted
quoted
Re-implement this as per the workaround detailed in the ERR006358
documentation - if there are packets waiting to be sent when we
service the transmit ring, and we see that the transmitter is not
running, kick the transmitter to run the pending entries in the ring.
Testing here with a 10Mbit half duplex link sees the resulting iperf
TCP bandwidth increase from between 1 to 2Mbps to between 8 to 9Mbps.
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/freescale/fec.h | 1 -
drivers/net/ethernet/freescale/fec_main.c | 31
+++++---------------------
-----
2 files changed, 5 insertions(+), 27 deletions(-)
diff --git a/drivers/net/ethernet/freescale/fec.h
b/drivers/net/ethernet/freescale/fec.h
index 96d2a18f1b99..17e294970207 100644
{
struct delayed_work delay_work;
bool timeout;
- bool trig_tx;
};
/* The FEC buffer descriptors track the ring buffers. The
rx_bd_base and diff --git a/drivers/net/ethernet/freescale/fec_main.c
b/drivers/net/ethernet/freescale/fec_main.c
index 62120e47ef5a..957bb98add5a 100644
If fep->dirty_tx == fep->cur_tx, which means tx bd entry is empty, it
doesn't need to trigger TDAR.
I think you're wrong. Upon initialisation (which is the empty case
because at initialisation time, the tx ring will be empty), fep->cur_tx is
set to the first tx bd entry, and fep->dirty_tx is set to the /preceding/
entry.
When a packet is loaded to be transmitted, it is placed in the fep->cur_tx
slot. In other words, fep->cur_tx points at the tx bd entry which takes
the _next_ packet to be loaded into the ring.
When packets are reaped, the first thing the reaping function does after
reading fep->dirty_tx is increment to the next ring entry.
So, the "ring full" case is fep->dirty_tx == fep->cur_tx, and the "ring
empty" case is next(fep->dirty_tx) == fep->cur_tx.
You've made this mistake in the SG patch, where you miscalculate the ring
free size when fep->dirty_tx == fep->cur_tx, indicating that the ring is
entirely free, when in fact it has no free entries.
Agree. Sorry, my mistake.
quoted
The issue is the last tx frame cannot be trasmited by uDMA since uDMA
had cleared the TDAR after user write TDAR.
User uDMA
Process the previous frame, detect the next
BD ready bit,
quoted
The next BD ready bit is not set
Set the next BD ready bit
Set TDAR
Since the next BD ready bit detected as not
set, start to clear TDAR.
Yes, I'm well aware of the mechanism that causes this problem.
quoted
The issue has happened customer product, and was fixed by previous
workaround. The issue only happened at little packets transmission.
Special for command communicate protocol to the other end.
So, please explain to me why, with the current solution, I get:
1. half duplex performance is abismal, to the point that the ethernet
only achieves about 12-25% of the bandwidth, and jumps massively
using the solution in this patch (which is as per the workaround
documented in the errata document.)
My understand is that you run tcp test bandwidth, so you may catch below cases:
Transmit Packet 1, 2, 3,.... n, wait tcp ack from the other end, and then transmit packet n+1, n+2,.....
uDMA bandwidth is enough fast for 10Mbps, maybe the number "n" packet is not transmited since the errata ERR006358.
So there introduce much latency for transmit flow.
So I guess that if you use UDP bandwidth, maybe there have no diff with your patch.
2. transmit timeouts when operating in half-duplex mode.
quoted
If you move the extra trigger to tx clean function, you must check:
bdp_pre = fec_enet_get_prevdesc(fep->cur_tx, fep); bdp_next =
fec_enet_get_nextdesc(fep->dirty_tx, fep); if (bdp_next == bdp_pre &&
bdp_pre->cbd_sc & BD_ENET_TX_READY &&
readl(fep->hwp + FEC_X_DES_ACTIVE) == 0)
writel(0, fep->hwp + FEC_X_DES_ACTIVE);
While I agree that we can read back to check whether the device indicates
that transmit is active, there's no point to the other tests. If there
are entries in the transmit ring but the transmitter indicates that it is
not active, then it is obvious that the bug has been hit. This is exactly
what my implementation above does.
The condition "fep->dirty_tx != fep->cur_tx" is not only limited for the errata.
I mean only add extra trigger TDAR for the issue.
--
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
While I agree that we can read back to check whether the device indicates
that transmit is active, there's no point to the other tests. If there
are entries in the transmit ring but the transmitter indicates that it is
not active, then it is obvious that the bug has been hit. This is exactly
what my implementation above does.
The condition "fep->dirty_tx != fep->cur_tx" is not only limited for the errata.
I mean only add extra trigger TDAR for the issue.
Yes, I agree that test is wrong (that's what comes from shuffling the
patches... subsequent patches modify the indexing mechanism). It should
be:
if (bdp != fep->cur_tx &&
readl(fep->hwp + FEC_X_DES_ACTIVE) == 0)
writel(0, fep->hwp + FEC_X_DES_ACTIVE);
Since "bdp" is the post-incremented dirty_tx pointer, which, when it is
equal to fep->cur_tx means that the ring is empty.
--
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
While I agree that we can read back to check whether the device indicates
that transmit is active, there's no point to the other tests. If there
are entries in the transmit ring but the transmitter indicates that it is
not active, then it is obvious that the bug has been hit. This is exactly
what my implementation above does.
The condition "fep->dirty_tx != fep->cur_tx" is not only limited for the errata.
I mean only add extra trigger TDAR for the issue.
Yes, I agree that test is wrong (that's what comes from shuffling the
patches... subsequent patches modify the indexing mechanism). It should
be:
if (bdp != fep->cur_tx &&
readl(fep->hwp + FEC_X_DES_ACTIVE) == 0)
writel(0, fep->hwp + FEC_X_DES_ACTIVE);
Since "bdp" is the post-incremented dirty_tx pointer, which, when it is
equal to fep->cur_tx means that the ring is empty.
Any further comments, or do I take the silence to mean that you agree
with the above statement? I would like to get this settled to I can
spin v2 of this set.
Thanks.
--
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
From: fugang.duan at freescale.com <hidden> Date: 2014-06-25 01:41:21
From: Russell King - ARM Linux <redacted> Data: Tuesday, June 24, 2014 6:29 PM
To: Duan Fugang-B38611
Cc: netdev@vger.kernel.org; linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH RFC 24/30] net: fec: better implementation of iMX6
ERR006358 quirk
On Sun, Jun 22, 2014 at 09:49:11AM +0100, Russell King - ARM Linux wrote:
quoted
On Sun, Jun 22, 2014 at 08:38:49AM +0000, fugang.duan@freescale.com
wrote:
quoted
quoted
From: Russell King - ARM Linux <redacted> Data:
Sunday, June 22, 2014 4:13 PM
quoted
To: Duan Fugang-B38611
Cc: linux-arm-kernel@lists.infradead.org; netdev@vger.kernel.org
Subject: Re: [PATCH RFC 24/30] net: fec: better implementation of
iMX6
ERR006358 quirk
On Sun, Jun 22, 2014 at 07:49:11AM +0000, fugang.duan@freescale.com
wrote:
quoted
quoted
quoted
quoted
From: Russell King <redacted> Data: Friday, June 20,
2014
8:14 PM
While I agree that we can read back to check whether the device
indicates that transmit is active, there's no point to the other
tests. If there are entries in the transmit ring but the
transmitter indicates that it is not active, then it is obvious
that the bug has been hit. This is exactly what my implementation
above does.
quoted
quoted
quoted
The condition "fep->dirty_tx != fep->cur_tx" is not only limited for
the errata.
quoted
quoted
I mean only add extra trigger TDAR for the issue.
Yes, I agree that test is wrong (that's what comes from shuffling the
patches... subsequent patches modify the indexing mechanism). It
should
be:
if (bdp != fep->cur_tx &&
readl(fep->hwp + FEC_X_DES_ACTIVE) == 0)
writel(0, fep->hwp + FEC_X_DES_ACTIVE);
Since "bdp" is the post-incremented dirty_tx pointer, which, when it
is equal to fep->cur_tx means that the ring is empty.
Any further comments, or do I take the silence to mean that you agree with
the above statement? I would like to get this settled to I can spin v2 of
this set.
Thanks.