Re: [Linuxptp-devel] [PATCH 01/11] synce4l: add config knobs for SyncE
Thanks for your work on this feature. I don't have hardware to test
it. Just some comments on the code.
On Mon, May 02, 2022 at 11:05:55AM +0200, Arkadiusz Kubalewski wrote:
> Allow config interface to parse SyncE related config files.
>
> + if (type == PTP) {
> + ci_tab = &config_tab_ptp[0];
> + n_items = N_CONFIG_ITEMS_PTP;
> + } else {
> + ci_tab = &config_tab_synce[0];
> + n_items = N_CONFIG_ITEMS_SYNCE;
> + }
> + n_items = (type == PTP ? N_CONFIG_ITEMS_PTP : N_CONFIG_ITEMS_SYNCE);
This looks duplicated. The last line shouldn't be there?
> +# Shell command to be executed in order to obtain current DPLL status of a
> +# device.
> +#
> +dpll_get_state_cmd cat /sys/class/net/enp1s0f0/device/cgu_state
If I understand it correctly, this shell command is executed 50 times
per second. Wouldn't it be sufficient to specify it as a file to be
read directly instead instead of calling popen()?
I assume this is a temporary solution until the kernel provides a
standard ethtool/netlink API for monitoring and configuring SyncE.
--
Miroslav Lichvar
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 03/11] synce4l: add esmc_socket interface
On Mon, May 02, 2022 at 11:05:57AM +0200, Arkadiusz Kubalewski wrote:
> Add interface for sending and receiving ESMC frames.
> Ethernet Synchronization Messaging Channel (ESMC) requires
> to operate on multicast raw L2 socket. This patch adds features
> to open socket, send and receive ESMC frames.
> +++ b/esmc_socket.c
> @@ -0,0 +1,111 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
linuxptp is GPLv2+, so I think the new code should be submitted under
the same or at least compatible license to not build binaries with
different licenses.
> +int recv_raw_esmc_frame(int socket, void *buff)
> +{
> + return recv(socket, buff, ETH_DATA_LEN, 0);
> +}
This looks dangerous to me. I'd suggest to add the length of the
buffer as an argument, or maybe pass it as esmc_data and sizeof the
data field.
--
Miroslav Lichvar
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 08/11] synce4l: add synce_port_ctrl interface
On Mon, May 02, 2022 at 11:06:02AM +0200, Arkadiusz Kubalewski wrote:
> Used by synce_port to create, control and destroy RX/TX threads.
> Ports in sync mode can use only TX thread (device in external input
> mode) or both RX and TX threads (device in internal input mode).
> +++ b/synce_port_ctrl.c
> @@ -0,0 +1,1077 @@
> +static int init_ql_str(struct allowed_qls_head *qls_stailq_head,
> +const char *allowed_qls)
> +{
...
> + ptr = allowed_qls;
> + while (*ptr) {
> + unsigned long value;
> + struct ql *newql;
> +
> + if (ptr > allowed_qls + allowed_qls_len) {
> + break;
> + }
This check needs to be performed before the "*ptr" in the while
condition to avoid invalid memory access.
> +static int init_allowed_ext_qls(struct synce_port_rx *rx, struct config *cfg,
> + const char *name)
> +{
> + const char *allowed_qls;
> +
> + STAILQ_INIT((struct allowed_qls_head *)&rx->allowed_ext_qls);
The casting looks wrong/unnecessary, or at least it is causing
compiler warnings about breaking strict aliasing.
> +static int thread_start_wait(struct thread_common_data *cd)
> +{
> + int cnt = (cd->heartbeat_usec / THREAD_STOP_SLEEP_USEC) + 1;
> +
> + if (cd->state == THREAD_STARTED) {
> + pr_debug("THREAD_STARTED");
> +
> + return 0;
> + }
> +
> + while (cnt-- && cd->state != THREAD_STARTED) {
> + usleep(THREAD_STOP_SLEEP_USEC);
> + }
This should be THREAD_START_SLEEP_USEC?
And shouldn't be there some locking between the threads, e.g. with
mutexes?
--
Miroslav Lichvar
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 10/11] synce4l: add synce_transport interface
On Mon, May 02, 2022 at 11:06:04AM +0200, Arkadiusz Kubalewski wrote:
> Interface is used to create and store an opened socket for sending and
> receiving SyncE PDU's on a given NIC interface.
> +++ b/synce_transport.c
> +struct synce_transport *synce_transport_create(const char *iface)
> +{
...
> + if (strnlen(iface, IFNAMSIZ) == IFNAMSIZ) {
> + pr_err("interface name too long");
> + goto err;
> + }
> + strncpy(transport->iface, iface,
> + (sizeof(transport->iface)/sizeof(*transport->iface)));
That division looks odd here as strncpy works with chars.
Anyway, this code seems to be confusing the compiler, assuming it can
produce an unterminated string, so maybe just use
"if (snprintf(...) >= IFNAMSIZ)" instead of strnlen() + strncpy()?
--
Miroslav Lichvar
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 11/11] synce4l: add synce4l application
On Mon, May 02, 2022 at 11:06:05AM +0200, Arkadiusz Kubalewski wrote: > Add main code of synce4l - user space application implementing > standard - Recommendation ITU-T G.8264/Y.1364. > On init the application parses SyncE config file and initializes > a synce_clock with SyncE devices and configured ports. > synce_clock is later polled until application termination. > > Add documentation in form of Linux manual and 'synce4l_README.md'. > +++ b/synce4l.8 > @@ -0,0 +1,239 @@ > +.IP external_input_ext_QL > +Extended Quality Level for "external input" mode. > +.P > +.RS > +Extended Quality Level for "external input" mode. This sentence is duplicated. -- Miroslav Lichvar ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 03/11] synce4l: add esmc_socket interface
On Tue, 3 May 2022 at 11:25, Miroslav Lichvar wrote:
> On Mon, May 02, 2022 at 11:05:57AM +0200, Arkadiusz Kubalewski wrote:
> > Add interface for sending and receiving ESMC frames.
> > Ethernet Synchronization Messaging Channel (ESMC) requires
> > to operate on multicast raw L2 socket. This patch adds features
> > to open socket, send and receive ESMC frames.
>
> > +++ b/esmc_socket.c
> > @@ -0,0 +1,111 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
>
> linuxptp is GPLv2+, so I think the new code should be submitted under
> the same or at least compatible license to not build binaries with
> different licenses.
>
I agree, new code should use the same license: "GPL-2.0+"
We could also add the "SPDX-FileCopyrightText:" tag.
It helps finding the copyright with the license per file :-)
Maybe it is time to update all the source headers to a format like:
/**
* @file XXX.c
* @brief X
* @note SPDX-FileCopyrightText: (C) 20XX NN FF
* @note SPDX-License-Identifier: GPL-2.0+
*/
Seems we are not uniform on that.
I think each copyright holder should update its own sources.
Erez
>
> > +int recv_raw_esmc_frame(int socket, void *buff)
> > +{
> > + return recv(socket, buff, ETH_DATA_LEN, 0);
> > +}
>
> This looks dangerous to me. I'd suggest to add the length of the
> buffer as an argument, or maybe pass it as esmc_data and sizeof the
> data field.
>
> --
> Miroslav Lichvar
>
>
>
> ___
> 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
