ieee1394: csr1212: proper refcounting

At least since nodemgr got rid of coarse global locking, accesses to
struct csr1212_keyval's reference counter should be atomic and coupled
with proper barriers.  Also, calls to csr1212_keep_keyval(kv) should
occur before kv is being used.

(We probably should convert refcnt to struct kref, but how to keep
csr1212_destroy_keyval's implementation non-recursively then?)

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>

3 files changed, 36 insertions, 31 deletions

diff --git a/drivers/ieee1394/csr1212.c b/drivers/ieee1394/csr1212.c
index d08166bda1c5..e8122def164d 100644
--- a/drivers/ieee1394/csr1212.c
+++ b/drivers/ieee1394/csr1212.c
@@ -218,12 +218,10 @@ static struct csr1212_keyval *csr1212_new_keyval(u8 type, u8 key)
         if (!kv)
                 return NULL;
 
+        atomic_set(&kv->refcnt, 1);
         kv->key.type = type;
         kv->key.id = key;
-
         kv->associate = NULL;
-        kv->refcnt = 1;
-
         kv->next = NULL;
         kv->prev = NULL;
         kv->offset = 0;
@@ -326,12 +324,13 @@ void csr1212_associate_keyval(struct csr1212_keyval *kv,
         if (kv->associate)
                 csr1212_release_keyval(kv->associate);
 
-        associate->refcnt++;
+        csr1212_keep_keyval(associate);
         kv->associate = associate;
 }
 
-int csr1212_attach_keyval_to_directory(struct csr1212_keyval *dir,
-                                       struct csr1212_keyval *kv)
+static int __csr1212_attach_keyval_to_directory(struct csr1212_keyval *dir,
+                                                struct csr1212_keyval *kv,
+                                                bool keep_keyval)
 {
         struct csr1212_dentry *dentry;
 
@@ -341,10 +340,10 @@ int csr1212_attach_keyval_to_directory(struct csr1212_keyval *dir,
         if (!dentry)
                 return -ENOMEM;
 
+        if (keep_keyval)
+                csr1212_keep_keyval(kv);
         dentry->kv = kv;
 
-        kv->refcnt++;
-
         dentry->next = NULL;
         dentry->prev = dir->value.directory.dentries_tail;
 
@@ -358,6 +357,12 @@ int csr1212_attach_keyval_to_directory(struct csr1212_keyval *dir,
         return CSR1212_SUCCESS;
 }
 
+int csr1212_attach_keyval_to_directory(struct csr1212_keyval *dir,
+                                       struct csr1212_keyval *kv)
+{
+        return __csr1212_attach_keyval_to_directory(dir, kv, true);
+}
+
 #define CSR1212_DESCRIPTOR_LEAF_DATA(kv) \
         (&((kv)->value.leaf.data[1]))
 
@@ -483,15 +488,18 @@ void csr1212_detach_keyval_from_directory(struct csr1212_keyval *dir,
 
 /* This function is used to free the memory taken by a keyval.  If the given
  * keyval is a directory type, then any keyvals contained in that directory
- * will be destroyed as well if their respective refcnts are 0.  By means of
+ * will be destroyed as well if noone holds a reference on them.  By means of
  * list manipulation, this routine will descend a directory structure in a
  * non-recursive manner. */
-static void csr1212_destroy_keyval(struct csr1212_keyval *kv)
+void csr1212_release_keyval(struct csr1212_keyval *kv)
 {
         struct csr1212_keyval *k, *a;
         struct csr1212_dentry dentry;
         struct csr1212_dentry *head, *tail;
 
+        if (!atomic_dec_and_test(&kv->refcnt))
+                return;
+
         dentry.kv = kv;
         dentry.next = NULL;
         dentry.prev = NULL;
@@ -503,9 +511,8 @@ static void csr1212_destroy_keyval(struct csr1212_keyval *kv)
                 k = head->kv;
 
                 while (k) {
-                        k->refcnt--;
-
-                        if (k->refcnt > 0)
+                        /* must not dec_and_test kv->refcnt again */
+                        if (k != kv && !atomic_dec_and_test(&k->refcnt))
                                 break;
 
                         a = k->associate;
@@ -536,14 +543,6 @@ static void csr1212_destroy_keyval(struct csr1212_keyval *kv)
         }
 }
 
-void csr1212_release_keyval(struct csr1212_keyval *kv)
-{
-        if (kv->refcnt > 1)
-                kv->refcnt--;
-        else
-                csr1212_destroy_keyval(kv);
-}
-
 void csr1212_destroy_csr(struct csr1212_csr *csr)
 {
         struct csr1212_csr_rom_cache *c, *oc;
@@ -1126,6 +1125,7 @@ csr1212_parse_dir_entry(struct csr1212_keyval *dir, u32 ki, u32 kv_pos)
         int ret = CSR1212_SUCCESS;
         struct csr1212_keyval *k = NULL;
         u32 offset;
+        bool keep_keyval = true;
 
         switch (CSR1212_KV_KEY_TYPE(ki)) {
         case CSR1212_KV_TYPE_IMMEDIATE:
@@ -1135,8 +1135,8 @@ csr1212_parse_dir_entry(struct csr1212_keyval *dir, u32 ki, u32 kv_pos)
                         ret = -ENOMEM;
                         goto out;
                 }
-
-                k->refcnt = 0;  /* Don't keep local reference when parsing. */
+                /* Don't keep local reference when parsing. */
+                keep_keyval = false;
                 break;
 
         case CSR1212_KV_TYPE_CSR_OFFSET:
@@ -1146,7 +1146,8 @@ csr1212_parse_dir_entry(struct csr1212_keyval *dir, u32 ki, u32 kv_pos)
                         ret = -ENOMEM;
                         goto out;
                 }
-                k->refcnt = 0;  /* Don't keep local reference when parsing. */
+                /* Don't keep local reference when parsing. */
+                keep_keyval = false;
                 break;
 
         default:
@@ -1174,8 +1175,10 @@ csr1212_parse_dir_entry(struct csr1212_keyval *dir, u32 ki, u32 kv_pos)
                         ret = -ENOMEM;
                         goto out;
                 }
-                k->refcnt = 0;  /* Don't keep local reference when parsing. */
-                k->valid = 0;   /* Contents not read yet so it's not valid. */
+                /* Don't keep local reference when parsing. */
+                keep_keyval = false;
+                /* Contents not read yet so it's not valid. */
+                k->valid = 0;
                 k->offset = offset;
 
                 k->prev = dir;
@@ -1183,7 +1186,7 @@ csr1212_parse_dir_entry(struct csr1212_keyval *dir, u32 ki, u32 kv_pos)
                 dir->next->prev = k;
                 dir->next = k;
         }
-        ret = csr1212_attach_keyval_to_directory(dir, k);
+        ret = __csr1212_attach_keyval_to_directory(dir, k, keep_keyval);
 out:
         if (ret != CSR1212_SUCCESS && k != NULL)
                 free_keyval(k);
diff --git a/drivers/ieee1394/csr1212.h b/drivers/ieee1394/csr1212.h
index df909ce66304..043039fc63ec 100644
--- a/drivers/ieee1394/csr1212.h
+++ b/drivers/ieee1394/csr1212.h
@@ -32,6 +32,7 @@
 
 #include <linux/types.h>
 #include <linux/slab.h>
+#include <asm/atomic.h>
 
 #define CSR1212_MALLOC(size)    kmalloc((size), GFP_KERNEL)
 #define CSR1212_FREE(ptr)       kfree(ptr)
@@ -149,7 +150,7 @@ struct csr1212_keyval {
                 struct csr1212_directory directory;
         } value;
         struct csr1212_keyval *associate;
-        int refcnt;
+        atomic_t refcnt;
 
         /* used in generating and/or parsing CSR image */
         struct csr1212_keyval *next, *prev;     /* flat list of CSR elements */
@@ -350,7 +351,8 @@ csr1212_get_keyval(struct csr1212_csr *csr, struct csr1212_keyval *kv);
  * need for code to retain a keyval that has been parsed. */
 static inline void csr1212_keep_keyval(struct csr1212_keyval *kv)
 {
-        kv->refcnt++;
+        atomic_inc(&kv->refcnt);
+        smp_mb__after_atomic_inc();
 }
 
 
diff --git a/drivers/ieee1394/nodemgr.c b/drivers/ieee1394/nodemgr.c
index ec8edd2f19de..90dc75be3418 100644
--- a/drivers/ieee1394/nodemgr.c
+++ b/drivers/ieee1394/nodemgr.c
@@ -1014,13 +1014,13 @@ static struct unit_directory *nodemgr_process_unit_directory
                             CSR1212_TEXTUAL_DESCRIPTOR_LEAF_LANGUAGE(kv) == 0) {
                                 switch (last_key_id) {
                                 case CSR1212_KV_ID_VENDOR:
-                                        ud->vendor_name_kv = kv;
                                         csr1212_keep_keyval(kv);
+                                        ud->vendor_name_kv = kv;
                                         break;
 
                                 case CSR1212_KV_ID_MODEL:
-                                        ud->model_name_kv = kv;
                                         csr1212_keep_keyval(kv);
+                                        ud->model_name_kv = kv;
                                         break;
 
                                 }