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
_______________________________________________
gPXE-devel mailing list
[email protected]
http://etherboot.org/mailman/listinfo/gpxe-devel
