Re: [PATCH 1/4] New netlink command for TID specific configuration

[Sergey Matyukevich <sergey.matyukevich.os@quantenna.com>]

Hello Tamizh,

...

> > > +static int nl80211_set_tid_config(struct sk_buff *skb,
> > > +                                 struct genl_info *info)
> > > +{
> > > +       struct cfg80211_registered_device *rdev = info->user_ptr[0];
> > > +       struct nlattr *attrs[NL80211_ATTR_TID_MAX + 1];
> > > +       struct nlattr *tid;
> > > +       struct net_device *dev = info->user_ptr[1];
> > > +       const char *peer = NULL;
> > > +       u8 tid_no;
> > > +       int ret = -EINVAL, retry_short = -1, retry_long = -1;
> > > +
> > > +       tid = info->attrs[NL80211_ATTR_TID_CONFIG];
> > > +       if (!tid)
> > > +               return -EINVAL;
> > > +
> > > +       ret = nla_parse_nested(attrs, NL80211_ATTR_TID_MAX, tid,
> > > +                              nl80211_attr_tid_policy, info->extack);
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       if (!attrs[NL80211_ATTR_TID])
> > > +               return -EINVAL;
> > > +
> > > +       if (attrs[NL80211_ATTR_TID_RETRY_SHORT]) {
> > > +               retry_short =
> > > nla_get_u8(attrs[NL80211_ATTR_TID_RETRY_SHORT]);
> > > +               if (!retry_short ||
> > > +                   retry_short > rdev->wiphy.max_data_retry_count)
> > > +                       return -EINVAL;
> > > +       }
> > > +
> > > +       if (attrs[NL80211_ATTR_TID_RETRY_LONG]) {
> > > +               retry_long =
> > > nla_get_u8(attrs[NL80211_ATTR_TID_RETRY_LONG]);
> > > +               if (!retry_long ||
> > > +                   retry_long > rdev->wiphy.max_data_retry_count)
> > > +                       return -EINVAL;
> > > +       }
> > > +
> > > +       tid_no = nla_get_u8(attrs[NL80211_ATTR_TID]);
> > > +       if (tid_no >= IEEE80211_FIRST_TSPEC_TSID)
> > > +               return -EINVAL;
> > > +
> > > +       if (info->attrs[NL80211_ATTR_MAC])
> > > +               peer = nla_data(info->attrs[NL80211_ATTR_MAC]);
> > > +
> > > +       if (nla_get_flag(attrs[NL80211_ATTR_TID_RETRY_CONFIG])) {
> > 
> > Do we really need this additional flag to indicate retry data ?
> > Maybe we can simply check retry attrs or even retry data, e.g.:
> 
> Yes, because this implementation gives provision to set default retry
> count for TID traffic type for a station.
> Since we use single NL command for all TID configurations, this flag
> will be useful to notify the driver about
> retry TID configuration change.

Ok. So if driver receives retry value (-1), it should reset to some
default value known to driver or firmware. IMHO it worth making it
more explicit: in its current form this convention will not be obvious
for driver authors. Though I don't have a good idea how to do it.
Maybe merge both aggregation and retry cfg80211 callbacks into one
and use structure for params and bitmask for operations...

> > 
> > if (attrs[NL80211_ATTR_TID_RETRY_LONG] ||
> >     attrs[NL80211_ATTR_TID_RETRY_SHORT]) {
> >     ...
> > 
> > if ((retry_short > 0) || (retry_long > 0)) {
> >     ...

Regards,
Sergey