On Thu, 6 Sep 2007 15:30:25 -0700 Andrew Morton wrote:

quoted

On Thu, 6 Sep 2007 17:34:10 +0200 Matteo Croce [off-list ref] wrote:
Driver for the cpmac 100M ethernet driver.
It works fine disabling napi support, enabling it gives a kernel panic
when the first IPv6 packet has to be forwarded.
Other than that works fine.

I'm not too sure why I got cc'ed on this (and not on patches 1-6?) but
whatever.

This patch introduces quite a number of basic coding-style mistakes. 
Please run it through scripts/checkpatch.pl and review the output.

The patch introduces vast number of volatile structure fields.  Please see
Documentation/volatile-considered-harmful.txt.

The patch inroduces a modest number of unneeded (and undesirable) casts of
void*, such as

+	struct cpmac_mdio_regs *regs = (struct cpmac_mdio_regs *)bus->priv;

please check for those and fix them up.

The driver implements a driver-private skb pool.  I don't know if this is
something which we like net drivers doing?  If it is approved then surely
there should be a common implementation for it somewhere?

The driver does a lot of open-coded dma_cache_inv() calls (in a way which
assumes a 32-bit bus, too).  I assume that dma_cache_inv() is some mips
thing.  I'd have thought that it would be better to use the dma mapping API
thoughout the driver, and its associated dma invalidation APIs.

The driver has some LINUX_VERSION_CODE ifdefs.  We usually prefer that such
code not be present in a merged-up driver.

quoted

+			priv->regs->mac_hash_low = 0xffffffff;
+			priv->regs->mac_hash_high = 0xffffffff;
+		} else {
+			for (i = 0, iter = dev->mc_list; i < dev->mc_count;
+			    i++, iter = iter->next) {
+				hash = 0;
+				tmp = iter->dmi_addr[0];
+				hash  ^= (tmp >> 2) ^ (tmp << 4);
+				tmp = iter->dmi_addr[1];
+				hash  ^= (tmp >> 4) ^ (tmp << 2);
+				tmp = iter->dmi_addr[2];
+				hash  ^= (tmp >> 6) ^ tmp;
+				tmp = iter->dmi_addr[4];
+				hash  ^= (tmp >> 2) ^ (tmp << 4);
+				tmp = iter->dmi_addr[5];
+				hash  ^= (tmp >> 4) ^ (tmp << 2);
+				tmp = iter->dmi_addr[6];
+				hash  ^= (tmp >> 6) ^ tmp;
+				hash &= 0x3f;
+				if (hash < 32) {
+					hashlo |= 1<<hash;
+				} else {
+					hashhi |= 1<<(hash - 32);
+				}
+			}
+
+			priv->regs->mac_hash_low = hashlo;
+			priv->regs->mac_hash_high = hashhi;
+		}

Do we not have a library function anywhere which will perform this little
multicasting hash?

Depends on the ethernet controller, but the ones that I know about
just use a CRC (crc-16 IIRC) calculation for the multicast hash.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***