[PATCH] use spinlock instead of binary mutex in idt77252 driver

[PATCH] use spinlock instead of binary mutex in idt77252 driver

[Matthias Kaehlcke]

use spinlock instead of binary mutex in idt77252 driver

Signed-off-by: Matthias Kaehlcke <matthias.kaehlcke@gmail.com>

--
 
diff --git a/drivers/atm/idt77252.c b/drivers/atm/idt77252.c
index b4b8014..e3cf141 100644
--- a/drivers/atm/idt77252.c
+++ b/drivers/atm/idt77252.c
@@ -2430,7 +2430,7 @@ idt77252_open(struct atm_vcc *vcc)
 
 	set_bit(ATM_VF_ADDR, &vcc->flags);
 
-	down(&card->mutex);
+	mutex_lock(&card->mutex);
 
 	OPRINTK("%s: opening vpi.vci: %d.%d\n", card->name, vpi, vci);
 
@@ -2441,7 +2441,7 @@ idt77252_open(struct atm_vcc *vcc)
 		break;
 	default:
 		printk("%s: Unsupported AAL: %d\n", card->name, vcc->qos.aal);
-		up(&card->mutex);
+		mutex_unlock(&card->mutex);
 		return -EPROTONOSUPPORT;
 	}
 
@@ -2450,7 +2450,7 @@ idt77252_open(struct atm_vcc *vcc)
 		card->vcs[index] = kzalloc(sizeof(struct vc_map), GFP_KERNEL);
 		if (!card->vcs[index]) {
 			printk("%s: can't alloc vc in open()\n", card->name);
-			up(&card->mutex);
+			mutex_unlock(&card->mutex);
 			return -ENOMEM;
 		}
 		card->vcs[index]->card = card;
@@ -2479,14 +2479,14 @@ idt77252_open(struct atm_vcc *vcc)
 	if (inuse) {
 		printk("%s: %s vci already in use.\n", card->name,
 		       inuse == 1 ? "tx" : inuse == 2 ? "rx" : "tx and rx");
-		up(&card->mutex);
+		mutex_unlock(&card->mutex);
 		return -EADDRINUSE;
 	}
 
 	if (vcc->qos.txtp.traffic_class != ATM_NONE) {
 		error = idt77252_init_tx(card, vc, vcc, &vcc->qos);
 		if (error) {
-			up(&card->mutex);
+			mutex_unlock(&card->mutex);
 			return error;
 		}
 	}
@@ -2494,14 +2494,14 @@ idt77252_open(struct atm_vcc *vcc)
 	if (vcc->qos.rxtp.traffic_class != ATM_NONE) {
 		error = idt77252_init_rx(card, vc, vcc, &vcc->qos);
 		if (error) {
-			up(&card->mutex);
+			mutex_unlock(&card->mutex);
 			return error;
 		}
 	}
 
 	set_bit(ATM_VF_READY, &vcc->flags);
 
-	up(&card->mutex);
+	mutex_unlock(&card->mutex);
 	return 0;
 }
 
@@ -2515,7 +2515,7 @@ idt77252_close(struct atm_vcc *vcc)
 	unsigned long addr;
 	unsigned long timeout;
 
-	down(&card->mutex);
+	mutex_lock(&card->mutex);
 
 	IPRINTK("%s: idt77252_close: vc = %d (%d.%d)\n",
 		card->name, vc->index, vcc->vpi, vcc->vci);
@@ -2586,7 +2586,7 @@ done:
 		free_scq(card, vc->scq);
 	}
 
-	up(&card->mutex);
+	mutex_unlock(&card->mutex);
 }
 
 static int
@@ -2597,7 +2597,7 @@ idt77252_change_qos(struct atm_vcc *vcc, struct atm_qos *qos, int flags)
 	struct vc_map *vc = vcc->dev_data;
 	int error = 0;
 
