Bug #1733

patch: update iwi(4)

Added by Johannes.Hofmann over 4 years ago. Updated over 4 years ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:-
Target version:-

Description

Hi,

http://leaf.dragonflybsd.org/~hofmann/iwi_update.diff
is a patch to bring in the newest iwi(4) from FreeBSD.
It basically works and I can associate via wpa2.
There are some issues though that need to be resolved:

* Locking is done completely with lockmgr locks as it is done for
ath now. What is the plan about if_serializer?
I noticed that e.g. parent_updown() in ieee80211_proto.c calls
if_ioctl without if_serializer held. Does this mean that
if_serializer use is deprecated?

* It still uses our old firmware API via wrapper functions, as I
didn't know how to create the firmware modules needed with the new API.
Therefore the patch brings kern_firmware.c back into the kernel
build, but all this can easily be switched to the new API.

* sysctl's are not removed on module unload, so when unloading/loading
I get warnings about reusing sysctl leafs. I didn't find the
relevent code in the other drivers, so maybe I'm missing something
here.

* The alloc_unr()/free_unr() stuff is just commented out. Are there
any plans to bring in this API from FreeBSD?

History

#1 Updated by rpaulo1 over 4 years ago

On 15 Apr 2010, at 13:43, Johannes Hofmann wrote:

> Hi,
>
> http://leaf.dragonflybsd.org/~hofmann/iwi_update.diff
> is a patch to bring in the newest iwi(4) from FreeBSD.
> It basically works and I can associate via wpa2.
> There are some issues though that need to be resolved:
>
> * Locking is done completely with lockmgr locks as it is done for
> ath now. What is the plan about if_serializer?
> I noticed that e.g. parent_updown() in ieee80211_proto.c calls
> if_ioctl without if_serializer held. Does this mean that
> if_serializer use is deprecated?

Dillon had patches for this at http://apollo.backplane.com/DFlyMisc/wifi03.patch.
I didn't commit them on my branch because it didn't fix the issue that Dillion is having with ath.

> * It still uses our old firmware API via wrapper functions, as I
> didn't know how to create the firmware modules needed with the new API.
> Therefore the patch brings kern_firmware.c back into the kernel
> build, but all this can easily be switched to the new API.

See sys/tools/fw_stub.awk in FreeBSD.

> * sysctl's are not removed on module unload, so when unloading/loading
> I get warnings about reusing sysctl leafs. I didn't find the
> relevent code in the other drivers, so maybe I'm missing something
> here.

ath needs this too, IIRC, so you're not missing anything.

>
> * The alloc_unr()/free_unr() stuff is just commented out. Are there
> any plans to bring in this API from FreeBSD?

This facility seems to be pretty straightforward to port. I'll let someone else comment on the desirability.

Regards,
--
Rui Paulo

#2 Updated by alexh over 4 years ago

fwiw, for such a basic use of alloc_unr, devfs_clone_bitmap functions will work
fine (see man devfs_clone_bitmap_get). Just note that they always start the
numbering at 0, so you might need to set a few as allocated if you don't want
them. An upper limit is provided.

Cheers,
Alex Hornung

#3 Updated by dillon over 4 years ago

:* Locking is done completely with lockmgr locks as it is done for
: ath now. What is the plan about if_serializer?
: I noticed that e.g. parent_updown() in ieee80211_proto.c calls
: if_ioctl without if_serializer held. Does this mean that
: if_serializer use is deprecated?

The serializer is already held at that point. The serializer is
not optional, it must be held properly when messing with the ifnet
(such as when dequeueing packets). I still have some work to do
on the ath driver but as Rui mentioned I'm having problems stabilizing
it on SMP.

The kernel will call into the driver with the serializer held for
entry points via ifnet, but things such as callouts and the device
function call API need to acquire the serializer.

The 80211 (wlan) code does acquire the serializer when making most
cross-ifnet calls which simplifies the other drivers but it's still
a real mess.

:* It still uses our old firmware API via wrapper functions, as I
: didn't know how to create the firmware modules needed with the new API.
: Therefore the patch brings kern_firmware.c back into the kernel
: build, but all this can easily be switched to the new API.
:
:* sysctl's are not removed on module unload, so when unloading/loading
: I get warnings about reusing sysctl leafs. I didn't find the
: relevent code in the other drivers, so maybe I'm missing something
: here.

Any SYSCTL declarations should be automatically added and removed.
Any manually added sysctl subtrees (made with procedure calls within
the driver) have to be explicitly removed by the driver.

An example can be found in /usr/src/sys/dev/coretemp/coretemp.c,
coretemp_attach() and coretemp_detach().

:* The alloc_unr()/free_unr() stuff is just commented out. Are there
: any plans to bring in this API from FreeBSD?
:

That doesn't look optional. Sigh. I suppose you could port subr_unit.c
but as is typical in FreeBSDland they have completely overengineered
the API. It would be a whole lot easier if they had just used the
subr_blist.c code for that.

-Matt
Matthew Dillon
<>

#4 Updated by dillon over 4 years ago

::* The alloc_unr()/free_unr() stuff is just commented out. Are there
:: any plans to bring in this API from FreeBSD?
::
:
: That doesn't look optional. Sigh. I suppose you could port subr_unit.c
: but as is typical in FreeBSDland they have completely overengineered
: the API. It would be a whole lot easier if they had just used the
: subr_blist.c code for that.

Rui also mentioned that AlexH thought the clone_bitmap code would
work just fine too.

-Matt
Matthew Dillon
<>

#5 Updated by Johannes.Hofmann over 4 years ago

Matthew Dillon <> wrote:
> ::* The alloc_unr()/free_unr() stuff is just commented out. Are there
> :: any plans to bring in this API from FreeBSD?
> ::
> :
> : That doesn't look optional. Sigh. I suppose you could port subr_unit.c
> : but as is typical in FreeBSDland they have completely overengineered
> : the API. It would be a whole lot easier if they had just used the
> : subr_blist.c code for that.
>
> Rui also mentioned that AlexH thought the clone_bitmap code would
> work just fine too.

Thanks for the feedback!
I've cleaned up the patch so that

* sysctl nodes are removed on detach.

* The new firmware infrastructure from FreeBSD is used.
This also needs a small bugfix in subr_firmware.c to avoid a panic
(see my mail on @bugs).
Note, that you now need to build and load a firmware kernel module
for iwi to work.
I'm uncertain how this should be automated. I think there might be
licensing issues if we would ship the Intel firmware.

* devfs_clone_bitmap_*() functions are used for unique number
allocation.

The new patch can be found at:
http://leaf.dragonflybsd.org/~hofmann/iwi_update2.diff

#6 Updated by tuxillo over 4 years ago

Hi Johannes,

If I'm not mistaken, this iwi(4) update has been committed in
0e474d750d8cddd6ef1c9562c0858c338dab536d.

Shall we mark this ticket as resolved?

Cheers,
Antonio Huete

#7 Updated by Johannes.Hofmann over 4 years ago

"Antonio Huete Jimenez \(via DragonFly issue tracker\)" <> wrote:
>
> Antonio Huete Jimenez <> added the comment:
>
> Hi Johannes,
>
> If I'm not mistaken, this iwi(4) update has been committed in
> 0e474d750d8cddd6ef1c9562c0858c338dab536d.
>
> Shall we mark this ticket as resolved?

Yes, it's committed and works fine for me - posting this via iwi(4) :)

Cheers,
Johannes

Also available in: Atom PDF