>-----Original Message-----
>From: Miroslav Lichvar <mlich...@redhat.com>
>Sent: Tuesday, May 3, 2022 11:35 AM
>To: Kubalewski, Arkadiusz <arkadiusz.kubalew...@intel.com>
>Cc: linuxptp-devel@lists.sourceforge.net; Kwapulinski, Piotr
><piotr.kwapulin...@intel.com>; Sawula, Andrzej <andrzej.saw...@intel.com>;
>Gerasymenko, Anatolii <anatolii.gerasyme...@intel.com>
>Subject: 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.
Makes perfect sense, will fix it.
>
>> +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.
Agree, will fix it.
>
>> +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?
That's true, will fix it.
>
>And shouldn't be there some locking between the threads, e.g. with
>mutexes?
Sure, we can introduce locks on accessing child-threads data.
Benefits from introducing locks:
- TX thread - almost instant reaction on rebuild request
- RX thread - almost instant reaction on new QL received
Initially we were not sure if increasing complexity of threads is worth the
benefits. ESMC is a slow protocol and the Synchronous Ethernet standard
describes automatic adjustment of the clock recovered from one of the ports
while feeding its clock to the others, but SyncE-enabled network configuration
is rather static and change of QL would happen rarely (mostly due to errors,
loosing proper reference signal on the SyncE-leader).
As we can see some benefits from having locks in place, I will add those
in the next patch version.
Thank you,
Arkadiusz
>
>--
>Miroslav Lichvar
>
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
