On Thursday, March 07, 2013 11:08:58 AM Johannes Berg wrote:
> > + 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?
It is P2P spec. P2P find operates on social channels; there are 3 social
channels on 2.4 band and 1 on 60. No social channels on 5.2.
I can rework to do dynamic allocation, but does it worth the effort?
>
> > + * 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.
Yes, I'll fix this
>
> > + * 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
Sure
>
> > + i = 0;
> > + if (attr_freq) {
>
> i can be inside the if()?
Moved to initializer
>
> > + /* 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?
Passive channel may be used as social for the listening; and may be
actively scanned if beacon found on the channel. Scan do not filter
based on these flags, I suppose same reason apply here.
>
> > + params.channels[i] = chan;
> > + i++;
> > + }
> > + if (!i)
> > + return -EINVAL;
> > + }
> > +
> > + params.n_channels = i;
>
> if you also move that assignment
>
> > + i = 0;
>
> Not needed.
Sure
>
> > + attr = info->attrs[NL80211_ATTR_IE];
> > + if (attr) {
>
> This is not typically done in nl80211, I'd prefer not having the
> variable I think.
It is to reasonable fit code into 80 columns. Let me know if this
argument don't play for you, I'll revert to variables.
>
> > + 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 ...
Yes, agree. I'll add indications, like ROC do.
>
> You're also entirely missing feature advertising, so userspace can only
> guess whether it's supported or not ...
Will do
>Also ...
>
> I really don't think this should be supported on IBSS/AP/... netdevs.
> Seems like the only reasonable ones are P2P_DEVICE and STATION, although
> it would probably be good to have feature advertising for both, or
> document that if P2P_DEVICE is supported at all then this doesn't have
> to be supported on STATION interfaces, or so.
Yes, sure. will add checks
>
> And then ... should this really be allowed to be concurrent with
> scanning/remain-on-channel? You haven't done any checking or
> documentation, so users and driver authors are left to guess.
I check for scan, but not for ROC. Btw, scan also don't check for ROC.
And, ROC don't check for scan.
I'll document that driver should do all checks for ROC/scan.
>
> johannes
>
Thanks, Vladimir
--
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