-	down(&card->mutex);
+	mutex_lock(&card->mutex);
 
 	if (qos->txtp.traffic_class != ATM_NONE) {
 	    	if (!test_bit(VCF_TX, &vc->flags)) {
@@ -2643,7 +2643,7 @@ idt77252_change_qos(struct atm_vcc *vcc, struct atm_qos *qos, int flags)
 	set_bit(ATM_VF_HASQOS, &vcc->flags);
 
 out:
-	up(&card->mutex);
+	mutex_unlock(&card->mutex);
 	return error;
 }
 
@@ -3703,7 +3703,7 @@ idt77252_init_one(struct pci_dev *pcidev, const struct pci_device_id *id)
 	membase = pci_resource_start(pcidev, 1);
 	srambase = pci_resource_start(pcidev, 2);
 
-	init_MUTEX(&card->mutex);
+	mutex_init(&card->mutex);
 	spin_lock_init(&card->cmd_lock);
 	spin_lock_init(&card->tst_lock);
 
diff --git a/drivers/atm/idt77252.h b/drivers/atm/idt77252.h
index 544b397..a3b2f74 100644
--- a/drivers/atm/idt77252.h
+++ b/drivers/atm/idt77252.h
@@ -359,7 +359,7 @@ struct idt77252_dev
 	unsigned long		srambase;	/* SAR's sram  base address */
 	void __iomem		*fbq[4];	/* FBQ fill addresses */
 
-	struct semaphore	mutex;
+	struct mutex		mutex;
 	spinlock_t		cmd_lock;	/* for r/w utility/sram */
 
 	unsigned long		softstat;

-- 
Matthias Kaehlcke
Linux Application Developer
Barcelona

              Insanity: doing the same thing over and over
                again and expecting different results
                            (Albert Einstein)
                                                                 .''`.
    using free software / Debian GNU/Linux | http://debian.org  : :'  :
                                                                `. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4                  `-

Re: [PATCH] use spinlock instead of binary mutex in idt77252 driver

[Kyle Moffett]

On Apr 22, 2007, at 17:39:59, Matthias Kaehlcke wrote:
> use spinlock instead of binary mutex in idt77252 driver

I think you really meant: "Use mutex instead of binary semaphore in  
idt77252 driver", since this is a binary semaphore (not a mutex,  
which are always binary):
> -	struct semaphore	mutex;

and this is a mutex, not a spinlock:
> +	struct mutex		mutex;

Everything else looks good though

Cheers,
Kyle Moffett

Re: [PATCH] use spinlock instead of binary mutex in idt77252 driver

[Matthias Kaehlcke]

El Sun, Apr 22, 2007 at 07:50:36PM -0400 Kyle Moffett ha dit:

> On Apr 22, 2007, at 17:39:59, Matthias Kaehlcke wrote:
> >use spinlock instead of binary mutex in idt77252 driver
> 
> I think you really meant: "Use mutex instead of binary semaphore in  
> idt77252 driver", since this is a binary semaphore (not a mutex,  
> which are always binary):
> >-	struct semaphore	mutex;
> 
> and this is a mutex, not a spinlock:
> >+	struct mutex		mutex;
> 
> Everything else looks good though

you're totally right. like in another patch i sent at the same time i
messed up the description. as you point out it should read "Use mutex
instead of binary semaphore in idt77252 driver". in the last days i
reported some spinlock related bugs, i suppose that made me write
spinlock instead of mutex ...

thanks for your comments

-- 
Matthias Kaehlcke
Linux Application Developer
Barcelona

                If you don't know where you are going,
               you will probably end up somewhere else
                         (Laurence J. Peter)
                                                                 .''`.
    using free software / Debian GNU/Linux | http://debian.org  : :'  :
                                                                `. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4                  `-

Re: [PATCH] use spinlock instead of binary mutex in idt77252 driver

[Eddie C. Dost]

Hi,

Please note that the semaphore is used to lock the idt77252 config
tables among multiple users including atmsigd even on single processor
machines. Does this work with mutexes?

Best regards,
Eddie

On Mon, Apr 23, 2007 at 08:55:20AM +0200, Matthias Kaehlcke wrote:
> El Sun, Apr 22, 2007 at 07:50:36PM -0400 Kyle Moffett ha dit:
> 
> > On Apr 22, 2007, at 17:39:59, Matthias Kaehlcke wrote:
> > >use spinlock instead of binary mutex in idt77252 driver
> > 
> > I think you really meant: "Use mutex instead of binary semaphore in  
> > idt77252 driver", since this is a binary semaphore (not a mutex,  
> > which are always binary):
> > >-	struct semaphore	mutex;
> > 
> > and this is a mutex, not a spinlock:
> > >+	struct mutex		mutex;
> > 
> > Everything else looks good though
> 
> you're totally right. like in another patch i sent at the same time i
> messed up the description. as you point out it should read "Use mutex
> instead of binary semaphore in idt77252 driver". in the last days i
> reported some spinlock related bugs, i suppose that made me write
> spinlock instead of mutex ...
> 
> thanks for your comments
> 
> -- 
> Matthias Kaehlcke
> Linux Application Developer
> Barcelona
> 
>                 If you don't know where you are going,
>                you will probably end up somewhere else
>                          (Laurence J. Peter)
>                                                                  .''`.
>     using free software / Debian GNU/Linux | http://debian.org  : :'  :
>                                                                 `. `'`
> gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4                  `-

-- 
Christian Dost
Technical Director R&D

ATecoM realizing visions GmbH
Pascalstrasse 67
D-52076 Aachen
Germany
Fon: +49/2408/9596-0
Fax: +49/2408/9596-900
Email: ecd@atecom.com
URL: http://www.atecom.com

Amtsgericht Aachen, HRB# 5941, Sitz: Aachen
Geschaeftsfuehrer: Bernd Leister, Robert Bonnie
USt.-Id. Nr. / VATID: DE 811 66 99 76
Steuernummer/Tax-ID: 225/5775/0558

Re: [PATCH] use spinlock instead of binary mutex in idt77252 driver

[Matthias Kaehlcke]

El Mon, Apr 23, 2007 at 09:16:08AM +0200 Eddie C. Dost ha dit:
 
> Please note that the semaphore is used to lock the idt77252 config
> tables among multiple users including atmsigd even on single processor
> machines. Does this work with mutexes?

afaik mutexes have the same behaviour as binary semaphores that are
used as mutexes (always locked and unlocked by the same
process/thread):

".. the semaphore type can officially be considered to be on its way
out. New code should not use semaphores, and old code which uses
semaphores as mutexes should be converted over when an opportunity
presents itself."

http://lwn.net/Articles/167034/

please correct me if i'm wrong, i'm just doing my first steps with
linux kernel development

regards

-- 
Matthias Kaehlcke
Linux Application Developer
Barcelona

    C treats you like a consenting adult. Pascal treats you like a
          naughty child. Ada treats you like a criminal
                      (Bruce Powel Douglass)
                                                                 .''`.
    using free software / Debian GNU/Linux | http://debian.org  : :'  :
                                                                `. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4                  `-

