Re: [Linuxptp-devel] [PATCH] Return 1 from port_is_ieee8021as if asCapable true
On Tue, Mar 14, 2023 at 07:42:19PM +0100, Andrew Zaborowski wrote: > On Tue, 7 Mar 2023 at 16:35, Richard Cochran wrote: > > On Tue, Mar 07, 2023 at 04:21:52PM +0100, Andrew Zaborowski wrote: > > > The (minor) problem this attempts to solve, and I didn't state that, > > > is the confusing semantics and reduced utility of port_is_ieee8021as > > > if one relies on the name. > > > > So there is no bug. Just the code is confusing, right? Then you must > > ensure that the patch does not actually change the program's behavior. > > Whether it's a bug depends on what users expect from asCapable=true. > Or if it has any users in the first place, but it is present in some > of the shipped config files. IIRc the whole (and only) point of asCapable=1 is too circumvent the normal port qualification logic in 802.1as, just for the "automotive" profile. > With asCapable=true your PdelayReq messages have their > header.logMessageInterval set to 0x7f. 802.1AS says it should be set > to the value of currentLogPdelayReqInterval. > currentLogPdelayReqInterval of 0x7f means that no PdelayReqs should be > sent, so in theory no PdelayReq with the value of 0x7f should go out. But the automotive profile doesn't care about this. Thanks, Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] Return 1 from port_is_ieee8021as if asCapable true
On Tue, 7 Mar 2023 at 16:35, Richard Cochran wrote: > On Tue, Mar 07, 2023 at 04:21:52PM +0100, Andrew Zaborowski wrote: > > The (minor) problem this attempts to solve, and I didn't state that, > > is the confusing semantics and reduced utility of port_is_ieee8021as > > if one relies on the name. > > So there is no bug. Just the code is confusing, right? Then you must > ensure that the patch does not actually change the program's behavior. Whether it's a bug depends on what users expect from asCapable=true. Or if it has any users in the first place, but it is present in some of the shipped config files. With asCapable=true your PdelayReq messages have their header.logMessageInterval set to 0x7f. 802.1AS says it should be set to the value of currentLogPdelayReqInterval. currentLogPdelayReqInterval of 0x7f means that no PdelayReqs should be sent, so in theory no PdelayReq with the value of 0x7f should go out. But this can worked around outside port_is_ieee8021as. If there are users of asCapable=true, such change is more likely to make their setup compliant than non-compliant -- that's a better wording than "more compliant". This is assuming that none of their other settings break relevant specs. Best regards ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] Return 1 from port_is_ieee8021as if asCapable true
On Tue, Mar 07, 2023 at 04:21:52PM +0100, Andrew Zaborowski wrote: > The (minor) problem this attempts to solve, and I didn't state that, > is the confusing semantics and reduced utility of port_is_ieee8021as > if one relies on the name. So there is no bug. Just the code is confusing, right? Then you must ensure that the patch does not actually change the program's behavior. Thanks, Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] Return 1 from port_is_ieee8021as if asCapable true
On Tue, 7 Mar 2023 at 07:33, Richard Cochran wrote: > On Tue, Mar 07, 2023 at 12:54:55AM +0100, Andrew Zaborowski wrote: > > It seems that port_is_ieee8021as(p) returning zero if p->as_capable == > > ALWAYS_CAPABLE was originally intended for skipping checks in > > port_capable but this is quite unclear. > > Maybe you should ask your Intel colleague what he had in mind? > > > There are only three callers > > left in the current code and this quirk doesn't seem to make them any > > more compliant with either 802.1AS version or the Automotive profile. > > More compliant? Either something complies or not. > > > On the other hand > > Where was the first hand? > > > the calculation of neighborRateRatio (in the Rx path) > > can now depend on the return value of port_is_ieee8021as() which seems > > to have been the intention, rather than on whether p->follow_up_info is > > set. The description of follow_up_info in the man page implies it > > rather affects the Tx path. > > It can be used on the client. > > > (The NRR will also need to be calculated > > in a future CMLDS instance, with or without 802.1AS, so this if clause > > will change again). > > Sorry, but this is completely incomprehensible. > > A proper commit message has three elements: > > 1. context > 2. problem > 3. solution Right, the commit message wasn't great after editing it a few times, sorry. The (minor) problem this attempts to solve, and I didn't state that, is the confusing semantics and reduced utility of port_is_ieee8021as if one relies on the name. Best regards ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] Return 1 from port_is_ieee8021as if asCapable true
On Tue, Mar 07, 2023 at 12:54:55AM +0100, Andrew Zaborowski wrote: > It seems that port_is_ieee8021as(p) returning zero if p->as_capable == > ALWAYS_CAPABLE was originally intended for skipping checks in > port_capable but this is quite unclear. Maybe you should ask your Intel colleague what he had in mind? > There are only three callers > left in the current code and this quirk doesn't seem to make them any > more compliant with either 802.1AS version or the Automotive profile. More compliant? Either something complies or not. > On the other hand Where was the first hand? > the calculation of neighborRateRatio (in the Rx path) > can now depend on the return value of port_is_ieee8021as() which seems > to have been the intention, rather than on whether p->follow_up_info is > set. The description of follow_up_info in the man page implies it > rather affects the Tx path. It can be used on the client. > (The NRR will also need to be calculated > in a future CMLDS instance, with or without 802.1AS, so this if clause > will change again). Sorry, but this is completely incomprehensible. A proper commit message has three elements: 1. context 2. problem 3. solution You need to clearly state the problem. (no) thanks, Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH] Return 1 from port_is_ieee8021as if asCapable true
It seems that port_is_ieee8021as(p) returning zero if p->as_capable ==
ALWAYS_CAPABLE was originally intended for skipping checks in
port_capable but this is quite unclear. There are only three callers
left in the current code and this quirk doesn't seem to make them any
more compliant with either 802.1AS version or the Automotive profile.
On the other hand the calculation of neighborRateRatio (in the Rx path)
can now depend on the return value of port_is_ieee8021as() which seems
to have been the intention, rather than on whether p->follow_up_info is
set. The description of follow_up_info in the man page implies it
rather affects the Tx path. (The NRR will also need to be calculated
in a future CMLDS instance, with or without 802.1AS, so this if clause
will change again).
Signed-off-by: Andrew Zaborowski
---
Vinicius Gomes and myself tried to analyze configurations in which this
behavior may be required but we didn't find any. The description of
asCapable=true in the man page doesn't talk about non-standard
behavior. A comment in as_capable.h does but it suggests this is in
order to better support the Automotive Profile which is not consistent
with port_is_ieee8021as() returning 0.
There's a short discussion in
https://github.com/nxp-archive/openil_linuxptp/issues/16
(non-conclusive)
---
port.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/port.c b/port.c
index 76f817c..5e9bd6f 100644
--- a/port.c
+++ b/port.c
@@ -852,7 +852,7 @@ static int port_sync_incapable(struct port *p)
static int port_is_ieee8021as(struct port *p)
{
if (p->asCapable == ALWAYS_CAPABLE) {
- return 0;
+ return 1;
}
return p->follow_up_info ? 1 : 0;
}
@@ -2435,7 +2435,7 @@ static void port_peer_delay(struct port *p)
calc:
t3c = tmv_add(t3, tmv_add(c1, c2));
- if (p->follow_up_info)
+ if (port_is_ieee8021as(p))
port_nrate_calculate(p, t3c, t4);
tsproc_set_clock_rate_ratio(p->tsproc, p->nrate.ratio *
--
2.34.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
