On Mon, 2009-11-30 at 08:54 -0800, Luis R. Rodriguez wrote:
> > +static int ieee80211_request_smps(struct ieee80211_sub_if_data *sdata,
> > + enum nl80211_smps_mode smps_mode)
> > +{
> > + const u8 *ap;
> > + enum nl80211_smps_mode old_req;
> > + int err = 0;
> > +
> > + mutex_lock(&sdata->u.mgd.mtx);
> > +
> > + old_req = sdata->u.mgd.req_smps;
> > + sdata->u.mgd.req_smps = smps_mode;
>
> sdata->u.mgd.mtx seems protected by sdata->u.mgd.mtx.
You mean req_smps, I gather.
> > +
> > + if (old_req == smps_mode &&
> > + smps_mode != NL80211_SMPS_AUTOMATIC)
> > + goto out;
> > +
> > + /*
> > + * If not associated, or current sasociation is not an HT
> > + * association, there's no need to send an action frame.
> > + */
> > + if (!sdata->u.mgd.associated ||
> > + sdata->local->oper_channel_type == NL80211_CHAN_NO_HT) {
> > + mutex_lock(&sdata->local->iflist_mtx);
> > + ieee80211_recalc_smps(sdata->local, sdata);
> > + mutex_unlock(&sdata->local->iflist_mtx);
> > + goto out;
> > + }
> > +
> > + ap = sdata->u.mgd.associated->cbss.bssid;
> > +
> > + if (smps_mode == NL80211_SMPS_AUTOMATIC) {
> > + if (sdata->u.mgd.powersave)
> > + smps_mode = NL80211_SMPS_DYNAMIC;
> > + else
> > + smps_mode = NL80211_SMPS_OFF;
> > + }
> > +
> > + /* send SM PS frame to AP */
> > + err = ieee80211_send_smps_action(sdata, smps_mode,
> > + ap, ap);
> > + if (err)
> > + sdata->u.mgd.req_smps = old_req;
> > + out:
> > + mutex_unlock(&sdata->u.mgd.mtx);
> > +
> > + return err;
> > +}
> > static int ieee80211_set_power_mgmt(struct wiphy *wiphy, struct net_device *dev,
> > bool enabled, int timeout)
> > {
> > @@ -1333,6 +1381,9 @@ static int ieee80211_set_power_mgmt(stru
> > sdata->u.mgd.powersave = enabled;
> > conf->dynamic_ps_timeout = timeout;
> >
> > + /* no change, but if automatic follow powersave */
> > + ieee80211_request_smps(sdata, sdata->u.mgd.req_smps);
>
> But yet the caller uses it without any locking. Seems racy between
> here and the respective mutex_lock().
Hmm, good catch. Although I think it doesn't matter since this is the
only code path that touches it apart from set_smps, and they both occur
under rtnl.
> > mutex_init(&ifmgd->mtx);
> > +
> > + if (sdata->local->hw.flags & IEEE80211_HW_SUPPORTS_DYNAMIC_SMPS)
> > + ifmgd->req_smps = NL80211_SMPS_AUTOMATIC;
> > + else
> > + ifmgd->req_smps = NL80211_SMPS_OFF;
>
> How about just checking the HT cap as well and if no HT cap has been
> set warn as it would indicate a driver bug actually.
You mean if it sets dynps w/o HT cap? We /could/ do that, but I don't
really see a reason for it. We'd find out soon enough by a non-HT driver
supporting SMPS etc.
> > + ieee80211_queue_work(&local->hw, &local->recalc_smps);
>
> OK from what I gather so far we just queue work if we received an ACK
> from the action frame we sent. Since we tell the device to
> ieee80211_stop_device() the device should have stopped RX'ing and that
> *should* mean preventing processing future tx status reports. We'll
> find out if that assumption is wrong.
It's not about receiving -- only about transmitting. But yeah, we'll
find out. I have a patch pending that'll flush queues etc., that would
make it completely impossible if implemented by the driver, but I don't
think we need to worry about it.
> > @@ -209,6 +243,10 @@ void ieee80211_tx_status(struct ieee8021
> > rate_control_tx_status(local, sband, sta, skb);
> > if (ieee80211_vif_is_mesh(&sta->sdata->vif))
> > ieee80211s_update_metric(local, sta, skb);
> > +
> > + if (!(info->flags & IEEE80211_TX_CTL_INJECTED) &&
> > + (info->flags & IEEE80211_TX_STAT_ACK))
> > + ieee80211_frame_acked(sta, skb);
> > }
> >
> > rcu_read_unlock();
>
> Nice, should the nullfunc ack check be handled here as well?
Well, it could, in theory. But I've now imposed a requirement that SMPS
capable devices also implement status notifications properly, which may
or may not be possible for all frames. In fact, we may need to change
this in the future but for now I prefer this requirement -- but for HT
only.
> > + if (!conf_is_ht(&local->hw.conf)) {
> > + /*
> > + * mac80211.h documents that this is only valid
> > + * when the channel is set to an HT type, and
> > + * that otherwise STATIC is used.
> > + */
> > + local->hw.conf.smps_mode = NL80211_SMPS_STATIC;
>
> Why wouldn't this just be SMPS_OFF ?
Well, it doesn't matter either way. However, it was easier this way for
iwlwifi because ultimately we want to turn off all but one chain if
non-HT and/or static SMPS. So at least there it made sense to use STATIC
here to avoid the extra check. But other drivers may or may not want to
ignore it completely in the HT case. I've documented that it sets it to
STATIC when a non-HT association is used just for more use. It could be
anything really, or even an invalid value. Also remember that SMPS_OFF
means all chains turned on.
> > --- wireless-testing.orig/net/mac80211/util.c 2009-11-29 11:33:00.000000000 +0100
> > +++ wireless-testing/net/mac80211/util.c 2009-11-29 11:35:06.000000000 +0100
> > @@ -1170,3 +1170,77 @@ int ieee80211_reconfig(struct ieee80211_
> > return 0;
> > }
> >
> > +static int check_mgd_smps(struct ieee80211_if_managed *ifmgd,
> > + enum nl80211_smps_mode *smps_mode)
> > +{
> > + if (ifmgd->associated) {
> > + *smps_mode = ifmgd->ap_smps;
> > +
> > + if (smps_mode == NL80211_SMPS_AUTOMATIC) {
>
> This should be *smps_mode.
Indeed, good catch, thanks!
johannes
Attachment:
signature.asc
Description: This is a digitally signed message part
