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

Reply via email to