[gPXE-devel] [PATCH v2][pcnet32] Replace pcnet32 driver

[Stefan Hajnoczi]

On Sat, Jun 19, 2010 at 2:27 PM, Andrei Faur <da3drus at gmail.com> wrote:
> +       DBG ( "PCnet chip version is 0x%X\n", chip_version );
[...]
> +       printf ( "PCnet version %#x, chipname %s\n", chip_version, chipname );

New drivers normally should not use printf().  For example, when gPXE
is used as an UNDI ROM then printing something to the screen would
interfere with the user's software.  I suggest changing this to DBG()
so that the first macro prints the chip version and the send prints
the chip name.

> +       if ( priv->tx_fill_ctr == RX_RING_SIZE ) {

Should be TX_RING_SIZE.

> +static void pcnet32_irq_enable ( struct pcnet32_private *priv )
> +{
> +       unsigned long ioaddr = priv->pci_dev->ioaddr;
> +       u16 val;
> +
> +       DBGP ( "pcnet32_irq_enable\n" );
> +
> +       /* Enable TINT and RINT masks */
> +       val = priv->a->read_csr ( ioaddr, 3 );
> +       val &= ~( RxIntMask | TxIntMask );
> +       priv->a->write_csr ( ioaddr, 3, val );
> +
> +       /* Enable interrupts */
> +       val = priv->a->read_csr ( ioaddr, 0 );
> +       priv->a->write_csr ( ioaddr, 0, val | IntEnable );
> +
> +       priv->irq_enabled = 1;
> +}
> +
> +static void pcnet32_irq_disable ( struct pcnet32_private *priv )
> +{
> +       unsigned long ioaddr = priv->pci_dev->ioaddr;
> +       u16 val;
> +
> +       DBGP ( "pcnet32_irq_disable\n" );
> +
> +       val = priv->a->read_csr ( ioaddr, 0 );
> +       priv->a->write_csr ( ioaddr, 0, val & ~IntEnable );
> +
> +       priv->irq_enabled = 0;
> +}

I think CSR0 should not be read here.  If .irq() is called after a tx
or rx event has been raised, the bit will be cleared by writing out
the 1 to CSR0.  So an tx or rx event would be lost and the next
.poll() would not notice.  The Linux driver re-enables interrupts by
writing just IntEnable instead of ORing the current CSR0 value.
Perhaps I'm missing something?

Thomas: Would it be possible to run the Norton Ghost UNDI test on
Andrei's next version of the patch?

Stefan