-----Original Message-----
From: Miroslav Lichvar <mlich...@redhat.com>
Sent: Wednesday, June 1, 2022 11:52 AM
> On Mon, May 30, 2022 at 09:34:16PM +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).
>
> > +static void *tx_thread(void *data)
> > +{
>
> > + cd = &tx->cd;
> > + state = &cd->state;
> > + if (*state != THREAD_NOT_STARTED) {
> > + pr_err("tx wrong state");
> > + goto out;
> > + }
>
> I think the access to cd->state here and in other places needs to be
> protected by the lock too.
Sure, can wrap it with setter and getter, and lock it inside.
>
> > +
> > + pr_debug("tx thread started on port %s", cd->name);
> > + * = THREAD_STARTED;
> > + while (*state == THREAD_STARTED) {
> > + if (lock_mutex(cd, __func__) == 0) {
> > + if (tx->rebuild_tlv) {
> > + if (tx_rebuild_tlv(tx)) {
> > + pr_err("tx rebuild failed");
> > + unlock_mutex(cd, __func__);
> > + goto out;
> > + }
> > + }
> > + unlock_mutex(cd, __func__);
> > + }
> > +
> > + /* any errors are traced inside */
> > + if (cd->enabled) {
>
> Same for cd->enabled.
The cd->enabled is set only or init, we don't expect it will change in runtime,
looks like there is no point to check it under the lock.
>
> > +static int rx_act(struct synce_port_rx *rx)
> > +{
> > + struct thread_common_data *cd = &rx->cd;
> > + struct synce_msg_ext_ql ext_ql;
> > + struct timespec now;
> > + uint8_t ql;
> > + int err;
> > +
> > + /* read socket for ESMC and fill pdu */
> > + err = synce_transport_recv_pdu(cd->transport, cd->pdu);
> > + if (!err) {
> > + rx->n_recv++;
> > + }
>
> And same for rx->n_recv and possibly other fields.
Actually rx_act function is called under lock, but we read n_recv in different
places, will try to fix in those places to have it under the lock as well.
>
> Maybe it would be simpler to keep everything locked by default and
> unlock only at some specific points, e.g. when sleeping or waiting for
> IO.
Well, in that case maybe it would be probably better to just get rid of
separated threads and peroform those tasks in synchronous manner?
>
> > +int synce_port_ctrl_init(struct synce_port_ctrl *pc, struct config *cfg,
> > + int rx_enabled, int extended_tlv, int recover_time,
> > + int network_option)
> > +{
>
> > + if (thread_start_wait(&pc->rx.cd)) {
> > + pr_err("tx thread start wait failed for %s", pc->name);
>
> Typo here in "tx".
yes, good catch, will fix
Thanks,
Arkadiusz
>
> --
> Miroslav Lichvar
>
>
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
