From e243c6b7dbd814322fa7bae54c7a6421866ab190 Mon Sep 17 00:00:00 2001 From: Esben Haabendal Date: Thu, 24 Jun 2021 13:38:04 +0200 Subject: [PATCH 1/4] Clarify purpose of master->io_sem In order to ensure proper use of locking, the purpose of each mutex/semaphore must be clear and documented. This is my best guess. The master->io_sem has a simple purpose, synchronizing access to the master->datagram_queue list structure. --- master/master.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/master/master.h b/master/master.h index 89debd1f..c51d79fb 100644 --- a/master/master.h +++ b/master/master.h @@ -260,6 +260,7 @@ struct ec_master { struct list_head datagram_queue; /**< Datagram queue. */ uint8_t datagram_index; /**< Current datagram index. */ + ec_lock_t io_sem; /**< Semaphore protecting the datagram_queue. */ struct list_head ext_datagram_queue; /**< Queue for non-application datagrams. */ @@ -290,8 +291,6 @@ struct ec_master { struct list_head eoe_handlers; /**< Ethernet over EtherCAT handlers. */ #endif - ec_lock_t io_sem; /**< Semaphore used in \a IDLE phase. */ - void (*send_cb)(void *); /**< Current send datagrams callback. */ void (*receive_cb)(void *); /**< Current receive datagrams callback. */ void *cb_data; /**< Current callback data. */ From 2372382259be9ecbe78b3bec256066bc33ee4c2e Mon Sep 17 00:00:00 2001 From: Esben Haabendal Date: Thu, 24 Jun 2021 13:45:08 +0200 Subject: [PATCH 2/4] Acquire master->io_sem lock where needed With this, we should have in place so that master->io_sem is always held when accessing master->datagram_queue. Note, the use of master->master_sem in ec_ioctl_send() and ec_ioctl_receive() were wrong (introduced in commit cef602586cd3). The only thing needing to be synchronized there is the access to master->datagram_queue, which is protected by master->io_sem. This corresponds roughly to the fixes for stable-1.5 that are seen in https://gitlab.com/etherlab.org/ethercat/-/merge_requests/36 . --- master/ioctl.c | 34 ++++++++++++++++++++++++++++------ master/master.c | 8 ++++---- 2 files changed, 32 insertions(+), 10 deletions(-) diff --git a/master/ioctl.c b/master/ioctl.c index 04c1ee28..957b9aa7 100755 --- a/master/ioctl.c +++ b/master/ioctl.c @@ -2112,7 +2112,7 @@ static ATTRIBUTES int ec_ioctl_send( /* Locking added as send is likely to be used by more than one application tasks */ - if (ec_ioctl_lock_down_interruptible(&master->master_sem)) + if (ec_ioctl_lock_down_interruptible(&master->io_sem)) return -EINTR; if (master->send_cb != NULL) { @@ -2121,7 +2121,7 @@ static ATTRIBUTES int ec_ioctl_send( } else sent_bytes = ecrt_master_send(master); - ec_ioctl_lock_up(&master->master_sem); + ec_ioctl_lock_up(&master->io_sem); if (copy_to_user((void __user *) arg, &sent_bytes, sizeof(sent_bytes))) { return -EFAULT; @@ -2148,7 +2148,7 @@ static ATTRIBUTES int ec_ioctl_receive( /* Locking added as receive is likely to be used by more than one application tasks */ - if (ec_ioctl_lock_down_interruptible(&master->master_sem)) + if (ec_ioctl_lock_down_interruptible(&master->io_sem)) return -EINTR; if (master->receive_cb != NULL) @@ -2156,7 +2156,8 @@ static ATTRIBUTES int ec_ioctl_receive( else ecrt_master_receive(master); - ec_ioctl_lock_up(&master->master_sem); + ec_ioctl_lock_up(&master->io_sem); + return 0; } @@ -2255,7 +2256,10 @@ static ATTRIBUTES int ec_ioctl_sync_ref( return -EPERM; } + if (ec_ioctl_lock_down_interruptible(&master->io_sem)) + return -EINTR; ecrt_master_sync_reference_clock(master); + ec_ioctl_lock_up(&master->io_sem); return 0; } @@ -2280,7 +2284,10 @@ static ATTRIBUTES int ec_ioctl_sync_ref_to( return -EFAULT; } + if (ec_ioctl_lock_down_interruptible(&master->io_sem)) + return -EINTR; ecrt_master_sync_reference_clock_to(master, time); + ec_ioctl_lock_up(&master->io_sem); return 0; } @@ -2300,7 +2307,10 @@ static ATTRIBUTES int ec_ioctl_sync_slaves( return -EPERM; } + if (ec_ioctl_lock_down_interruptible(&master->io_sem)) + return -EINTR; ecrt_master_sync_slave_clocks(master); + ec_ioctl_lock_up(&master->io_sem); return 0; } @@ -2351,7 +2361,10 @@ static ATTRIBUTES int ec_ioctl_64bit_ref_clock_time_queue( return -EPERM; } + if (ec_ioctl_lock_down_interruptible(&master->io_sem)) + return -EINTR; ecrt_master_64bit_reference_clock_time_queue(master); + ec_ioctl_lock_up(&master->io_sem); return 0; } @@ -2402,7 +2415,10 @@ static ATTRIBUTES int ec_ioctl_sync_mon_queue( return -EPERM; } + if (ec_ioctl_lock_down_interruptible(&master->io_sem)) + return -EINTR; ecrt_master_sync_monitor_queue(master); + ec_ioctl_lock_up(&master->io_sem); return 0; } @@ -3515,10 +3531,13 @@ static ATTRIBUTES int ec_ioctl_domain_queue( return -ENOENT; } - ecrt_domain_queue(domain); - ec_ioctl_lock_up(&master->master_sem); + if (ec_ioctl_lock_down_interruptible(&master->io_sem)) + return -EINTR; + ecrt_domain_queue(domain); + ec_ioctl_lock_up(&master->io_sem); + return 0; } @@ -4236,7 +4255,10 @@ static ATTRIBUTES int ec_ioctl_voe_exec( return -ENOENT; } + if (ec_ioctl_lock_down_interruptible(&master->io_sem)) + return -EINTR; data.state = ecrt_voe_handler_execute(voe); + ec_ioctl_lock_up(&master->io_sem); if (data.state == EC_REQUEST_SUCCESS && voe->dir == EC_DIR_INPUT) data.size = ecrt_voe_handler_data_size(voe); else diff --git a/master/master.c b/master/master.c index 8ee2ec56..bfaa893e 100644 --- a/master/master.c +++ b/master/master.c @@ -564,9 +564,7 @@ void ec_master_internal_send_cb( ) { ec_master_t *master = (ec_master_t *) cb_data; - ec_lock_down(&master->io_sem); ecrt_master_send_ext(master); - ec_lock_up(&master->io_sem); } /*****************************************************************************/ @@ -578,9 +576,7 @@ void ec_master_internal_receive_cb( ) { ec_master_t *master = (ec_master_t *) cb_data; - ec_lock_down(&master->io_sem); ecrt_master_receive(master); - ec_lock_up(&master->io_sem); } /*****************************************************************************/ @@ -1889,7 +1885,9 @@ static int ec_master_eoe_thread(void *priv_data) } // receive datagrams + ec_lock_down(&master->io_sem); master->receive_cb(master->cb_data); + ec_lock_up(&master->io_sem); // actual EoE processing sth_to_send = 0; @@ -1904,9 +1902,11 @@ static int ec_master_eoe_thread(void *priv_data) } if (sth_to_send) { + ec_lock_down(&master->io_sem); list_for_each_entry(eoe, &master->eoe_handlers, list) { ec_eoe_queue(eoe); } + ec_lock_up(&master->io_sem); // (try to) send datagrams ec_lock_down(&master->ext_queue_sem); master->send_cb(master->cb_data); From 19b8ddea18cf6265858dae4b8657690582b4882c Mon Sep 17 00:00:00 2001 From: Esben Haabendal Date: Wed, 17 Jan 2018 09:30:50 +0100 Subject: [PATCH 3/4] Add lock protection of domain->datagram_pairs While domain->datagram_pairs was previously partly protected by the master->master_sem lock (it was not done consistently though), the fix to protection of master->datagram_queue made it even worse. This change adds protection of domain->datagram_pairs using a new lock dedicated to this. The domain->datagram_pairs_lock is acquired in the actual functions accessing the domain->datagram_pairs, and this currently means that it is acquired both in situations where master->master_sem is held and in situations where master->io_sem is held. --- master/domain.c | 11 +++++++++++ master/domain.h | 3 +++ 2 files changed, 14 insertions(+) diff --git a/master/domain.c b/master/domain.c index 79b815dd..b303ea97 100755 --- a/master/domain.c +++ b/master/domain.c @@ -76,6 +76,7 @@ void ec_domain_init( domain->data_origin = EC_ORIG_INTERNAL; domain->logical_base_address = 0x00000000; INIT_LIST_HEAD(&domain->datagram_pairs); + ec_lock_init(&domain->datagram_pairs_lock); for (dev_idx = EC_DEVICE_MAIN; dev_idx < ec_master_num_devices(master); dev_idx++) { domain->working_counter[dev_idx] = 0x0000; @@ -98,12 +99,14 @@ void ec_domain_clear(ec_domain_t *domain /**< EtherCAT domain */) { ec_datagram_pair_t *datagram_pair, *next_pair; + ec_lock_down(&domain->datagram_pairs_lock); // dequeue and free datagrams list_for_each_entry_safe(datagram_pair, next_pair, &domain->datagram_pairs, list) { ec_datagram_pair_clear(datagram_pair); kfree(datagram_pair); } + ec_lock_up(&domain->datagram_pairs_lock); ec_domain_clear_data(domain); } @@ -216,7 +219,9 @@ int ec_domain_add_datagram_pair( datagram_pair->expected_working_counter); + ec_lock_down(&domain->datagram_pairs_lock); list_add_tail(&datagram_pair->list, &domain->datagram_pairs); + ec_lock_up(&domain->datagram_pairs_lock); return 0; } @@ -389,6 +394,7 @@ int ec_domain_finish( domain->logical_base_address, domain->data_size, domain->expected_working_counter); + ec_lock_down(&domain->datagram_pairs_lock); list_for_each_entry(datagram_pair, &domain->datagram_pairs, list) { const ec_datagram_t *datagram = &datagram_pair->datagrams[EC_DEVICE_MAIN]; @@ -397,6 +403,7 @@ int ec_domain_finish( EC_READ_U32(datagram->address), datagram->data_size, ec_datagram_type_string(datagram), datagram); } + ec_lock_up(&domain->datagram_pairs_lock); return 0; } @@ -551,6 +558,7 @@ void ecrt_domain_process(ec_domain_t *domain) EC_MASTER_DBG(domain->master, 1, "domain %u process\n", domain->index); #endif + ec_lock_down(&domain->datagram_pairs_lock); list_for_each_entry(pair, &domain->datagram_pairs, list) { #if EC_MAX_NUM_DEVICES > 1 datagram_pair_wc = ec_datagram_pair_process(pair, wc_sum); @@ -638,6 +646,7 @@ void ecrt_domain_process(ec_domain_t *domain) } #endif // EC_MAX_NUM_DEVICES > 1 } + ec_lock_up(&domain->datagram_pairs_lock); #if EC_MAX_NUM_DEVICES > 1 redundant_wc = 0; @@ -726,6 +735,7 @@ void ecrt_domain_queue(ec_domain_t *domain) ec_datagram_pair_t *datagram_pair; ec_device_index_t dev_idx; + ec_lock_down(&domain->datagram_pairs_lock); list_for_each_entry(datagram_pair, &domain->datagram_pairs, list) { #if EC_MAX_NUM_DEVICES > 1 @@ -747,6 +757,7 @@ void ecrt_domain_queue(ec_domain_t *domain) &datagram_pair->datagrams[dev_idx]); } } + ec_lock_up(&domain->datagram_pairs_lock); } /*****************************************************************************/ diff --git a/master/domain.h b/master/domain.h index 1170217f..3148a030 100755 --- a/master/domain.h +++ b/master/domain.h @@ -43,6 +43,7 @@ #include "datagram.h" #include "master.h" #include "fmmu_config.h" +#include "locks.h" /*****************************************************************************/ @@ -65,6 +66,8 @@ struct ec_domain process data. */ struct list_head datagram_pairs; /**< Datagrams pairs (main/backup) for process data exchange. */ + ec_lock_t datagram_pairs_lock; /**< Semaphore protecting the + datagram_pairs. */ uint16_t working_counter[EC_MAX_NUM_DEVICES]; /**< Last working counter values. */ uint16_t expected_working_counter; /**< Expected working counter. */ From 81758004fd4c243ac044ebbb9121ddeaed69f0f9 Mon Sep 17 00:00:00 2001 From: Esben Haabendal Date: Fri, 26 Jan 2018 12:05:10 +0100 Subject: [PATCH 4/4] Improve synchronization of master->domains list Introduce a new lock for synchronization of the master->domains list, and use that instead of the big master_sem lock, improving real-time performance of fx. ecrt_domain_process() when EoE thread is being shut down (or other use-cases where master_sem is held for extended periods). The following locking order is used: * master->master_sem * master->domains_lock * master->io_sem * domain->datagram_pairs_lock Meaning that it is allowed to take domains_lock while holding master_sem, and it is allowed to hold domains_lock while locking io_sem and datagram_pairs_lock. [RV: For now, the lockdep_assert_held() annotations are commented out, since they don't compile with CONFIG_PROVE_LOCKING and (the default) --disable-rtmutex.] --- master/domain.c | 7 ++-- master/ioctl.c | 107 ++++++++++++++++++++++++++++++------------------ master/master.c | 15 ++++--- master/master.h | 1 + 4 files changed, 82 insertions(+), 48 deletions(-) diff --git a/master/domain.c b/master/domain.c index b303ea97..b1c7aa5c 100755 --- a/master/domain.c +++ b/master/domain.c @@ -99,6 +99,7 @@ void ec_domain_clear(ec_domain_t *domain /**< EtherCAT domain */) { ec_datagram_pair_t *datagram_pair, *next_pair; + // lockdep_assert_held(&domain->master->domains_lock); ec_lock_down(&domain->datagram_pairs_lock); // dequeue and free datagrams list_for_each_entry_safe(datagram_pair, next_pair, @@ -119,6 +120,7 @@ void ec_domain_clear_data( ec_domain_t *domain /**< EtherCAT domain. */ ) { + // lockdep_assert_held(&domain->master->domains_lock); if (domain->data_origin == EC_ORIG_INTERNAL && domain->data) { kfree(domain->data); } @@ -519,14 +521,11 @@ void ecrt_domain_external_memory(ec_domain_t *domain, uint8_t *mem) EC_MASTER_DBG(domain->master, 1, "ecrt_domain_external_memory(" "domain = 0x%p, mem = 0x%p)\n", domain, mem); - ec_lock_down(&domain->master->master_sem); - + // lockdep_assert_held(&domain->master->domains_lock); ec_domain_clear_data(domain); domain->data = mem; domain->data_origin = EC_ORIG_EXTERNAL; - - ec_lock_up(&domain->master->master_sem); } /*****************************************************************************/ diff --git a/master/ioctl.c b/master/ioctl.c index 957b9aa7..95c3cefd 100755 --- a/master/ioctl.c +++ b/master/ioctl.c @@ -488,11 +488,11 @@ static ATTRIBUTES int ec_ioctl_domain( return -EFAULT; } - if (ec_lock_down_interruptible(&master->master_sem)) + if (ec_lock_down_interruptible(&master->domains_lock)) return -EINTR; if (!(domain = ec_master_find_domain_const(master, data.index))) { - ec_lock_up(&master->master_sem); + ec_lock_up(&master->domains_lock); EC_MASTER_ERR(master, "Domain %u does not exist!\n", data.index); return -EINVAL; } @@ -506,7 +506,7 @@ static ATTRIBUTES int ec_ioctl_domain( data.expected_working_counter = domain->expected_working_counter; data.fmmu_count = ec_domain_fmmu_count(domain); - ec_lock_up(&master->master_sem); + ec_lock_up(&master->domains_lock); if (copy_to_user((void __user *) arg, &data, sizeof(data))) return -EFAULT; @@ -533,18 +533,18 @@ static ATTRIBUTES int ec_ioctl_domain_fmmu( return -EFAULT; } - if (ec_lock_down_interruptible(&master->master_sem)) + if (ec_lock_down_interruptible(&master->domains_lock)) return -EINTR; if (!(domain = ec_master_find_domain_const(master, data.domain_index))) { - ec_lock_up(&master->master_sem); + ec_lock_up(&master->domains_lock); EC_MASTER_ERR(master, "Domain %u does not exist!\n", data.domain_index); return -EINVAL; } if (!(fmmu = ec_domain_find_fmmu(domain, data.fmmu_index))) { - ec_lock_up(&master->master_sem); + ec_lock_up(&master->domains_lock); EC_MASTER_ERR(master, "Domain %u has less than %u" " fmmu configurations.\n", data.domain_index, data.fmmu_index + 1); @@ -558,7 +558,7 @@ static ATTRIBUTES int ec_ioctl_domain_fmmu( data.logical_address = fmmu->domain->logical_base_address + fmmu->logical_domain_offset; data.data_size = fmmu->data_size; - ec_lock_up(&master->master_sem); + ec_lock_up(&master->domains_lock); if (copy_to_user((void __user *) arg, &data, sizeof(data))) return -EFAULT; @@ -584,18 +584,18 @@ static ATTRIBUTES int ec_ioctl_domain_data( return -EFAULT; } - if (ec_lock_down_interruptible(&master->master_sem)) + if (ec_lock_down_interruptible(&master->domains_lock)) return -EINTR; if (!(domain = ec_master_find_domain_const(master, data.domain_index))) { - ec_lock_up(&master->master_sem); + ec_lock_up(&master->domains_lock); EC_MASTER_ERR(master, "Domain %u does not exist!\n", data.domain_index); return -EINVAL; } if (domain->data_size != data.data_size) { - ec_lock_up(&master->master_sem); + ec_lock_up(&master->domains_lock); EC_MASTER_ERR(master, "Data size mismatch %u/%zu!\n", data.data_size, domain->data_size); return -EFAULT; @@ -603,11 +603,11 @@ static ATTRIBUTES int ec_ioctl_domain_data( if (copy_to_user((void __user *) data.target, domain->data, domain->data_size)) { - ec_lock_up(&master->master_sem); + ec_lock_up(&master->domains_lock); return -EFAULT; } - ec_lock_up(&master->master_sem); + ec_lock_up(&master->domains_lock); return 0; } @@ -1875,15 +1875,13 @@ static ATTRIBUTES int ec_ioctl_setup_domain_memory( ctx->process_data_size = 0; - if (ec_lock_down_interruptible(&master->master_sem)) + if (ec_lock_down_interruptible(&master->domains_lock)) return -EINTR; list_for_each_entry(domain, &master->domains, list) { ctx->process_data_size += ecrt_domain_size(domain); } - ec_lock_up(&master->master_sem); - if (ctx->process_data_size) { ctx->process_data = vmalloc(ctx->process_data_size); if (!ctx->process_data) { @@ -1901,6 +1899,8 @@ static ATTRIBUTES int ec_ioctl_setup_domain_memory( offset += ecrt_domain_size(domain); } + ec_lock_up(&master->domains_lock); + #ifdef EC_IOCTL_RTDM /* RTDM uses a different approach for memory-mapping, which has to be * initiated by the kernel. @@ -1912,6 +1912,8 @@ static ATTRIBUTES int ec_ioctl_setup_domain_memory( return ret; } #endif + } else { + ec_lock_up(&master->domains_lock); } io.process_data_size = ctx->process_data_size; @@ -1959,15 +1961,13 @@ static ATTRIBUTES int ec_ioctl_activate( ctx->process_data_size = 0; - if (ec_lock_down_interruptible(&master->master_sem)) + if (ec_lock_down_interruptible(&master->domains_lock)) return -EINTR; list_for_each_entry(domain, &master->domains, list) { ctx->process_data_size += ecrt_domain_size(domain); } - ec_lock_up(&master->master_sem); - if (ctx->process_data_size) { ctx->process_data = vmalloc(ctx->process_data_size); if (!ctx->process_data) { @@ -1998,6 +1998,8 @@ static ATTRIBUTES int ec_ioctl_activate( #endif } + ec_lock_up(&master->domains_lock); + io.process_data_size = ctx->process_data_size; } else @@ -2775,12 +2777,24 @@ static ATTRIBUTES int ec_ioctl_sc_reg_pdo_entry( return -ENOENT; } + ec_lock_up(&master->master_sem); + + if (ec_lock_down_interruptible(&master->domains_lock)) + return -EINTR; if (!(domain = ec_master_find_domain(master, data.domain_index))) { - ec_lock_up(&master->master_sem); + ec_lock_up(&master->domains_lock); return -ENOENT; } + ec_lock_up(&master->domains_lock); - ec_lock_up(&master->master_sem); /** \todo sc or domain could be invalidated */ + /** \todo sc or domain could be invalidated */ + + /* FIXME: As ecrt_slave_config_reg_pdo_entry() calls + * ec_slave_config_prepare_fmmu() which locks master_sem we must release + * domains_lock, as it would break lock ordering, and thus cause + * deadlocks. Unfortunately, this leaves a race condition, as the domain + * could be cleared/deleted while ecrt_slave_config_reg_pdo_entry() is + * called. So the above todo statement still holds :( */ ret = ecrt_slave_config_reg_pdo_entry(sc, data.entry_index, data.entry_subindex, domain, &data.bit_position); @@ -2824,13 +2838,24 @@ static ATTRIBUTES int ec_ioctl_sc_reg_pdo_pos( ec_lock_up(&master->master_sem); return -ENOENT; } + ec_lock_up(&master->master_sem); + if (ec_lock_down_interruptible(&master->domains_lock)) + return -EINTR; if (!(domain = ec_master_find_domain(master, io.domain_index))) { - ec_lock_up(&master->master_sem); + ec_lock_up(&master->domains_lock); return -ENOENT; } + ec_lock_up(&master->domains_lock); - ec_lock_up(&master->master_sem); /** \todo sc or domain could be invalidated */ + /** \todo sc or domain could be invalidated */ + + /* FIXME: As ecrt_slave_config_reg_pdo_entry_pos() calls + * ec_slave_config_prepare_fmmu() which locks master_sem we must release + * domains_lock, as it would break lock ordering, and thus cause + * deadlocks. Unfortunately, this leaves a race condition, as the domain + * could be cleared/deleted while ecrt_slave_config_reg_pdo_entry() is + * called. So the above todo statement still holds :( */ ret = ecrt_slave_config_reg_pdo_entry_pos(sc, io.sync_index, io.pdo_pos, io.entry_pos, domain, &io.bit_position); @@ -3421,19 +3446,19 @@ static ATTRIBUTES int ec_ioctl_domain_size( return -EPERM; } - if (ec_lock_down_interruptible(&master->master_sem)) { + if (ec_lock_down_interruptible(&master->domains_lock)) { return -EINTR; } list_for_each_entry(domain, &master->domains, list) { if (domain->index == (unsigned long) arg) { size_t size = ecrt_domain_size(domain); - ec_lock_up(&master->master_sem); + ec_lock_up(&master->domains_lock); return size; } } - ec_lock_up(&master->master_sem); + ec_lock_up(&master->domains_lock); return -ENOENT; } @@ -3455,19 +3480,19 @@ static ATTRIBUTES int ec_ioctl_domain_offset( if (unlikely(!ctx->requested)) return -EPERM; - if (ec_lock_down_interruptible(&master->master_sem)) { + if (ec_lock_down_interruptible(&master->domains_lock)) { return -EINTR; } list_for_each_entry(domain, &master->domains, list) { if (domain->index == (unsigned long) arg) { - ec_lock_up(&master->master_sem); + ec_lock_up(&master->domains_lock); return offset; } offset += ecrt_domain_size(domain); } - ec_lock_up(&master->master_sem); + ec_lock_up(&master->domains_lock); return -ENOENT; } @@ -3490,17 +3515,17 @@ static ATTRIBUTES int ec_ioctl_domain_process( /* Locking added as domain processing is likely to be used by more than one application tasks */ - if (ec_ioctl_lock_down_interruptible(&master->master_sem)) { + if (ec_ioctl_lock_down_interruptible(&master->domains_lock)) { return -EINTR; } if (!(domain = ec_master_find_domain(master, (unsigned long) arg))) { - ec_ioctl_lock_up(&master->master_sem); + ec_ioctl_lock_up(&master->domains_lock); return -ENOENT; } ecrt_domain_process(domain); - ec_ioctl_lock_up(&master->master_sem); + ec_ioctl_lock_up(&master->domains_lock); return 0; } @@ -3523,21 +3548,22 @@ static ATTRIBUTES int ec_ioctl_domain_queue( /* Locking added as domain queing is likely to be used by more than one application tasks */ - if (ec_ioctl_lock_down_interruptible(&master->master_sem)) + if (ec_ioctl_lock_down_interruptible(&master->domains_lock)) return -EINTR; - if (!(domain = ec_master_find_domain(master, (unsigned long) arg))) { - ec_ioctl_lock_up(&master->master_sem); + ec_ioctl_lock_up(&master->domains_lock); return -ENOENT; } - ec_ioctl_lock_up(&master->master_sem); - - if (ec_ioctl_lock_down_interruptible(&master->io_sem)) + if (ec_ioctl_lock_down_interruptible(&master->io_sem)) { + ec_ioctl_lock_up(&master->domains_lock); return -EINTR; + } ecrt_domain_queue(domain); ec_ioctl_lock_up(&master->io_sem); + ec_ioctl_lock_up(&master->domains_lock); + return 0; } @@ -3564,15 +3590,18 @@ static ATTRIBUTES int ec_ioctl_domain_state( return -EFAULT; } - /* no locking of master_sem needed, because domain will not be deleted in - * the meantime. */ + if (ec_ioctl_lock_down_interruptible(&master->domains_lock)) + return -EINTR; if (!(domain = ec_master_find_domain_const(master, data.domain_index))) { + ec_ioctl_lock_up(&master->domains_lock); return -ENOENT; } ecrt_domain_state(domain, &state); + ec_ioctl_lock_up(&master->domains_lock); + if (copy_to_user((void __user *) data.state, &state, sizeof(state))) return -EFAULT; diff --git a/master/master.c b/master/master.c index bfaa893e..81d56383 100644 --- a/master/master.c +++ b/master/master.c @@ -186,6 +186,7 @@ int ec_master_init(ec_master_t *master, /**< EtherCAT master */ INIT_LIST_HEAD(&master->configs); INIT_LIST_HEAD(&master->domains); + ec_lock_init(&master->domains_lock); master->app_time = 0ULL; master->dc_ref_time = 0ULL; @@ -534,11 +535,13 @@ void ec_master_clear_domains(ec_master_t *master) { ec_domain_t *domain, *next; + ec_lock_down(&master->domains_lock); list_for_each_entry_safe(domain, next, &master->domains, list) { list_del(&domain->list); ec_domain_clear(domain); kfree(domain); } + ec_lock_up(&master->domains_lock); } /*****************************************************************************/ @@ -2089,9 +2092,11 @@ unsigned int ec_master_domain_count( const ec_domain_t *domain; unsigned int count = 0; + ec_lock_down(&master->domains_lock); list_for_each_entry(domain, &master->domains, list) { count++; } + ec_lock_up(&master->domains_lock); return count; } @@ -2437,7 +2442,7 @@ ec_domain_t *ecrt_master_create_domain_err( return ERR_PTR(-ENOMEM); } - ec_lock_down(&master->master_sem); + ec_lock_down(&master->domains_lock); if (list_empty(&master->domains)) { index = 0; @@ -2449,7 +2454,7 @@ ec_domain_t *ecrt_master_create_domain_err( ec_domain_init(domain, master, index); list_add_tail(&domain->list, &master->domains); - ec_lock_up(&master->master_sem); + ec_lock_up(&master->domains_lock); EC_MASTER_DBG(master, 1, "Created domain %u.\n", domain->index); @@ -2492,21 +2497,21 @@ int ecrt_master_activate(ec_master_t *master) return 0; } - ec_lock_down(&master->master_sem); + ec_lock_down(&master->domains_lock); // finish all domains domain_offset = 0; list_for_each_entry(domain, &master->domains, list) { ret = ec_domain_finish(domain, domain_offset); if (ret < 0) { - ec_lock_up(&master->master_sem); + ec_lock_up(&master->domains_lock); EC_MASTER_ERR(master, "Failed to finish domain 0x%p!\n", domain); return ret; } domain_offset += domain->data_size; } - ec_lock_up(&master->master_sem); + ec_lock_up(&master->domains_lock); // restart EoE process and master thread with new locking diff --git a/master/master.h b/master/master.h index c51d79fb..9541df5b 100644 --- a/master/master.h +++ b/master/master.h @@ -229,6 +229,7 @@ struct ec_master { /* Configuration applied by the application. */ struct list_head configs; /**< List of slave configurations. */ struct list_head domains; /**< List of domains. */ + ec_lock_t domains_lock; /**< Lock for access to domains list. */ u64 app_time; /**< Time of the last ecrt_master_sync() call. */ u64 dc_ref_time; /**< Common reference timestamp for DC start times. */