[PATCH 7/7] TWT: Add support to configure TWT of a session using offset in microseconds

[Jouni Malinen]

On Wed, Apr 26, 2023 at 12:59:19PM +0530, Gokul Sivakumar wrote:
> On Tue, Apr 25, 2023 at 07:21:50PM +0200, Johannes Berg wrote:
> > On Tue, 2023-04-25 at 21:33 +0530, Gokul Sivakumar wrote:
> > > @@ -238,6 +240,7 @@ enum ifx_vendor_attr_twt_param {
> > >       IFX_VENDOR_ATTR_TWT_PARAM_SETUP_CMD_TYPE,
> > >       IFX_VENDOR_ATTR_TWT_PARAM_DIALOG_TOKEN,
> > >       IFX_VENDOR_ATTR_TWT_PARAM_WAKE_TIME,
> > > +     IFX_VENDOR_ATTR_TWT_PARAM_WAKE_TIME_OFFSET,
> > >       IFX_VENDOR_ATTR_TWT_PARAM_MIN_WAKE_DURATION,
> > >       IFX_VENDOR_ATTR_TWT_PARAM_WAKE_INTVL_EXPONENT,
> > >       IFX_VENDOR_ATTR_TWT_PARAM_WAKE_INTVL_MANTISSA,
> > 
> > I don't know how you manage this, but ... adding a netlink atribute in
> > the middle of the existing enum is ... really awful?

> As you may have seen, the "enum ifx_vendor_attr_twt_params" declaration was introduced only
> in [Patch 5/7], which is part of the same patch series. So we thought that modifying the
> enum numbering in [Patch 7/7] by introducing a new netlink attr would not cause an issue.
> 
> And I have seggregated this "twt_offset" changes into a separate patch, as this is a new
> cmd line arg which is not available currently in the "wpa_cli twt_setup" cmd line arg list.
> 
> The new enum member for WAKE_TIME_OFFSET is inserted next to the the enum member WAKE_TIME
> because both attrs are related to the Target Wake Time Selection. Let me know if you still
> prefer to move this attr to the end of the enum.

If these definitions are not used anywhere yet (e.g., in a driver
snapshot released for some purposes), it probably does not make much of
a difference. However, in general I would strongly recommend maintaining
any kernel interfaces in a manner that does not renumber previously
assigned values. In this particular case, that would mean either moving
that IFX_VENDOR_ATTR_TWT_PARAM_WAKE_TIME_OFFSET definition into an
earlier patch in the series so that it would be added in this location
with the rest of the enum definition without showing any renumbering
between commits or by adding it as the last entry in the enum in this
patch.

-- 
Jouni Malinen                                            PGP id EFC895FA