[PATCH net-next 1/7] net:fec: remove unnecessary ip stack includes

[PATCH net-next 1/7] net:fec: remove unnecessary ip stack includes

[Frank Li]

From: Russell King <rmk+kernel@arm.linux.org.uk>

Commit 4c09eed9dc42 (net: fec: Enable imx6 enet checksum acceleration.)
added support for hardware checksum support, adding six new includes
for things like tcp, udp, ip, icmp, but never made use of anything
provided by those header files.  Let's remove this bloat.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Frank Li <Frank.Li@freescale.com>
---
 drivers/net/ethernet/freescale/fec_main.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 8d69e43..b151eaf 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -33,12 +33,6 @@
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
 #include <linux/skbuff.h>
-#include <linux/in.h>
-#include <linux/ip.h>
-#include <net/ip.h>
-#include <linux/tcp.h>
-#include <linux/udp.h>
-#include <linux/icmp.h>
 #include <linux/spinlock.h>
 #include <linux/workqueue.h>
 #include <linux/bitops.h>
-- 
1.7.8

[PATCH net-next 2/7] net:fec: reorder ethtool ops to match order in struct declaration

[Frank Li]

From: Russell King <rmk+kernel@arm.linux.org.uk>

This allows us to merge two separate preprocessor conditionals together.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Frank Li <Frank.Li@freescale.com>
---
 drivers/net/ethernet/freescale/fec_main.c |   12 +++++-------
 1 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index b151eaf..875c0ca 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1639,21 +1639,19 @@ static int fec_enet_nway_reset(struct net_device *dev)
 }
 
 static const struct ethtool_ops fec_enet_ethtool_ops = {
-#if !defined(CONFIG_M5272)
-	.get_pauseparam		= fec_enet_get_pauseparam,
-	.set_pauseparam		= fec_enet_set_pauseparam,
-#endif
 	.get_settings		= fec_enet_get_settings,
 	.set_settings		= fec_enet_set_settings,
 	.get_drvinfo		= fec_enet_get_drvinfo,
-	.get_link		= ethtool_op_get_link,
-	.get_ts_info		= fec_enet_get_ts_info,
 	.nway_reset		= fec_enet_nway_reset,
+	.get_link		= ethtool_op_get_link,
 #ifndef CONFIG_M5272
-	.get_ethtool_stats	= fec_enet_get_ethtool_stats,
+	.get_pauseparam		= fec_enet_get_pauseparam,
+	.set_pauseparam		= fec_enet_set_pauseparam,
 	.get_strings		= fec_enet_get_strings,
+	.get_ethtool_stats	= fec_enet_get_ethtool_stats,
 	.get_sset_count		= fec_enet_get_sset_count,
 #endif
+	.get_ts_info		= fec_enet_get_ts_info,
 };
 
 static int fec_enet_ioctl(struct net_device *ndev, struct ifreq *rq, int cmd)
-- 
1.7.8

[PATCH net-next 3/7] net:fec: remove checking for NULL phy_dev in fec_enet_close()

[Frank Li]

From: Russell King <rmk+kernel@arm.linux.org.uk>

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 <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Frank Li <Frank.Li@freescale.com>
---
 drivers/net/ethernet/freescale/fec_main.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 875c0ca..b7a5614 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1798,10 +1798,8 @@ fec_enet_close(struct net_device *ndev)
 	netif_stop_queue(ndev);
 	fec_stop(ndev);
 
-	if (fep->phy_dev) {
-		phy_stop(fep->phy_dev);
-		phy_disconnect(fep->phy_dev);
-	}
+	phy_stop(fep->phy_dev);
+	phy_disconnect(fep->phy_dev);
 
 	fec_enet_free_buffers(ndev);
 
-- 
1.7.8

[PATCH net-next 4/7] net:fec: ensure that a disconnected phy isn't configured

[Frank Li]

