<REAL> iBurst (TM) compatible driver

<REAL> iBurst (TM) compatible driver

[Nicholas Jefferson]

The latest release of the iBurst (TM) compatible driver [1] for the
2.6 kernel is now available. Thanks to ArrayComm (R) for the original
code under the GPL, and Personal Broadband Australia (PBA) and
Independent Service Providers Australia (ISP) for support. Special
thanks to developers David Barr and Nik Trevallyn-Jones.

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

Disclaimer: This driver was not written by ArrayComm (R) and it is not
supported by them. This driver is distributed in the hope that it will
be useful, but without any warranty; without even the implied warranty
of merchantability or fitness for a particular purpose.

Signed-off-by: Nicholas Jefferson <xanthophile@gmail.com>

Kind regards,

Nicholas

Re: <REAL> iBurst (TM) compatible driver

[John W. Linville]

On Fri, Oct 28, 2005 at 02:24:20AM +1000, Nicholas Jefferson wrote:
> The latest release of the iBurst (TM) compatible driver [1] for the
> 2.6 kernel is now available. Thanks to ArrayComm (R) for the original

<snip>

> Signed-off-by: Nicholas Jefferson <xanthophile@gmail.com>

-ENOPATCH?

-- 
John W. Linville
linville@tuxdriver.com

Re: <REAL> iBurst (TM) compatible driver

[Andi Kleen]

On Thursday 27 October 2005 19:07, John W. Linville wrote:
> On Fri, Oct 28, 2005 at 02:24:20AM +1000, Nicholas Jefferson wrote:
> > The latest release of the iBurst (TM) compatible driver [1] for the
> > 2.6 kernel is now available. Thanks to ArrayComm (R) for the original
> 
> <snip>
> 
> > Signed-off-by: Nicholas Jefferson <xanthophile@gmail.com>
> 
> -ENOPATCH?

And no description what an "iBurst (tm)" actually is. It sounds like
something to clear blocked drains with.

-Andi

Re: <REAL> iBurst (TM) compatible driver

[Lennert Buytenhek]

On Thu, Oct 27, 2005 at 07:44:12PM +0200, Andi Kleen wrote:

> And no description what an "iBurst (tm)" actually is. It sounds like
> something to clear blocked drains with.

>From reading http://sourceforge.net/projects/ibdriver, it appears
to be some kind of wireless adapter:

	Linux driver for the ArrayComm* "iBurst*" wireless broadband devices.


--L

Re: <REAL> iBurst (TM) compatible driver

[Bongani Hlope]

On Thursday 27 October 2005 19:44, Andi Kleen wrote:
> On Thursday 27 October 2005 19:07, John W. Linville wrote:
> > On Fri, Oct 28, 2005 at 02:24:20AM +1000, Nicholas Jefferson wrote:
> > > The latest release of the iBurst (TM) compatible driver [1] for the
> > > 2.6 kernel is now available. Thanks to ArrayComm (R) for the original
> >
> > <snip>
> >
> > > Signed-off-by: Nicholas Jefferson <xanthophile@gmail.com>
> >
> > -ENOPATCH?
>
> And no description what an "iBurst (tm)" actually is. It sounds like
> something to clear blocked drains with.
>
> -Andi

iBurst is a wireless broadband internet service, so far I only know that it is 
available in Austrailer and South Africa. Here in South Africa, there are two 
devices you can use for connectivity an internal and external "modem". With 
the external modem, you can use either  USB or ethernet to connect (pppoe). 
The PCMCIA currently is not documented and there is no driver for Linux.

I ordered the external modem (still waiting for it), I'll try to get some guys 
from our LUG to test it (only the PCMCIA version is causing pain), hopefuly 
someone is stuck with that version ;)

Re: <REAL> iBurst (TM) compatible driver

[Stephen Hemminger]

It is good to see more hardware supported, but like many drivers
you are adding a lot of racy ugliness just for debug crap.
Did you actually read any of the other network device drivers first
or try to write code from scratch off some book?

In order of severity.

1. Your network device registration code is wrong.
   Read Documentation/netdevices.txt
   You need to use alloc_netdev() and make the device specific data
   be allocated there, not bury netdevice in a kmalloc'd structure.

2. Transmit routine is wrong. Drop packets if out of memory.
   You can only return NETDEV_TX_OKAY or NETDEV_TX_BUSY
   You probably don't need to allocate memory anyway if you use
   hard header space properly.

3. You need to flow control the transmit queue. When it gets full
   use netdev_stop_queue() and call netdev_wake_queue when you
   have more space.

4. WTF is the whole ib_net_tap file stuff, and why do you need it?
   You can eliminate a lot of module races etc, by just pulling 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.
8. Then you can get rid of having a global array of device structures
   which is hard to maintain properly.
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

-- 
Stephen Hemminger <shemminger@osdl.org>
OSDL http://developer.osdl.org/~shemminger

Re: <REAL> iBurst (TM) compatible driver

[Nicholas Jefferson]

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

Re: <REAL> iBurst (TM) compatible driver

[Stephen Hemminger]

On Tue, 1 Nov 2005 16:00:19 +1100
Nicholas Jefferson <xanthophile@gmail.com> wrote:

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

Where is updated code?

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

There is still a race with poll and tx_start.
Better to have tx_start check for space after queuing and have
poll wake_queue when space becomes available.


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

Some possibilities:
1. Could you support a subset of the existing wireless functions. Then
   all the cool tools like wireless strength stuff would work.
2. Or add a sysfs status files
3. Or a /proc file per device

It makes sense for the driver to give as much information to the user
as possible. Just try and use existing API's first.

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

One way I use is to keep a separate patch around that adds in the
/proc stuff for when debugging, but not put it in the main kernel.

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

* Default name should be "ib%d" so you can handle multiple cards without
  getting an error.
* Why so many symbols exported?
* Ethtool and standard message level support would be cool (but not required).


-- 
Stephen Hemminger <shemminger@osdl.org>
OSDL http://developer.osdl.org/~shemminger