Re: [PATCH] use spinlock instead of binary mutex in idt77252 driver

[Eddie C. Dost]

Hi,

as long as mutexes are not converted to nop when CONFIG_SMP is not
defined (I don't know what current kernels do), this is of course
correct. You need to verify the headerfiles for the above.

Regards,
Eddie

On Mon, Apr 23, 2007 at 09:40:26AM +0200, Matthias Kaehlcke wrote:
> El Mon, Apr 23, 2007 at 09:16:08AM +0200 Eddie C. Dost ha dit:
>  
> > Please note that the semaphore is used to lock the idt77252 config
> > tables among multiple users including atmsigd even on single processor
> > machines. Does this work with mutexes?
> 
> afaik mutexes have the same behaviour as binary semaphores that are
> used as mutexes (always locked and unlocked by the same
> process/thread):
> 
> ".. the semaphore type can officially be considered to be on its way
> out. New code should not use semaphores, and old code which uses
> semaphores as mutexes should be converted over when an opportunity
> presents itself."
> 
> http://lwn.net/Articles/167034/
> 
> please correct me if i'm wrong, i'm just doing my first steps with
> linux kernel development
> 
> regards
> 
> -- 
> Matthias Kaehlcke
> Linux Application Developer
> Barcelona
> 
>     C treats you like a consenting adult. Pascal treats you like a
>           naughty child. Ada treats you like a criminal
>                       (Bruce Powel Douglass)
>                                                                  .''`.
>     using free software / Debian GNU/Linux | http://debian.org  : :'  :
>                                                                 `. `'`
> gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4                  `-

-- 
Christian Dost
Technical Director R&D

ATecoM realizing visions GmbH
Pascalstrasse 67
D-52076 Aachen
Germany
Fon: +49/2408/9596-0
Fax: +49/2408/9596-900
Email: ecd@atecom.com
URL: http://www.atecom.com

Amtsgericht Aachen, HRB# 5941, Sitz: Aachen
Geschaeftsfuehrer: Bernd Leister, Robert Bonnie
USt.-Id. Nr. / VATID: DE 811 66 99 76
Steuernummer/Tax-ID: 225/5775/0558

Re: [PATCH] use spinlock instead of binary mutex in idt77252 driver

[Matthias Kaehlcke]

hi,

El Mon, Apr 23, 2007 at 09:40:19AM +0200 Eddie C. Dost ha dit:

> as long as mutexes are not converted to nop when CONFIG_SMP is not
> defined (I don't know what current kernels do), this is of course
> correct. You need to verify the headerfiles for the above.

i just checked this, neither the mutex header nor implementation files
handle things different for CONFIG_SMP. 

thanks for your comments

> On Mon, Apr 23, 2007 at 09:40:26AM +0200, Matthias Kaehlcke wrote:
> > El Mon, Apr 23, 2007 at 09:16:08AM +0200 Eddie C. Dost ha dit:
> >  
> > > Please note that the semaphore is used to lock the idt77252 config
> > > tables among multiple users including atmsigd even on single processor
> > > machines. Does this work with mutexes?
> > 
> > afaik mutexes have the same behaviour as binary semaphores that are
> > used as mutexes (always locked and unlocked by the same
> > process/thread):
> > 
> > ".. the semaphore type can officially be considered to be on its way
> > out. New code should not use semaphores, and old code which uses
> > semaphores as mutexes should be converted over when an opportunity
> > presents itself."
> > 
> > http://lwn.net/Articles/167034/
> > 
> > please correct me if i'm wrong, i'm just doing my first steps with
> > linux kernel development

-- 
Matthias Kaehlcke
Linux Application Developer
Barcelona

             You must have a plan. If you don't have a plan,
               you'll become part of somebody else's plan
                                                                 .''`.
    using free software / Debian GNU/Linux | http://debian.org  : :'  :
                                                                `. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4                  `-

Re: [PATCH] use spinlock instead of binary mutex in idt77252 driver

[Satyam Sharma]

Hi,

On 4/23/07, Eddie C. Dost <ecd@atecom.com> wrote:
> as long as mutexes are not converted to nop when CONFIG_SMP is not
> defined (I don't know what current kernels do), this is of course
> correct. You need to verify the headerfiles for the above.

Yes, even on UP different threads accessing the same data could race.
Mutexes (== binary semaphores) are *required* to synchronize access to
shared data.

You might be confusing mutexes with spinlocks. Spinlocks _are_
compiled away on UP (actually !CONFIG_SMP && !CONFIG_PREEMPT) kernels,
but that is still safe because spinlocks are busy-waiting loops
(unlike mutexes and semaphores that block) and hence no thread is
allowed to sleep when holding a spinlock.

> On Mon, Apr 23, 2007 at 09:40:26AM +0200, Matthias Kaehlcke wrote:
> > El Mon, Apr 23, 2007 at 09:16:08AM +0200 Eddie C. Dost ha dit:
> > > Please note that the semaphore is used to lock the idt77252 config
> > > tables among multiple users including atmsigd even on single processor
> > > machines. Does this work with mutexes?
> >
> > afaik mutexes have the same behaviour as binary semaphores that are
> > used as mutexes (always locked and unlocked by the same
> > process/thread):

Mutexes / binary semaphores / spinlocks are used to synchronize access
to shared data by *multiple* threads ... there is no meaning in
locking access to something if we know only one thread will ever touch
it.

Cheers,
S

Re: [PATCH] use spinlock instead of binary mutex in idt77252 driver

[Christoph Hellwig]

On Sun, Apr 22, 2007 at 07:50:36PM -0400, Kyle Moffett wrote:
> On Apr 22, 2007, at 17:39:59, Matthias Kaehlcke wrote:
> >use spinlock instead of binary mutex in idt77252 driver
> 
> I think you really meant: "Use mutex instead of binary semaphore in  
> idt77252 driver", since this is a binary semaphore (not a mutex,  
> which are always binary):

And the binary semaphore terminology i a little confusing. struct
semaphore is a full counting semaphore that is only used as a binary
semaphore if we want to speak in CS terms.  Than everyone else just
caled them semaphore before these patches started to show up :)

