Date: Mon, 12 Jan 2009 15:50:43 +0200
From: Jouni Malinen <j@w1.fi>
To: Johannes Berg <johannes@sipsolutions.net>
Cc: linux-wireless@vger.kernel.org
Subject: Re: [RFC] nl80211: New command for adding extra IE(s) into
	management frames
Message-ID: <20090112135043.GA12109@jm.kir.nu> (sfid-20090112_145104_603202_948E5C72)
References: <20090112122605.GB9153@jm.kir.nu> <1231765724.2998.10.camel@home.berg>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
In-Reply-To: <1231765724.2998.10.camel@home.berg>
Sender: linux-wireless-owner@vger.kernel.org

On Mon, Jan 12, 2009 at 02:08:44PM +0100, Johannes Berg wrote:
> Looks sane to me. Would it make sense to provide support for getting
> this information as well?

I did not come up with any good use case for this.

> > +	if (!drv->ops->set_mgmt_extra_ie) {
> > +		err = -EOPNOTSUPP;
> > +		goto out;
> > +	}
> 
> You could check for that earlier and avoid the goto, I think.

Nope.. Need to cfg80211_put_dev(drv) after this check. If you want to
get rid of goto, I can do that by moving the rtnl locking and actual
call into else {}..

> > +/**
> > + * struct mgmt_extra_ie_params - Extra management frame IE parameters
> > + *
> > + * Used to add extra IE(s) into management frames in client modes.
> 
> That could be a little more specific, I think. You seem to implement it
> only for STA/IBSS, but it could make sense for mesh as well, for
> instance to add things into the beacon there? Same with IBSS I guess?

Mesh could make sense, but I did not want to go through all the details
for it now. I would have added IBSS Beacon, had I found a place where we
actually configure it.. Do we or is it just completely missing? The
original d80211 submission did generate the Beacon template when joining
IBSS.. Now mac80211 seems to be just generating a Probe Response frame.
Anyway, defining the interface to be more generic sounds reasonable.

> I think we should provide, to userspace, a guarantee such as:
> 
>   If this operation completes successfully, then any frame generated by
>   the kernel or hardware with that subtype will include the information.
>
> This obviously excludes injected frames and that could be stated
> explicitly. Also means that the "client modes" above should be removed
> and instead the handler has to just fail in other modes and for
> unsupported frame types.

Agreed.

> Therefore, beacons should include it in ibss/mesh, and mac80211's cfg
> handler should reject attempts to configure anything it doesn't support.
> Since then you need to do things based on the frame type, I think I'd
> rather open code the subtypes array into ie_presp, ie_preq, etc.

OK. I'll change the design to do that, but will probably not implement
actual handlers for anything else than managed and adhoc now, i.e.,
net/mac80211/cfg.c will see the config attempts, but it will reject them
for now.

-- 
Jouni Malinen                                            PGP id EFC895FA