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

[Stefan Hajnoczi]

On Thu, Jun 17, 2010 at 2:22 PM, Faur Andrei <da3drus at gmail.com> wrote:
>> 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.

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

For both of these points I'd tighten up the code unless you intend to
add these features shortly.  Your code is on the mailing list or in
git so it will be possible to come back to it and see how it was
structured.

If you do want to leave them in then comments could be used to
indicate why the code is like this.

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

What happens when the following sequence of calls is made:
.irq(netdev, 1)
.irq(netdev, 0)
.transmit(netdev, iobuf)

I think interrupts will be re-enabled at this point?

Two ideas:
1. Pass the status read in pcnet32_poll() in so status | TxDemand can be used.
2. Keep an irq_enabled variable in pcnet32_private so
pcnet32_transmit() knows whether or not IntEnable is required.

Stefan