2005-10-27 16:24:22

by Nicholas Jefferson

[permalink] [raw]
Subject: <REAL> iBurst (TM) compatible driver

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 <[email protected]>

Kind regards,

Nicholas


2005-10-27 17:07:37

by John W. Linville

[permalink] [raw]
Subject: Re: <REAL> iBurst (TM) compatible driver

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 <[email protected]>

-ENOPATCH?

--
John W. Linville
[email protected]

2005-10-27 17:43:27

by Andi Kleen

[permalink] [raw]
Subject: Re: <REAL> iBurst (TM) compatible driver

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 <[email protected]>
>
> -ENOPATCH?

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

-Andi

2005-10-27 17:45:30

by Lennert Buytenhek

[permalink] [raw]
Subject: Re: <REAL> iBurst (TM) compatible driver

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

2005-10-27 21:24:29

by Bongani Hlope

[permalink] [raw]
Subject: Re: <REAL> iBurst (TM) compatible driver

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 <[email protected]>
> >
> > -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 ;)

2005-10-27 22:31:10

by Stephen Hemminger

[permalink] [raw]
Subject: Re: <REAL> iBurst (TM) compatible driver

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 <[email protected]>
OSDL http://developer.osdl.org/~shemminger

2005-11-01 05:00:22

by Nicholas Jefferson

[permalink] [raw]
Subject: Re: <REAL> iBurst (TM) compatible driver

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

2005-11-01 23:21:33

by Stephen Hemminger

[permalink] [raw]
Subject: Re: <REAL> iBurst (TM) compatible driver

On Tue, 1 Nov 2005 16:00:19 +1100
Nicholas Jefferson <[email protected]> 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 <[email protected]>
OSDL http://developer.osdl.org/~shemminger