From: Russell King <rmk+kernel@arm.linux.org.uk>

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 <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Frank Li <Frank.Li@freescale.com>
---
 drivers/net/ethernet/freescale/fec_main.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index b7a5614..9de899e 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1496,6 +1496,9 @@ static int fec_enet_set_pauseparam(struct net_device *ndev,
 {
 	struct fec_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");
@@ -1800,6 +1803,7 @@ fec_enet_close(struct net_device *ndev)
 
 	phy_stop(fep->phy_dev);
 	phy_disconnect(fep->phy_dev);
+	fep->phy_dev = NULL;
 
 	fec_enet_free_buffers(ndev);
 
-- 
1.7.8

[PATCH net-next 5/7] net:fec: add support for dumping transmit ring on timeout

[Frank Li]

From: Russell King <rmk+kernel@arm.linux.org.uk>

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 <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Frank Li <Frank.Li@freescale.com>
---
 drivers/net/ethernet/freescale/fec_main.c |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 9de899e..7d506ae 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -291,6 +291,27 @@ static void *swap_buffer(void *bufaddr, int len)
 	return bufaddr;
 }
 
+static void fec_dump(struct net_device *ndev)
+{
+	struct fec_enet_private *fep = netdev_priv(ndev);
+	struct bufdesc *bdp = fep->tx_bd_base;
+	unsigned index = 0;
+
+	netdev_info(ndev, "TX ring dump\n");
+	pr_info("Nr     SC     addr       len  SKB\n");
+
+	do {
+		pr_info("%3u %c%c 0x%04x 0x%08lx %4u %p\n",
+			index,
+			bdp == fep->cur_tx ? 'S' : ' ',
+			bdp == fep->dirty_tx ? 'H' : ' ',
+			bdp->cbd_sc, bdp->cbd_bufaddr, bdp->cbd_datlen,
+			fep->tx_skbuff[index]);
+		bdp = fec_enet_get_nextdesc(bdp, fep);
+		index++;
+	} while (bdp != fep->tx_bd_base);
+}
+
 static int
 fec_enet_clear_csum(struct sk_buff *skb, struct net_device *ndev)
 {
@@ -717,6 +738,8 @@ fec_timeout(struct net_device *ndev)
 {
 	struct fec_enet_private *fep = netdev_priv(ndev);
 
+	fec_dump(ndev);
+
 	ndev->stats.tx_errors++;
 
 	fep->delay_work.timeout = true;
-- 
1.7.8

[PATCH net-next 6/7] net:fec: use netif_tx_disable() rather than netif_stop_queue()

[Frank Li]

From: Russell King <rmk+kernel@arm.linux.org.uk>

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 <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Frank Li <Frank.Li@freescale.com>
---
 drivers/net/ethernet/freescale/fec_main.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 7d506ae..1c84f3a 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -522,7 +522,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);
 	}
 
@@ -1821,7 +1821,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);
 
 	phy_stop(fep->phy_dev);
-- 
1.7.8

[PATCH net-next 7/7] net:fec: iMX6 FEC does not support half-duplex gigabit

[Frank Li]

From: Russell King <rmk+kernel@arm.linux.org.uk>

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 <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Frank Li <Frank.Li@freescale.com>
---
 drivers/net/ethernet/freescale/fec_main.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 1c84f3a..d9109ac 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1316,6 +1316,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
-- 
1.7.8

RE: [PATCH net-next 5/7] net:fec: add support for dumping transmit ring on timeout

[David Laight]

From: Of Frank Li
...
> 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.
...
> +static void fec_dump(struct net_device *ndev)
> +{
> +	struct fec_enet_private *fep = netdev_priv(ndev);
> +	struct bufdesc *bdp = fep->tx_bd_base;
> +	unsigned index = 0;
> +
> +	netdev_info(ndev, "TX ring dump\n");
> +	pr_info("Nr     SC     addr       len  SKB\n");
> +
> +	do {
> +		pr_info("%3u %c%c 0x%04x 0x%08lx %4u %p\n",
> +			index,
> +			bdp == fep->cur_tx ? 'S' : ' ',
> +			bdp == fep->dirty_tx ? 'H' : ' ',
> +			bdp->cbd_sc, bdp->cbd_bufaddr, bdp->cbd_datlen,
> +			fep->tx_skbuff[index]);
> +		bdp = fec_enet_get_nextdesc(bdp, fep);
> +		index++;
> +	} while (bdp != fep->tx_bd_base);
> +}
> +

