On Friday 28 May 2010 18:27:00 [email protected] wrote:
> A new driver for JMicron Ethernet controller.
Looks pretty nice! :)
A few comments, based on a very quick look:
> +#if __x86_64__
> + rxdesc->desc1.flags = RXFLAG_64BIT;
> +#endif
What does this flag actually do? It's implausible that the card cares which
CPU is running on the host system.
> + rxdesc += idx;
> + if (jme_make_new_rx_buf(rxring->bufinf + idx)) {
> + DBG("Dropped packet due to memory allocation error.\n");
> + netdev_rx_err(netdev, NULL, -ENOMEM);
> + } else {
This will behave poorly when memory runs out. Better is to have a refill
routine that runs after the poll(), and always attempts to refill up to a
specified level (failing silently if it cannot allocate a buffer).
> + DBG2("Received packet: "
> + "from %02x:%02x:%02x:%02x:%02x:%02x "
> + "to %02x:%02x:%02x:%02x:%02x:%02x "
> + "type %04x\n",
> + *(uint8_t *)(rxbi->data + 6),
> + *(uint8_t *)(rxbi->data + 7),
> + *(uint8_t *)(rxbi->data + 8),
> + *(uint8_t *)(rxbi->data + 9),
> + *(uint8_t *)(rxbi->data + 10),
> + *(uint8_t *)(rxbi->data + 11),
> + *(uint8_t *)(rxbi->data + 0),
> + *(uint8_t *)(rxbi->data + 1),
> + *(uint8_t *)(rxbi->data + 2),
> + *(uint8_t *)(rxbi->data + 3),
> + *(uint8_t *)(rxbi->data + 4),
> + *(uint8_t *)(rxbi->data + 5),
> + be16_to_cpu(*(uint16_t *)(rxbi->data + 12)));
If you really want to dump out that information, use struct ethhdr and
eth_ntoa(). Alternatively (and I think preferably), just don't print it;
drivers should generally not try to parse the packets they transmit and
receive. (For example, your code will break if/when software VLAN support is
added.)
> + if (memcmp(addr, netdev->hw_addr, ETH_ALEN)) {
No need for this compare; just always write the MAC address. (This code will
fail if you set the MAC to a non-default value, then try to set it back.)
> + DBG2("TX buffer address: %08lx(%08lx+%x)\n",
> + (unsigned long)iob->data, mapping, len);
%p should be used for printing out pointer values, rather than casting to
unsigned long.
> + /*
> + * Clean all other interrupt status
> + */
> + jwrite32(jme, JME_IEVE,
> + intrstat & ~(INTR_RX0 | INTR_RX0EMP | INTR_TX0));
> +}
This has to happen *before* examining the descriptor rings, to avoid a
potential race condition that could cause packets to become delayed
indefinitely.
Michael
_______________________________________________
gPXE-devel mailing list
[email protected]
http://etherboot.org/mailman/listinfo/gpxe-devel
