Re: ipw(4) driver

[Joerg Sonnenberger <joerg@xxxxxxxxxxxxxxxxx>]

On Tue, Nov 09, 2004 at 11:08:39PM +0100, Damien Bergamini wrote:
> | Hm. This is bad. As I read the LICENSE file of the ipw-firmware,
> | the only problem is EXHIBIT "A", point 2. If they would remove that 
> one,
> | I think we could live with the rest.
> 
> The license permits the redistribution of the firmware through packages
> or ports: http://ipw2100.sourceforge.net/firmware_faq.php
> So there is no problem with the current ioctl-based implementation. 
> That's what it was initially designed for.

Yes, I know :) But that's the only point disallowing redistribution.

> You don't want to upload the firmware in attach(). First because you 
> don't have access to the firmware in attach(). Second because of power
> saving.
> Yes, the adapter consumes more when the firmware is loaded. That's bad 
> if you don't use the it.

The adapter uses more indepent of power management settings or does it
automatically use settings for D1 or higher if no firmware is present?
Anyway, loading the firmware in iwi_attach is fine, as long

> Initializing tx and rx rings in iwi_init() instead of attach() reduces
> kernel memory footprint when the adapter is not used.

Well, on the other hand it creates problems for memory allocation, which
can actually fail, e.g. if bounce buffers have to be created. This can
actually be true in the near feature if Intel ships notebooks with
64-bit extension :)

Memory allocation beside, there's no reason not to allocate the DMA tags
and maps before, they don't use that much memory. If you really care about
memory used to make your availible hardware working, you can compile
a custom kernel or load the kernel module on demand.

> 
> | BTW, there's a race in the attach code. If the interrupt assigned to
> | iwi(4) is really shared, iwi_intr can actually be called. This could 
> lead
> | to corruption and other bad things.
> 
> Interrupts are shared. I don't see any race condition in attach().
> Shared interrupts are dropped immediately in iwi_intr() without 
> accessing
> non-initialized data structures. The adapter itself can't raise any
> interrupt (they are explicitly disabled at the time we install the 
> interrupt
> handler).

What happened if the adapter already had an interrupt pending, e.g. from
before a reboot. In that case IWI_CSR_INTR can be != 0 and code path
hits. The easiest and typical solution to avoid such issues is to hook
the interrupt handler into the system as last part of XX_attach, avoiding
any locking or spl protection.

> 
> | One reason to not like the ioctl interface is that it's adding even
> | more driver specific configuration programs.
> 
> That's where a common framework for loading firmwares makes sense.
> Linux has such a feature (called firmware hotplug AFAIK).

I guess Linux is not the best reference for hotplugging support :)
But having a way to register firmware images both statically
(not unloadable, directly compiled into the kernel), dynamically
(via modules, can be unloaded on demand and preloaded via loader) or
controllable from userland (via sysctl) should be easy to implement
and quite useful.

> | wicontrol is now general program for the many 802.11 devices, it has
> | grown over it's initial use.
> 
> Yes. But that's bad IMO. wicontrol should have stayed bound to wi(4).
> NetBSD has wlanctl(8) for common IEEE802.11 settings (like displaying 
> the
> list of available APs). The mix of WI_* and IEEE80211_* ioctls in
> ieee80211_ioctl.c is a real mess.
> ioctls like WI_RID_PRISM2 and WI_RID_SYMBOL_DIVERSITY have nothing to do
> in a generic 802.11 layer.

I agree, it's bad that we effectively are forced to consider
if_wavelan_ieee.h a part of the generic 802.11 framework. 

> | E.g. the SIOCGTABLE0 and SIOCGRADIO can be better handled with a 
> sysctl
> | handler, I'll start writing some support code for it soon.
> 
> Ok for SIOCGRADIO. For SIOCGTABLE0: I don't like printf's in the kernel
> (prevent localization and such). I prefer the driver to output raw data.

Sure, no problem. You can expose them as BLOB or hex-number to be translated
by userland. A shell script can do that easily.

Joerg

> 
> --
> Damien Bergamini
> damien@xxxxxxxxxxx