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 <stefa...@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
_______________________________________________
gPXE-devel mailing list
gPXE-devel@etherboot.org
http://etherboot.org/mailman/listinfo/gpxe-devel
