fixed spinlock use in hysdn_log_close()

PATCH In review Matthias Kaehlcke 2 participants 3 messages Apr 11, 2007 – Apr 21, 2007 Kernel janitors

Share & tools

Matthias Kaehlcke

author

2007-04-11 17:09 UTC #0 lore raw

PATCH 1/1 fixed spinlock use in hysdn_log_close()

fixed incorrect spinlock use in hysdn_log_close(). the function declared a spinlock on the stack and used it to 'protect' a shared driver structure. the patch removes the declaration of hysdn_lock and uses card->hysdn_lock instead.

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

---

drivers/isdn/hysdn/hysdn_proclog.c +2 -3

index f7e83a8..32f0b75 100644
@@ -299,7 +299,6 @@ hysdn_log_close(struct inode *ino, struct file *filep)
 	hysdn_card *card;
 	int retval = 0;
 	unsigned long flags;
-	spinlock_t hysdn_lock = SPIN_LOCK_UNLOCKED;
 
 	lock_kernel();
 	if ((filep->f_mode & (FMODE_READ | FMODE_WRITE)) = FMODE_WRITE) {

@@ -309,7 +308,7 @@ hysdn_log_close(struct inode *ino, struct file *filep)
 		/* read access -> log/debug read, mark one further file as closed */
 
 		pd = NULL;
-		spin_lock_irqsave(&hysdn_lock, flags);
+		spin_lock_irqsave(&card->hysdn_lock, flags);
 		inf = *((struct log_data **) filep->private_data);	/* get first log entry */
 		if (inf)
 			pd = (struct procdata *) inf->proc_ctrl;	/* still entries there */

@@ -332,7 +331,7 @@ hysdn_log_close(struct inode *ino, struct file *filep)
 			inf->usage_cnt--;	/* decrement usage count for buffers */
 			inf = inf->next;
 		}
-		spin_unlock_irqrestore(&hysdn_lock, flags);
+		spin_unlock_irqrestore(&card->hysdn_lock, flags);
 
 		if (pd)
 			if (pd->if_used <= 0)	/* delete buffers if last file closed */

Signature · 16 lines

