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

[Stefan Hajnoczi]

Please sign off patches.  See linux-2.6/Documentation/SubmittingPatches "12)
Sign your work" for details.  It helps track who was involved with a patch and
states that the developer has permission to contribute.  For example, I add the
following to my commit messages:
Signed-off-by: Stefan Hajnoczi <stefanha at gmail.com>

Mostly cleanup suggestions:

pcnet32.h:

struct pcnet32_private {
	struct pcnet32_init_block init_block;
Is there a reason to keep the init_block around?  The pcnet32_open() function
and its pcnet32_setup_init_block() helper seem to be the only users.

	unsigned int rx_ring_size;
	unsigned int tx_ring_size;
rx_ring_size and tx_ring_size are unused.

	unsigned int	shared_irq:1,
			dxsuflo:1,
			mii:1,
			full_duplex:1;
shared_irq and dxsuflo are unused.

	char phycount;
phycount is local to pcnet32_setup_probe_phy().

	/* each bit indicates an available PHY */
	u32 phymask;
phymask is unused.

#endif /* _PCNET32_H__ */
Trailing underscore.

pcnet32.c:pcnet32_setup_tx_resources:
	memset ( priv->tx_base, 0, TX_RING_BYTES );
	[...]
	/* Clear ownership bit from tx descriptors */
	for ( i = 0; i < TX_RING_SIZE; i++ ) {
		tx_curr_desc = ( priv->tx_base ) + i;
		tx_curr_desc->status = 0;
	}
The descriptors are already initialized to zero.  No need to clear status
field.

pcnet32.c:pcnet32_chip_detect:
chipname is unused.

pcnet32.c:pcnet32_setup_mac_addr:
	for ( i = 0; i < ETH_ALEN; i++ )
		promaddr[i] = inb ( ioaddr + i );
This can be replaced with:
	insb ( ioaddr, promaddr, ETH_ALEN );

/*TODO check MAC address validity */
This can be done now that your patch to make MAC address checking common has
been merged.

pcnet32.c:pcnet32_setup_mii:
This function and the mii_if field aren't really necessary since the driver
doesn't use the PHY directly (no link speed or status).

pcnet32.c:pcnet32_open:
	/* Configure the network port based on what we've established so far */
	priv->init_block.mode =
		cpu_to_le16 ( ( priv->options & PCNET32_PORT_PORTSEL ) << 7 );

	/* No multicasting scheme, accept everything */
	priv->init_block.filter[0] = 0xffffffff;
	priv->init_block.filter[1] = 0xffffffff;
These are partially duplicated in pcnet32_setup_init_block().

Are there any alignment requirements for the init_block?  The structure is
currently embedded in pcnet32_private and doesn't have explicit alignment.

err_init_block:
	return rc;
What about freeing rx/tx resources if pcnet32_setup_init_block() fails?
Current it is impossible for pcnet32_setup_init_block() to fail and it always
returns 0.

pcnet32.c:pcnet32_transmit:
	/* Trigger an immediate send poll */
	priv->a->write_csr ( ioaddr, 0, IntEnable | TxDemand );
Why IntEnable?

pcnet32.c:pcnet32_process_rx_packets:
	for ( i = 0; i < RX_RING_SIZE; i++ ) {
		rx_curr_desc = priv->rx_base + priv->rx_curr;

		status = le16_to_cpu ( rx_curr_desc->status );
I would put a rmb() after this line to ensure that the descriptor status field
is loaded before any other accesses to the descriptor.  Otherwise a junk
msg_length could be loaded because the hardware has cleared DescOwn, then the
hardware clears DescOwn, then status is loaded and we use the junk msg_length
value.

Stefan