On Tue, Jun 14, 2022 at 05:58:13AM -0700, Richard Cochran wrote:
> On Tue, Nov 23, 2021 at 02:14:16AM +0200, Vladimir Oltean wrote:
>
> > +static enum port_state ts2phc_normalize_state(enum port_state state)
> > +{
> > + if (state != PS_MASTER && state != PS_SLAVE &&
> > + state != PS_PRE_MASTER && state != PS_UNCALIBRATED) {
> > + /* treat any other state as "not a master nor a slave" */
> > + state = PS_DISABLED;
> > + }
> > + return state;
> > +}
>
> Please make this explicit by using switch/case over all the enumerated values.
I'm having trouble applying this feedback.
I assume you mean something like this?
/**
* Reduce all port states for which the sync direction isn't known to
* PS_DISABLED, and report the given port state otherwise. This minimizes port
* state transitions for PMC agents when nothing interesting happened.
*/
enum port_state port_state_normalize(enum port_state state)
{
switch (state) {
case PS_MASTER:
case PS_SLAVE:
case PS_PRE_MASTER:
case PS_UNCALIBRATED:
return state;
case PS_INITIALIZING:
case PS_FAULTY:
case PS_DISABLED:
case PS_LISTENING:
case PS_PASSIVE:
case PS_GRAND_MASTER:
default:
return PS_DISABLED;
}
}
Can't I just delete "case PS_INITIALIZING:" ... "case PS_GRAND_MASTER:"
since I have to put "default:" anyway (we may get arbitrary input over
the management messages)?
Anyway, it seems silly to treat PS_GRAND_MASTER as part of this helper,
since we're not expected to receive it over management messages. It's as
if PMC agents and ptp4l should have their own separate enum port_state.
I'm also wondering if PS_PRE_MASTER and PS_UNCALIBRATED (which are
transient states) couldn't be treated as PS_DISABLED too (as an
admittedly unsolicited change).
Also, I'd like to share this helper between phc2sys and ts2phc.
Is fsm.{c,h} a good place to put it?
> > +static int auto_init_ports(struct ts2phc_private *priv)
> > +{
> > + int number_ports, timestamping, err;
> > + struct ts2phc_clock *clock;
> > + struct ts2phc_port *port;
> > + enum port_state state;
> > + char iface[IFNAMSIZ];
> > + unsigned int i;
> > +
> > + while (1) {
> > + if (!is_running())
> > + return -1;
> > + err = pmc_agent_query_dds(priv->agent, 1000);
> > + if (!err)
> > + break;
> > + if (err == -ETIMEDOUT)
> > + pr_notice("Waiting for ptp4l...");
> > + else
> > + return -1;
> > + }
> > +
> > + number_ports = pmc_agent_get_number_ports(priv->agent);
> > + if (number_ports <= 0) {
> > + pr_err("failed to get number of ports");
> > + return -1;
> > + }
> > +
> > + err = pmc_agent_subscribe(priv->agent, 1000);
> > + if (err) {
> > + pr_err("failed to subscribe");
> > + return -1;
> > + }
> > +
> > + for (i = 1; i <= number_ports; i++) {
> > + err = pmc_agent_query_port_properties(priv->agent, 1000, i,
> > + &state, ×tamping,
> > + iface);
> > + if (err == -ENODEV) {
> > + /* port does not exist, ignore the port */
> > + continue;
> > + }
> > + if (err) {
> > + pr_err("failed to get port properties");
> > + return -1;
> > + }
> > + if (timestamping == TS_SOFTWARE) {
> > + /* ignore ports with software time stamping */
> > + continue;
> > + }
> > + port = ts2phc_port_add(priv, i, iface);
> > + if (!port)
> > + return -1;
> > + port->state = ts2phc_normalize_state(state);
> > + }
> > + if (LIST_EMPTY(&priv->clocks)) {
> > + pr_err("no suitable ports available");
> > + return -1;
> > + }
> > + LIST_FOREACH(clock, &priv->clocks, list) {
> > + clock->new_state = ts2phc_clock_compute_state(priv, clock);
> > + }
> > +
> > + return 0;
> > +}
>
> This is very similar to the code in phc2sys. Can't you refactor to
> share that logic?
The problem is that, even though the logic is identical, the data
structures are not ("struct clock" vs "struct ts2phc_clock",
"struct port" vs "struct ts2phc_port").
I could probably add micro helper blocks to pmc_agent.c like
pmc_agent_wait_ptp4l() and pmc_agent_enumerate_port_properties(), the
latter having an int (*cb) for each MID_PORT_PROPERTIES_NP response.
Is that in line with what you're thinking?
_______________________________________________
Linuxptp-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
