Re: [PATCH] [2.4] forcedeth network driver

[Jeff Garzik]

Carl-Daniel Hailfinger wrote:
> Jeff Garzik wrote:
>
> > Manfred Spraul wrote:
> >
> >
> > > Jeff wrote:
> > >
> > >
> > > > * Interrupt handler is SCARY.  You can potentially take and release
> > > > the spinlock -many times- during a single interrupt.
> > >
> > > I think that can happen only in theory: A new packet is completed
> > > while the driver processes rx packets. I all normal cases there should
> > > be one spinlock operation per tx irq, and 0 per rx irq.
> > > And error handling IMHO doesn't count: it should be rare.
> > > Or do I overlook a common case?
> >
> >
> > Any amount of load at all will lead to multiple interations of the
> > master loop in the interrupt handler.
>
>
> +static int max_interrupt_work = 5;
> [...]
> +       for (i=0; ; i++) {
> +               if (i > max_interrupt_work) {
> +                       spin_lock(&np->lock);
> +                       /* disable interrupts on the nic */
> +                       writel(0, base + NvRegIrqMask);
> +                       pci_push(base);
> +
> +                       if (!np->in_shutdown)
> +                               mod_timer(&np->nic_poll, jiffies + POLL_WAIT);
> +                       printk(KERN_DEBUG "%s: too many iterations (%d) in
> nic_irq.\n", dev->name, i);
> +                       spin_unlock(&np->lock);
> +                       break;
> +               }
>
> So actually it should not happen that often since we exit the master loop
> after 5 iterations.

<shrug>  Well, it's not a must-change item IMO


> > > > > +#define NV_MIIPHY_DELAY    10
> > > > > +#define NV_MIIPHY_DELAYMAX    10000
> > > >
> > > >
> > > >
> > > > Style:  it's fairly silly to mix enums and constants.
> > >
> > > Right now: enum for the nic registers, #define for the rest. If you
> > > don't like it I can change it.
> >
> >
> > enums are definitely preferred...  communicates more type/symbol
> > information to the compiler and more symbol info to the debugger.
>
>
> The current order has the benefit that the values which might be written
> to a register (most of which are constants) are located next to the enum
> for the register.

You can do this with enums, too :)

> > Fair enough.  You may wish to (after testing!) increase DEFAULT_MTU by 4
> > bytes, to support VLAN.
>
>
> Already tried. If we increase the length of the packet the nic has to send
> beyond 1500 bytes, we simply get an TX error/timeout. So far, I have not
> seen any hints that the nic might do VLAN tagging internally.

The current limit sounds correct, then.


> Jeff, there is another bug in some hardware revisions: For every received
> packet it reports 1500 bytes length regardless of the real length. We take
> this len as correct and hope for the best. At best, it ruins our RX
> statistics. At worst, it might leak kernel memory to userspace since the
> unused remainder of the 1500 bytes is not initialized with anything, yet
> passed up with netif_rx(skb). Does the networking core provide any
> built-in function to address this problem?
>
> +               prd = &np->rx_ring[i];
> [...]
> +               len = le16_to_cpu(prd->Length);
> [...]
> +               skb_put(skb, len);
> [...]
> +               netif_rx(skb);
>
> The above code would be correct if the hardware actually gave us the right
> values. I know that at least some nForce2 systems suffer from this.

Peek at the ethernet frame header and see what it thinks the payload 
length is?  If less than 1500...

	Jeff



-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/