You probably want the read and write indexes as well.

	David

Re: [PATCH net-next 5/7] net:fec: add support for dumping transmit ring on timeout

[Frank Li]

On Tue, Apr 29, 2014 at 8:38 AM, David Laight <David.Laight@aculab.com> wrote:
> From: Of Frank Li
> ...
>> 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.
> ...
>> +static void fec_dump(struct net_device *ndev)
>> +{
>> +     struct fec_enet_private *fep = netdev_priv(ndev);
>> +     struct bufdesc *bdp = fep->tx_bd_base;
>> +     unsigned index = 0;
>> +
>> +     netdev_info(ndev, "TX ring dump\n");
>> +     pr_info("Nr     SC     addr       len  SKB\n");
>> +
>> +     do {
>> +             pr_info("%3u %c%c 0x%04x 0x%08lx %4u %p\n",
>> +                     index,
>> +                     bdp == fep->cur_tx ? 'S' : ' ',
>> +                     bdp == fep->dirty_tx ? 'H' : ' ',
>> +                     bdp->cbd_sc, bdp->cbd_bufaddr, bdp->cbd_datlen,
>> +                     fep->tx_skbuff[index]);
>> +             bdp = fec_enet_get_nextdesc(bdp, fep);
>> +             index++;
>> +     } while (bdp != fep->tx_bd_base);
>> +}
>> +
>
> You probably want the read and write indexes as well.

                     bdp == fep->cur_tx ? 'S' : ' ',
                     bdp == fep->dirty_tx ? 'H' : ' ',

Above code already print read and write index. 'S', 'H'

Frank Li

>
>         David
>
>
>

RE: [PATCH net-next 5/7] net:fec: add support for dumping transmit ring on timeout

[David Laight]

From: Frank ...
> > You probably want the read and write indexes as well.
> 
>                      bdp == fep->cur_tx ? 'S' : ' ',
>                      bdp == fep->dirty_tx ? 'H' : ' ',
> 
> Above code already print read and write index. 'S', 'H'

Gah I must be asleep!
Something made be think that was to do with the ring ownership bit!

	David

Re: [PATCH net-next 5/7] net:fec: add support for dumping transmit ring on timeout

[Russell King - ARM Linux]

On Tue, Apr 29, 2014 at 02:01:54PM +0000, David Laight wrote:
> From: Frank ...
> > > You probably want the read and write indexes as well.
> > 
> >                      bdp == fep->cur_tx ? 'S' : ' ',
> >                      bdp == fep->dirty_tx ? 'H' : ' ',
> > 
> > Above code already print read and write index. 'S', 'H'
> 
> Gah I must be asleep!
> Something made be think that was to do with the ring ownership bit!

Err, what's going on... this is my patch.  If it's been submitted by
others, why wasn't its submission at least Cc'd to me?

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [PATCH net-next 5/7] net:fec: add support for dumping transmit ring on timeout

[Frank Li]

On Tue, Apr 29, 2014 at 9:01 AM, David Laight <David.Laight@aculab.com> wrote:
> From: Frank ...
>> > You probably want the read and write indexes as well.
>>
>>                      bdp == fep->cur_tx ? 'S' : ' ',
>>                      bdp == fep->dirty_tx ? 'H' : ' ',
>>
>> Above code already print read and write index. 'S', 'H'
>
> Gah I must be asleep!
> Something made be think that was to do with the ring ownership bit!

I think it is same thing. If I am wrong, please tell me difference.

>
>         David
>

Re: [PATCH net-next 5/7] net:fec: add support for dumping transmit ring on timeout

