Messages in this thread Patch in this message |  | | Date | Thu, 12 Oct 2000 13:12:13 -0400 | From | Yong Chi <> | Subject | Re: TODO: drivers/pcmcia/ds.c: ds_read & ds_write. SMP locks are missing fix - TAKE 2 |
| |
Take 2 based on semaphore -)
Jan-Simon Pendry wrote:
> holding a spin lock across a (potential) sleep is a bug - it can > lead to deadlock. > > jan-simon. > > Jakub Jelinek wrote: > > > > On Thu, Oct 12, 2000 at 11:38:11AM -0400, Yong Chi wrote: > > > Hopefully this will do for SMP locks. =) > > > > Holding a spinlock for this long (especially when you might sleep there in two > > places (interruptible_sleep_on, put_user)) is basically a bad idea. > > spinlocks are designed to be holded only for short time. > > Either protect just a small critical section with a spinlock, or use > > semaphores. > > > > > --- ds.c.bak Wed Oct 11 13:05:16 2000 > > > +++ ds.c Thu Oct 12 11:25:20 2000 > > > @@ -95,6 +95,7 @@ > > > u_int user_magic; > > > int event_head, event_tail; > > > event_t event[MAX_EVENTS]; > > > + spinlock_t lock; > > > struct user_info_t *next; > > > } user_info_t; > > > > > > @@ -567,6 +568,7 @@ > > > user->event_tail = user->event_head = 0; > > > user->next = s->user; > > > user->user_magic = USER_MAGIC; > > > + spin_lock_init(&user->lock); > > > s->user = user; > > > file->private_data = user; > > > > > > @@ -616,6 +618,7 @@ > > > socket_t i = MINOR(file->f_dentry->d_inode->i_rdev); > > > socket_info_t *s; > > > user_info_t *user; > > > + ssize_t retval=4; > > > > > > DEBUG(2, "ds_read(socket %d)\n", i); > > > > > > @@ -625,16 +628,23 @@ > > > return -EINVAL; > > > s = &socket_table[i]; > > > user = file->private_data; > > > - if (CHECK_USER(user)) > > > - return -EIO; > > > - > > > + spin_lock(&user->lock); > > > + if (CHECK_USER(user)) { > > > + retval= -EIO; > > > + goto read_out; > > > + } > > > + > > > if (queue_empty(user)) { > > > interruptible_sleep_on(&s->queue); > > > if (signal_pending(current)) > > > - return -EINTR; > > > + retval= -EINTR; > > > + goto read_out; > > > } > > > put_user(get_queued_event(user), (int *)buf); > > > - return 4; > > > + > > > +read_out: > > > + spin_unlock(&user->lock); > > > + return retval; > > > } /* ds_read */ > > > > > > /*====================================================================*/ > > > @@ -645,6 +655,7 @@ > > > socket_t i = MINOR(file->f_dentry->d_inode->i_rdev); > > > socket_info_t *s; > > > user_info_t *user; > > > + ssize_t retval=4; > > > > > > DEBUG(2, "ds_write(socket %d)\n", i); > > > > > > @@ -656,18 +667,25 @@ > > > return -EBADF; > > > s = &socket_table[i]; > > > user = file->private_data; > > > - if (CHECK_USER(user)) > > > - return -EIO; > > > + spin_lock(&user->lock); > > > + if (CHECK_USER(user)) { > > > + retval= -EIO; > > > + goto write_out; > > > + } > > > > > > if (s->req_pending) { > > > s->req_pending--; > > > get_user(s->req_result, (int *)buf); > > > if ((s->req_result != 0) || (s->req_pending == 0)) > > > wake_up_interruptible(&s->request); > > > - } else > > > - return -EIO; > > > + } else { > > > + retval= -EIO; > > > + goto write_out; > > > + } > > > > > > - return 4; > > > +write_out: > > > + spin_unlock(&user->lock); > > > + return retval; > > > } /* ds_write */ > > > > > > /*====================================================================*/ > > > > Jakub > > - > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > > the body of a message to majordomo@vger.kernel.org > > Please read the FAQ at http://www.tux.org/lkml/ --- ds.c.bak Wed Oct 11 13:05:16 2000 +++ ds.c Thu Oct 12 13:05:28 2000 @@ -95,6 +95,7 @@ u_int user_magic; int event_head, event_tail; event_t event[MAX_EVENTS]; + struct semaphore sem; struct user_info_t *next; } user_info_t; @@ -567,6 +568,7 @@ user->event_tail = user->event_head = 0; user->next = s->user; user->user_magic = USER_MAGIC; + init_MUTEX(&user->sem); s->user = user; file->private_data = user; @@ -616,6 +618,7 @@ socket_t i = MINOR(file->f_dentry->d_inode->i_rdev); socket_info_t *s; user_info_t *user; + ssize_t retval=4; DEBUG(2, "ds_read(socket %d)\n", i); @@ -625,16 +628,26 @@ return -EINVAL; s = &socket_table[i]; user = file->private_data; - if (CHECK_USER(user)) - return -EIO; - + down_interruptible(&user->sem); + if (signal_pending(current)) + return -EINTR; + + if (CHECK_USER(user)) { + retval= -EIO; + goto read_out; + } + if (queue_empty(user)) { interruptible_sleep_on(&s->queue); if (signal_pending(current)) - return -EINTR; + retval= -EINTR; + goto read_out; } put_user(get_queued_event(user), (int *)buf); - return 4; + +read_out: + up(&user->sem); + return retval; } /* ds_read */ /*====================================================================*/ @@ -645,6 +658,7 @@ socket_t i = MINOR(file->f_dentry->d_inode->i_rdev); socket_info_t *s; user_info_t *user; + ssize_t retval=4; DEBUG(2, "ds_write(socket %d)\n", i); @@ -656,18 +670,28 @@ return -EBADF; s = &socket_table[i]; user = file->private_data; - if (CHECK_USER(user)) - return -EIO; + down_interruptible(&user->sem); + if (signal_pending(current)) + return -EINTR; + + if (CHECK_USER(user)) { + retval= -EIO; + goto write_out; + } if (s->req_pending) { s->req_pending--; get_user(s->req_result, (int *)buf); if ((s->req_result != 0) || (s->req_pending == 0)) wake_up_interruptible(&s->request); - } else - return -EIO; + } else { + retval= -EIO; + goto write_out; + } - return 4; +write_out: + up(&user->sem); + return retval; } /* ds_write */ /*====================================================================*/ |  |