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

[Faur Andrei]

On Wed, Jun 16, 2010 at 11:29 AM, Stefan Hajnoczi <stefanha at gmail.com> wrote:
> 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>

Will do.

> 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.

It was there before because in the Linux driver it is used both in
.probe and in .open. I moved the related code from .probe to .open
because I didn't want to initialize the descriptor rings until the driver's
.open method was called (their address is required in the init block).
Like you noticed, as a result, all of the uses of init_block are now in
.open, which makes it a candidate for scope limitation.

It is used in the Linux driver, in several other places, all related to
multicast filtering. If this will be required in the future, it might be
a good idea to leave init_block where it is. If not, I'll limit the struct
to .open.

>        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.

Will fix.

> 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.

Correct

> pcnet32.c:pcnet32_chip_detect:
> chipname is unused.

Will add a debug message that will use it.

> 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 );

Settled in follow-up emails.

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

Correct.

> 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).

I suggest leaving them in so it's easier for a future patch
to address these issues.

> 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().

Merged the two into pcnet32_setup_init_block(). The initial setup had
no effect since the INIT bit isn't enabled anywhere on the NIC.

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

Added an __attribute__((aligned(32)).

> 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.

True, it never fails. Removed the label.

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

Because writing a 0 to it disables interrupts which causes problems
under UNDI. Also we don't want to read and write back the register
because that causes acknowledgment of interrupts.

> 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.

Done

> Stefan
>

I will send an updated patch soon.

--
Andrei Faur