Re: [PATCH 0/4] atl1: Revised Attansic L1 ethernet driver

From: Chris Snook
Date: Mon Nov 20 2006 - 01:36:24 EST


Alan wrote:
Would be nice if it used atl_ not at_ so its less likely to cause
namespace clashes.

Some of this code looked like Attansic may have meant to share it between drivers for atl1/atl2/atf1/atf2, but seeing as I can't find any code for those, I'll convert it all to atl1_ and let Attansic generalize the code if they ever decide they want to submit drivers.

You have various macros for swaps that are pretty ugly - we have
cpu_to and le/be_to_cpu functions for most swapping cases and these are
generally optimised assembler (eg bswap on x86)

AT_DESC_USED/UNUSED would be better as inline functions but thats not a
serious concern.

Be careful with :1 bitfields when working with hardware - the compiler
has more than one choice about how to pack them.

Lacking a spec, I'm not entirely sure what the original intent was, so we're stuck with testing. Is there a specific disambiguation technique you recommend?

The irq enable/disable use for locking on vlan appears unsafe. PCI
interrupt delivery is asynchronous which means you can get this happen


card sends PCI interrupt
We call irq_disable
We take lock
We poke bits
We drop lock

PCI interrupt arrives


This really does happen, typically its nasty to debug as well because you
usually only get it on PIII boards on the one in n-zillion times a
message collides and is retransmitted on the APIC bus.

Nice catch. I admit the VLAN code is not so well audited or tested. Fortunately, the chip only seems to be on Asus M2V motherboards, at least for now, but I want to audit all of the locking code at some point.

skb->len is unsigned so <= 0 can be == 0. More importantly the subtraction
before the test will wrap and is completely unsafe (see at_xmit_frame)

Thanks!

-- Chris
-
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/