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.
> +
> + pr_debug("tx thread started on port %s", cd->name);
> + *state = 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.
> +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.
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.
> +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".
--
Miroslav Lichvar
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
