Re: [PATCH] Rx FIFO Overrun error found

[Manfred Spraul <manfred@colorfullife.com>]

Tim Hockin wrote:
> 
> >       if (dspcfg != DSPCFG_VAL) {
> >               if (!netif_queue_stopped(dev)) {
> > +                     spin_unlock_irq(&np->lock);
> >                       printk(KERN_INFO
> >                               "%s: possible phy reset: re-initializing\n",
> >                               dev->name);
> >                       disable_irq(dev->irq);
> >                       spin_lock_irq(&np->lock);
> > +                     natsemi_reset(dev);
> > +                     reinit_ring(dev);
> >                       init_registers(dev);
> >                       spin_unlock_irq(&np->lock);
> >                       enable_irq(dev->irq);
> 
> I'm not sure you want or need a natsemi_reset here - I'll need to check my
> notes on this when I get back to work.  Can I ask why this change was made?
> This is a very hard case to reproduce, so I'm not very comfortable changing
> the codepath :)  We've had National looking at this buggy behavior for
> months now, with little result.
>

init_registers() reinitialized the rx and tx ring pointers as seen by
the nic. You called it without init_ring(), i.e. the ring pointers as
seen by the driver didn't match the values used by the nic. It probably
only worked by chance. And changing the ring pointers while the nic is
in the middle of a data transfers just asks for trouble.
I agree that natsemi_stop_rxtx() instead of reset would be enough.

> >               /* enable the WOL interrupt.
> >                * Could be used to send a netlink message.
> >                */
> > -             writel(readl(ioaddr + IntrMask) | WOLPkt, ioaddr + IntrMask);
> > +             writel(WOLPkt, ioaddr + IntrMask);
> > +             writel(1, ioaddr + IntrEnable);
> 
> is this intended to blow away the other bits in IntrMask?  Keep in mind
> that Wake-On-Phy requires the PHY interrupt enabled, but I don't know if it
> needs it on in intrmask or just in the Phy intr reg.
>

Yes, they are always 0. Actually the code is (and was) never executed.
Everyone calls enable_wol_mode(,0).

--
        Manfred