From 273be9d36227bed494f4758ebd0a8f250feebc82 Mon Sep 17 00:00:00 2001 From: Martin Troxler Date: Wed, 5 Jan 2011 09:50:35 +0100 Subject: [PATCH] FoE wait_event() deadlock fixed: refcount ec_master_foe_request_t objects with kref --- master/cdev.c | 133 ++++++++++++++++++++------------------------ master/fsm_master.c | 17 +++++- master/fsm_master.h | 6 +- master/fsm_slave.c | 23 ++++---- master/fsm_slave.h | 2 +- master/slave.c | 1 + 6 files changed, 96 insertions(+), 86 deletions(-) diff --git a/master/cdev.c b/master/cdev.c index 0a1b5c47..e84288f3 100644 --- a/master/cdev.c +++ b/master/cdev.c @@ -3270,74 +3270,70 @@ int ec_cdev_ioctl_slave_foe_read( ) { ec_ioctl_slave_foe_t data; - ec_master_foe_request_t request; + ec_master_foe_request_t* request; int retval; if (copy_from_user(&data, (void __user *) arg, sizeof(data))) { return -EFAULT; } - ec_foe_request_init(&request.req, data.file_name); - ec_foe_request_read(&request.req); - ec_foe_request_alloc(&request.req, 10000); // FIXME + request = kmalloc(sizeof(*request), GFP_KERNEL); + if (!request) + return -ENOMEM; + kref_init(&request->refcount); - if (ec_mutex_lock_interruptible(&master->master_mutex)) + ec_foe_request_init(&request->req, data.file_name); + ec_foe_request_read(&request->req); + ec_foe_request_alloc(&request->req, 10000); // FIXME + + if (ec_mutex_lock_interruptible(&master->master_mutex)) { + kref_put(&request->refcount,ec_master_foe_request_release); return -EINTR; - - if (!(request.slave = ec_master_find_slave( + } + if (!(request->slave = ec_master_find_slave( master, 0, data.slave_position))) { ec_mutex_unlock(&master->master_mutex); - ec_foe_request_clear(&request.req); + kref_put(&request->refcount,ec_master_foe_request_release); EC_MASTER_ERR(master, "Slave %u does not exist!\n", data.slave_position); return -EINVAL; } // schedule request. - list_add_tail(&request.list, &request.slave->foe_requests); + list_add_tail(&request->list, &request->slave->foe_requests); + kref_get(&request->refcount); ec_mutex_unlock(&master->master_mutex); - EC_SLAVE_DBG(request.slave, 1, "Scheduled FoE read request.\n"); + EC_SLAVE_DBG(request->slave, 1, "Scheduled FoE read request %p.\n",request); // wait for processing through FSM - if (wait_event_interruptible(request.slave->foe_queue, - request.req.state != EC_INT_REQUEST_QUEUED)) { + if (wait_event_interruptible(request->slave->foe_queue, + ((request->req.state == EC_INT_REQUEST_SUCCESS) || (request->req.state == EC_INT_REQUEST_FAILURE)))) { // interrupted by signal - ec_mutex_lock(&master->master_mutex); - if (request.req.state == EC_INT_REQUEST_QUEUED) { - list_del(&request.list); - ec_mutex_unlock(&master->master_mutex); - ec_foe_request_clear(&request.req); - return -EINTR; - } - // request already processing: interrupt not possible. - ec_mutex_unlock(&master->master_mutex); + kref_put(&request->refcount,ec_master_foe_request_release); + return -EINTR; } - // wait until master FSM has finished processing - wait_event(request.slave->foe_queue, - request.req.state != EC_INT_REQUEST_BUSY); + data.result = request->req.result; + data.error_code = request->req.error_code; - data.result = request.req.result; - data.error_code = request.req.error_code; + EC_SLAVE_DBG(request->slave, 1, "Read %zd bytes via FoE" + " (result = 0x%x).\n", request->req.data_size, request->req.result); - EC_SLAVE_DBG(request.slave, 1, "Read %zd bytes via FoE" - " (result = 0x%x).\n", request.req.data_size, request.req.result); - - if (request.req.state != EC_INT_REQUEST_SUCCESS) { + if (request->req.state != EC_INT_REQUEST_SUCCESS) { data.data_size = 0; retval = -EIO; } else { - if (request.req.data_size > data.buffer_size) { + if (request->req.data_size > data.buffer_size) { EC_MASTER_ERR(master, "Buffer too small.\n"); - ec_foe_request_clear(&request.req); + kref_put(&request->refcount,ec_master_foe_request_release); return -EOVERFLOW; } - data.data_size = request.req.data_size; + data.data_size = request->req.data_size; if (copy_to_user((void __user *) data.buffer, - request.req.buffer, data.data_size)) { - ec_foe_request_clear(&request.req); + request->req.buffer, data.data_size)) { + kref_put(&request->refcount,ec_master_foe_request_release); return -EFAULT; } retval = 0; @@ -3347,9 +3343,8 @@ int ec_cdev_ioctl_slave_foe_read( retval = -EFAULT; } - EC_SLAVE_DBG(request.slave, 1, "Finished FoE read request.\n"); - - ec_foe_request_clear(&request.req); + EC_SLAVE_DBG(request->slave, 1, "Finished FoE read request %p.\n",request); + kref_put(&request->refcount,ec_master_foe_request_release); return retval; } @@ -3364,79 +3359,73 @@ int ec_cdev_ioctl_slave_foe_write( ) { ec_ioctl_slave_foe_t data; - ec_master_foe_request_t request; + ec_master_foe_request_t* request; int retval; if (copy_from_user(&data, (void __user *) arg, sizeof(data))) { return -EFAULT; } - INIT_LIST_HEAD(&request.list); + request = kmalloc(sizeof(*request), GFP_KERNEL); + if (!request) + return -ENOMEM; + kref_init(&request->refcount); - ec_foe_request_init(&request.req, data.file_name); + INIT_LIST_HEAD(&request->list); - if (ec_foe_request_alloc(&request.req, data.buffer_size)) { - ec_foe_request_clear(&request.req); + ec_foe_request_init(&request->req, data.file_name); + + if (ec_foe_request_alloc(&request->req, data.buffer_size)) { + kref_put(&request->refcount,ec_master_foe_request_release); return -ENOMEM; } - if (copy_from_user(request.req.buffer, + if (copy_from_user(request->req.buffer, (void __user *) data.buffer, data.buffer_size)) { - ec_foe_request_clear(&request.req); + kref_put(&request->refcount,ec_master_foe_request_release); return -EFAULT; } - request.req.data_size = data.buffer_size; - ec_foe_request_write(&request.req); + request->req.data_size = data.buffer_size; + ec_foe_request_write(&request->req); if (ec_mutex_lock_interruptible(&master->master_mutex)) return -EINTR; - if (!(request.slave = ec_master_find_slave( + if (!(request->slave = ec_master_find_slave( master, 0, data.slave_position))) { ec_mutex_unlock(&master->master_mutex); EC_MASTER_ERR(master, "Slave %u does not exist!\n", data.slave_position); - ec_foe_request_clear(&request.req); + kref_put(&request->refcount,ec_master_foe_request_release); return -EINVAL; } - EC_SLAVE_DBG(request.slave, 1, "Scheduling FoE write request.\n"); + EC_SLAVE_DBG(request->slave, 1, "Scheduling FoE write request %p.\n",request); // schedule FoE write request. - list_add_tail(&request.list, &request.slave->foe_requests); + list_add_tail(&request->list, &request->slave->foe_requests); + kref_get(&request->refcount); ec_mutex_unlock(&master->master_mutex); // wait for processing through FSM - if (wait_event_interruptible(request.slave->foe_queue, - request.req.state != EC_INT_REQUEST_QUEUED)) { + if (wait_event_interruptible(request->slave->foe_queue, + ((request->req.state == EC_INT_REQUEST_SUCCESS) || (request->req.state == EC_INT_REQUEST_FAILURE)))) { // interrupted by signal - ec_mutex_lock(&master->master_mutex); - if (request.req.state == EC_INT_REQUEST_QUEUED) { - // abort request - list_del(&request.list); - ec_mutex_unlock(&master->master_mutex); - ec_foe_request_clear(&request.req); - return -EINTR; - } - ec_mutex_unlock(&master->master_mutex); + kref_put(&request->refcount,ec_master_foe_request_release); + return -EINTR; } - // wait until master FSM has finished processing - wait_event(request.slave->foe_queue, - request.req.state != EC_INT_REQUEST_BUSY); + data.result = request->req.result; + data.error_code = request->req.error_code; - data.result = request.req.result; - data.error_code = request.req.error_code; - - retval = request.req.state == EC_INT_REQUEST_SUCCESS ? 0 : -EIO; + retval = request->req.state == EC_INT_REQUEST_SUCCESS ? 0 : -EIO; if (__copy_to_user((void __user *) arg, &data, sizeof(data))) { retval = -EFAULT; } - ec_foe_request_clear(&request.req); - - EC_SLAVE_DBG(request.slave, 1, "Finished FoE write request.\n"); + EC_SLAVE_DBG(request->slave, 1, "Finished FoE write request %p.\n",request); + kref_put(&request->refcount,ec_master_foe_request_release); return retval; } diff --git a/master/fsm_master.c b/master/fsm_master.c index 2526159c..23082b43 100644 --- a/master/fsm_master.c +++ b/master/fsm_master.c @@ -1228,13 +1228,26 @@ void ec_fsm_master_state_reg_request( /*****************************************************************************/ -/** called by kref_put if the request's refcount becomes zero. +/** called by kref_put if the SDO request's refcount becomes zero. * */ void ec_master_sdo_request_release(struct kref *ref) { ec_master_sdo_request_t *request = container_of(ref, ec_master_sdo_request_t, refcount); - EC_SLAVE_DBG(request->slave, 1, "Releasing request %p.\n",request); + EC_SLAVE_DBG(request->slave, 1, "Releasing SDO request %p.\n",request); ec_sdo_request_clear(&request->req); kfree(request); } + +/*****************************************************************************/ + +/** called by kref_put if the FoE request's refcount becomes zero. + * + */ +void ec_master_foe_request_release(struct kref *ref) +{ + ec_master_foe_request_t *request = container_of(ref, ec_master_foe_request_t, refcount); + EC_SLAVE_DBG(request->slave, 1, "Releasing FoE request %p.\n",request); + ec_foe_request_clear(&request->req); + kfree(request); +} diff --git a/master/fsm_master.h b/master/fsm_master.h index 928773d0..02c8467d 100644 --- a/master/fsm_master.h +++ b/master/fsm_master.h @@ -73,7 +73,6 @@ typedef struct { ec_internal_request_state_t state; /**< State of the request. */ } ec_reg_request_t; -void ec_master_sdo_request_release(struct kref *); /*****************************************************************************/ @@ -86,6 +85,8 @@ typedef struct { struct kref refcount; } ec_master_sdo_request_t; +void ec_master_sdo_request_release(struct kref *); + /*****************************************************************************/ /** FoE request. @@ -94,8 +95,11 @@ typedef struct { struct list_head list; /**< List head. */ ec_slave_t *slave; /**< EtherCAT slave. */ ec_foe_request_t req; /**< FoE request. */ + struct kref refcount; } ec_master_foe_request_t; +void ec_master_foe_request_release(struct kref *); + /*****************************************************************************/ /** SoE request. diff --git a/master/fsm_slave.c b/master/fsm_slave.c index 5403b49c..aed7c624 100644 --- a/master/fsm_slave.c +++ b/master/fsm_slave.c @@ -267,9 +267,10 @@ int ec_fsm_slave_action_process_foe( // search the first request to be processed list_for_each_entry_safe(request, next, &slave->foe_requests, list) { if (slave->current_state & EC_SLAVE_STATE_ACK_ERR) { - EC_SLAVE_WARN(slave, "Aborting FOE request," - " slave has error flag set.\n"); + EC_SLAVE_WARN(slave, "Aborting FOE request %p," + " slave has error flag set.\n",request); request->req.state = EC_INT_REQUEST_FAILURE; + kref_put(&request->refcount,ec_master_foe_request_release); wake_up(&slave->foe_queue); fsm->sdo_request = NULL; fsm->state = ec_fsm_slave_state_idle; @@ -278,9 +279,9 @@ int ec_fsm_slave_action_process_foe( list_del_init(&request->list); // dequeue request->req.state = EC_INT_REQUEST_BUSY; - EC_SLAVE_DBG(slave, 1, "Processing FoE request.\n"); + EC_SLAVE_DBG(slave, 1, "Processing FoE request %p.\n",request); - fsm->foe_request = &request->req; + fsm->foe_request = request; fsm->state = ec_fsm_slave_state_foe_request; ec_fsm_foe_transfer(&fsm->fsm_foe, slave, &request->req); ec_fsm_foe_exec(&fsm->fsm_foe); @@ -299,7 +300,7 @@ void ec_fsm_slave_state_foe_request( ) { ec_slave_t *slave = fsm->slave; - ec_foe_request_t *request = fsm->foe_request; + ec_master_foe_request_t *request = fsm->foe_request; if (ec_fsm_foe_exec(&fsm->fsm_foe)) { @@ -308,8 +309,9 @@ void ec_fsm_slave_state_foe_request( } if (!ec_fsm_foe_success(&fsm->fsm_foe)) { - EC_SLAVE_ERR(slave, "Failed to handle FoE request.\n"); - request->state = EC_INT_REQUEST_FAILURE; + EC_SLAVE_ERR(slave, "Failed to handle FoE request %p.\n",request); + request->req.state = EC_INT_REQUEST_FAILURE; + kref_put(&request->refcount,ec_master_foe_request_release); wake_up(&slave->foe_queue); fsm->foe_request = NULL; fsm->state = ec_fsm_slave_state_idle; @@ -317,10 +319,11 @@ void ec_fsm_slave_state_foe_request( } // finished transferring FoE - EC_SLAVE_DBG(slave, 1, "Successfully transferred %zu bytes of FoE" - " data.\n", request->data_size); + EC_SLAVE_DBG(slave, 1, "FoE request %p successfully transferred %zu bytes.\n", + request,request->req.data_size); - request->state = EC_INT_REQUEST_SUCCESS; + request->req.state = EC_INT_REQUEST_SUCCESS; + kref_put(&request->refcount,ec_master_foe_request_release); wake_up(&slave->foe_queue); fsm->foe_request = NULL; diff --git a/master/fsm_slave.h b/master/fsm_slave.h index 12e0ef07..2d6b246a 100644 --- a/master/fsm_slave.h +++ b/master/fsm_slave.h @@ -54,7 +54,7 @@ struct ec_fsm_slave { void (*state)(ec_fsm_slave_t *); /**< master state function */ ec_master_sdo_request_t *sdo_request; /**< SDO request to process. */ - ec_foe_request_t *foe_request; /**< FoE request to process. */ + ec_master_foe_request_t *foe_request; /**< FoE request to process. */ off_t foe_index; /**< index to FoE write request data */ ec_soe_request_t *soe_request; /**< SoE request to process. */ diff --git a/master/slave.c b/master/slave.c index 05d0049e..6190c1ca 100644 --- a/master/slave.c +++ b/master/slave.c @@ -209,6 +209,7 @@ void ec_slave_clear(ec_slave_t *slave /**< EtherCAT slave */) EC_SLAVE_WARN(slave, "Discarding FoE request," " slave about to be deleted.\n"); request->req.state = EC_INT_REQUEST_FAILURE; + kref_put(&request->refcount,ec_master_foe_request_release); wake_up(&slave->foe_queue); }