On 03/28/2013 05:17 PM, Johannes Berg wrote:
> On Thu, 2013-03-28 at 13:11 +0100, Arend van Spriel wrote:
>> Some protocols need a more reliable connection to complete
>> successful in reasonable time. This patch adds a user-space
>> API to indicate the wireless driver that a critical protocol
>> is about to commence and when it is done, using nl80211 primitives
>> NL80211_CMD_CRIT_PROTOCOL_START and NL80211_CRIT_PROTOCOL_STOP.
>>
>> The driver can support this by implementing the cfg80211 callbacks
>> .crit_proto_start() and .crit_proto_stop(). Examples of protocols
>> that can benefit from this are DHCP, EAPOL, APIPA. Exactly how the
>> link can/should be made more reliable is up to the driver. Things
>> to consider are avoid scanning, no multi-channel operations, and
>> alter coexistence schemes.
>
> Looks better than the BT coex thing :-)
>
These are only words, but thanks ;-)
>> + * @NL80211_CMD_CRIT_PROTOCOL_START: Indicates user-space will start running
>> + * a critical protocol that needs more reliability in the connection to
>> + * complete.
>
> I think this needs a little bit more documentation, addressing
> specifically:
>
> * What is the protocol ID? You haven't defined that at all, I don't
> like that
> you're effectively introducing a private driver API there, and
> userspace tools
> can't really know what to put into it. What's the value in that
> anyway, is it
> to allow nesting multiple protocols? That seems a bit excessive for
> the
> kernel, if needed it could be managed by the supplicant?
> If it doesn't go away, then it should probably be an enum and be
> checked ...
> but then you might need to have drivers advertise which ones they
> support? I
> fail to see the point right now.
I was already hesitant to put the protocol (ETH_P_*) in here. I do not
have a clear use-case for it so let us drop it.
> * I think there should probably be some sort of timeout. Would that
> timeout be
> configurable by userspace, or should this be determined in the
> driver? It
> seems userspace has a better idea of what kind of timeout would be
> needed.
> Either way, does userspace need an indication that it ended?
> Also, if there's a timeout, is a per-device maximum advertisement
> needed?
>
>> + NL80211_ATTR_CRIT_PROT_ID,
>> + NL80211_ATTR_MAX_CRIT_PROT_DURATION,
>
> Oh, you do have a duration, but I don't see it used in the cfg80211 API?
>
>> +++ b/net/wireless/core.c
>> @@ -745,6 +745,8 @@ static void wdev_cleanup_work(struct work_struct *work)
>> wdev = container_of(work, struct wireless_dev, cleanup_work);
>> rdev = wiphy_to_dev(wdev->wiphy);
>>
>> + schedule_delayed_work(&wdev->crit_proto_work, 0);
>
> That's icky -- will cause all kinds of locking problems if this
> schedules out because it'll be hard to ensure it really finished. You
> should probably cancel and do it inline here?
>
>> +void wdev_cancel_crit_proto(struct work_struct *work)
>> +{
>> + struct delayed_work *dwork;
>> + struct cfg80211_registered_device *rdev;
>> + struct wireless_dev *wdev;
>> +
>> + dwork = container_of(work, struct delayed_work, work);
>> + wdev = container_of(dwork, struct wireless_dev, crit_proto_work);
>> + rdev = wiphy_to_dev(wdev->wiphy);
>> +
>> + rdev_crit_proto_stop(rdev, wdev, wdev->crit_proto);
>> + wdev->crit_proto = 0;
>
> Why even store the protocol value in the wdev? Or was that intended to
> be used to verify that you're not canceling something that doesn't
> exist?
>
>> +#define NL80211_MIN_CRIT_PROT_DURATION 2500 /* msec */
>
> That seems pretty long? Why have such a long *minimum* duration? At 2.5
> seconds, it's way long, and then disabling most of the
> protections/powersave/whatever no longer makes sense for this period of
> time since really mostly what this does will be reducing the wifi
> latency.
>
Ok, so what minimum do you (or someone else can chime in here) think a
DHCP exchange takes as that was considered a likely protocol that can
benefit from this API.
>> @@ -1417,6 +1419,8 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *dev,
>> }
>> CMD(start_p2p_device, START_P2P_DEVICE);
>> CMD(set_mcast_rate, SET_MCAST_RATE);
>> + CMD(crit_proto_start, CRIT_PROTOCOL_START);
>> + CMD(crit_proto_stop, CRIT_PROTOCOL_STOP);
>
> Ah, a tricky problem -- unrelated to your patch. You probably saw the
> wiphy size limit problem. If we keep adding commands here, we'll
> eventually break older userspace completely, so I think we shouldn't. A
> new way will probably be required, either adding new commands only
> conditionally on split wiphy dump, or inventing a new way. I suppose
> making it conditional is acceptable for all new commands since new tools
> in userspace will be required anyway to make them work.
>
Indeed noticed some emails on this. I simply added these lines without
looking what this code fragment does.
>> + cancel_delayed_work(&wdev->crit_proto_work);
>
> That should probably be _sync. Is it even valid to start one while an
> old one is present? What's the expected behaviour? Right now you
> override, but is that really desired?
Good point. Maybe keep track that crit_proto is started and reject a
subsequent call (-EBUSY). Ideally, the start and stop should be done by
the same user-space process/application. Is that possible?
> Actually ... I think you should just get rid of the work, or at least
> make it optional. If we were going to implement this, for example, we'd
> definitely push the timing all the way into the firmware, and thus
> wouldn't want to have this cancel work. Isn't that similar for you?
>
>> + /* skip delayed work if no timeout provided */
>
> I suspect a timeout should be required?
>
>> + schedule_delayed_work(&wdev->crit_proto_work,
>> + msecs_to_jiffies(duration));
>
> Ah, ok, I guess I misunderstood it. You do use it, but don't pass it to
> the driver since you let cfg80211 handle the timing...
>
Indeed. I wanted to be sure that the duration provided by user-space is
applicable independent from a driver implementation. Do you think it
makes sense to have this dealt with by cfg80211?
>> + wdev->crit_proto = proto;
>> +
>> +done:
>> + return rdev_crit_proto_start(rdev, wdev, proto);
>
> If this fails, you get confusion, the work will run anyway.
>
Good point.
>> + if (info->attrs[NL80211_ATTR_CRIT_PROT_ID])
>> + proto = nla_get_u16(info->attrs[NL80211_ATTR_CRIT_PROT_ID]);
>> +
>> + return rdev_crit_proto_stop(rdev, wdev, proto);
>
> You stored it before, so why not use that here as well? Or would
> start(proto=1)
> stop(proto=2)
>
> be valid?
Let's make life a bit easier by getting rid of the proto as we do not
currently see a use-case for the protocol.
Thanks,
Arend
--
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
