Skip to contentSkip to conversation

Rocket port:use mutex instead of binary semaphore

PATCH In review Milind Arun Choudhary 2 participants 3 messages Apr 16, 2007

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Rocket port:use mutex instead of binary semaphore

Use mutex instead of binary semaphore for mutual exclusion.

---
 rocket.c     |   22 ++++++++++++----------
 rocket_int.h |    2 +-
 2 files changed, 13 insertions(+), 11 deletions(-)
drivers/char/rocket.c +12 -10
index 76357c8..07255c3 100644
@@ -35,7 +35,7 @@
  *    is data to be transmitted.  Protected by atomic bit operations.
  * -  rp_num_ports, int indicating number of open ports, protected by atomic operations.
  * 
- * rp_write() and rp_write_char() functions use a per port semaphore to protect against
+ * rp_write() and rp_write_char() functions use a per port mutex to protect against
  * simultaneous access to the same port by more than one process.
  */
 
@@ -67,7 +67,7 @@
 
 #ifdef MODVERSIONS
 #include <config/modversions.h>
-#endif				
+#endif
 
 #include <linux/module.h>
 #include <linux/errno.h>
@@ -93,7 +93,7 @@
 #include <asm/atomic.h>
 #include <linux/bitops.h>
 #include <linux/spinlock.h>
-#include <asm/semaphore.h>
+#include <linux/mutex.h>
 #include <linux/init.h>
 
 /****** RocketPort includes ******/
@@ -702,7 +702,7 @@ static void init_r_port(int board, int aiop, int chan, struct pci_dev *pci_dev)
 		}
 	}
 	spin_lock_init(&info->slock);
-	sema_init(&info->write_sem, 1);
+	mutex_init(&info->write_lock);
 	rp_table[line] = info;
 	if (pci_dev)
 		tty_register_device(rocket_driver, line, &pci_dev->dev);
@@ -1661,8 +1661,8 @@ static void rp_put_char(struct tty_struct *tty, unsigned char ch)
 	if (rocket_paranoia_check(info, "rp_put_char"))
 		return;
 
-	/*  Grab the port write semaphore, locking out other processes that try to write to this port */
-	down(&info->write_sem);
+	/*  Grab the port write mutex, locking out other processes that try to write to this port */
+	mutex_lock(&info->write_lock);
 
 #ifdef ROCKET_DEBUG_WRITE
 	printk(KERN_INFO "rp_put_char %c...", ch);
@@ -1684,12 +1684,12 @@ static void rp_put_char(struct tty_struct *tty, unsigned char ch)
 		info->xmit_fifo_room--;
 	}
 	spin_unlock_irqrestore(&info->slock, flags);
-	up(&info->write_sem);
+	mutex_unlock(&info->write_lock);
 }
 
 /*
  *  Exception handler - write routine, called when user app writes to the device.
- *  A per port write semaphore is used to protect from another process writing to
+ *  A per port write mutex is used to protect from another process writing to
  *  this port at the same time.  This other process could be running on the other CPU
  *  or get control of the CPU if the copy_from_user() blocks due to a page fault (swapped out). 
  *  Spinlocks protect the info xmit members.
@@ -1706,7 +1706,9 @@ static int rp_write(struct tty_struct *tty,
 	if (count <= 0 || rocket_paranoia_check(info, "rp_write"))
 		return 0;
 
-	down_interruptible(&info->write_sem);
+	if(mutex_lock_interruptible(&info->write_lock)){
+		return -ERESTARTSYS;
+	}
 
 #ifdef ROCKET_DEBUG_WRITE
 	printk(KERN_INFO "rp_write %d chars...", count);
@@ -1777,7 +1779,7 @@ end:
 		wake_up_interruptible(&tty->poll_wait);
 #endif
 	}
-	up(&info->write_sem);
+	mutex_unlock(&info->write_lock);
 	return retval;
 }
 
drivers/char/rocket_int.h +1 -1
index 3a8bcc8..e91a1cb 100644
@@ -1171,7 +1171,7 @@ struct r_port {
 	struct wait_queue *close_wait;
 #endif
 	spinlock_t slock;
-	struct semaphore write_sem;
+	struct mutex write_lock;
 };
 
 #define RPORT_MAGIC 0x525001
Signature · 1 line
Milind Arun Choudhary
#1 lore raw

drivers/char/rocket.c static int rp_write(struct tty_struct *tty, · line 1711

Quoted 4 lines
drivers/char/rocket.c +3 -1
-	down_interruptible(&info->write_sem);
+	if(mutex_lock_interruptible(&info->write_lock)){
+		return -ERESTARTSYS;
+	}

1) This is a semantic change. Of course using down_interruptible() without checking the return value is almost certainly a bug, but have you thought about whether returning ERESTARTSYS is correct here? If you have, then please include the reasoning in your patch description. (Another possibility would be to just use an uninterruptible mutex_lock())

2) The coding style for the if statement is not quite right. The correct way is to do

	if (condition)
		one_liner;

(note the space between the 'if' and the '(', and no braces used)

- R.
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors

#2 lore raw
Quoted 10 lines · Roland Dreier

drivers/char/rocket.c static int rp_write(struct tty_struct *tty, · line 1711

Quoted 4 lines
drivers/char/rocket.c +3 -1
-	down_interruptible(&info->write_sem);
+	if(mutex_lock_interruptible(&info->write_lock)){
+		return -ERESTARTSYS;
+	}

1) This is a semantic change. Of course using down_interruptible() without checking the return value is almost certainly a bug, but have you thought about whether returning ERESTARTSYS is correct here? If you have, then please include the reasoning in your patch description. (Another possibility would be to just use an uninterruptible mutex_lock())

no,frankly not much..
and something which i didn't understand is
why a down in one control path & an down_interruptible in other?

but i had in mind,which you rightly pointed out
handling the return from _interruptible.

i have some bits and pieces of information regarding thie, searching for more, and correction to my current understanding,
so that i can make something meaningfull out of it.

earlier kernels used to return in EINTR & it was up to the use space to handle the interrupted system call. ..reissue..

then ERESTARTSYS mechanism was added and the kernel could restart the syscall on its own

so its up to us whether we want the interruption to be taken care by the userspace or kernel.
i.e return EINTR or ERESTARTSYS respectively.
CMIIW.

also i read somewhere that
ERESTARTSYS should never be seen by user space code. The kernel should either restart the current syscall or convert the code to EINTR before returning to user space. If you are seeing ERESTARTSYS in user space then it is a kernel bug.

Quoted 8 lines

2) The coding style for the if statement is not quite right. The correct way is to do

	if (condition)
		one_liner;

(note the space between the 'if' and the '(', and no braces used)

ok.. got it..

Signature · 1 line
Milind Arun Choudhary