2008-02-13 18:34:52

by Matthias Kaehlcke

[permalink] [raw]
Subject: [PATCH] Adaptec AACRAID: convert fibctx->wait_sem to completion

Adaptec AACRAID driver: The semaphore fibctx->wait_sem is used for
signalling an event. Convert it to a completion.

Signed-off-by: Matthias Kaehlcke <[email protected]>

--
diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
index 3195d29..eef4939 100644
--- a/drivers/scsi/aacraid/aacraid.h
+++ b/drivers/scsi/aacraid/aacraid.h
@@ -717,7 +717,7 @@ struct aac_fib_context {
u32 unique; // unique value representing this context
ulong jiffies; // used for cleanup - dmb changed to ulong
struct list_head next; // used to link context's into a linked list
- struct semaphore wait_sem; // this is used to wait for the next fib to arrive.
+ struct completion compl; // this is used to wait for the next fib to arrive.
int wait; // Set to true when thread is in WaitForSingleObject
unsigned long count; // total number of FIBs on FibList
struct list_head fib_list; // this holds fibs and their attachd hw_fibs
diff --git a/drivers/scsi/aacraid/commctrl.c b/drivers/scsi/aacraid/commctrl.c
index abef051..98efda6 100644
--- a/drivers/scsi/aacraid/commctrl.c
+++ b/drivers/scsi/aacraid/commctrl.c
@@ -33,6 +33,7 @@
#include <linux/types.h>
#include <linux/pci.h>
#include <linux/spinlock.h>
+#include <linux/completion.h>
#include <linux/slab.h>
#include <linux/completion.h>
#include <linux/dma-mapping.h>
@@ -177,9 +178,9 @@ static int open_getadapter_fib(struct aac_dev * dev, void __user *arg)
*/
fibctx->unique = (u32)((ulong)fibctx & 0xFFFFFFFF);
/*
- * Initialize the mutex used to wait for the next AIF.
+ * Initialize the completion used to wait for the next AIF.
*/
- init_MUTEX_LOCKED(&fibctx->wait_sem);
+ init_completion(&fibctx->compl);
fibctx->wait = 0;
/*
* Initialize the fibs and set the count of fibs on
@@ -310,7 +311,7 @@ return_fib:
ssleep(1);
}
if (f.wait) {
- if(down_interruptible(&fibctx->wait_sem) < 0) {
+ if(wait_for_completion_interruptible(&fibctx->compl) < 0) {
status = -EINTR;
} else {
/* Lock again and retry */
diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
index 81b3692..486bdb3 100644
--- a/drivers/scsi/aacraid/commsup.c
+++ b/drivers/scsi/aacraid/commsup.c
@@ -1440,7 +1440,7 @@ int aac_check_health(struct aac_dev * aac)
* Set the event to wake up the
* thread that will waiting.
*/
- up(&fibctx->wait_sem);
+ complete(&fibctx->compl);
} else {
printk(KERN_WARNING "aifd: didn't allocate NewFib.\n");
kfree(fib);
@@ -1671,7 +1671,7 @@ int aac_command_thread(void *data)
* Set the event to wake up the
* thread that is waiting.
*/
- up(&fibctx->wait_sem);
+ complete(&fibctx->compl);
} else {
printk(KERN_WARNING "aifd: didn't allocate NewFib.\n");
}
--
Matthias Kaehlcke
Linux System Developer
Barcelona