[Frank Li]

On Tue, Apr 29, 2014 at 9:15 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Apr 29, 2014 at 02:01:54PM +0000, David Laight wrote:
>> From: Frank ...
>> > > You probably want the read and write indexes as well.
>> >
>> >                      bdp == fep->cur_tx ? 'S' : ' ',
>> >                      bdp == fep->dirty_tx ? 'H' : ' ',
>> >
>> > Above code already print read and write index. 'S', 'H'
>>
>> Gah I must be asleep!
>> Something made be think that was to do with the ring ownership bit!
>
> Err, what's going on... this is my patch.  If it's been submitted by
> others, why wasn't its submission at least Cc'd to me?

I added " --to rmk+kernel@arm.linux.org.uk" when I send patch.
I don't know what's wrong.
I kept your name.

I ask you if need someone help send to david millar before.

>
> --
> FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
> improving, and getting towards what was expected from it.

RE: [PATCH net-next 5/7] net:fec: add support for dumping transmit ring on timeout

[David Laight]

From: Frank Li [mailto:lznuaa@gmail.com]
> On Tue, Apr 29, 2014 at 9:01 AM, David Laight <David.Laight@aculab.com> wrote:
> > From: Frank ...
> >> > You probably want the read and write indexes as well.
> >>
> >>                      bdp == fep->cur_tx ? 'S' : ' ',
> >>                      bdp == fep->dirty_tx ? 'H' : ' ',
> >>
> >> Above code already print read and write index. 'S', 'H'
> >
> > Gah I must be asleep!
> > Something made be think that was to do with the ring ownership bit!
> 
> I think it is same thing. If I am wrong, please tell me difference.

The ownership bit in the ring flags - that the hardware uses.
Which are printed in the next field.

I'm guessing that the reason the tx ring is 'interesting' is that there
have been bugs where the driver and hardware disagree about which entry
each should process next.
Otherwise the full tx ring is likely to be very very boring.

	David

Re: [PATCH net-next 5/7] net:fec: add support for dumping transmit ring on timeout

[Frank Li]

On Tue, Apr 29, 2014 at 9:22 AM, Frank Li <lznuaa@gmail.com> wrote:
> On Tue, Apr 29, 2014 at 9:15 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Tue, Apr 29, 2014 at 02:01:54PM +0000, David Laight wrote:
>>> From: Frank ...
>>> > > You probably want the read and write indexes as well.
>>> >
>>> >                      bdp == fep->cur_tx ? 'S' : ' ',
>>> >                      bdp == fep->dirty_tx ? 'H' : ' ',
>>> >
>>> > Above code already print read and write index. 'S', 'H'
>>>
>>> Gah I must be asleep!
>>> Something made be think that was to do with the ring ownership bit!
>>
>> Err, what's going on... this is my patch.  If it's been submitted by
>> others, why wasn't its submission at least Cc'd to me?
>
> I added " --to rmk+kernel@arm.linux.org.uk" when I send patch.
> I don't know what's wrong.
> I kept your name.
>
> I ask you if need someone help send to david millar before.

I found the reason.
Your email in MAINTAINS is
Russell King <linux@arm.linux.org.uk>

But signed off in your original patch is
Russell King <rmk+kernel@arm.linux.org.uk>

Which one should be kept in patch?

>
>>
>> --
>> FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
>> improving, and getting towards what was expected from it.

Re: [PATCH net-next 5/7] net:fec: add support for dumping transmit ring on timeout

[Russell King - ARM Linux]

On Tue, Apr 29, 2014 at 09:22:13AM -0500, Frank Li wrote:
> On Tue, Apr 29, 2014 at 9:15 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Tue, Apr 29, 2014 at 02:01:54PM +0000, David Laight wrote:
> >> From: Frank ...
> >> > > You probably want the read and write indexes as well.
> >> >
> >> >                      bdp == fep->cur_tx ? 'S' : ' ',
> >> >                      bdp == fep->dirty_tx ? 'H' : ' ',
> >> >
> >> > Above code already print read and write index. 'S', 'H'
> >>
> >> Gah I must be asleep!
> >> Something made be think that was to do with the ring ownership bit!
> >
> > Err, what's going on... this is my patch.  If it's been submitted by
> > others, why wasn't its submission at least Cc'd to me?
> 
> I added " --to rmk+kernel@arm.linux.org.uk" when I send patch.
> I don't know what's wrong.

