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

Thomas Miletich thomas.miletich at gmail.com
Tue Jul 13 04:19:06 EDT 2010


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 <stefanha at gmail.com> 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
>


More information about the gPXE-devel mailing list