Re: TODO: drivers/pcmcia/ds.c: ds_read
Yong Chi wrote:
> Hopefully this will do for SMP locks. =)
You must not hold a spinlock across put_user - instead you must copy the
get_queued_event(user) into a local variable, spinunlock and then copy
it to userspace.
Compare drivers/sbus/char/sunkbd.c, function kbd_read from 2.2 and 2.4:
2.2.17 is bad, 2.4.0 is fixed.
>
> Todo list also said that on UP, sleep_on() use is unsafe. It uses
> "interruptible_sleep_on()" and "wake_up_interruptible()" calls. Are they
> not safe on UP?
>
I depends: there are exactly 2 safe uses for sleep_on(), all other
combinations can lock up:
1) The wake-up occurs from process context (neither bh nor interrupt),
and _both_ the thread that goes to sleep and the thread that wakes up
use lock_kernel().
2) If the wake-up occurs from interrupt context (only real interrupt or
bottom half, NOT from tasklet/softirq), then the thread that goes to
sleep must protect itself with the global cli lock.
wake_up_sleeper()
{
new_data = 1;
wake_up(_queue);
}
go_to_sleep()
{
/* local interrupts must be enabled */
cli();
if(!new_data) {
sleep_on(_queue);
}
sti();
}
IIRC handle_event is called from interrupt context, thus a wake-up can
happen from within an interrupt, but there is no cli() before the
sleep_on() --> lock-up, even on UP possible.
But do not add cli() - remove sleep_on() and replace it with something
like wait_event_irq() [from include/linux/raid/md_k.h]
--
Manfred
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
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_intuser_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(>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 = _table[i];
> > > user = file->private_data;
> > > -if (CHECK_USER(user))
> > > - return -EIO;
> > > -
> > > +spin_lock(>lock);
> > > +if (CHECK_USER(user)) {
> > > + retval= -EIO;
> > > +goto read_out;
> > > +}
> > > +
> > > if (queue_empty(user)) {
> > > interruptible_sleep_on(>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(>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 = _table[i];
> > > user = file->private_data;
> > > -if (CHECK_USER(user))
> > > - return -EIO;
> > > +spin_lock(>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(>request);
> > > -} else
> > > - return -EIO;
> > > +} else {
> > > + retval= -EIO;
> > > + goto write_out;
> > > +}
> > >
> > > -return 4;
> > > +write_out:
> > > +spin_unlock(>lock);
> > > +return retval;
> > > } /* ds_write */
> > >
> > > /**/
> >
> > Jakub
> > -
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to [EMAIL PROTECTED]
> > Please read the FAQ at http://www.tux.org/lkml/
--- ds.c.bakWed Oct 11 13:05:16 2000
+++ ds.cThu Oct 12 13:05:28 2000
@@ -95,6 +95,7 @@
u_int user_magic;
intevent_head, event_tail;
event_tevent[MAX_EVENTS];
+struct semaphoresem;
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(>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 = _table[i];
user = file->private_data;
-if (CHECK_USER(user))
- return -EIO;
-
+down_interruptible(>sem);
+if (signal_pending(current))
+return -EINTR;
+
+if (CHECK_USER(user)) {
+ retval= -EIO;
+goto read_out;
+}
+
if (queue_empty(user)) {
interruptible_sleep_on(>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(>sem);
+return retval;
} /*
Re: TODO: drivers/pcmcia/ds.c: ds_read & ds_write. SMP locks are missing fix
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_intuser_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(>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 = _table[i];
> > user = file->private_data;
> > -if (CHECK_USER(user))
> > - return -EIO;
> > -
> > +spin_lock(>lock);
> > +if (CHECK_USER(user)) {
> > + retval= -EIO;
> > +goto read_out;
> > +}
> > +
> > if (queue_empty(user)) {
> > interruptible_sleep_on(>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(>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 = _table[i];
> > user = file->private_data;
> > -if (CHECK_USER(user))
> > - return -EIO;
> > +spin_lock(>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(>request);
> > -} else
> > - return -EIO;
> > +} else {
> > + retval= -EIO;
> > + goto write_out;
> > +}
> >
> > -return 4;
> > +write_out:
> > +spin_unlock(>lock);
> > +return retval;
> > } /* ds_write */
> >
> > /**/
>
> Jakub
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [EMAIL PROTECTED]
> Please read the FAQ at http://www.tux.org/lkml/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
Re: TODO: drivers/pcmcia/ds.c: ds_read & ds_write. SMP locks are missing fix
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_intuser_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(>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 = _table[i];
> user = file->private_data;
> -if (CHECK_USER(user))
> - return -EIO;
> -
> +spin_lock(>lock);
> +if (CHECK_USER(user)) {
> + retval= -EIO;
> +goto read_out;
> +}
> +
> if (queue_empty(user)) {
> interruptible_sleep_on(>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(>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 = _table[i];
> user = file->private_data;
> -if (CHECK_USER(user))
> - return -EIO;
> +spin_lock(>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(>request);
> -} else
> - return -EIO;
> +} else {
> + retval= -EIO;
> + goto write_out;
> +}
>
> -return 4;
> +write_out:
> +spin_unlock(>lock);
> +return retval;
> } /* ds_write */
>
> /**/
Jakub
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
Re: TODO: drivers/pcmcia/ds.c: ds_read ds_write. SMP locks are missing fix
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_intuser_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 [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
Re: TODO: drivers/pcmcia/ds.c: ds_read ds_write. SMP locks are missing fix
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_intuser_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 [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
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_intuser_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 [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
--- ds.c.bakWed Oct 11 13:05:16 2000
+++ ds.cThu Oct 12 13:05:28 2000
@@ -95,6 +95,7 @@
u_int user_magic;
intevent_head, event_tail;
event_tevent[MAX_EVENTS];
+struct semaphoresem;
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
Re: TODO: drivers/pcmcia/ds.c: ds_read
Yong Chi wrote:
Hopefully this will do for SMP locks. =)
You must not hold a spinlock across put_user - instead you must copy the
get_queued_event(user) into a local variable, spinunlock and then copy
it to userspace.
Compare drivers/sbus/char/sunkbd.c, function kbd_read from 2.2 and 2.4:
2.2.17 is bad, 2.4.0 is fixed.
Todo list also said that on UP, sleep_on() use is unsafe. It uses
"interruptible_sleep_on()" and "wake_up_interruptible()" calls. Are they
not safe on UP?
I depends: there are exactly 2 safe uses for sleep_on(), all other
combinations can lock up:
1) The wake-up occurs from process context (neither bh nor interrupt),
and _both_ the thread that goes to sleep and the thread that wakes up
use lock_kernel().
2) If the wake-up occurs from interrupt context (only real interrupt or
bottom half, NOT from tasklet/softirq), then the thread that goes to
sleep must protect itself with the global cli lock.
wake_up_sleeper()
{
new_data = 1;
wake_up(wait_queue);
}
go_to_sleep()
{
/* local interrupts must be enabled */
cli();
if(!new_data) {
sleep_on(wait_queue);
}
sti();
}
IIRC handle_event is called from interrupt context, thus a wake-up can
happen from within an interrupt, but there is no cli() before the
sleep_on() -- lock-up, even on UP possible.
But do not add cli() - remove sleep_on() and replace it with something
like wait_event_irq() [from include/linux/raid/md_k.h]
--
Manfred
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
Please read the FAQ at http://www.tux.org/lkml/