Re: [PATCH] use spinlock instead of binary mutex in idt77252 driver

[Matthias Kaehlcke]

El Mon, Apr 23, 2007 at 09:17:53AM +0100 Christoph Hellwig ha dit:

> On Sun, Apr 22, 2007 at 07:50:36PM -0400, Kyle Moffett wrote:
> > On Apr 22, 2007, at 17:39:59, Matthias Kaehlcke wrote:
> > >use spinlock instead of binary mutex in idt77252 driver
> > 
> > I think you really meant: "Use mutex instead of binary semaphore in  
> > idt77252 driver", since this is a binary semaphore (not a mutex,  
> > which are always binary):
> 
> And the binary semaphore terminology i a little confusing. struct
> semaphore is a full counting semaphore that is only used as a binary
> semaphore if we want to speak in CS terms.  Than everyone else just
> caled them semaphore before these patches started to show up :)

i'll take your suggestion into account for future patches. my
intention behind the usage of the term binary semaphore was to be more
precise, but i agree that it can be confusing

-- 
Matthias Kaehlcke
Linux Application Developer
Barcelona

    I am incapable of conceiving infinity, and yet I do not accept finity
                          (Simone de Beauvoir)
                                                                 .''`.
    using free software / Debian GNU/Linux | http://debian.org  : :'  :
                                                                `. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4                  `-

Re: [PATCH] use spinlock instead of binary mutex in idt77252 driver

[Satyam Sharma]

On 4/23/07, Matthias Kaehlcke <matthias.kaehlcke@gmail.com> wrote:
> use spinlock instead of binary mutex in idt77252 driver
>
> Signed-off-by: Matthias Kaehlcke <matthias.kaehlcke@gmail.com>
>
> --
>
> diff --git a/drivers/atm/idt77252.c b/drivers/atm/idt77252.c
> index b4b8014..e3cf141 100644
> --- a/drivers/atm/idt77252.c
> +++ b/drivers/atm/idt77252.c
> @@ -2430,7 +2430,7 @@ idt77252_open(struct atm_vcc *vcc)
>
>         set_bit(ATM_VF_ADDR, &vcc->flags);
>
> -       down(&card->mutex);
> +       mutex_lock(&card->mutex);

Note that you're actually replacing a semaphore with a mutex here (and
not a mutex with a spinlock). I guess that should be fine and
desirable as long as the semaphore was indeed being used a mutex
(binary) in this code.