[gPXE-devel] [PATCH] [ipg] Add driver for IC+ IP1000 based cards.

Stefan Hajnoczi stefanha at gmail.com
Tue Jul 13 04:03:12 EDT 2010


uint32_t txd_map; /* base address for tx descriptors */
uint32_t rxd_map; /* base address for rx descriptors */

Please use unsigned long to match the virt_to_bus() return type.

/* tell the NIC not to use this rxfd */
rxfd->rfs = IPG_RFS_RFDDONE;

Missing cpu_to_le64()?

/* enable interrupt notifications */
sp->interrupts = 1;
ipg_w16(IPG_IE_TX_DMA_COMPLETE | IPG_IE_RX_DMA_COMPLETE |
		IPG_IE_HOST_ERROR | IPG_IE_INT_REQUESTED |
		IPG_IE_TX_COMPLETE | IPG_IE_LINK_EVENT |
		IPG_IE_UPDATE_STATS, INT_ENABLE);

Is this enabling ISR bits or actually enabling the interrupt line?  Is there a
reason that interrupts must be enabled?

ipg_nic_hard_start_xmit() doesn't check if tx descriptor ring is full.

/* get status, acknowledge interrupts */
status = ipg_r16(INT_STATUS_ACK);

Why acknowledge interrupts in the transmit function?

This driver uses volatile descriptor fields instead of memory barriers.
Volatile only forces the compiler to emit loads/stores but doesn't affect CPU
memory ordering.  I'd get rid of volatile and use memory barriers.

out:
	/* only re-enable interrupts, if not disabled by ipg_nic_irq */
	if(sp->interrupts) {
		ipg_w16(IPG_INTERRUPTS, INT_ENABLE);
	}

What happens when an event occurs after reading the ISR but before interrupts
are re-enabled?  Does the NIC queue them and raise the interrupt immediately
when it gets re-enabled, otherwise the event could be lost?  The solution would
be to re-enable interrupts and check the rings afterwards.

Byte-swapping is inconsistent for rfs in ipg_nic_rx(). In particular,
this line doesn't work:
                if ((le64_to_cpu(rfs & (IPG_RFS_RXFIFOOVERRUN |
                                        IPG_RFS_RXRUNTFRAME |
                                        IPG_RFS_RXALIGNMENTERROR |
                                        IPG_RFS_RXFCSERROR |
                                        IPG_RFS_RXOVERSIZEDFRAME |
                                        IPG_RFS_RXLENGTHERROR))))
The simplest solution would be:
rfs = le64_to_cpu(rxfd->rfs);
Then the rest of the code can use rfs without worrying about endianness and
__RFS_MASK can drop its byte-swap.

Stefan


More information about the gPXE-devel mailing list