On Thu, 2013-03-07 at 11:31 +0200, Vladimir Kondratiev wrote:
> /**
> + * struct cfg80211_p2p_find_params - parameters for P2P find
> + * @probe_ie: IE's for probe frames
> + * @probe_ie_len: length, bytes, of @probe_ie
> + * @probe_resp_ie: IE's for probe response frames
> + * @probe_resp_ie_len: length, bytes, of @probe_resp_ie
> + * @n_channels: number of channels to operate on
> + * @channels: channels to operate on
> + */
> +struct cfg80211_p2p_find_params {
> + const u8 *probe_ie;
> + size_t probe_ie_len;
> + const u8 *probe_resp_ie;
> + size_t probe_resp_ie_len;
> +
> + int n_channels;
> + /* Up to 4 social channels: 3 in 2.4GHz band + 1 in 60GHz band */
> + struct ieee80211_channel *channels[4];
Hmm, is 4 really a good limit? If you wanted to implement progressive
search as part of the find, for example, you'd need more? It seems like
a pretty arbitrary limit -- should it be advertised to userspace?
> + * start_p2p_find: start P2P find phase
> + * Parameters include IEs for probe/probe resp and channels to operate on
The parameters?
you could spell out "resp" ...
Also needs a period (".") at the end or so, if the sentence runs on in
the next line it doesn't make sense.
> + * Parameters are not retained after call, driver need to copy data if
> + * it need it later.
> + * stop_p2p_find: stop P2P find phase
> + *
don't need that blank line
> + i = 0;
> + if (attr_freq) {
i can be inside the if()?
> + /* user specified, bail out if channel not found */
> + nla_for_each_nested(attr, attr_freq, tmp) {
> + struct ieee80211_channel *chan;
> +
> + chan = ieee80211_get_channel(wiphy, nla_get_u32(attr));
> +
> + if (!chan)
> + return -EINVAL;
> +
> + /* ignore disabled channels */
> + if (chan->flags & IEEE80211_CHAN_DISABLED)
> + continue;
I think you should reject them, also if they have passive scan or
various other flags set, I think?
> + params.channels[i] = chan;
> + i++;
> + }
> + if (!i)
> + return -EINVAL;
> + }
> +
> + params.n_channels = i;
if you also move that assignment
> + i = 0;
Not needed.
> + attr = info->attrs[NL80211_ATTR_IE];
> + if (attr) {
This is not typically done in nl80211, I'd prefer not having the
variable I think.
> + msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> + if (!msg)
> + return -ENOMEM;
> +
> + hdr = nl80211hdr_put(msg, info->snd_portid, info->snd_seq, 0,
> + NL80211_CMD_START_P2P_FIND);
Err? What's the value of sending a reply back here? It would seem maybe
appropriate to send one when it *actually* started, but you haven't
implemented that.
> + hdr = nl80211hdr_put(msg, info->snd_portid, info->snd_seq, 0,
> + NL80211_CMD_STOP_P2P_FIND);
same here ...
You're also entirely missing feature advertising, so userspace can only
guess whether it's supported or not ...
johannes
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
