On 3/29/2023 1:42 PM, Erez wrote:
> 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
>
I have no idea of the values being stored need to be 32bit or not.
However, I think we can migrate our internal "config value storage" to a
signed 64bit to allow us to keep track of all the possible unsigned
32bit values. We could make CFG_TYPE_INT be a s64 and then just have the
range check limit to 32bits without needing to add an extra unsigned
type. (unless we ever want to have a full 64bit unsigned value..)
That's probably better than this patch.
Thanks,
Jake
>>
>> 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
