Re: [PATCH v7 3/4] mac80211: implement chanctx reservation

[Luca Coelho <luca@coelho.fi>]

On Wed, 2014-03-12 at 14:21 +0100, Michal Kazior wrote:
> On 12 March 2014 14:06, Luciano Coelho <luciano.coelho@intel.com> wrote:
> 
> [...]
> 
> > +       if (old_ctx == ctx) {
> > +               /* This is our own context, just change it */
> > +               ret = __ieee80211_vif_change_channel(sdata, old_ctx,
> > +                                                    &tmp_changed);
> > +               if (ret)
> > +                       goto out;
> > +       } else {
> > +               if (sdata->vif.bss_conf.chandef.width !=
> > +                   sdata->reserved_chandef.width)
> > +                       tmp_changed |= BSS_CHANGED_BANDWIDTH;
> > +
> > +               sdata->vif.bss_conf.chandef = sdata->reserved_chandef;
> > +
> > +               ret = ieee80211_assign_vif_chanctx(sdata, ctx);
> > +               if (old_ctx->refcount == 0)
> > +                       ieee80211_free_chanctx(local, old_ctx);
> > +               if (ret) {
> > +                       /* if assign fails refcount stays the same */
> > +                       if (ctx->refcount == 0)
> > +                               ieee80211_free_chanctx(local, ctx);
> > +                       goto out;
> > +               }
> > +
> > +               *changed = tmp_changed;
> > +
> > +               ieee80211_vif_copy_chanctx_to_vlans(sdata, false);
> 
> Ah, you can't call it like that. This will deadlock because the
> function tries to grab chanctx_mtx but you're already holding it. You
> need to first split the vif_copy_chanctx() into 2 variants - with and
> without grabbing the chanctx_mtx.

Grrrr! Concentrate, Luca, concentrate!


> > +               ieee80211_recalc_chanctx_chantype(local, ctx);
> > +               ieee80211_recalc_smps_chanctx(local, ctx);
> > +               ieee80211_recalc_radar_chanctx(local, ctx);
> > +               ieee80211_recalc_chanctx_min_def(local, ctx);
> 
> It seems you need to call these recalc* regardless what if() branch is
> taken so calling them in this branch is pointless, no?
> 
> > +       }
> > +
> > +       *changed = tmp_changed;
> > +
> > +       ieee80211_recalc_chanctx_chantype(local, ctx);
> > +       ieee80211_recalc_smps_chanctx(local, ctx);
> > +       ieee80211_recalc_radar_chanctx(local, ctx);
> > +       ieee80211_recalc_chanctx_min_def(local, ctx);

More stuff I screwed up in my rebases...

--
Luca.