Hello Stefan thanks for your thorough review :) I will post an updated patch as soon as I have one.
-- Thomas On Tue, Jul 13, 2010 at 10:03 AM, Stefan Hajnoczi <[email protected]> wrote: > 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
