Rocket port:use mutex instead of binary semaphore
Subsystems (2)
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
@@ -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
@@ -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
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
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 uninterruptiblemutex_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