greybus: protect cookie with a mutex

When a Greybus message is sent, the host driver supplies a cookie
for Greybus to use to identify the sent message in the event it
needs to be canceled.  The cookie will be non-null while the message
is in flight, and a null pointer otherwise.

There are two problems with this, which arise out of the fact that a
message can be canceled at any time--even concurrent with it getting
sent (such as when Greybus is getting shut down).

First, the host driver's buffer_send method can return an error
value, which is non-null but not a valid cookie.  So we need to
ensure such a bogus cookie is never used to cancel a message.

Second, we can't resolve that problem by assigning message->cookie
only after we've determined it's not an error.  The instant
buffer_send() returns, the message may well be in flight and *should*
be canceled at shutdown, so we need the cookie value to reflect
that.

In order to avoid these problems, protect access to a message's
cookie value with a mutex.  A spin lock can't be used because the
window that needs protecting covers code that can block.  We
reset the cookie value to NULL as soon as the host driver has
notified us it has been sent (or failed to).

Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>

diff --git a/drivers/staging/greybus/operation.c b/drivers/staging/greybus/operation.c
index b4ef8201575060e9945c6ef805ceeb8791d1f69f..226565d4503a21f43e43141bf22a315ae900821a 100644 (file)
--- a/drivers/staging/greybus/operation.c
+++ b/drivers/staging/greybus/operation.c
@@ -32,6 +32,9 @@ static struct kmem_cache *gb_operation_cache;
 /* Workqueue to handle Greybus operation completions. */
 static struct workqueue_struct *gb_operation_workqueue;
 
+/* Protects the cookie representing whether a message is in flight */
+static DEFINE_MUTEX(gb_message_mutex);
+
 /*
  * All operation messages (both requests and responses) begin with
  * a header that encodes the size of the data (header included).
@@ -170,16 +173,20 @@ static int gb_message_send(struct gb_message *message, gfp_t gfp_mask)
        struct gb_connection *connection = message->operation->connection;
        u16 dest_cport_id = connection->interface_cport_id;
        int ret = 0;
+       void *cookie;
 
-       message->cookie = connection->hd->driver->buffer_send(connection->hd,
+       mutex_lock(&gb_message_mutex);
+       cookie = connection->hd->driver->buffer_send(connection->hd,
                                        dest_cport_id,
                                        message->header,
                                        message->size,
                                        gfp_mask);
-       if (IS_ERR(message->cookie)) {
-               ret = PTR_ERR(message->cookie);
-               message->cookie = NULL;
-       }
+       if (IS_ERR(cookie))
+               ret = PTR_ERR(cookie);
+       else
+               message->cookie = cookie;
+       mutex_unlock(&gb_message_mutex);
+
        return ret;
 }
 
@@ -189,13 +196,14 @@ static int gb_message_send(struct gb_message *message, gfp_t gfp_mask)
  */
 static void gb_message_cancel(struct gb_message *message)
 {
-       struct greybus_host_device *hd;
-
-       if (!message->cookie)
-               return; /* Don't bother if the message isn't in flight */
+       mutex_lock(&gb_message_mutex);
+       if (message->cookie) {
+               struct greybus_host_device *hd;
 
-       hd = message->operation->connection->hd;
-       hd->driver->buffer_cancel(message->cookie);
+               hd = message->operation->connection->hd;
+               hd->driver->buffer_cancel(message->cookie);
+       }
+       mutex_unlock(&gb_message_mutex);
 }
 
 #if 0
@@ -550,12 +558,16 @@ greybus_data_sent(struct greybus_host_device *hd, void *header, int status)
        struct gb_message *message;
        struct gb_operation *operation;
 
-       /* If there's no error, there's really nothing to do */
+       /* XXX Right now we assume we're an outgoing request */
+       message = gb_hd_message_find(hd, header);
+
+       /* Record that the message is no longer in flight */
+       message->cookie = NULL;
+
+       /* If there's no error, there's really nothing more to do */
        if (!status)
                return; /* Mark it complete? */
 
-       /* XXX Right now we assume we're an outgoing request */
-       message = gb_hd_message_find(hd, header);
        operation = message->operation;
        if (gb_operation_result_set(operation, status))
                queue_work(gb_operation_workqueue, &operation->work);