Re: [GIT PULL]: firmware patches for building firmware into kernel

[Jaswinder Singh <jaswinder@infradead.org>]

Hello Dave,

On Thu, 2008-08-07 at 14:21 -0400, David Dillow wrote:

> I just looked at the tree; it still has locking issues, and needs
> further review. You must protect the list from modification while you
> iterate it looking for an match on the requested firmware. 

So here is updated patch:

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 6074321..71ec20d 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -419,9 +419,9 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 		return -EINVAL;
 
 	/* Return firmware pointer from firmware list if already allocated */
+	mutex_lock(&fw_lock);
 	list_for_each_entry(flst, &firmwarelist, list)
 		if (strcmp(name, flst->name) == 0) {
-			mutex_lock(&fw_lock);
 			flst->count++;
 			*firmware_p = flst->fw;
 			mutex_unlock(&fw_lock);
@@ -429,6 +429,7 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 			       name, flst->count);
 			return 0;
 		}
+	mutex_unlock(&fw_lock);
 
 	*firmware_p = firmware = kzalloc(sizeof(*firmware), GFP_KERNEL);
 	if (!firmware)
@@ -568,19 +569,22 @@ void release_firmware(const struct firmware *fw)
 {
 	struct firmware_list *flst;
 
+	mutex_lock(&fw_lock);
 	if (fw)
 		list_for_each_entry(flst, &firmwarelist, list)
 			if (fw == flst->fw) {
 				printk(KERN_INFO
 				       "firmware: releasing %s count %d\n",
 				       flst->name, flst->count);
-				mutex_lock(&fw_lock);
 				flst->count--;
-				mutex_unlock(&fw_lock);
-				if (flst->count == 0)
-					__release_firmware(fw, flst);
-				return;
+				if (flst->count == 0) {
+					mutex_unlock(&fw_lock);
+					return __release_firmware(fw, flst);
+				}
+				goto out;
 			}
+out:
+	mutex_unlock(&fw_lock);
 }
 
 /*
@@ -598,6 +602,7 @@ void release_firmware_all(const struct firmware *fw)
 {
 	struct firmware_list *flst;
 
+	mutex_lock(&fw_lock);
 	if (fw)
 		list_for_each_entry(flst, &firmwarelist, list)
 			if (fw == flst->fw) {
@@ -605,13 +610,14 @@ void release_firmware_all(const struct firmware *fw)
 				       "firmware: release_all %s count %d\n",
 				       flst->name, flst->count);
 				if (flst->count) {
-					mutex_lock(&fw_lock);
 					flst->count = 0;
 					mutex_unlock(&fw_lock);
-					__release_firmware(fw, flst);
+					return __release_firmware(fw, flst);
 				}
-				return;
+				goto out;
 			}
+out:
+	mutex_unlock(&fw_lock);
 }
 
 /* Async support */


> Also, was it legal to call release_firmware() from an atomic context? It can now
> sleep, which may be an issue...
> 

yes, release_firmware can sleep. 
So now release_firmware also joined the family of request_firmware.

If you can do request_firmware you can also do release_firmware.

Any how release_firmware will be called below request_firmware or during
exit, I do not think this will make any issue.

Thank you,

Jaswinder Singh.