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 _______________________________________________ Hostap mailing list Hostap@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/hostap