On Wed, 29 Mar 2023 at 20:36, Jacob Keller <jacob.e.kel...@intel.com> wrote:
> The power profile configuration options added in commit 7059a05a3fb2 > ("Introduce the power profile.") specify their maximum range as INT_MAX. > The values are stored in UInteger32 values, and the default 0xFFFFFFFF is > outside the range of a 32-bit integer. Because of this, on platforms which > have 32-bit integers, ptp4l is unable to read the default configuration: > Just thinking. Why not move to unsigned 64 bits? It can solve future limitations. I would also change the int to signed 64, but I can understand it may require too many changes in the code. Erez > > 0xFFFFFFFF is an out of range value for option > power_profile.2011.grandmasterTimeInaccuracy at line 44 > failed to parse configuration file configs/default.cfg > These values are unsigned and stored as fixed width values of 32 bits. > Correct the configuration specification to allow the true maximum of an > unsigned 32 bit integer. > > Signed-off-by: Jacob Keller <jacob.e.kel...@intel.com> > --- > I'm not sure if there is a simpler or better way to fix this. We could just > update the range of CFG_TYPE_INT to be long (or long long) instead, which > would give it a range big enough to cover all our current values... > > config.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++--- > config.h | 3 +++ > port.c | 6 +++--- > 3 files changed, 57 insertions(+), 6 deletions(-) > > diff --git a/config.c b/config.c > index cb4421f572c7..baf8352538ea 100644 > --- a/config.c > +++ b/config.c > @@ -51,6 +51,7 @@ enum config_section { > > enum config_type { > CFG_TYPE_INT, > + CFG_TYPE_UINT, > CFG_TYPE_DOUBLE, > CFG_TYPE_ENUM, > CFG_TYPE_STRING, > @@ -63,6 +64,7 @@ struct config_enum { > > typedef union { > int i; > + unsigned int u; > double d; > char *s; > } any_t; > @@ -109,6 +111,14 @@ struct config_item { > .min.i = _min, \ > .max.i = _max, \ > } > +#define CONFIG_ITEM_UINT(_label, _port, _default, _min, _max) { \ > + .label = _label, \ > + .type = CFG_TYPE_UINT, \ > + .flags = _port ? CFG_ITEM_PORT : 0, \ > + .val.u = _default, \ > + .min.u = _min, \ > + .max.u = _max, \ > +} > #define CONFIG_ITEM_STRING(_label, _port, _default) { \ > .label = _label, \ > .type = CFG_TYPE_STRING, \ > @@ -125,6 +135,9 @@ struct config_item { > #define GLOB_ITEM_INT(label, _default, min, max) \ > CONFIG_ITEM_INT(label, 0, _default, min, max) > > +#define GLOB_ITEM_UINT(label, _default, min, max) \ > + CONFIG_ITEM_UINT(label, 0, _default, min, max) > + > #define GLOB_ITEM_STR(label, _default) \ > CONFIG_ITEM_STRING(label, 0, _default) > > @@ -137,6 +150,9 @@ struct config_item { > #define PORT_ITEM_INT(label, _default, min, max) \ > CONFIG_ITEM_INT(label, 1, _default, min, max) > > +#define PORT_ITEM_UINT(label, _default, min, max) \ > + CONFIG_ITEM_UINT(label, 1, _default, min, max) > + > #define PORT_ITEM_STR(label, _default) \ > CONFIG_ITEM_STRING(label, 1, _default) > > @@ -309,9 +325,9 @@ struct config_item config_tab[] = { > GLOB_ITEM_DBL("pi_proportional_norm_max", 0.7, DBL_MIN, 1.0), > GLOB_ITEM_DBL("pi_proportional_scale", 0.0, 0.0, DBL_MAX), > PORT_ITEM_ENU("power_profile.version", IEEE_C37_238_VERSION_NONE, > ieee_c37_238_enu), > - PORT_ITEM_INT("power_profile.2011.grandmasterTimeInaccuracy", > 0xFFFFFFFF, 0, INT_MAX), > - PORT_ITEM_INT("power_profile.2011.networkTimeInaccuracy", 0, 0, > INT_MAX), > - PORT_ITEM_INT("power_profile.2017.totalTimeInaccuracy", > 0xFFFFFFFF, 0, INT_MAX), > + PORT_ITEM_UINT("power_profile.2011.grandmasterTimeInaccuracy", > 0xFFFFFFFF, 0, UINT32_MAX), > + PORT_ITEM_UINT("power_profile.2011.networkTimeInaccuracy", 0, 0, > UINT32_MAX), > + PORT_ITEM_UINT("power_profile.2017.totalTimeInaccuracy", > 0xFFFFFFFF, 0, UINT32_MAX), > PORT_ITEM_INT("power_profile.grandmasterID", 0, 0, 0xFFFF), > GLOB_ITEM_INT("priority1", 128, 0, UINT8_MAX), > GLOB_ITEM_INT("priority2", 128, 0, UINT8_MAX), > @@ -559,6 +575,7 @@ static enum parser_result parse_item(struct config > *cfg, > enum parser_result r; > struct config_item *cgi, *dst; > struct config_enum *cte; > + unsigned int uval; > double df; > int val; > > @@ -578,6 +595,9 @@ static enum parser_result parse_item(struct config > *cfg, > case CFG_TYPE_INT: > r = get_ranged_int(value, &val, cgi->min.i, cgi->max.i); > break; > + case CFG_TYPE_UINT: > + r = get_ranged_uint(value, &uval, cgi->min.u, cgi->max.u); > + break; > case CFG_TYPE_DOUBLE: > r = get_ranged_double(value, &df, cgi->min.d, cgi->max.d); > break; > @@ -623,6 +643,9 @@ static enum parser_result parse_item(struct config > *cfg, > case CFG_TYPE_ENUM: > dst->val.i = val; > break; > + case CFG_TYPE_UINT: > + dst->val.u = uval; > + break; > case CFG_TYPE_DOUBLE: > dst->val.d = df; > break; > @@ -1013,6 +1036,7 @@ int config_get_int(struct config *cfg, const char > *section, const char *option) > exit(-1); > } > switch (ci->type) { > + case CFG_TYPE_UINT: > case CFG_TYPE_DOUBLE: > case CFG_TYPE_STRING: > pr_err("bug: config option %s type mismatch!", option); > @@ -1025,6 +1049,29 @@ int config_get_int(struct config *cfg, const char > *section, const char *option) > return ci->val.i; > } > > +unsigned int > +config_get_uint(struct config *cfg, const char *section, const char > *option) > +{ > + struct config_item *ci = config_find_item(cfg, section, option); > + > + if (!ci) { > + pr_err("bug: config option %s missing!", option); > + exit(-1); > + } > + switch (ci->type) { > + case CFG_TYPE_INT: > + case CFG_TYPE_ENUM: > + case CFG_TYPE_DOUBLE: > + case CFG_TYPE_STRING: > + pr_err("bug: config option %s type mismatch!", option); > + exit(-1); > + case CFG_TYPE_UINT: > + break; > + } > + pr_debug("config item %s.%s is %u", section, option, ci->val.u); > + return ci->val.u; > +} > + > char *config_get_string(struct config *cfg, const char *section, > const char *option) > { > @@ -1130,6 +1177,7 @@ int config_set_section_int(struct config *cfg, const > char *section, > return -1; > } > switch (cgi->type) { > + case CFG_TYPE_UINT: > case CFG_TYPE_DOUBLE: > case CFG_TYPE_STRING: > pr_err("bug: config option %s type mismatch!", option); > diff --git a/config.h b/config.h > index 14d2f64415dc..0d7697ab8f90 100644 > --- a/config.h > +++ b/config.h > @@ -61,6 +61,9 @@ double config_get_double(struct config *cfg, const char > *section, > int config_get_int(struct config *cfg, const char *section, > const char *option); > > +unsigned int config_get_uint(struct config *cfg, const char *section, > + const char *option); > + > char *config_get_string(struct config *cfg, const char *section, > const char *option); > > diff --git a/port.c b/port.c > index 3453716f6020..697cf08ce421 100644 > --- a/port.c > +++ b/port.c > @@ -3401,11 +3401,11 @@ struct port *port_open(const char *phc_device, > p->pwr.grandmasterID = > config_get_int(cfg, p->name, > "power_profile.grandmasterID"); > p->pwr.grandmasterTimeInaccuracy = > - config_get_int(cfg, p->name, > "power_profile.2011.grandmasterTimeInaccuracy"); > + config_get_uint(cfg, p->name, > "power_profile.2011.grandmasterTimeInaccuracy"); > p->pwr.networkTimeInaccuracy = > - config_get_int(cfg, p->name, > "power_profile.2011.networkTimeInaccuracy"); > + config_get_uint(cfg, p->name, > "power_profile.2011.networkTimeInaccuracy"); > p->pwr.totalTimeInaccuracy = > - config_get_int(cfg, p->name, > "power_profile.2017.totalTimeInaccuracy"); > + config_get_uint(cfg, p->name, > "power_profile.2017.totalTimeInaccuracy"); > p->slave_event_monitor = clock_slave_monitor(clock); > > if (!port_is_uds(p) && unicast_client_initialize(p)) { > -- > 2.40.0.131.gc918699d9952 > > > > _______________________________________________ > Linuxptp-devel mailing list > Linuxptp-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linuxptp-devel >
_______________________________________________ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel