Re: <REAL> iBurst (TM) compatible driver

[Nicholas Jefferson <xanthophile@gmail.com>]

Hi Stephen,

Thank you for your comments. A new version of the iBurst (TM) wireless
compatible driver (and a patch against 2.6.13.4) is now available [1]
under the linux-2.6-ibdriver-2.0 release.

[1] http://sourceforge.net/projects/ibdriver

> 1. Your network device registration code is wrong.

Okay. ib_net_register now uses alloc_netdev with the appropriate
changes elsewhere.

> 2. Transmit routine is wrong. Drop packets if out of memory.

Okay. ib_net_tx_start does not allocate memory now.

> 3. You need to flow control the transmit queue.

ib_net_tx_prepare already did netif_stop_queue and netif_wake_queue.
Would you prefer this to be in ib_net_tx_start instead?

> 4. WTF is the whole ib_net_tap file stuff, and why do you need it?

The modem can return status information (signal strength, etc.). This
information is accessed from usermode by device files. There is also
an interactive "talk" channel to control the modem, but I don't know
what it can do. The ib-file module implements these device files. It
is not essential for the driver and we don't yet know the modem
protocol anyway, so I've removed it.

> 5. Why bother with a /proc interface?
> 6. If you must then use seq_file.
> 7. If you must then do one device per file.

The /proc interface was for debugging and may later be used to provide
the status information instead of the device files. It's not
essential, so I've removed this as well.

> 8. Then you can get rid of having a global array of device structures
>    which is hard to maintain properly.

The global array was used to set up the correspondence between the
device files (via the minor device file numbers) and the modem
structures (via the index to the global array). It's gone now ;-)

> 9. If you don't support ioctl's just leave dev->ioctl as NULL
> 10. Error code's from call's like register_netdev() should propogate back out.
> 11. ib_net_open, ib_net_close only called from user context don't need
> irqsave use spin_lock_irq()
> 12. Coding style: don't use u_long it's a BSDism
> 13. Please have source code laid out as patch to kernel, not out of tree driver

Okay.

Kind regards,

Nicholas