SDO wait_event() deadlock fixed: refcount ec_master_sdo_request_t objects with kref
--- a/master/cdev.c Thu Dec 23 09:28:39 2010 +0100
+++ b/master/cdev.c Thu Dec 23 09:48:56 2010 +0100
@@ -840,78 +840,74 @@
)
{
ec_ioctl_slave_sdo_upload_t data;
- ec_master_sdo_request_t request;
+ ec_master_sdo_request_t* request;
int retval;
if (copy_from_user(&data, (void __user *) arg, sizeof(data))) {
return -EFAULT;
}
- ec_sdo_request_init(&request.req);
- ec_sdo_request_address(&request.req,
- data.sdo_index, data.sdo_entry_subindex);
- ecrt_sdo_request_read(&request.req);
-
- if (down_interruptible(&master->master_sem))
- return -EINTR;
-
- if (!(request.slave = ec_master_find_slave(
+ request = kmalloc(sizeof(*request), GFP_KERNEL);
+ if (!request)
+ return -ENOMEM;
+ kref_init(&request->refcount);
+
+ ec_sdo_request_init(&request->req);
+ ec_sdo_request_address(&request->req,
+ data.sdo_index, data.sdo_entry_subindex);
+ ecrt_sdo_request_read(&request->req);
+
+ if (down_interruptible(&master->master_sem)) {
+ kref_put(&request->refcount,ec_master_sdo_request_release);
+ return -EINTR;
+ }
+ if (!(request->slave = ec_master_find_slave(
master, 0, data.slave_position))) {
up(&master->master_sem);
- ec_sdo_request_clear(&request.req);
+ kref_put(&request->refcount,ec_master_sdo_request_release);
EC_MASTER_ERR(master, "Slave %u does not exist!\n",
data.slave_position);
return -EINVAL;
}
- EC_SLAVE_DBG(request.slave, 1, "Schedule SDO upload request.\n");
+ EC_SLAVE_DBG(request->slave, 1, "Schedule SDO upload request %p.\n",request);
// schedule request.
- list_add_tail(&request.list, &request.slave->slave_sdo_requests);
+ kref_get(&request->refcount);
+ list_add_tail(&request->list, &request->slave->slave_sdo_requests);
up(&master->master_sem);
// wait for processing through FSM
- if (wait_event_interruptible(request.slave->sdo_queue,
- request.req.state != EC_INT_REQUEST_QUEUED)) {
+ if (wait_event_interruptible(request->slave->sdo_queue,
+ ((request->req.state == EC_INT_REQUEST_SUCCESS) || (request->req.state == EC_INT_REQUEST_FAILURE)))) {
// interrupted by signal
- down(&master->master_sem);
- if (request.req.state == EC_INT_REQUEST_QUEUED) {
- list_del(&request.list);
- up(&master->master_sem);
- ec_sdo_request_clear(&request.req);
- return -EINTR;
- }
- // request already processing: interrupt not possible.
- up(&master->master_sem);
- }
-
- // wait until master FSM has finished processing
- wait_event(request.slave->sdo_queue,
- request.req.state != EC_INT_REQUEST_BUSY);
-
- EC_SLAVE_DBG(request.slave, 1, "Finished SDO upload request.\n");
-
- data.abort_code = request.req.abort_code;
-
- if (request.req.state != EC_INT_REQUEST_SUCCESS) {
+ kref_put(&request->refcount,ec_master_sdo_request_release);
+ return -EINTR;
+ }
+
+ EC_SLAVE_DBG(request->slave, 1, "Finished SDO upload request %p.\n",request);
+
+ data.abort_code = request->req.abort_code;
+
+ if (request->req.state != EC_INT_REQUEST_SUCCESS) {
data.data_size = 0;
- if (request.req.errno) {
- retval = -request.req.errno;
+ if (request->req.errno) {
+ retval = -request->req.errno;
} else {
retval = -EIO;
}
} else {
- if (request.req.data_size > data.target_size) {
+ if (request->req.data_size > data.target_size) {
EC_MASTER_ERR(master, "Buffer too small.\n");
- ec_sdo_request_clear(&request.req);
+ kref_put(&request->refcount,ec_master_sdo_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.target,
- request.req.data, data.data_size)) {
- ec_sdo_request_clear(&request.req);
+ request->req.data, data.data_size)) {
+ kref_put(&request->refcount,ec_master_sdo_request_release);
return -EFAULT;
}
retval = 0;
@@ -921,7 +917,7 @@
retval = -EFAULT;
}
- ec_sdo_request_clear(&request.req);
+ kref_put(&request->refcount,ec_master_sdo_request_release);
return retval;
}
@@ -935,7 +931,7 @@
)
{
ec_ioctl_slave_sdo_download_t data;
- ec_master_sdo_request_t request;
+ ec_master_sdo_request_t* request;
int retval;
if (copy_from_user(&data, (void __user *) arg, sizeof(data))) {
@@ -948,67 +944,63 @@
return -EINVAL;
}
- ec_sdo_request_init(&request.req);
- ec_sdo_request_address(&request.req,
+ request = kmalloc(sizeof(*request), GFP_KERNEL);
+ if (!request)
+ return -ENOMEM;
+ kref_init(&request->refcount);
+
+ ec_sdo_request_init(&request->req);
+ ec_sdo_request_address(&request->req,
data.sdo_index, data.sdo_entry_subindex);
- if (ec_sdo_request_alloc(&request.req, data.data_size)) {
- ec_sdo_request_clear(&request.req);
+ if (ec_sdo_request_alloc(&request->req, data.data_size)) {
+ kref_put(&request->refcount,ec_master_sdo_request_release);
return -ENOMEM;
}
- if (copy_from_user(request.req.data,
+ if (copy_from_user(request->req.data,
(void __user *) data.data, data.data_size)) {
- ec_sdo_request_clear(&request.req);
- return -EFAULT;
- }
- request.req.data_size = data.data_size;
- ecrt_sdo_request_write(&request.req);
-
- if (down_interruptible(&master->master_sem))
- return -EINTR;
-
- if (!(request.slave = ec_master_find_slave(
+ kref_put(&request->refcount,ec_master_sdo_request_release);
+ return -EFAULT;
+ }
+ request->req.data_size = data.data_size;
+ ecrt_sdo_request_write(&request->req);
+
+ if (down_interruptible(&master->master_sem)) {
+ kref_put(&request->refcount,ec_master_sdo_request_release);
+ return -EINTR;
+ }
+ if (!(request->slave = ec_master_find_slave(
master, 0, data.slave_position))) {
up(&master->master_sem);
EC_MASTER_ERR(master, "Slave %u does not exist!\n",
data.slave_position);
- ec_sdo_request_clear(&request.req);
+ kref_put(&request->refcount,ec_master_sdo_request_release);
return -EINVAL;
}
- EC_SLAVE_DBG(request.slave, 1, "Schedule SDO download request.\n");
+ EC_SLAVE_DBG(request->slave, 1, "Schedule SDO download request %p.\n",request);
// schedule request.
- list_add_tail(&request.list, &request.slave->slave_sdo_requests);
+ kref_get(&request->refcount);
+ list_add_tail(&request->list, &request->slave->slave_sdo_requests);
up(&master->master_sem);
// wait for processing through FSM
- if (wait_event_interruptible(request.slave->sdo_queue,
- request.req.state != EC_INT_REQUEST_QUEUED)) {
+ if (wait_event_interruptible(request->slave->sdo_queue,
+ ((request->req.state == EC_INT_REQUEST_SUCCESS) || (request->req.state == EC_INT_REQUEST_FAILURE)))) {
// interrupted by signal
- down(&master->master_sem);
- if (request.req.state == EC_INT_REQUEST_QUEUED) {
- list_del(&request.list);
- up(&master->master_sem);
- ec_sdo_request_clear(&request.req);
- return -EINTR;
- }
- // request already processing: interrupt not possible.
- up(&master->master_sem);
- }
-
- // wait until master FSM has finished processing
- wait_event(request.slave->sdo_queue,
- request.req.state != EC_INT_REQUEST_BUSY);
-
- EC_SLAVE_DBG(request.slave, 1, "Finished SDO download request.\n");
-
- data.abort_code = request.req.abort_code;
-
- if (request.req.state == EC_INT_REQUEST_SUCCESS) {
+ kref_put(&request->refcount,ec_master_sdo_request_release);
+ return -EINTR;
+ }
+
+ EC_SLAVE_DBG(request->slave, 1, "Finished SDO download request %p.\n",request);
+
+ data.abort_code = request->req.abort_code;
+
+ if (request->req.state == EC_INT_REQUEST_SUCCESS) {
retval = 0;
- } else if (request.req.errno) {
- retval = -request.req.errno;
+ } else if (request->req.errno) {
+ retval = -request->req.errno;
} else {
retval = -EIO;
}
@@ -1017,7 +1009,7 @@
retval = -EFAULT;
}
- ec_sdo_request_clear(&request.req);
+ kref_put(&request->refcount,ec_master_sdo_request_release);
return retval;
}
--- a/master/fsm_master.c Thu Dec 23 09:28:39 2010 +0100
+++ b/master/fsm_master.c Thu Dec 23 09:48:56 2010 +0100
@@ -1227,3 +1227,14 @@
}
/*****************************************************************************/
+
+/** called by kref_put if the 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_sdo_request_clear(&request->req);
+ kfree(request);
+}
--- a/master/fsm_master.h Thu Dec 23 09:28:39 2010 +0100
+++ b/master/fsm_master.h Thu Dec 23 09:48:56 2010 +0100
@@ -73,6 +73,8 @@
ec_internal_request_state_t state; /**< State of the request. */
} ec_reg_request_t;
+void ec_master_sdo_request_release(struct kref *);
+
/*****************************************************************************/
/** Slave/SDO request record for master's SDO request list.
@@ -81,6 +83,7 @@
struct list_head list; /**< List element. */
ec_slave_t *slave; /**< Slave. */
ec_sdo_request_t req; /**< SDO request. */
+ struct kref refcount;
} ec_master_sdo_request_t;
/*****************************************************************************/
--- a/master/fsm_slave.c Thu Dec 23 09:28:39 2010 +0100
+++ b/master/fsm_slave.c Thu Dec 23 09:48:56 2010 +0100
@@ -178,9 +178,10 @@
list_del_init(&request->list); // dequeue
if (slave->current_state & EC_SLAVE_STATE_ACK_ERR) {
- EC_SLAVE_WARN(slave, "Aborting SDO request,"
- " slave has error flag set.\n");
+ EC_SLAVE_WARN(slave, "Aborting SDO request %p,"
+ " slave has error flag set.\n",request);
request->req.state = EC_INT_REQUEST_FAILURE;
+ kref_put(&request->refcount,ec_master_sdo_request_release);
wake_up(&slave->sdo_queue);
fsm->sdo_request = NULL;
fsm->state = ec_fsm_slave_state_idle;
@@ -188,8 +189,9 @@
}
if (slave->current_state == EC_SLAVE_STATE_INIT) {
- EC_SLAVE_WARN(slave, "Aborting SDO request, slave is in INIT.\n");
+ EC_SLAVE_WARN(slave, "Aborting SDO request %p, slave is in INIT.\n",request);
request->req.state = EC_INT_REQUEST_FAILURE;
+ kref_put(&request->refcount,ec_master_sdo_request_release);
wake_up(&slave->sdo_queue);
fsm->sdo_request = NULL;
fsm->state = ec_fsm_slave_state_idle;
@@ -199,10 +201,10 @@
request->req.state = EC_INT_REQUEST_BUSY;
// Found pending SDO request. Execute it!
- EC_SLAVE_DBG(slave, 1, "Processing SDO request...\n");
+ EC_SLAVE_DBG(slave, 1, "Processing SDO request %p...\n",request);
// Start SDO transfer
- fsm->sdo_request = &request->req;
+ fsm->sdo_request = request;
fsm->state = ec_fsm_slave_state_sdo_request;
ec_fsm_coe_transfer(&fsm->fsm_coe, slave, &request->req);
ec_fsm_coe_exec(&fsm->fsm_coe); // execute immediately
@@ -221,7 +223,7 @@
)
{
ec_slave_t *slave = fsm->slave;
- ec_sdo_request_t *request = fsm->sdo_request;
+ ec_master_sdo_request_t *request = fsm->sdo_request;
if (ec_fsm_coe_exec(&fsm->fsm_coe))
{
@@ -229,18 +231,20 @@
return;
}
if (!ec_fsm_coe_success(&fsm->fsm_coe)) {
- EC_SLAVE_ERR(slave, "Failed to process SDO request.\n");
- request->state = EC_INT_REQUEST_FAILURE;
+ EC_SLAVE_ERR(slave, "Failed to process SDO request %p.\n",request);
+ request->req.state = EC_INT_REQUEST_FAILURE;
+ kref_put(&request->refcount,ec_master_sdo_request_release);
wake_up(&slave->sdo_queue);
fsm->sdo_request = NULL;
fsm->state = ec_fsm_slave_state_idle;
return;
}
- EC_SLAVE_DBG(slave, 1, "Finished SDO request.\n");
+ EC_SLAVE_DBG(slave, 1, "Finished SDO request %p.\n",request);
// SDO request finished
- request->state = EC_INT_REQUEST_SUCCESS;
+ request->req.state = EC_INT_REQUEST_SUCCESS;
+ kref_put(&request->refcount,ec_master_sdo_request_release);
wake_up(&slave->sdo_queue);
fsm->sdo_request = NULL;
--- a/master/fsm_slave.h Thu Dec 23 09:28:39 2010 +0100
+++ b/master/fsm_slave.h Thu Dec 23 09:48:56 2010 +0100
@@ -42,6 +42,7 @@
#include "fsm_coe.h"
#include "fsm_foe.h"
#include "fsm_soe.h"
+#include "fsm_master.h"
typedef struct ec_fsm_slave ec_fsm_slave_t; /**< \see ec_fsm_slave */
@@ -52,7 +53,7 @@
ec_datagram_t *datagram; /**< datagram used in the state machine */
void (*state)(ec_fsm_slave_t *); /**< master state function */
- ec_sdo_request_t *sdo_request; /**< SDO request to process. */
+ ec_master_sdo_request_t *sdo_request; /**< SDO request to process. */
ec_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. */
--- a/master/slave.c Thu Dec 23 09:28:39 2010 +0100
+++ b/master/slave.c Thu Dec 23 09:48:56 2010 +0100
@@ -194,9 +194,10 @@
list_entry(slave->slave_sdo_requests.next,
ec_master_sdo_request_t, list);
list_del_init(&request->list); // dequeue
- EC_SLAVE_WARN(slave, "Discarding SDO request,"
- " slave about to be deleted.\n");
+ EC_SLAVE_WARN(slave, "Discarding SDO request %p,"
+ " slave about to be deleted.\n",request);
request->req.state = EC_INT_REQUEST_FAILURE;
+ kref_put(&request->refcount,ec_master_sdo_request_release);
wake_up(&slave->sdo_queue);
}