Yes... I guess you don't know what went wrong because you'll never know
if it failed to be delivered, because you sent them with an envelope
address of "b20596@shlinux1.ap.freescale.net" which doesn't actually
exist.  Therefore, DSNs can't be returned to you.

> I ask you if need someone help send to david millar before.

You did, but I didn't agree to it.  I did point out that they need a
certain amount of rework first:

  I know they need to re-worked to convert unsigned -> unsigned int before
  David sees them, otherwise it'd be an instant review failure.  Doing the
  conversion is the easy bit, doing it without hitting the 80 column limit
  is much harder...

Some of them need a little more work in addition to that before being
sent to David.

I've also subsequently received in private email one issue which indicates
a problem with the patch set - as yet I've not been able to look at it,
but it sounds like the RX ring is full, but NAPI is in hard-irq mode,
and the hardware won't deliver an IRQ without free space in the RX ring.

Since people have got wind that I've been looking at this driver, everyone
has started sending their various FEC issues directly to me...

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [PATCH net-next 5/7] net:fec: add support for dumping transmit ring on timeout

[Russell King - ARM Linux]

On Tue, Apr 29, 2014 at 02:23:09PM +0000, David Laight wrote:
> From: Frank Li [mailto:lznuaa@gmail.com]
> > On Tue, Apr 29, 2014 at 9:01 AM, David Laight <David.Laight@aculab.com> wrote:
> > > From: Frank ...
> > >> > You probably want the read and write indexes as well.
> > >>
> > >>                      bdp == fep->cur_tx ? 'S' : ' ',
> > >>                      bdp == fep->dirty_tx ? 'H' : ' ',
> > >>
> > >> Above code already print read and write index. 'S', 'H'
> > >
> > > Gah I must be asleep!
> > > Something made be think that was to do with the ring ownership bit!
> > 
> > I think it is same thing. If I am wrong, please tell me difference.
> 
> The ownership bit in the ring flags - that the hardware uses.
> Which are printed in the next field.
> 
> I'm guessing that the reason the tx ring is 'interesting' is that there
> have been bugs where the driver and hardware disagree about which entry
> each should process next.
> Otherwise the full tx ring is likely to be very very boring.

There have been several bugs.

One is where the ring is completely owned by software (because all the
entries have been transmitted) but the driver is buggy and hasn't reaped
the ring at all, leading to a tx timeout.

The second one is where the ring appears to be completely full, because
the hardware hasn't been transmitting for various reasons (eg, there are
bugs in the way the transmitter is started.)

The third one is where the transmitter skips a ring entry on earlier iMX
hardware.

However, those are specific bugs.  The point of dumping the whole ring is
to allow bugs to be diagnosed, because we can then see the state of the
ring, and start looking for likely causes of the symptoms that are visible.
With the driver as it currently is, the only thing we know is "oops, the
transmit seemed to stop for some reason" and we hope that resetting the
device gets it going again - after many seconds of it being non-responsive.

This is how I've sorted out many issues with this driver.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [PATCH net-next 5/7] net:fec: add support for dumping transmit ring on timeout

[Frank Li]

