Re: if_iwn.c patch to support the 5100... (analysis + patch)

[Sverre Froyen <sverre%viewmark.com@localhost>]

On Tue September 15 2009 08:24:21 Patrick Welche wrote:
> On Fri, Aug 28, 2009 at 02:52:56PM -0400, Christos Zoulas wrote:
> > On Aug 28,  7:08pm, prlw1%cam.ac.uk@localhost (Patrick Welche) wrote:
> > -- Subject: Re: if_iwn.c patch to support the 5100...
> >
> > | > then built kernel with PCI_INTR_FIXUP PCI_BUS_FIXUP PCI_ADDR_FIXUP
> >
> > Looks like your laptop is really weird...
> 
> It does doesn't it... though all those timing errors have disappeared with
> a newer kernel. I just tried with the in-tree iwn as opposed to the
> patch, and the interface is found successfully.
> 
>   iwn0 at pci1 dev 0 function 0: Intel Pro/Wireless LAN 5100AGN Mini-PCI
>  Adapter (rev. 0x 0) iwn0: interrupting at ioapic0 pin 16
>   , MIMO 1T2R, MoW, address 00:22:fa:65:8c:f8
>   iwn0: 11a rates: 6Mbps 9Mbps 12Mbps 18Mbps 24Mbps 36Mbps 48Mbps 54Mbps
>   iwn0: 11b rates: 1Mbps 2Mbps 5.5Mbps 11Mbps
>   iwn0: 11g rates: 1Mbps 2Mbps 5.5Mbps 11Mbps 6Mbps 9Mbps 12Mbps 18Mbps
>  24Mbps 36Mbps 48Mbps 54Mbps
> 
> (yes, MIMO is on a separate line)
> 
> I can't use it though:
> 
>   ifconfig iwn0 up
> 
> iwn0: timeout waiting for adapter to initialize 35
> iwn0: could not initialize hardware
> iwn0: cannot assign link-local address
> iwn0: timeout waiting for adapter to initialize 35

It looks like these errors may be because iwn_init is called a second time 
before the first call has completed.  It is called via the "if_init" pointer 
and also through iwn_ioctl.  The call via the ioctl path only happens if the 
IFF_UP is set.  I notice that, e.g., net/if_ethersubr.c sets this flag before 
the call to if_init.  While this seems counterintuitive to me it is appears to 
be the way it's always worked.  The simplest fix is to remove the call to 
iwn_init from iwn_ioctl:

Index: src/sys/dev/pci/if_iwn.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/if_iwn.c,v
retrieving revision 1.33
diff -u -p -r1.33 if_iwn.c
--- src/sys/dev/pci/if_iwn.c    11 Sep 2009 01:28:20 -0000      1.33
+++ src/sys/dev/pci/if_iwn.c    17 Sep 2009 15:19:54 -0000
@@ -3213,7 +3213,9 @@ ewhere in driver */
                                if (ifp->if_flags & IFF_RUNNING)
                                        iwn_stop(ifp, 1);
                        } else if (!(ifp->if_flags & IFF_RUNNING))
-                               error = iwn_init(ifp);
+                               aprint_error_dev(sc->sc_dev,
+                                   "iwn_init call from iwn_ioctl skipped\n");
+                               /* error = iwn_init(ifp); */
                } else {
                        if (ifp->if_flags & IFF_RUNNING)
                                iwn_stop(ifp, 1);

A likely side effect is failure to restart the adaptor later.  I better fix 
may be to implement additional driver-internal flags.

> Mutex error: lockdebug_barrier: spin lock held
> ...
> panic: LOCKDEBUG

I was seeing various panics that I believe are related to attempting a second 
firmware load (because of the sencond call to iwn_init).  I have seen:

mutex_enter
ffs_sync
VFS_SYNC
sync_fsync

and

pool_get
pool_cache_put_slow
pool_cache_put_paddr
namei
vn_open
firmware_open
iwn_init
ether_ioctl
...

The above patch does not not address the root cause of these panics (but 
rather avoids them).

Regards,
Sverre

PS I believe that the problem described in kern/42069 is similar to this.