[gPXE-devel] [PATCH] [jme] Adding JMicron Ethernet driver

Michael Brown mbrown at fensystems.co.uk
Fri May 28 20:04:52 EDT 2010


On Friday 28 May 2010 18:27:00 cooldavid at cooldavid.org 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


More information about the gPXE-devel mailing list