On Tue, Apr 29, 2014 at 9:32 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Apr 29, 2014 at 09:22:13AM -0500, Frank Li wrote:
>> On Tue, Apr 29, 2014 at 9:15 AM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > On Tue, Apr 29, 2014 at 02:01:54PM +0000, David Laight wrote:
>> >> From: Frank ...
>> >> > > You probably want the read and write indexes as well.
>> >> >
>> >> >                      bdp == fep->cur_tx ? 'S' : ' ',
>> >> >                      bdp == fep->dirty_tx ? 'H' : ' ',
>> >> >
>> >> > Above code already print read and write index. 'S', 'H'
>> >>
>> >> Gah I must be asleep!
>> >> Something made be think that was to do with the ring ownership bit!
>> >
>> > Err, what's going on... this is my patch.  If it's been submitted by
>> > others, why wasn't its submission at least Cc'd to me?
>>
>> I added " --to rmk+kernel@arm.linux.org.uk" when I send patch.
>> I don't know what's wrong.
>
> Yes... I guess you don't know what went wrong because you'll never know
> if it failed to be delivered, because you sent them with an envelope
> address of "b20596@shlinux1.ap.freescale.net" which doesn't actually
> exist.  Therefore, DSNs can't be returned to you.
>
>> I ask you if need someone help send to david millar before.
>
> You did, but I didn't agree to it.  I did point out that they need a
> certain amount of rework first:

Sorry, I miss understood your means.
I will stop working on this.

Actually, we want to upstream imx6 SoloX enet.
Solox have not more features, like AVB.

If we upstream base on David's tree, there will be big conflict what
you already done.

best regards
Frank Li

>
>   I know they need to re-worked to convert unsigned -> unsigned int before
>   David sees them, otherwise it'd be an instant review failure.  Doing the
>   conversion is the easy bit, doing it without hitting the 80 column limit
>   is much harder...
>
> Some of them need a little more work in addition to that before being
> sent to David.
>
> I've also subsequently received in private email one issue which indicates
> a problem with the patch set - as yet I've not been able to look at it,
> but it sounds like the RX ring is full, but NAPI is in hard-irq mode,
> and the hardware won't deliver an IRQ without free space in the RX ring.
>
> Since people have got wind that I've been looking at this driver, everyone
> has started sending their various FEC issues directly to me...
>
> --
> FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
> improving, and getting towards what was expected from it.

Re: [PATCH net-next 5/7] net:fec: add support for dumping transmit ring on timeout

[Russell King - ARM Linux]

On Tue, Apr 29, 2014 at 09:54:09AM -0500, Frank Li wrote:
> On Tue, Apr 29, 2014 at 9:32 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Tue, Apr 29, 2014 at 09:22:13AM -0500, Frank Li wrote:
> >> On Tue, Apr 29, 2014 at 9:15 AM, Russell King - ARM Linux
> >> <linux@arm.linux.org.uk> wrote:
> >> > On Tue, Apr 29, 2014 at 02:01:54PM +0000, David Laight wrote:
> >> >> From: Frank ...
> >> >> > > You probably want the read and write indexes as well.
> >> >> >
> >> >> >                      bdp == fep->cur_tx ? 'S' : ' ',
> >> >> >                      bdp == fep->dirty_tx ? 'H' : ' ',
> >> >> >
> >> >> > Above code already print read and write index. 'S', 'H'
> >> >>
> >> >> Gah I must be asleep!
> >> >> Something made be think that was to do with the ring ownership bit!
> >> >
> >> > Err, what's going on... this is my patch.  If it's been submitted by
> >> > others, why wasn't its submission at least Cc'd to me?
> >>
> >> I added " --to rmk+kernel@arm.linux.org.uk" when I send patch.
> >> I don't know what's wrong.
> >
> > Yes... I guess you don't know what went wrong because you'll never know
> > if it failed to be delivered, because you sent them with an envelope
> > address of "b20596@shlinux1.ap.freescale.net" which doesn't actually
> > exist.  Therefore, DSNs can't be returned to you.
> >
> >> I ask you if need someone help send to david millar before.
> >
> > You did, but I didn't agree to it.  I did point out that they need a
> > certain amount of rework first:
> 
> Sorry, I miss understood your means.
> I will stop working on this.

