[PATCH] qla2xxx: firmware semaphore to mutex

[PATCH] qla2xxx: firmware semaphore to mutex

[Daniel Walker]

Signed-off-by: Daniel Walker <dwalker@mvista.com>

---
 drivers/scsi/qla2xxx/qla_os.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Index: linux-2.6.23/drivers/scsi/qla2xxx/qla_os.c
===================================================================
--- linux-2.6.23.orig/drivers/scsi/qla2xxx/qla_os.c
+++ linux-2.6.23/drivers/scsi/qla2xxx/qla_os.c
@@ -10,6 +10,7 @@
 #include <linux/vmalloc.h>
 #include <linux/delay.h>
 #include <linux/kthread.h>
+#include <linux/mutex.h>
 
 #include <scsi/scsi_tcq.h>
 #include <scsi/scsicam.h>
@@ -2782,7 +2783,7 @@ qla2x00_down_timeout(struct semaphore *s
 #define FW_FILE_ISP24XX	"ql2400_fw.bin"
 #define FW_FILE_ISP25XX	"ql2500_fw.bin"
 
-static DECLARE_MUTEX(qla_fw_lock);
+static DEFINE_MUTEX(qla_fw_lock);
 
 static struct fw_blob qla_fw_blobs[FW_BLOBS] = {
 	{ .name = FW_FILE_ISP21XX, .segs = { 0x1000, 0 }, },
@@ -2813,7 +2814,7 @@ qla2x00_request_firmware(scsi_qla_host_t
 		blob = &qla_fw_blobs[FW_ISP25XX];
 	}
 
-	down(&qla_fw_lock);
+	mutex_lock(&qla_fw_lock);
 	if (blob->fw)
 		goto out;
 
@@ -2826,7 +2827,7 @@ qla2x00_request_firmware(scsi_qla_host_t
 	}
 
 out:
-	up(&qla_fw_lock);
+	mutex_unlock(&qla_fw_lock);
 	return blob;
 }
 
@@ -2835,11 +2836,11 @@ qla2x00_release_firmware(void)
 {
 	int idx;
 
-	down(&qla_fw_lock);
+	mutex_lock(&qla_fw_lock);
 	for (idx = 0; idx < FW_BLOBS; idx++)
 		if (qla_fw_blobs[idx].fw)
 			release_firmware(qla_fw_blobs[idx].fw);
-	up(&qla_fw_lock);
+	mutex_unlock(&qla_fw_lock);
 }
 
 static pci_ers_result_t
-- 

-- 

Re: [PATCH] qla2xxx: firmware semaphore to mutex

[Matthew Wilcox]

On Mon, Apr 28, 2008 at 10:47:42AM -0700, Daniel Walker wrote:
> Signed-off-by: Daniel Walker <dwalker@mvista.com>

I did this one too ... my version had a nice little twist.

> @@ -2813,7 +2814,7 @@ qla2x00_request_firmware(scsi_qla_host_t
>  		blob = &qla_fw_blobs[FW_ISP25XX];
>  	}
>  
> -	down(&qla_fw_lock);
> +	mutex_lock(&qla_fw_lock);
>  	if (blob->fw)
>  		goto out;
>  

This one we can do as:

-       down(&qla_fw_lock);
+       if (mutex_lock_killable(&qla_fw_lock))
+               return NULL;
+


Otherwise, I think our patches are bit-for-bit identical.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] qla2xxx: firmware semaphore to mutex

[Daniel Walker]

On Mon, 2008-04-28 at 12:08 -0600, Matthew Wilcox wrote:
> On Mon, Apr 28, 2008 at 10:47:42AM -0700, Daniel Walker wrote:
> > Signed-off-by: Daniel Walker <dwalker@mvista.com>
> 
> I did this one too ... my version had a nice little twist.
> 
> > @@ -2813,7 +2814,7 @@ qla2x00_request_firmware(scsi_qla_host_t
> >  		blob = &qla_fw_blobs[FW_ISP25XX];
> >  	}
> >  
> > -	down(&qla_fw_lock);
> > +	mutex_lock(&qla_fw_lock);
> >  	if (blob->fw)
> >  		goto out;
> >  
> 
> This one we can do as:
> 
> -       down(&qla_fw_lock);
> +       if (mutex_lock_killable(&qla_fw_lock))
> +               return NULL;

I'm not sure what mutex_lock_killable() does, so I would imagine that
should be in it's own patch .. I don't like doubling up on changes
because I feel it's harder to review, and harder on git bisect once it's
in mainline.. That's why I was saying "one lock per patch." ..

Daniel

Re: [PATCH] qla2xxx: firmware semaphore to mutex

[Matthew Wilcox]

