FoE wait_event() deadlock fixed: refcount ec_master_foe_request_t objects with kref
authorMartin Troxler <ch1010277@ch10pc446>
Wed, 05 Jan 2011 09:50:35 +0100
changeset 2029 5ef6507fc77a
parent 2028 55854f070c4a
child 2030 2bd8ad8bf41f
FoE wait_event() deadlock fixed: refcount ec_master_foe_request_t objects with kref
master/cdev.c
master/fsm_master.c
master/fsm_master.h
master/fsm_slave.c
master/fsm_slave.h
master/slave.c
--- a/master/cdev.c	Wed Jan 05 08:36:53 2011 +0100
+++ b/master/cdev.c	Wed Jan 05 09:50:35 2011 +0100
@@ -3270,74 +3270,70 @@
         )
 {
     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
-
-    if (ec_mutex_lock_interruptible(&master->master_mutex))
-        return -EINTR;
-
-    if (!(request.slave = ec_master_find_slave(
+    request = kmalloc(sizeof(*request), GFP_KERNEL);
+    if (!request)
+        return -ENOMEM;
+    kref_init(&request->refcount);
+
+    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(
                     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);
-
-    ec_mutex_unlock(&master->master_mutex);
-
-    EC_SLAVE_DBG(request.slave, 1, "Scheduled FoE read request.\n");
+    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 %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);
-    }
-
-    // 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;
-
-    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) {
+        kref_put(&request->refcount,ec_master_foe_request_release);
+        return -EINTR;
+    }
+
+    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);
+
+    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 @@
         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 @@
         )
 {
     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);
-
-    ec_foe_request_init(&request.req, data.file_name);
-
-    if (ec_foe_request_alloc(&request.req, data.buffer_size)) {
-        ec_foe_request_clear(&request.req);
+    request = kmalloc(sizeof(*request), GFP_KERNEL);
+    if (!request)
         return -ENOMEM;
-    }
-    if (copy_from_user(request.req.buffer,
+    kref_init(&request->refcount);
+
+    INIT_LIST_HEAD(&request->list);
+
+    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,
                 (void __user *) data.buffer, data.buffer_size)) {
-        ec_foe_request_clear(&request.req);
-        return -EFAULT;
-    }
-    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(
+        kref_put(&request->refcount,ec_master_foe_request_release);
+        return -EFAULT;
+    }
+    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(
                     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);
-        return -EINVAL;
-    }
-
-    EC_SLAVE_DBG(request.slave, 1, "Scheduling FoE write request.\n");
+        kref_put(&request->refcount,ec_master_foe_request_release);
+        return -EINVAL;
+    }
+
+    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);
-    }
-
-    // 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;
-
-    retval = request.req.state == EC_INT_REQUEST_SUCCESS ? 0 : -EIO;
+        kref_put(&request->refcount,ec_master_foe_request_release);
+        return -EINTR;
+    }
+
+    data.result = request->req.result;
+    data.error_code = request->req.error_code;
+
+    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;
 }
--- a/master/fsm_master.c	Wed Jan 05 08:36:53 2011 +0100
+++ b/master/fsm_master.c	Wed Jan 05 09:50:35 2011 +0100
@@ -1228,13 +1228,26 @@
 
 /*****************************************************************************/
 
-/** 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);
+}
--- a/master/fsm_master.h	Wed Jan 05 08:36:53 2011 +0100
+++ b/master/fsm_master.h	Wed Jan 05 09:50:35 2011 +0100
@@ -73,7 +73,6 @@
     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 @@
     struct kref refcount;
 } ec_master_sdo_request_t;
 
+void ec_master_sdo_request_release(struct kref *);
+
 /*****************************************************************************/
 
 /** FoE request.
@@ -94,8 +95,11 @@
     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.
--- a/master/fsm_slave.c	Wed Jan 05 08:36:53 2011 +0100
+++ b/master/fsm_slave.c	Wed Jan 05 09:50:35 2011 +0100
@@ -267,9 +267,10 @@
     // 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 @@
         list_del_init(&request->list); // dequeue
         request->req.state = EC_INT_REQUEST_BUSY;
 
-        EC_SLAVE_DBG(slave, 1, "Processing FoE request.\n");
-
-        fsm->foe_request = &request->req;
+        EC_SLAVE_DBG(slave, 1, "Processing FoE request %p.\n",request);
+
+        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 @@
         )
 {
     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 @@
     }
 
     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 @@
     }
 
     // finished transferring FoE
-    EC_SLAVE_DBG(slave, 1, "Successfully transferred %zu bytes of FoE"
-            " data.\n", request->data_size);
-
-    request->state = EC_INT_REQUEST_SUCCESS;
+    EC_SLAVE_DBG(slave, 1, "FoE request %p successfully transferred %zu bytes.\n",
+            request,request->req.data_size);
+
+    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;
--- a/master/fsm_slave.h	Wed Jan 05 08:36:53 2011 +0100
+++ b/master/fsm_slave.h	Wed Jan 05 09:50:35 2011 +0100
@@ -54,7 +54,7 @@
 
     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. */
 
--- a/master/slave.c	Wed Jan 05 08:36:53 2011 +0100
+++ b/master/slave.c	Wed Jan 05 09:50:35 2011 +0100
@@ -209,6 +209,7 @@
         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);
     }