[patch] iwi(4) suspend/resume broken

[Bernhard Schmidt]

On Friday, March 16, 2012 03:05:47 PM Mitsuru IWASAKI wrote:
> Hi, thanks for comments.
> 
> > Hmm, I'm not sure I like this direct, unconditional fiddling with a
> > VAP's state in the stop function, have you made sure that there are no
> > side effects? eg. in case no VAP exists? I remember running into issue
> > while doing that on iwn(4) and it helped to not have any knowledge
> > about ic or vap in either init() or stop(). Well, I've settled for
> 
> I don't warry about sc_ifp->if_l2com, because it is allocated at
> attach(), but there is not guarantee that sc_ifp->if_l2com->ic_vaps is
> allocated, as you pointed out.
> 
> Updated patches at:
> http://people.freebsd.org/~iwasaki/wlan/ipw-20120316.diff
> http://people.freebsd.org/~iwasaki/wlan/iww-20120316.diff
> http://people.freebsd.org/~iwasaki/wlan/wpi-20120316.diff
> 
> > about ic or vap in either init() or stop(). Well, I've settled for
> > ieee80211_stop() and ieee80211_start() in suspend/resume, which seems
> > to work.
> 
> I was thinking so too, but ieee80211_stop()/ieee80211_init() are never
> called from suspend/resume.  
> Also wpa_supplicant(8) seems to set roaming mode to manual, so
> ieee80211 state doesn't change in this case.
> That's why iwi(4) suspend/resume is broken, I think.
> 
> ieee80211_proto.c:ieee80211_start_locked()
> ----
>          /*
>           * If the parent is up and running, then kick the
>           * 802.11 state machine as appropriate.
>           */
>          if ((parent->if_drv_flags & IFF_DRV_RUNNING) &&
>              vap->iv_roaming != IEEE80211_ROAMING_MANUAL) {
>                   if (vap->iv_opmode == IEEE80211_M_STA) {
> #if 0
>                            /* XXX bypasses scan too easily; disable for now */
>                            /*
>                             * Try to be intelligent about clocking the state
>                             * machine.  If we're currently in RUN state then
>                             * we should be able to apply any new state/parameters
>                             * simply by re-associating.  Otherwise we need to
>                             * re-scan to select an appropriate ap.
>                             */
>                            if (vap->iv_state >= IEEE80211_S_RUN)
>                                     ieee80211_new_state_locked(vap,
>                                         IEEE80211_S_ASSOC, 1);
>                            else
> #endif
>                                     ieee80211_new_state_locked(vap,
>                                         IEEE80211_S_SCAN, 0);
>                   } else {
>                            /*
>                             * For monitor+wds mode there's nothing to do but
>                             * start running.  Otherwise if this is the first
>                             * vap to be brought up, start a scan which may be
>                             * preempted if the station is locked to a particular
>                             * channel.
>                             */
>                            vap->iv_flags_ext |= IEEE80211_FEXT_REINIT;
>                            if (vap->iv_opmode == IEEE80211_M_MONITOR ||
>                                vap->iv_opmode == IEEE80211_M_WDS)
>                                     ieee80211_new_state_locked(vap,
>                                         IEEE80211_S_RUN, -1);
>                            else
>                                     ieee80211_new_state_locked(vap,
>                                         IEEE80211_S_SCAN, 0);
>                   }
>          }
> }
> ----
> 
> Should we call ieee80211_stop()/ieee80211_init() from suspend/resume
> and fix above code?
> I prefer calling ieee80211_new_state(IEEE80211_S_INIT) in
> iwi_stop_locked() because it's equivalent to RELENG_7's
> behavior and fix iwi_restart() too.

Sorry, for the delay.

I'd really prefer calling ieee80211_stop/init() in the suspend/resume
functions, while the driver is not holding the lock (net80211 might
call into the driver again).

I roughly checked a few things and I even think that calling
ieee80211_stop_all() might be enough. Still playing around though.

-- 
Bernhard