On Mon, Apr 28, 2008 at 11:57:13AM -0700, Daniel Walker wrote:
> On Mon, 2008-04-28 at 12:08 -0600, Matthew Wilcox wrote:
> > On Mon, Apr 28, 2008 at 10:47:42AM -0700, Daniel Walker wrote:
> > > Signed-off-by: Daniel Walker <dwalker@mvista.com>
> > 
> > I did this one too ... my version had a nice little twist.
> > 
> > > @@ -2813,7 +2814,7 @@ qla2x00_request_firmware(scsi_qla_host_t
> > >  		blob = &qla_fw_blobs[FW_ISP25XX];
> > >  	}
> > >  
> > > -	down(&qla_fw_lock);
> > > +	mutex_lock(&qla_fw_lock);
> > >  	if (blob->fw)
> > >  		goto out;
> > >  
> > 
> > This one we can do as:
> > 
> > -       down(&qla_fw_lock);
> > +       if (mutex_lock_killable(&qla_fw_lock))
> > +               return NULL;
> 
> I'm not sure what mutex_lock_killable() does, so I would imagine that
> should be in it's own patch .. I don't like doubling up on changes
> because I feel it's harder to review, and harder on git bisect once it's
> in mainline.. That's why I was saying "one lock per patch." ..

mutex_lock_killable() is documented.  Basically, if you receive a _fatal_
signal while trying to acquire the lock, it returns -EINTR and it's
up to the caller to handle it and unwind back to userspace.  ie it's
halfway between mutex_lock_interruptible() (any signal) and mutex_lock()
(no signal).

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] qla2xxx: firmware semaphore to mutex

[Daniel Walker]

On Mon, 2008-04-28 at 13:04 -0600, Matthew Wilcox wrote:
> it returns -EINTR and it's
> up to the caller to handle it and unwind back to userspace.  ie it's
> halfway between mutex_lock_interruptible() (any signal) and
> mutex_lock()
> (no signal).


In the hunk you quoted (below),

> > -       down(&qla_fw_lock);
> > +       if (mutex_lock_killable(&qla_fw_lock))
> > +               return NULL;

Don't you need to return the -EINTR so the caller knows the nature of
the failure? You might also need to re-factor the caller of this
function so it properly reports the failure to userspace .. In this case
your just returning NULL .. 

(CC'd Andrew since I think this was something he was concerned about,
with mutex_lock_interruptible().)

Daniel

Re: [PATCH] qla2xxx: firmware semaphore to mutex

[Matthew Wilcox]

On Mon, Apr 28, 2008 at 12:21:58PM -0700, Daniel Walker wrote:
> Don't you need to return the -EINTR so the caller knows the nature of
> the failure? You might also need to re-factor the caller of this
> function so it properly reports the failure to userspace .. In this case
> your just returning NULL .. 

The signal is _fatal_.  Userspace doesn't get to check the return value.
It's dead.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] qla2xxx: firmware semaphore to mutex

[Ingo Molnar]

* Matthew Wilcox <matthew@wil.cx> wrote:

> On Mon, Apr 28, 2008 at 12:21:58PM -0700, Daniel Walker wrote:
> > Don't you need to return the -EINTR so the caller knows the nature of
> > the failure? You might also need to re-factor the caller of this
> > function so it properly reports the failure to userspace .. In this case
> > your just returning NULL .. 
> 
> The signal is _fatal_.  Userspace doesn't get to check the return 
> value. It's dead.

i agree with your change, nevertheless Daniel is right in that the 
change should be in a separate patch. "sem2mutex" patches are supposed 
to be NOP-ish patches, with nothing else mixed in.

	Ingo

Re: [PATCH] qla2xxx: firmware semaphore to mutex

[Daniel Walker]

On Mon, 2008-04-28 at 13:24 -0600, Matthew Wilcox wrote:
> On Mon, Apr 28, 2008 at 12:21:58PM -0700, Daniel Walker wrote:
> > Don't you need to return the -EINTR so the caller knows the nature of
> > the failure? You might also need to re-factor the caller of this
> > function so it properly reports the failure to userspace .. In this case
> > your just returning NULL .. 
> 
> The signal is _fatal_.  Userspace doesn't get to check the return value.
> It's dead.

Kernel space does still observe the failure, right? Otherwise you
wouldn't return anything from mutex_lock_killable() .. If that's the
case just returning NULL , is like just injecting a failure.. I'm not
against adding mutex_lock_killable() , I'm just wondering if your
changes are complete in this case..

Daniel

Re: [PATCH] qla2xxx: firmware semaphore to mutex

[Andrew Vasquez]

On Mon, 28 Apr 2008, Daniel Walker wrote:

> Signed-off-by: Daniel Walker <dwalker@mvista.com>
> 
> ---
>  drivers/scsi/qla2xxx/qla_os.c |   11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)

Acked-by: Andrew Vasquez <andrew.vasquez@qlogic.com>