I'd just like to see what you're doing.  Note that your submission also
didn't get through the linux-arm-kernel list either, so I didn't get it
via that path either.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [PATCH net-next 5/7] net:fec: add support for dumping transmit ring on timeout

[Frank Li]

On Tue, Apr 29, 2014 at 10:04 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> I'd just like to see what you're doing.  Note that your submission also
> didn't get through the linux-arm-kernel list either, so I didn't get it
> via that path either.

We have not sent out to public mail list yet.
But there are really big change.
We need support 3 ring queues for tx and rx.

best regards
Frank Li

Re: [PATCH net-next 5/7] net:fec: add support for dumping transmit ring on timeout

[Shawn Guo]

On Tue, Apr 29, 2014 at 10:11:18AM -0500, Frank Li wrote:
> On Tue, Apr 29, 2014 at 10:04 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > I'd just like to see what you're doing.  Note that your submission also
> > didn't get through the linux-arm-kernel list either, so I didn't get it
> > via that path either.
> 
> We have not sent out to public mail list yet.
> But there are really big change.
> We need support 3 ring queues for tx and rx.

Frank,

I guess Russell was saying that these 7 patches did not reach
linux-arm-kernel list for some reason.

Shawn

RE: [PATCH net-next 5/7] net:fec: add support for dumping transmit ring on timeout

[fugang.duan]

[-- Attachment #1: Type: text/plain, Size: 2844 bytes --]

Hi, Russell,

From: Russell King - ARM Linux <linux@arm.linux.org.uk> Data: Tuesday, April 29, 2014 10:38 PM
>To: David Laight
>Cc: 'Frank Li'; Li Frank-B20596; shawn.guo@linaro.org; Duan Fugang-B38611;
>davem@davemloft.net; linux-arm-kernel@lists.infradead.org;
>netdev@vger.kernel.org
>Subject: Re: [PATCH net-next 5/7] net:fec: add support for dumping
>transmit ring on timeout
>
>On Tue, Apr 29, 2014 at 02:23:09PM +0000, David Laight wrote:
>> From: Frank Li [mailto:lznuaa@gmail.com]
>> > On Tue, Apr 29, 2014 at 9:01 AM, David Laight <David.Laight@aculab.com>
>wrote:
>> > > From: Frank ...
>> > >> > You probably want the read and write indexes as well.
>> > >>
>> > >>                      bdp == fep->cur_tx ? 'S' : ' ',
>> > >>                      bdp == fep->dirty_tx ? 'H' : ' ',
>> > >>
>> > >> Above code already print read and write index. 'S', 'H'
>> > >
>> > > Gah I must be asleep!
>> > > Something made be think that was to do with the ring ownership bit!
>> >
>> > I think it is same thing. If I am wrong, please tell me difference.
>>
>> The ownership bit in the ring flags - that the hardware uses.
>> Which are printed in the next field.
>>
>> I'm guessing that the reason the tx ring is 'interesting' is that
>> there have been bugs where the driver and hardware disagree about
>> which entry each should process next.
>> Otherwise the full tx ring is likely to be very very boring.
>
>There have been several bugs.
>
>One is where the ring is completely owned by software (because all the
>entries have been transmitted) but the driver is buggy and hasn't reaped
>the ring at all, leading to a tx timeout.
>
>The second one is where the ring appears to be completely full, because
>the hardware hasn't been transmitting for various reasons (eg, there are
>bugs in the way the transmitter is started.)
>
>The third one is where the transmitter skips a ring entry on earlier iMX
>hardware.
>
>However, those are specific bugs.  The point of dumping the whole ring is
>to allow bugs to be diagnosed, because we can then see the state of the
>ring, and start looking for likely causes of the symptoms that are visible.
>With the driver as it currently is, the only thing we know is "oops, the
>transmit seemed to stop for some reason" and we hope that resetting the
>device gets it going again - after many seconds of it being non-responsive.
>
>This is how I've sorted out many issues with this driver.
>

