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

[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

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

[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

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

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

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

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

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

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

[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

[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

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

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

[David Laight]

From: Frank Li [mailto:lznuaa at 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

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