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

Guo-Fu Tseng cooldavid at cooldavid.org
Sat May 29 04:35:10 EDT 2010


On Sat, 29 May 2010 01:04:52 +0100, Michael Brown wrote
> 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.
Ahh... Forget to remove it.
It was intended for preventing gPXE provide over 4G DMA
address on x86_64 arch. Since the gPXE won't provide any
address above 4G, this FLAG is not needed.

> 
> > +	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).
I'm sorry I don't quite sure about the idea.
Do you mean I should treat this packet as it's avaliable(Which makes sence
in intuition), and try to allocate the receive buffer later in poll()?

> 
> > +		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.)
I didn't notice that we have eth_ntoa(), it'll be nice to have it. :)
I agreed with all your points.
Anyway, it's just for debug, since the driver seems to work now, we
can revome it. :)

> 
> > +	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.)
Ahh.. Yes!!
I should remove the compare!!

> 
> > +	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.
I noticed that we have %p format later while tracing TCP stack. :)
I did not use it because I was not sure if we have it. :)
I'll modify it. :)

> 
> > +	/*
> > +	 * 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.
I did not clear any interrupt status that related to packet receive
or transmit here. The driver would actually work without it.
In fact this code is only important when the driver was interrupt
driven, it prevent the hardware somehow stops the interrupts due
to un-cleared interrupt status.

Am I missing your point?

> 
> Michael

Guo-Fu Tseng



More information about the gPXE-devel mailing list