Si deseas mantener tu libertad, debes estar preparado para defenderla
(Richard Stallman)
.''`.
using free software / Debian GNU/Linux | http://debian.org : :' :
`. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4 `-


2008-02-13 19:05:46

by Mark Salyzyn

[permalink] [raw]
Subject: RE: [PATCH] Adaptec AACRAID: convert fibctx->wait_sem to completion

linux/completion is already included in commctrl.c, in the patch fragment it is noted two lines below the addition. Please explain in the description the need/advantages for the change from semaphore to completion.

Sincerely -- Mark Salyzyn

> -----Original Message-----
> From: Matthias Kaehlcke [mailto:[email protected]]
> Sent: Wednesday, February 13, 2008 1:35 PM
> To: AACRAID; [email protected]
> Cc: [email protected]; [email protected]
> Subject: [PATCH] Adaptec AACRAID: convert fibctx->wait_sem to
> completion
>
> Adaptec AACRAID driver: The semaphore fibctx->wait_sem is used for
> signalling an event. Convert it to a completion.
>
> Signed-off-by: Matthias Kaehlcke <[email protected]>
>
> --
> diff --git a/drivers/scsi/aacraid/aacraid.h
> b/drivers/scsi/aacraid/aacraid.h
> index 3195d29..eef4939 100644
> --- a/drivers/scsi/aacraid/aacraid.h
> +++ b/drivers/scsi/aacraid/aacraid.h
> @@ -717,7 +717,7 @@ struct aac_fib_context {
> u32 unique; // unique
> value representing this context
> ulong jiffies; // used for
> cleanup - dmb changed to ulong
> struct list_head next; // used to
> link context's into a linked list
> - struct semaphore wait_sem; // this is
> used to wait for the next fib to arrive.
> + struct completion compl; // this is
> used to wait for the next fib to arrive.
> int wait; // Set to
> true when thread is in WaitForSingleObject
> unsigned long count; // total
> number of FIBs on FibList
> struct list_head fib_list; // this holds
> fibs and their attachd hw_fibs
> diff --git a/drivers/scsi/aacraid/commctrl.c
> b/drivers/scsi/aacraid/commctrl.c
> index abef051..98efda6 100644
> --- a/drivers/scsi/aacraid/commctrl.c
> +++ b/drivers/scsi/aacraid/commctrl.c
> @@ -33,6 +33,7 @@
> #include <linux/types.h>
> #include <linux/pci.h>
> #include <linux/spinlock.h>
> +#include <linux/completion.h>
> #include <linux/slab.h>
> #include <linux/completion.h>
> #include <linux/dma-mapping.h>
> @@ -177,9 +178,9 @@ static int open_getadapter_fib(struct
> aac_dev * dev, void __user *arg)
> */
> fibctx->unique = (u32)((ulong)fibctx & 0xFFFFFFFF);
> /*
> - * Initialize the mutex used to wait for
> the next AIF.
> + * Initialize the completion used to
> wait for the next AIF.
> */
> - init_MUTEX_LOCKED(&fibctx->wait_sem);
> + init_completion(&fibctx->compl);
> fibctx->wait = 0;
> /*
> * Initialize the fibs and set the count
> of fibs on
> @@ -310,7 +311,7 @@ return_fib:
> ssleep(1);
> }
> if (f.wait) {
> -
> if(down_interruptible(&fibctx->wait_sem) < 0) {
> +
> if(wait_for_completion_interruptible(&fibctx->compl) < 0) {
> status = -EINTR;
> } else {
> /* Lock again and retry */
> diff --git a/drivers/scsi/aacraid/commsup.c
> b/drivers/scsi/aacraid/commsup.c
> index 81b3692..486bdb3 100644
> --- a/drivers/scsi/aacraid/commsup.c
> +++ b/drivers/scsi/aacraid/commsup.c
> @@ -1440,7 +1440,7 @@ int aac_check_health(struct aac_dev * aac)
> * Set the event to wake up the
> * thread that will waiting.
> */
> - up(&fibctx->wait_sem);
> + complete(&fibctx->compl);
> } else {
> printk(KERN_WARNING "aifd: didn't
> allocate NewFib.\n");
> kfree(fib);
> @@ -1671,7 +1671,7 @@ int aac_command_thread(void *data)
> * Set the
> event to wake up the
> * thread
> that is waiting.
> */
> - up(&fibctx->wait_sem);
> +
> complete(&fibctx->compl);
> } else {
>
> printk(KERN_WARNING "aifd: didn't allocate NewFib.\n");
> }
> --
> Matthias Kaehlcke
> Linux System Developer
> Barcelona
>
> Si deseas mantener tu libertad, debes estar preparado para
> defenderla
> (Richard Stallman)
> .''`.
> using free software / Debian GNU/Linux |
> http://debian.org : :' :
> `. `'`
> gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4 `-
>

2008-02-13 19:09:24

by James Bottomley

[permalink] [raw]
Subject: Re: [PATCH] Adaptec AACRAID: convert fibctx->wait_sem to completion

On Wed, 2008-02-13 at 19:34 +0100, Matthias Kaehlcke wrote:
> Adaptec AACRAID driver: The semaphore fibctx->wait_sem is used for
> signalling an event. Convert it to a completion.

What's the purpose of doing this? The aacraid implementation seems to
be a classic PV semaphore, so why does it need to become a completion?

James

2008-02-13 20:15:50

by Matthias Kaehlcke

[permalink] [raw]
Subject: Re: [PATCH] Adaptec AACRAID: convert fibctx->wait_sem to completion

El Wed, Feb 13, 2008 at 01:09:00PM -0600 James Bottomley ha dit:

> On Wed, 2008-02-13 at 19:34 +0100, Matthias Kaehlcke wrote:
> > Adaptec AACRAID driver: The semaphore fibctx->wait_sem is used for
> > signalling an event. Convert it to a completion.
>
> What's the purpose of doing this? The aacraid implementation seems to
> be a classic PV semaphore, so why does it need to become a completion?

i had another look at the code and i agree with you. i didn't realize that
the semaphore is used as counting semaphore

please redirect the patch to /dev/null

thanks for reviewing the patch and pointing out my mistake

--
Matthias Kaehlcke
Linux System Developer
Barcelona

In itself, homosexuality is as limiting as heterosexuality: the
ideal should be to be capable of loving a woman or a man; either,
a human being, without feeling fear, restraint, or obligation
(Simone de Beauvoir)
.''`.
using free software / Debian GNU/Linux | http://debian.org : :' :
`. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4 `-