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

[Gokul Sivakumar]

On Wed, Apr 26, 2023 at 11:00:10AM +0300, Jouni Malinen wrote:
> 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
>
Thanks Jouni and Johannes for the comment. The suggestion for moving the netlink attribute
IFX_VENDOR_ATTR_TWT_PARAM_WAKE_TIME_OFFSET definition into the earlier [PATCH 5/5] itself,
can be adopted. And this would with help with retaining the current enum ID number (5) for
the WAKE_TIME_OFFSET attribute used by the driver, while not showing any enum re-numbering
across commits in this patch series. I will send a V2 series.

Gokul