Matthias Kaehlcke
Linux Application 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                  `-
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors

Andrew Morton

2007-04-21 00:42 UTC #1 lore raw

Quoted 5 lines · On Sat, 14 Apr 2007 07:09:07 +0200 Matthias Kaehlcke…

fixed incorrect spinlock use in hysdn_log_close(). the function declared a spinlock on the stack and used it to 'protect' a shared driver structure. the patch removes the declaration of hysdn_lock and uses card->hysdn_lock instead.

Interesting.

Quoted 15 lines

---

drivers/isdn/hysdn/hysdn_proclog.c +0 -1

index f7e83a8..32f0b75 100644
@@ -299,7 +299,6 @@ hysdn_log_close(struct inode *ino, struct file *filep)
 	hysdn_card *card;
 	int retval = 0;
 	unsigned long flags;
-	spinlock_t hysdn_lock = SPIN_LOCK_UNLOCKED;
 
 	lock_kernel();
 	if ((filep->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_WRITE) {

drivers/isdn/hysdn/hysdn_proclog.c hysdn_log_close(struct inode *ino, struct file *filep) · line 311

Quoted 5 lines

drivers/isdn/hysdn/hysdn_proclog.c +1 -1

@@ -309,7 +308,7 @@ hysdn_log_close(struct inode *ino, struct file *filep)
 		/* read access -> log/debug read, mark one further file as closed */
 
 		pd = NULL;
-		spin_lock_irqsave(&hysdn_lock, flags);
+		spin_lock_irqsave(&card->hysdn_lock, flags);

I guess it won't hurt - are you actually able to test this code?

afaict most of the data in there is locked with lock_kernel(), if it's locked at all.

If you had some runtime problem and this patch fixed it then fine. If however you're not able to test this code then perhaps the safest option is to simply remove that locking altogether, which is pretty much a runtime-equivalent change.

Matthias Kaehlcke

author

2007-04-21 15:08 UTC #2 lore raw

El Fri, Apr 20, 2007 at 05:41:46PM -0700 Andrew Morton ha dit:

Quoted 34 lines

Quoted 5 lines · On Sat, 14 Apr 2007 07:09:07 +0200 Matthias Kaehlcke…
fixed incorrect spinlock use in hysdn_log_close(). the function declared a spinlock on the stack and used it to 'protect' a shared driver structure. the patch removes the declaration of hysdn_lock and uses card->hysdn_lock instead.
Interesting.
Quoted 15 lines
---
drivers/isdn/hysdn/hysdn_proclog.c +0 -1
index f7e83a8..32f0b75 100644
@@ -299,7 +299,6 @@ hysdn_log_close(struct inode *ino, struct file *filep)
 	hysdn_card *card;
 	int retval = 0;
 	unsigned long flags;
-	spinlock_t hysdn_lock = SPIN_LOCK_UNLOCKED;
 
 	lock_kernel();
 	if ((filep->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_WRITE) {
drivers/isdn/hysdn/hysdn_proclog.c hysdn_log_close(struct inode *ino, struct file *filep) · line 311
Quoted 5 lines
drivers/isdn/hysdn/hysdn_proclog.c +1 -1
@@ -309,7 +308,7 @@ hysdn_log_close(struct inode *ino, struct file *filep)
 		/* read access -> log/debug read, mark one further file as closed */
 
 		pd = NULL;
-		spin_lock_irqsave(&hysdn_lock, flags);
+		spin_lock_irqsave(&card->hysdn_lock, flags);
I guess it won't hurt - are you actually able to test this code?

Quoted 7 lines

afaict most of the data in there is locked with lock_kernel(), if it's locked at all.
If you had some runtime problem and this patch fixed it then fine. If however you're not able to test this code then perhaps the safest option is to simply remove that locking altogether, which is pretty much a runtime-equivalent change.

i'm sorry, but i can't test it, i don't have the device. initially the patch was a kernel janitorial with the idea to replace the assignation to SPIN_LOCK_UNLOCKED by DEFINE_SPINLOCK. when i sent the first patch to the kj list someone pointed me to the spinlock on stack.

i also wondered about the use of the spinlock after aquiring the big kernel lock, i agree that it's probable the safest to remove the spinlockaltogether. below you find a patch that does this.

thanks for your comments

remove spinlock hysdn_lock (declared on the stack) from HYSDN driver. the code region is already protected by the big kernel lock.

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

drivers/isdn/hysdn/hysdn_proclog.c +0 -4

index f7e83a8..ab18965 100644
@@ -298,8 +298,6 @@ hysdn_log_close(struct inode *ino, struct file *filep)
 	struct procdata *pd;
 	hysdn_card *card;
 	int retval = 0;
-	unsigned long flags;
-	spinlock_t hysdn_lock = SPIN_LOCK_UNLOCKED;
 
 	lock_kernel();
 	if ((filep->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_WRITE) {

@@ -309,7 +307,6 @@ hysdn_log_close(struct inode *ino, struct file *filep)
 		/* read access -> log/debug read, mark one further file as closed */
 
 		pd = NULL;
-		spin_lock_irqsave(&hysdn_lock, flags);
 		inf = *((struct log_data **) filep->private_data);	/* get first log entry */
 		if (inf)
 			pd = (struct procdata *) inf->proc_ctrl;	/* still entries there */

@@ -332,7 +329,6 @@ hysdn_log_close(struct inode *ino, struct file *filep)
 			inf->usage_cnt--;	/* decrement usage count for buffers */
 			inf = inf->next;
 		}
-		spin_unlock_irqrestore(&hysdn_lock, flags);
 
 		if (pd)
 			if (pd->if_used <= 0)	/* delete buffers if last file closed */

Signature · 10 lines

Matthias Kaehlcke
Linux Application Developer
Barcelona

               If sharing a thing in no way diminishes it,
               it is not rightly owned if it is not shared
                                                                 .''`.
    using free software / Debian GNU/Linux | http://debian.org  : :'  :
                                                                `. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4                  `-