I see linux next and net "imx_v6_v7_defconfig" don't enable "CONFIG_CMA", if you enable the feature,
Fec issue may disapper. I tested it with the config, fec BD dma coherent issue, watchdog timeout issue don't happen.

I attach the document (not secretive), you can glance over them.
The issue happened on imx6q/dl fec, usb, audio...

Thanks,
Andy

[-- Attachment #2: i.MX6 dma memory bufferable  issue.pptx --]
[-- Type: application/vnd.openxmlformats-officedocument.presentationml.presentation, Size: 983557 bytes --]

Re: [PATCH net-next 5/7] net:fec: add support for dumping transmit ring on timeout

[Russell King - ARM Linux]

On Thu, May 29, 2014 at 08:17:07AM +0000, fugang.duan@freescale.com wrote:
> Hi, Russell,
> 
> I see linux next and net "imx_v6_v7_defconfig" don't enable "CONFIG_CMA",
> if you enable the feature,

Sorry, telling people that CMA is required to avoid that is just not an
acceptable "solution".

With CMA not enabled, the DMA memory provided to the driver is still
perfectly valid and should be no different from what CMA provides.

I'd strongly suggest investigating why there is this difference, and
what the difference is - I'd assume freescale have the ability to do
that with their own devices much more than I have, and probably to a
greater depth too - especially as there is no possibility what so ever
of using any kind of hardware debug on the iMX6 platforms I have.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

RE: [PATCH net-next 5/7] net:fec: add support for dumping transmit ring on timeout

[fugang.duan]

From: Russell King - ARM Linux <linux@arm.linux.org.uk> Data: Friday, May 30, 2014 8:04 AM
>To: Duan Fugang-B38611
>Cc: David Laight; 'Frank Li'; Li Frank-B20596; shawn.guo@linaro.org;
>davem@davemloft.net; netdev@vger.kernel.org
>Subject: Re: [PATCH net-next 5/7] net:fec: add support for dumping
>transmit ring on timeout
>
>On Thu, May 29, 2014 at 08:17:07AM +0000, fugang.duan@freescale.com wrote:
>> Hi, Russell,
>>
>> I see linux next and net "imx_v6_v7_defconfig" don't enable
>> "CONFIG_CMA", if you enable the feature,
>
>Sorry, telling people that CMA is required to avoid that is just not an
>acceptable "solution".
>
>With CMA not enabled, the DMA memory provided to the driver is still
>perfectly valid and should be no different from what CMA provides.
>
>I'd strongly suggest investigating why there is this difference, and what
>the difference is - I'd assume freescale have the ability to do that with
>their own devices much more than I have, and probably to a greater depth
>too - especially as there is no possibility what so ever of using any kind
>of hardware debug on the iMX6 platforms I have.
>
I accept your suggestion to inverstigate the difference. Thanks.
Can you take some time to glance over the attached document by previous mail, we indeed did some tests/analyze on the issue.
Because the DMA memory coherent issue happened at fec, usb, audio modules.

>From the CMA article: http://lwn.net/Articles/450286/:
If a region of low memory is turned into a DMA buffer with an uncached mapping, the system will have two conflicting mappings for the same memory and will have moved into "undefined behavior" territory.

For imx6 DMA memory mapping:
One physical mapping for lowmem map: cacheable with allocate
The physical another mapping for DMA zone remap: Non-cacheable bufferable (kernel with CONFIG_ARM_DMA_MEM_BUFFERABLE enable)

According to the PL301 L2 Cache Controller TRM, memory with normal memory type, shared, outer-non-cacheable attribute can be treated as cacheable memory:
    - Non-cacheable shared reads are treated as cacheable non-allocatable.
    - Non-cacheable shared writes are treated as cacheable write-through no write-allocate. 
Since the access is treated as non-allocate, if the memory is not cached in the L2, the read/write access will always go to the L3 memory. So most of the time, there is no issue with this feature. 

But when there are multiple virtual addresses mapping to same physical address, and if one of the virtual address has a real "cacheable" attribute, this feature would cause us trouble. 

Thanks,
Andy