Re: [RFC V4] cfg80211: introduce critical protocol indication from user-space

["Arend van Spriel" <arend@broadcom.com>]

On 04/05/2013 04:03 PM, Ben Greear wrote:
> On 04/05/2013 06:25 AM, 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.
>>
>> There can be only on critical protocol session started per
>> registered cfg80211 device. 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.
>>
>> Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
>> Reviewed-by: Franky (Zhenhui) Lin <frankyl@broadcom.com>
>> Signed-off-by: Arend van Spriel <arend@broadcom.com>
>> ---

>> +
>> +    duration = max_t(u16, duration, NL80211_MAX_CRIT_PROT_DURATION);
>
> Maybe that should be min_t(....) ?

Yes, definitely.

>> cfg80211_registered_device *rdev,
>> +                    struct wireless_dev *wdev,
>> +                    enum nl80211_crit_proto_id protocol,
>> +                    u16 duration)
>> +{
>> +    int ret;
>> +
>> +    trace_rdev_crit_proto_start(&rdev->wiphy, wdev, protocol, duration);
>> +    ret = rdev->ops->crit_proto_start(&rdev->wiphy, wdev,
>> +                      protocol, duration);
>> +    rdev->crit_proto_started = !ret;
>> +    trace_rdev_return_int(&rdev->wiphy, ret);
>> +    return ret;
>> +}
>> +
>> +static inline int rdev_crit_proto_stop(struct
>> cfg80211_registered_device *rdev,
>> +                       struct wireless_dev *wdev)
>> +{
>> +    int ret = 0;
>> +
>> +    trace_rdev_crit_proto_stop(&rdev->wiphy, wdev);
>> +    if (rdev->crit_proto_started) {
>> +        ret = rdev->ops->crit_proto_stop(&rdev->wiphy, wdev);
>> +        rdev->crit_proto_started = ret != 0;
>
> Maybe:  rdev->crit_proto_started = !ret;

Maybe not. If ret == 0, crit_proto_started should be false. If ret != 0, 
ie. crit_proto_stop() failed, it should remain true. It is a bit of 
reverse logic. I could change it to !!ret or make it more clear using 
!ret ? false : true. Another idea is changing the return type of 
crit_proto_stop() to void and always set crit_proto_started to false.

Gr. AvS