The functions sleep_on, sleep_on_timeout, interruptible_sleep_on
and interruptible_sleep_on_timeout have been deprecated for as
long as I can remember, and a number of people have contributed
patches in the past to remove them from various drivers.
This has recently popped up again and I decided to spend some
of my time on tracking down the last users and kill it for good.
The work is somewhat related to the BKL removal I did a couple
of years ago, since most of these drivers were using the BKL
in a way that actually made the sleep_on function safe to call.
As the BKL was converted into mutexes, the semantics changed
slightly and typically opened up a race between checking
a condition and going to sleep.
I don't have any of the hardware that I'm sending the patches
for, so it would be great if someone could test them before
they get applied. Otherwise please review very carefully.
I'm definitely happy for any patches to go into maintainer
trees right away. Obviously the final patch cannot go in
until everything else gets merged first and I suspect there
will be a series of patches for maintainerless drivers that
will go along with it.
Arnd
Arnd Bergmann (30):
ataflop: fix sleep_on races
scsi: atari_scsi: fix sleep_on race
DAC960: remove sleep_on usage
swim3: fix interruptible_sleep_on race
[media] omap_vout: avoid sleep_on race
[media] usbvision: remove bogus sleep_on_timeout
[media] radio-cadet: avoid interruptible_sleep_on race
[media] arv: fix sleep_on race
staging: serqt_usb2: don't use sleep_on
staging: gdm72xx: fix interruptible_sleep_on race
staging: panel: fix interruptible_sleep_on race
parport: fix interruptible_sleep_on race
cris: sync_serial: remove interruptible_sleep_on
tty/amiserial: avoid interruptible_sleep_on
usbserial: stop using interruptible_sleep_on
tty: synclink: avoid sleep_on race
atm: nicstar: remove interruptible_sleep_on_timeout
atm: firestream: fix interruptible_sleep_on race
isdn: pcbit: fix interruptible_sleep_on race
isdn: hisax/elsa: fix sleep_on race in elsa FSM
isdn: divert, hysdn: fix interruptible_sleep_on race
isdn: fix multiple sleep_on races
oss: msnd_pinnacle: avoid interruptible_sleep_on_timeout
oss: midibuf: fix sleep_on races
oss: vwsnd: avoid interruptible_sleep_on
oss: dmasound: kill SLEEP() macro to avoid race
oss: remove last sleep_on users
sgi-xp: open-code interruptible_sleep_on_timeout
char: nwbutton: open-code interruptible_sleep_on
sched: remove sleep_on() and friends
Documentation/DocBook/kernel-hacking.tmpl | 10 ------
arch/cris/arch-v10/drivers/sync_serial.c | 4 ++-
arch/cris/arch-v32/drivers/sync_serial.c | 4 ++-
drivers/atm/firestream.c | 4 +--
drivers/atm/nicstar.c | 13 ++++----
drivers/block/DAC960.c | 34 +++++++++----------
drivers/block/ataflop.c | 16 ++++-----
drivers/block/swim3.c | 18 ++++++----
drivers/char/nwbutton.c | 5 ++-
drivers/char/pcmcia/synclink_cs.c | 4 +--
drivers/isdn/divert/divert_procfs.c | 7 ++--
drivers/isdn/hisax/elsa.c | 9 +++--
drivers/isdn/hisax/elsa_ser.c | 3 +-
drivers/isdn/hysdn/hysdn_proclog.c | 7 ++--
drivers/isdn/i4l/isdn_common.c | 13 +++++---
drivers/isdn/pcbit/drv.c | 6 ++--
drivers/media/platform/arv.c | 4 +--
drivers/media/platform/omap/omap_vout_vrfb.c | 3 +-
drivers/media/radio/radio-cadet.c | 12 +++++--
drivers/media/usb/usbvision/usbvision.h | 4 +--
drivers/misc/sgi-xp/xpc_channel.c | 5 ++-
drivers/parport/share.c | 3 +-
drivers/scsi/atari_scsi.c | 12 +++++--
drivers/staging/gdm72xx/gdm_usb.c | 5 +--
drivers/staging/panel/panel.c | 4 +--
drivers/staging/serqt_usb2/serqt_usb2.c | 17 ++++------
drivers/tty/amiserial.c | 26 ++++++++++-----
drivers/tty/synclink.c | 4 +--
drivers/tty/synclink_gt.c | 4 +--
drivers/tty/synclinkmp.c | 4 +--
drivers/usb/serial/ch341.c | 29 +++++++++++-----
drivers/usb/serial/cypress_m8.c | 49 ++++++++++++++++++----------
drivers/usb/serial/f81232.c | 29 +++++++++++-----
drivers/usb/serial/pl2303.c | 29 +++++++++++-----
include/linux/wait.h | 11 -------
kernel/sched/core.c | 46 --------------------------
sound/oss/dmabuf.c | 14 ++++----
sound/oss/dmasound/dmasound.h | 1 -
sound/oss/dmasound/dmasound_core.c | 28 +++++++++++-----
sound/oss/midibuf.c | 18 +++++-----
sound/oss/msnd_pinnacle.c | 31 ++++++++++--------
sound/oss/sequencer.c | 16 ++++-----
sound/oss/sleep.h | 18 ++++++++++
sound/oss/swarm_cs4297a.c | 14 ++++----
sound/oss/vwsnd.c | 14 +++++---
45 files changed, 334 insertions(+), 277 deletions(-)
create mode 100644 sound/oss/sleep.h
--
1.8.3.2
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
sleep_on and its variants are broken and going away soon. This changes
the omap vout driver to use interruptible_sleep_on_timeout instead,
which fixes potential race where the dma is complete before we
schedule.
Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: [email protected]
---
drivers/media/platform/omap/omap_vout_vrfb.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/media/platform/omap/omap_vout_vrfb.c b/drivers/media/platform/omap/omap_vout_vrfb.c
index cf1c437..62e7e57 100644
--- a/drivers/media/platform/omap/omap_vout_vrfb.c
+++ b/drivers/media/platform/omap/omap_vout_vrfb.c
@@ -270,7 +270,8 @@ int omap_vout_prepare_vrfb(struct omap_vout_device *vout,
omap_dma_set_global_params(DMA_DEFAULT_ARB_RATE, 0x20, 0);
omap_start_dma(tx->dma_ch);
- interruptible_sleep_on_timeout(&tx->wait, VRFB_TX_TIMEOUT);
+ wait_event_interruptible_timeout(tx->wait, tx->tx_status == 1,
+ VRFB_TX_TIMEOUT);
if (tx->tx_status == 0) {
omap_stop_dma(tx->dma_ch);
--
1.8.3.2
interruptible_sleep_on is racy and going away. In the arv driver that
race has probably never caused problems since it would require a whole
video frame to be captured before the read function has a chance to
go to sleep, but using wait_event_interruptible lets us kill off the
old interface. In order to do this, we have to slightly adapt the
meaning of the ar->start_capture field to distinguish between not having
started a frame and having completed it.
Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: [email protected]
---
drivers/media/platform/arv.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/arv.c b/drivers/media/platform/arv.c
index e346d32d..32f6d70 100644
--- a/drivers/media/platform/arv.c
+++ b/drivers/media/platform/arv.c
@@ -307,11 +307,11 @@ static ssize_t ar_read(struct file *file, char *buf, size_t count, loff_t *ppos)
/*
* Okay, kick AR LSI to invoke an interrupt
*/
- ar->start_capture = 0;
+ ar->start_capture = -1;
ar_outl(arvcr1 | ARVCR1_HIEN, ARVCR1);
local_irq_restore(flags);
/* .... AR interrupts .... */
- interruptible_sleep_on(&ar->wait);
+ wait_event_interruptible(ar->wait, ar->start_capture == 0);
if (signal_pending(current)) {
printk(KERN_ERR "arv: interrupted while get frame data.\n");
ret = -EINTR;
--
1.8.3.2
The interruptible_sleep_on function is can still lead to the
deadlock mentioned in the comment above the caller, and we want
to remove it soon, so replace it now with the race-free
wait_event_interruptible.
Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Andrew Morton <[email protected]>
---
drivers/parport/share.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/parport/share.c b/drivers/parport/share.c
index 6a83ee1..3fa6624 100644
--- a/drivers/parport/share.c
+++ b/drivers/parport/share.c
@@ -905,7 +905,8 @@ int parport_claim_or_block(struct pardevice *dev)
/* If dev->waiting is clear now, an interrupt
gave us the port and we would deadlock if we slept. */
if (dev->waiting) {
- interruptible_sleep_on (&dev->wait_q);
+ wait_event_interruptible(dev->wait_q,
+ !dev->waiting);
if (signal_pending (current)) {
return -EINTR;
}
--
1.8.3.2
There are three files in oss for which I could not find an easy way to
replace interruptible_sleep_on_timeout with a non-racy version. This
patch instead just adds a private implementation of the function, now
named oss_broken_sleep_on, and changes over the remaining users in
sound/oss/ so we can remove the global interface.
Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Jaroslav Kysela <[email protected]>
Cc: Takashi Iwai <[email protected]>
Cc: [email protected]
---
sound/oss/dmabuf.c | 14 ++++++--------
sound/oss/sequencer.c | 16 +++++++---------
sound/oss/sleep.h | 18 ++++++++++++++++++
sound/oss/swarm_cs4297a.c | 14 ++++++++------
4 files changed, 39 insertions(+), 23 deletions(-)
create mode 100644 sound/oss/sleep.h
diff --git a/sound/oss/dmabuf.c b/sound/oss/dmabuf.c
index 461d94c..e3f2913 100644
--- a/sound/oss/dmabuf.c
+++ b/sound/oss/dmabuf.c
@@ -28,6 +28,7 @@
#include <linux/mm.h>
#include <linux/gfp.h>
#include "sound_config.h"
+#include "sleep.h"
#define DMAP_FREE_ON_CLOSE 0
#define DMAP_KEEP_ON_CLOSE 1
@@ -351,8 +352,7 @@ static void dma_reset_output(int dev)
if (!signal_pending(current) && adev->dmap_out->qlen &&
adev->dmap_out->underrun_count == 0){
spin_unlock_irqrestore(&dmap->lock,flags);
- interruptible_sleep_on_timeout(&adev->out_sleeper,
- dmabuf_timeout(dmap));
+ oss_broken_sleep_on(&adev->out_sleeper, dmabuf_timeout(dmap));
spin_lock_irqsave(&dmap->lock,flags);
}
adev->dmap_out->flags &= ~(DMA_SYNCING | DMA_ACTIVE);
@@ -446,7 +446,7 @@ int DMAbuf_sync(int dev)
long t = dmabuf_timeout(dmap);
spin_unlock_irqrestore(&dmap->lock,flags);
/* FIXME: not safe may miss events */
- t = interruptible_sleep_on_timeout(&adev->out_sleeper, t);
+ t = oss_broken_sleep_on(&adev->out_sleeper, t);
spin_lock_irqsave(&dmap->lock,flags);
if (!t) {
adev->dmap_out->flags &= ~DMA_SYNCING;
@@ -466,7 +466,7 @@ int DMAbuf_sync(int dev)
while (!signal_pending(current) &&
adev->d->local_qlen(dev)){
spin_unlock_irqrestore(&dmap->lock,flags);
- interruptible_sleep_on_timeout(&adev->out_sleeper,
+ oss_broken_sleep_on(&adev->out_sleeper,
dmabuf_timeout(dmap));
spin_lock_irqsave(&dmap->lock,flags);
}
@@ -587,8 +587,7 @@ int DMAbuf_getrdbuffer(int dev, char **buf, int *len, int dontblock)
timeout = dmabuf_timeout(dmap);
spin_unlock_irqrestore(&dmap->lock,flags);
- timeout = interruptible_sleep_on_timeout(&adev->in_sleeper,
- timeout);
+ timeout = oss_broken_sleep_on(&adev->in_sleeper, timeout);
if (!timeout) {
/* FIXME: include device name */
err = -EIO;
@@ -768,8 +767,7 @@ static int output_sleep(int dev, int dontblock)
timeout_value = dmabuf_timeout(dmap);
else
timeout_value = MAX_SCHEDULE_TIMEOUT;
- timeout_value = interruptible_sleep_on_timeout(&adev->out_sleeper,
- timeout_value);
+ timeout_value = oss_broken_sleep_on(&adev->out_sleeper, timeout_value);
if (timeout != MAX_SCHEDULE_TIMEOUT && !timeout_value) {
printk(KERN_WARNING "Sound: DMA (output) timed out - IRQ/DRQ config error?\n");
dma_reset_output(dev);
diff --git a/sound/oss/sequencer.c b/sound/oss/sequencer.c
index 4ff60a6..c855a75 100644
--- a/sound/oss/sequencer.c
+++ b/sound/oss/sequencer.c
@@ -19,6 +19,7 @@
#include "sound_config.h"
#include "midi_ctrl.h"
+#include "sleep.h"
static int sequencer_ok;
static struct sound_timer_operations *tmr;
@@ -100,8 +101,7 @@ int sequencer_read(int dev, struct file *file, char __user *buf, int count)
return -EAGAIN;
}
- interruptible_sleep_on_timeout(&midi_sleeper,
- pre_event_timeout);
+ oss_broken_sleep_on(&midi_sleeper, pre_event_timeout);
spin_lock_irqsave(&lock,flags);
if (!iqlen)
{
@@ -343,7 +343,7 @@ static int seq_queue(unsigned char *note, char nonblock)
/*
* Sleep until there is enough space on the queue
*/
- interruptible_sleep_on(&seq_sleeper);
+ oss_broken_sleep_on(&seq_sleeper, MAX_SCHEDULE_TIMEOUT);
}
if (qlen >= SEQ_MAX_QUEUE)
{
@@ -1122,8 +1122,7 @@ static void seq_drain_midi_queues(void)
*/
if (n)
- interruptible_sleep_on_timeout(&seq_sleeper,
- HZ/10);
+ oss_broken_sleep_on(&seq_sleeper, HZ/10);
}
}
@@ -1145,8 +1144,7 @@ void sequencer_release(int dev, struct file *file)
while (!signal_pending(current) && qlen > 0)
{
seq_sync();
- interruptible_sleep_on_timeout(&seq_sleeper,
- 3*HZ);
+ oss_broken_sleep_on(&seq_sleeper, 3*HZ);
/* Extra delay */
}
}
@@ -1201,7 +1199,7 @@ static int seq_sync(void)
seq_startplay();
if (qlen > 0)
- interruptible_sleep_on_timeout(&seq_sleeper, HZ);
+ oss_broken_sleep_on(&seq_sleeper, HZ);
return qlen;
}
@@ -1224,7 +1222,7 @@ static void midi_outc(int dev, unsigned char data)
spin_lock_irqsave(&lock,flags);
while (n && !midi_devs[dev]->outputc(dev, data)) {
- interruptible_sleep_on_timeout(&seq_sleeper, HZ/25);
+ oss_broken_sleep_on(&seq_sleeper, HZ/25);
n--;
}
spin_unlock_irqrestore(&lock,flags);
diff --git a/sound/oss/sleep.h b/sound/oss/sleep.h
new file mode 100644
index 0000000..a20fc92
--- /dev/null
+++ b/sound/oss/sleep.h
@@ -0,0 +1,18 @@
+#include <linux/wait.h>
+
+/*
+ * Do not use. This is a replacement for the old
+ * "interruptible_sleep_on_timeout" function that has been
+ * deprecated for ages. All users should instead try to use
+ * wait_event_interruptible_timeout.
+ */
+
+static inline long
+oss_broken_sleep_on(wait_queue_head_t *q, long timeout)
+{
+ DEFINE_WAIT(wait);
+ prepare_to_wait(q, &wait, TASK_INTERRUPTIBLE);
+ timeout = schedule_timeout(timeout);
+ finish_wait(q, &wait);
+ return timeout;
+}
diff --git a/sound/oss/swarm_cs4297a.c b/sound/oss/swarm_cs4297a.c
index 7d8803a..59defdc 100644
--- a/sound/oss/swarm_cs4297a.c
+++ b/sound/oss/swarm_cs4297a.c
@@ -90,6 +90,8 @@
#include <asm/sibyte/sb1250_mac.h>
#include <asm/sibyte/sb1250.h>
+#include "sleep.h"
+
struct cs4297a_state;
static DEFINE_MUTEX(swarm_cs4297a_mutex);
@@ -748,7 +750,7 @@ static int serdma_reg_access(struct cs4297a_state *s, u64 data)
/* Since a writer has the DSP open, we have to mux the
request in */
s->reg_request = data;
- interruptible_sleep_on(&s->dma_dac.reg_wait);
+ oss_broken_sleep_on(&s->dma_dac.reg_wait, MAX_SCHEDULE_TIMEOUT);
/* XXXKW how can I deal with the starvation case where
the opener isn't writing? */
} else {
@@ -790,7 +792,7 @@ static int cs4297a_read_ac97(struct cs4297a_state *s, u32 offset,
if (serdma_reg_access(s, (0xCLL << 60) | (1LL << 47) | ((u64)(offset & 0x7F) << 40)))
return -1;
- interruptible_sleep_on(&s->dma_adc.reg_wait);
+ oss_broken_sleep_on(&s->dma_adc.reg_wait, MAX_SCHEDULE_TIMEOUT);
*value = s->read_value;
CS_DBGOUT(CS_AC97, 2,
printk(KERN_INFO "cs4297a: rdr reg %x -> %x\n", s->read_reg, s->read_value));
@@ -1740,7 +1742,7 @@ static ssize_t cs4297a_read(struct file *file, char *buffer, size_t count,
start_adc(s);
if (file->f_flags & O_NONBLOCK)
return ret ? ret : -EAGAIN;
- interruptible_sleep_on(&s->dma_adc.wait);
+ oss_broken_sleep_on(&s->dma_adc.wait, MAX_SCHEDULE_TIMEOUT);
if (signal_pending(current))
return ret ? ret : -ERESTARTSYS;
continue;
@@ -1836,7 +1838,7 @@ static ssize_t cs4297a_write(struct file *file, const char *buffer,
start_dac(s);
if (file->f_flags & O_NONBLOCK)
return ret ? ret : -EAGAIN;
- interruptible_sleep_on(&d->wait);
+ oss_broken_sleep_on(&d->wait, MAX_SCHEDULE_TIMEOUT);
if (signal_pending(current))
return ret ? ret : -ERESTARTSYS;
continue;
@@ -2452,7 +2454,7 @@ static int cs4297a_locked_open(struct inode *inode, struct file *file)
return -EBUSY;
}
mutex_unlock(&s->open_sem_dac);
- interruptible_sleep_on(&s->open_wait_dac);
+ oss_broken_sleep_on(&s->open_wait_dac, MAX_SCHEDULE_TIMEOUT);
if (signal_pending(current)) {
printk("open - sig pending\n");
@@ -2469,7 +2471,7 @@ static int cs4297a_locked_open(struct inode *inode, struct file *file)
return -EBUSY;
}
mutex_unlock(&s->open_sem_adc);
- interruptible_sleep_on(&s->open_wait_adc);
+ oss_broken_sleep_on(&s->open_wait_adc, MAX_SCHEDULE_TIMEOUT);
if (signal_pending(current)) {
printk("open - sig pending\n");
--
1.8.3.2
We probably should have done this 15 years ago. I have created patches
for every remaining user of the four sleep_on variants that are all
broken, and this patch should get applied as soon as all of the other
patches are merged.
If you are reading this changeset because you are looking for the
functions I removed, please convert your code to use wait_event,
wait_for_completion or an open-coded prepare_to_wait loop.
Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Ingo Molnar <[email protected]>
---
Documentation/DocBook/kernel-hacking.tmpl | 10 -------
include/linux/wait.h | 11 --------
kernel/sched/core.c | 46 -------------------------------
3 files changed, 67 deletions(-)
diff --git a/Documentation/DocBook/kernel-hacking.tmpl b/Documentation/DocBook/kernel-hacking.tmpl
index d0758b24..bd9015d 100644
--- a/Documentation/DocBook/kernel-hacking.tmpl
+++ b/Documentation/DocBook/kernel-hacking.tmpl
@@ -850,16 +850,6 @@ printk(KERN_INFO "my ip: %pI4\n", &ipaddress);
<returnvalue>-ERESTARTSYS</returnvalue> if a signal is received.
The <function>wait_event()</function> version ignores signals.
</para>
- <para>
- Do not use the <function>sleep_on()</function> function family -
- it is very easy to accidentally introduce races; almost certainly
- one of the <function>wait_event()</function> family will do, or a
- loop around <function>schedule_timeout()</function>. If you choose
- to loop around <function>schedule_timeout()</function> remember
- you must set the task state (with
- <function>set_current_state()</function>) on each iteration to avoid
- busy-looping.
- </para>
</sect1>
diff --git a/include/linux/wait.h b/include/linux/wait.h
index eaa00b1..02c5a31 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -803,17 +803,6 @@ do { \
__ret; \
})
-
-/*
- * These are the old interfaces to sleep waiting for an event.
- * They are racy. DO NOT use them, use the wait_event* interfaces above.
- * We plan to remove these interfaces.
- */
-extern void sleep_on(wait_queue_head_t *q);
-extern long sleep_on_timeout(wait_queue_head_t *q, signed long timeout);
-extern void interruptible_sleep_on(wait_queue_head_t *q);
-extern long interruptible_sleep_on_timeout(wait_queue_head_t *q, signed long timeout);
-
/*
* Waitqueues which are removed from the waitqueue_head at wakeup time
*/
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index a88f4a4..6272dee 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2701,52 +2701,6 @@ int default_wake_function(wait_queue_t *curr, unsigned mode, int wake_flags,
}
EXPORT_SYMBOL(default_wake_function);
-static long __sched
-sleep_on_common(wait_queue_head_t *q, int state, long timeout)
-{
- unsigned long flags;
- wait_queue_t wait;
-
- init_waitqueue_entry(&wait, current);
-
- __set_current_state(state);
-
- spin_lock_irqsave(&q->lock, flags);
- __add_wait_queue(q, &wait);
- spin_unlock(&q->lock);
- timeout = schedule_timeout(timeout);
- spin_lock_irq(&q->lock);
- __remove_wait_queue(q, &wait);
- spin_unlock_irqrestore(&q->lock, flags);
-
- return timeout;
-}
-
-void __sched interruptible_sleep_on(wait_queue_head_t *q)
-{
- sleep_on_common(q, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
-}
-EXPORT_SYMBOL(interruptible_sleep_on);
-
-long __sched
-interruptible_sleep_on_timeout(wait_queue_head_t *q, long timeout)
-{
- return sleep_on_common(q, TASK_INTERRUPTIBLE, timeout);
-}
-EXPORT_SYMBOL(interruptible_sleep_on_timeout);
-
-void __sched sleep_on(wait_queue_head_t *q)
-{
- sleep_on_common(q, TASK_UNINTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
-}
-EXPORT_SYMBOL(sleep_on);
-
-long __sched sleep_on_timeout(wait_queue_head_t *q, long timeout)
-{
- return sleep_on_common(q, TASK_UNINTERRUPTIBLE, timeout);
-}
-EXPORT_SYMBOL(sleep_on_timeout);
-
#ifdef CONFIG_RT_MUTEXES
/*
--
1.8.3.2
We are trying to finally kill off interruptible_sleep_on_timeout.
the two uses in the nicstar driver can be trivially replaced
with wait_event_interruptible_lock_irq_timeout, which prevents the
wake-up race and is able to check the buffer state with scq->lock
held.
Signed-off-by: Arnd Bergmann <[email protected]>
Cc: David S. Miller <[email protected]>
Cc: Chas Williams <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/atm/nicstar.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/atm/nicstar.c b/drivers/atm/nicstar.c
index 5aca5f4..3c079eb 100644
--- a/drivers/atm/nicstar.c
+++ b/drivers/atm/nicstar.c
@@ -1739,10 +1739,9 @@ static int push_scqe(ns_dev * card, vc_map * vc, scq_info * scq, ns_scqe * tbd,
}
scq->full = 1;
- spin_unlock_irqrestore(&scq->lock, flags);
- interruptible_sleep_on_timeout(&scq->scqfull_waitq,
- SCQFULL_TIMEOUT);
- spin_lock_irqsave(&scq->lock, flags);
+ wait_event_interruptible_lock_irq_timeout(scq->scqfull_waitq,
+ scq->tail != scq->next, scq->lock,
+ SCQFULL_TIMEOUT);
if (scq->full) {
spin_unlock_irqrestore(&scq->lock, flags);
@@ -1789,10 +1788,10 @@ static int push_scqe(ns_dev * card, vc_map * vc, scq_info * scq, ns_scqe * tbd,
scq->full = 1;
if (has_run++)
break;
- spin_unlock_irqrestore(&scq->lock, flags);
- interruptible_sleep_on_timeout(&scq->scqfull_waitq,
+ wait_event_interruptible_lock_irq_timeout(scq->scqfull_waitq,
+ scq->tail != scq->next,
+ scq->lock,
SCQFULL_TIMEOUT);
- spin_lock_irqsave(&scq->lock, flags);
}
if (!scq->full) {
--
1.8.3.2
Interruptible_sleep_on is racy and we want to remove it. This replaces
the use in the vwsnd driver with an open-coded prepare_to_wait
loop that fixes the race between concurrent open() and close() calls,
and also drops the global mutex while waiting here, which restores
the original behavior that was changed during the BKL removal.
Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Jaroslav Kysela <[email protected]>
Cc: Takashi Iwai <[email protected]>
Cc: [email protected]
---
sound/oss/vwsnd.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/sound/oss/vwsnd.c b/sound/oss/vwsnd.c
index 4bbcc0f..a077e9c 100644
--- a/sound/oss/vwsnd.c
+++ b/sound/oss/vwsnd.c
@@ -2921,6 +2921,7 @@ static int vwsnd_audio_open(struct inode *inode, struct file *file)
vwsnd_dev_t *devc;
int minor = iminor(inode);
int sw_samplefmt;
+ DEFINE_WAIT(wait);
DBGE("(inode=0x%p, file=0x%p)\n", inode, file);
@@ -2937,21 +2938,26 @@ static int vwsnd_audio_open(struct inode *inode, struct file *file)
}
mutex_lock(&devc->open_mutex);
- while (devc->open_mode & file->f_mode) {
+ while (1) {
+ prepare_to_wait(&devc->open_wait, &wait, TASK_INTERRUPTIBLE);
+ if (!(devc->open_mode & file->f_mode))
+ break;
+
mutex_unlock(&devc->open_mutex);
+ mutex_unlock(&vwsnd_mutex);
if (file->f_flags & O_NONBLOCK) {
DEC_USE_COUNT;
- mutex_unlock(&vwsnd_mutex);
return -EBUSY;
}
- interruptible_sleep_on(&devc->open_wait);
+ schedule();
if (signal_pending(current)) {
DEC_USE_COUNT;
- mutex_unlock(&vwsnd_mutex);
return -ERESTARTSYS;
}
+ mutex_lock(&vwsnd_mutex);
mutex_lock(&devc->open_mutex);
}
+ finish_wait(&devc->open_wait, &wait);
devc->open_mode |= file->f_mode & (FMODE_READ | FMODE_WRITE);
mutex_unlock(&devc->open_mutex);
--
1.8.3.2
interruptible_sleep_on_timeout is deprecated and going away soon.
The use in the sgi-xp driver leaves me puzzled, so I'd prefer not
to touch it. This patch replaces it with an open-coded prepare_to_wait
and finish_wait pair, which should be completely equivalent, so it
doesn't fix an existing race, but lets us get away with removing
the function so we can not get any new users.
In order to remove the typical sleep_on race, one would have to
replace the call with wait_event_interruptible_timeout and add
a condition to wait for. The fact that there is a one-jiffy timeout
suggests that we don't actually expect to get woken up properly
and the caller just uses this as a short sleeping function
if it doesn't wake up properly.
Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Cliff Whickman <[email protected]>
Cc: Robin Holt <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
---
drivers/misc/sgi-xp/xpc_channel.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/misc/sgi-xp/xpc_channel.c b/drivers/misc/sgi-xp/xpc_channel.c
index 652593f..128d561 100644
--- a/drivers/misc/sgi-xp/xpc_channel.c
+++ b/drivers/misc/sgi-xp/xpc_channel.c
@@ -828,6 +828,7 @@ enum xp_retval
xpc_allocate_msg_wait(struct xpc_channel *ch)
{
enum xp_retval ret;
+ DEFINE_WAIT(wait);
if (ch->flags & XPC_C_DISCONNECTING) {
DBUG_ON(ch->reason == xpInterrupted);
@@ -835,7 +836,9 @@ xpc_allocate_msg_wait(struct xpc_channel *ch)
}
atomic_inc(&ch->n_on_msg_allocate_wq);
- ret = interruptible_sleep_on_timeout(&ch->msg_allocate_wq, 1);
+ prepare_to_wait(&ch->msg_allocate_wq, &wait, TASK_INTERRUPTIBLE);
+ ret = schedule_timeout(1);
+ finish_wait(&ch->msg_allocate_wq, &wait);
atomic_dec(&ch->n_on_msg_allocate_wq);
if (ch->flags & XPC_C_DISCONNECTING) {
--
1.8.3.2
interruptible_sleep_on is racy and going away. This replaces the one use
in the firestream driver with the appropriate wait_event_interruptible
variant.
Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Chas Williams <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/atm/firestream.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/atm/firestream.c b/drivers/atm/firestream.c
index b41c948..f43e1c1 100644
--- a/drivers/atm/firestream.c
+++ b/drivers/atm/firestream.c
@@ -736,8 +736,8 @@ static void process_txdone_queue (struct fs_dev *dev, struct queue *q)
skb = td->skb;
if (skb == FS_VCC (ATM_SKB(skb)->vcc)->last_skb) {
- wake_up_interruptible (& FS_VCC (ATM_SKB(skb)->vcc)->close_wait);
FS_VCC (ATM_SKB(skb)->vcc)->last_skb = NULL;
+ wake_up_interruptible (& FS_VCC (ATM_SKB(skb)->vcc)->close_wait);
}
td->dev->ntxpckts--;
@@ -1123,7 +1123,7 @@ static void fs_close(struct atm_vcc *atm_vcc)
this sleep_on, we'll lose any reference to these packets. Memory leak!
On the other hand, it's awfully convenient that we can abort a "close" that
is taking too long. Maybe just use non-interruptible sleep on? -- REW */
- interruptible_sleep_on (& vcc->close_wait);
+ wait_event_interruptible(vcc->close_wait, !vcc->last_skb);
}
txtp = &atm_vcc->qos.txtp;
--
1.8.3.2
sleep_on and its variants are racy and going away. This replaces
the two uses in the cris sync_serial drivers with the equivalent
but race-free wait_event_interruptible.
Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Jesper Nilsson <[email protected]>
Cc: Mikael Starvik <[email protected]>
Cc: [email protected]
---
arch/cris/arch-v10/drivers/sync_serial.c | 4 +++-
arch/cris/arch-v32/drivers/sync_serial.c | 4 +++-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/arch/cris/arch-v10/drivers/sync_serial.c b/arch/cris/arch-v10/drivers/sync_serial.c
index a1c498d..485f2bb 100644
--- a/arch/cris/arch-v10/drivers/sync_serial.c
+++ b/arch/cris/arch-v10/drivers/sync_serial.c
@@ -22,6 +22,7 @@
#include <linux/init.h>
#include <linux/mutex.h>
#include <linux/timer.h>
+#include <linux/wait.h>
#include <asm/irq.h>
#include <asm/dma.h>
#include <asm/io.h>
@@ -1136,7 +1137,8 @@ static ssize_t sync_serial_read(struct file *file, char *buf,
if (file->f_flags & O_NONBLOCK)
return -EAGAIN;
- interruptible_sleep_on(&port->in_wait_q);
+ wait_event_interruptible(port->in_wait_q,
+ !(start == end && !port->full));
if (signal_pending(current))
return -EINTR;
diff --git a/arch/cris/arch-v32/drivers/sync_serial.c b/arch/cris/arch-v32/drivers/sync_serial.c
index 219f704..bbb806b 100644
--- a/arch/cris/arch-v32/drivers/sync_serial.c
+++ b/arch/cris/arch-v32/drivers/sync_serial.c
@@ -19,6 +19,7 @@
#include <linux/init.h>
#include <linux/timer.h>
#include <linux/spinlock.h>
+#include <linux/wait.h>
#include <asm/io.h>
#include <dma.h>
@@ -1144,7 +1145,8 @@ static ssize_t sync_serial_read(struct file * file, char * buf,
if (file->f_flags & O_NONBLOCK)
return -EAGAIN;
- interruptible_sleep_on(&port->in_wait_q);
+ wait_event_interruptible(port->in_wait_q,
+ !(start == end && !port->full));
if (signal_pending(current))
return -EINTR;
--
1.8.3.2
interruptible_sleep_on is racy and going away. This replaces the one
caller in the panel driver with the appropriate wait_event_interruptible
variant.
Signed-off-by: Arnd Bergmann <[email protected]>
Cc: [email protected]
Cc: Willy Tarreau <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
---
drivers/staging/panel/panel.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index cbc15c1..ec4b1fd 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -1590,8 +1590,8 @@ static ssize_t keypad_read(struct file *file,
if (file->f_flags & O_NONBLOCK)
return -EAGAIN;
- interruptible_sleep_on(&keypad_read_wait);
- if (signal_pending(current))
+ if (wait_event_interruptible(keypad_read_wait,
+ keypad_buflen != 0))
return -EINTR;
}
--
1.8.3.2
The use of interruptible_sleep_on_timeout in the dmasound driver
is questionable and we want to kill off all sleep_on variants.
This replaces the calls with wait_event_interruptible_timeout
where possible, to wait for a particular event instead of blocking
in a racy way. In the sq_write function, the easiest solution is
an open-coded prepare_to_wait loop.
Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Jaroslav Kysela <[email protected]>
Cc: Takashi Iwai <[email protected]>
Cc: [email protected]
---
sound/oss/dmasound/dmasound.h | 1 -
sound/oss/dmasound/dmasound_core.c | 28 +++++++++++++++++++---------
2 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/sound/oss/dmasound/dmasound.h b/sound/oss/dmasound/dmasound.h
index 1308d8d..01019f0 100644
--- a/sound/oss/dmasound/dmasound.h
+++ b/sound/oss/dmasound/dmasound.h
@@ -239,7 +239,6 @@ struct sound_queue {
int busy, syncing, xruns, died;
};
-#define SLEEP(queue) interruptible_sleep_on_timeout(&queue, HZ)
#define WAKE_UP(queue) (wake_up_interruptible(&queue))
extern struct sound_queue dmasound_write_sq;
diff --git a/sound/oss/dmasound/dmasound_core.c b/sound/oss/dmasound/dmasound_core.c
index bac43b5..f4ee85a 100644
--- a/sound/oss/dmasound/dmasound_core.c
+++ b/sound/oss/dmasound/dmasound_core.c
@@ -619,15 +619,27 @@ static ssize_t sq_write(struct file *file, const char __user *src, size_t uLeft,
}
while (uLeft) {
+ DEFINE_WAIT(wait);
+
while (write_sq.count >= write_sq.max_active) {
+ prepare_to_wait(&write_sq.action_queue, &wait, TASK_INTERRUPTIBLE);
sq_play();
- if (write_sq.non_blocking)
+ if (write_sq.non_blocking) {
+ finish_wait(&write_sq.action_queue, &wait);
return uWritten > 0 ? uWritten : -EAGAIN;
- SLEEP(write_sq.action_queue);
- if (signal_pending(current))
+ }
+ if (write_sq.count < write_sq.max_active)
+ break;
+
+ schedule_timeout(HZ);
+ if (signal_pending(current)) {
+ finish_wait(&write_sq.action_queue, &wait);
return uWritten > 0 ? uWritten : -EINTR;
+ }
}
+ finish_wait(&write_sq.action_queue, &wait);
+
/* Here, we can avoid disabling the interrupt by first
* copying and translating the data, and then updating
* the write_sq variables. Until this is done, the interrupt
@@ -707,11 +719,8 @@ static int sq_open2(struct sound_queue *sq, struct file *file, fmode_t mode,
if (file->f_flags & O_NONBLOCK)
return rc;
rc = -EINTR;
- while (sq->busy) {
- SLEEP(sq->open_queue);
- if (signal_pending(current))
- return rc;
- }
+ if (wait_event_interruptible(sq->open_queue, !sq->busy))
+ return rc;
rc = 0;
#else
/* OSS manual says we will return EBUSY regardless
@@ -844,7 +853,8 @@ static int sq_fsync(void)
sq_play(); /* there may be an incomplete frame waiting */
while (write_sq.active) {
- SLEEP(write_sq.sync_queue);
+ wait_event_interruptible_timeout(write_sq.sync_queue,
+ !write_sq.active, HZ);
if (signal_pending(current)) {
/* While waiting for audio output to drain, an
* interrupt occurred. Stop audio output immediately
--
1.8.3.2
We want to remove all sleep_on variants from the kernel because they are
racy. In case of the pinnacle driver, we can replace
interruptible_sleep_on_timeout with wait_event_interruptible_timeout
by changing the meaning of a few flags used in the driver so they
are cleared at wakeup time, which is a somewhat more appropriate
way to do the same, although probably still racy.
Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Andrew Veliath <[email protected]>
Cc: Jaroslav Kysela <[email protected]>
Cc: Takashi Iwai <[email protected]>
Cc: [email protected]
---
sound/oss/msnd_pinnacle.c | 31 ++++++++++++++++++-------------
1 file changed, 18 insertions(+), 13 deletions(-)
diff --git a/sound/oss/msnd_pinnacle.c b/sound/oss/msnd_pinnacle.c
index 11ff7c5..c23f9f9 100644
--- a/sound/oss/msnd_pinnacle.c
+++ b/sound/oss/msnd_pinnacle.c
@@ -664,12 +664,15 @@ static long dev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
static void dsp_write_flush(void)
{
+ int timeout = get_play_delay_jiffies(dev.DAPF.len);
+
if (!(dev.mode & FMODE_WRITE) || !test_bit(F_WRITING, &dev.flags))
return;
set_bit(F_WRITEFLUSH, &dev.flags);
- interruptible_sleep_on_timeout(
- &dev.writeflush,
- get_play_delay_jiffies(dev.DAPF.len));
+ wait_event_interruptible_timeout(
+ dev.writeflush,
+ !test_bit(F_WRITEFLUSH, &dev.flags),
+ timeout);
clear_bit(F_WRITEFLUSH, &dev.flags);
if (!signal_pending(current)) {
current->state = TASK_INTERRUPTIBLE;
@@ -897,6 +900,7 @@ static int dsp_read(char __user *buf, size_t len)
{
int count = len;
char *page = (char *)__get_free_page(GFP_KERNEL);
+ int timeout = get_rec_delay_jiffies(DAR_BUFF_SIZE);
if (!page)
return -ENOMEM;
@@ -936,11 +940,11 @@ static int dsp_read(char __user *buf, size_t len)
if (count > 0) {
set_bit(F_READBLOCK, &dev.flags);
- if (!interruptible_sleep_on_timeout(
- &dev.readblock,
- get_rec_delay_jiffies(DAR_BUFF_SIZE)))
+ if (wait_event_interruptible_timeout(
+ dev.readblock,
+ test_bit(F_READBLOCK, &dev.flags),
+ timeout) <= 0)
clear_bit(F_READING, &dev.flags);
- clear_bit(F_READBLOCK, &dev.flags);
if (signal_pending(current)) {
free_page((unsigned long)page);
return -EINTR;
@@ -955,6 +959,7 @@ static int dsp_write(const char __user *buf, size_t len)
{
int count = len;
char *page = (char *)__get_free_page(GFP_KERNEL);
+ int timeout = get_play_delay_jiffies(DAP_BUFF_SIZE);
if (!page)
return -ENOMEM;
@@ -995,10 +1000,10 @@ static int dsp_write(const char __user *buf, size_t len)
if (count > 0) {
set_bit(F_WRITEBLOCK, &dev.flags);
- interruptible_sleep_on_timeout(
- &dev.writeblock,
- get_play_delay_jiffies(DAP_BUFF_SIZE));
- clear_bit(F_WRITEBLOCK, &dev.flags);
+ wait_event_interruptible_timeout(
+ dev.writeblock,
+ test_bit(F_WRITEBLOCK, &dev.flags),
+ timeout);
if (signal_pending(current)) {
free_page((unsigned long)page);
return -EINTR;
@@ -1044,7 +1049,7 @@ static __inline__ void eval_dsp_msg(register WORD wMessage)
clear_bit(F_WRITING, &dev.flags);
}
- if (test_bit(F_WRITEBLOCK, &dev.flags))
+ if (test_and_clear_bit(F_WRITEBLOCK, &dev.flags))
wake_up_interruptible(&dev.writeblock);
break;
@@ -1055,7 +1060,7 @@ static __inline__ void eval_dsp_msg(register WORD wMessage)
pack_DARQ_to_DARF(dev.last_recbank);
- if (test_bit(F_READBLOCK, &dev.flags))
+ if (test_and_clear_bit(F_READBLOCK, &dev.flags))
wake_up_interruptible(&dev.readblock);
break;
--
1.8.3.2
The nwbutton driver uses interruptible_sleep_on to wait for buttons
getting pressed after we enter the read() function, which is inherently
racy and cannot be fixed by using wait_event without changing the
driver's user space interface.
Instead, this patch just uses an open-coded variant of the same
interruptible_sleep_on() call, so the driver behavior doesn't change
but we remove the sleep_on family from the kernel.
Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
---
drivers/char/nwbutton.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/char/nwbutton.c b/drivers/char/nwbutton.c
index 1fd00dc..76c490f 100644
--- a/drivers/char/nwbutton.c
+++ b/drivers/char/nwbutton.c
@@ -168,7 +168,10 @@ static irqreturn_t button_handler (int irq, void *dev_id)
static int button_read (struct file *filp, char __user *buffer,
size_t count, loff_t *ppos)
{
- interruptible_sleep_on (&button_wait_queue);
+ DEFINE_WAIT(wait);
+ prepare_to_wait(&button_wait_queue, &wait, TASK_INTERRUPTIBLE);
+ schedule();
+ finish_wait(&button_wait_queue, &wait);
return (copy_to_user (buffer, &button_output_buffer, bcount))
? -EFAULT : bcount;
}
--
1.8.3.2
interruptible_sleep_on is generally problematic and we want to get
rid of it. In case of TIOCMIWAIT, that race is actually in user
space and does not get fixed since we can only detect changes after
entering the ioctl handler, but it removes one more caller.
This instance can not be trivially replaced with wait_event, so
I chose to open-code the wait loop using prepare_to_wait/finish_wait.
Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
---
drivers/tty/amiserial.c | 26 ++++++++++++++++++--------
1 file changed, 18 insertions(+), 8 deletions(-)
diff --git a/drivers/tty/amiserial.c b/drivers/tty/amiserial.c
index 71630a2..979e7c3 100644
--- a/drivers/tty/amiserial.c
+++ b/drivers/tty/amiserial.c
@@ -1248,6 +1248,8 @@ static int rs_ioctl(struct tty_struct *tty,
struct async_icount cprev, cnow; /* kernel counter temps */
void __user *argp = (void __user *)arg;
unsigned long flags;
+ DEFINE_WAIT(wait);
+ int ret;
if (serial_paranoia_check(info, tty->name, "rs_ioctl"))
return -ENODEV;
@@ -1288,25 +1290,33 @@ static int rs_ioctl(struct tty_struct *tty,
cprev = info->icount;
local_irq_restore(flags);
while (1) {
- interruptible_sleep_on(&info->tport.delta_msr_wait);
- /* see if a signal did it */
- if (signal_pending(current))
- return -ERESTARTSYS;
+ prepare_to_wait(&info->tport.delta_msr_wait,
+ &wait, TASK_INTERRUPTIBLE);
local_irq_save(flags);
cnow = info->icount; /* atomic copy */
local_irq_restore(flags);
if (cnow.rng == cprev.rng && cnow.dsr == cprev.dsr &&
- cnow.dcd == cprev.dcd && cnow.cts == cprev.cts)
- return -EIO; /* no change => error */
+ cnow.dcd == cprev.dcd && cnow.cts == cprev.cts) {
+ ret = -EIO; /* no change => error */
+ break;
+ }
if ( ((arg & TIOCM_RNG) && (cnow.rng != cprev.rng)) ||
((arg & TIOCM_DSR) && (cnow.dsr != cprev.dsr)) ||
((arg & TIOCM_CD) && (cnow.dcd != cprev.dcd)) ||
((arg & TIOCM_CTS) && (cnow.cts != cprev.cts)) ) {
- return 0;
+ ret = 0;
+ break;
+ }
+ schedule();
+ /* see if a signal did it */
+ if (signal_pending(current)) {
+ ret = -ERESTARTSYS;
+ break;
}
cprev = cnow;
}
- /* NOTREACHED */
+ finish_wait(&info->tport.delta_msr_wait, &wait);
+ return ret;
case TIOCSERGWILD:
case TIOCSERSWILD:
--
1.8.3.2
interruptible_sleep_on is racy and going away. In case of pcbit,
the driver would run into a timeout if the card is initialized
before we start waiting for it. This uses wait_event to fix the
race. In order to do this, the state machine handling for the
timeout case has to get trivially reorganized so we actually know
whether the timeout has occorred or not.
Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Karsten Keil <[email protected]>
Cc: [email protected]
---
drivers/isdn/pcbit/drv.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/isdn/pcbit/drv.c b/drivers/isdn/pcbit/drv.c
index 1eaf622..f02cc50 100644
--- a/drivers/isdn/pcbit/drv.c
+++ b/drivers/isdn/pcbit/drv.c
@@ -796,6 +796,7 @@ static void set_running_timeout(unsigned long ptr)
#endif
dev = (struct pcbit_dev *) ptr;
+ dev->l2_state = L2_DOWN;
wake_up_interruptible(&dev->set_running_wq);
}
@@ -818,7 +819,8 @@ static int set_protocol_running(struct pcbit_dev *dev)
add_timer(&dev->set_running_timer);
- interruptible_sleep_on(&dev->set_running_wq);
+ wait_event(dev->set_running_wq, dev->l2_state == L2_RUNNING ||
+ dev->l2_state == L2_DOWN);
del_timer(&dev->set_running_timer);
@@ -842,8 +844,6 @@ static int set_protocol_running(struct pcbit_dev *dev)
printk(KERN_DEBUG "pcbit: initialization failed\n");
printk(KERN_DEBUG "pcbit: firmware not loaded\n");
- dev->l2_state = L2_DOWN;
-
#ifdef DEBUG
printk(KERN_DEBUG "Bank3 = %02x\n",
readb(dev->sh_mem + BANK3));
--
1.8.3.2
interruptible_sleep_on is racy and going away. This replaces the
use in the gdm72xx driver with the appropriate
wait_event_interruptible_lock_irq.
Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: [email protected]
---
drivers/staging/gdm72xx/gdm_usb.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/gdm72xx/gdm_usb.c b/drivers/staging/gdm72xx/gdm_usb.c
index e0cb2ff..f8788bf 100644
--- a/drivers/staging/gdm72xx/gdm_usb.c
+++ b/drivers/staging/gdm72xx/gdm_usb.c
@@ -780,9 +780,10 @@ static int k_mode_thread(void *arg)
spin_lock_irqsave(&k_lock, flags2);
}
+ wait_event_interruptible_lock_irq(k_wait,
+ !list_empty(&k_list) || k_mode_stop,
+ k_lock);
spin_unlock_irqrestore(&k_lock, flags2);
-
- interruptible_sleep_on(&k_wait);
}
return 0;
}
--
1.8.3.2
The state machine code in the elsa driver uses interruptible_sleep_on
to wait for state changes, which is racy. A closer look at the possible
states reveals that it is always used to wait for getting back into
ARCOFI_NOP, so we can use wait_event_interruptible instead.
Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Karsten Keil <[email protected]>
Cc: [email protected]
---
drivers/isdn/hisax/elsa.c | 9 ++++++---
drivers/isdn/hisax/elsa_ser.c | 3 ++-
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/isdn/hisax/elsa.c b/drivers/isdn/hisax/elsa.c
index 2be1c8a..d8ef64d 100644
--- a/drivers/isdn/hisax/elsa.c
+++ b/drivers/isdn/hisax/elsa.c
@@ -509,7 +509,8 @@ static void
set_arcofi(struct IsdnCardState *cs, int bc) {
cs->dc.isac.arcofi_bc = bc;
arcofi_fsm(cs, ARCOFI_START, &ARCOFI_COP_5);
- interruptible_sleep_on(&cs->dc.isac.arcofi_wait);
+ wait_event_interruptible(cs->dc.isac.arcofi_wait,
+ cs->dc.isac.arcofi_state == ARCOFI_NOP);
}
static int
@@ -528,7 +529,8 @@ check_arcofi(struct IsdnCardState *cs)
}
cs->dc.isac.arcofi_bc = 0;
arcofi_fsm(cs, ARCOFI_START, &ARCOFI_VERSION);
- interruptible_sleep_on(&cs->dc.isac.arcofi_wait);
+ wait_event_interruptible(cs->dc.isac.arcofi_wait,
+ cs->dc.isac.arcofi_state == ARCOFI_NOP);
if (!test_and_clear_bit(FLG_ARCOFI_ERROR, &cs->HW_Flags)) {
debugl1(cs, "Arcofi response received %d bytes", cs->dc.isac.mon_rxp);
p = cs->dc.isac.mon_rx;
@@ -595,7 +597,8 @@ check_arcofi(struct IsdnCardState *cs)
Elsa_Types[cs->subtyp],
cs->hw.elsa.base + 8);
arcofi_fsm(cs, ARCOFI_START, &ARCOFI_XOP_0);
- interruptible_sleep_on(&cs->dc.isac.arcofi_wait);
+ wait_event_interruptible(cs->dc.isac.arcofi_wait,
+ cs->dc.isac.arcofi_state == ARCOFI_NOP);
return (1);
}
return (0);
diff --git a/drivers/isdn/hisax/elsa_ser.c b/drivers/isdn/hisax/elsa_ser.c
index 3f84dd8..a2a358c 100644
--- a/drivers/isdn/hisax/elsa_ser.c
+++ b/drivers/isdn/hisax/elsa_ser.c
@@ -573,7 +573,8 @@ modem_l2l1(struct PStack *st, int pr, void *arg)
test_and_clear_bit(BC_FLG_ACTIV, &bcs->Flag);
bcs->cs->dc.isac.arcofi_bc = st->l1.bc;
arcofi_fsm(bcs->cs, ARCOFI_START, &ARCOFI_XOP_0);
- interruptible_sleep_on(&bcs->cs->dc.isac.arcofi_wait);
+ wait_event_interruptible(bcs->cs->dc.isac.arcofi_wait,
+ bcs->cs->dc.isac.arcofi_state == ARCOFI_NOP);
bcs->cs->hw.elsa.MFlag = 1;
} else {
printk(KERN_WARNING "ElsaSer: unknown pr %x\n", pr);
--
1.8.3.2
The isdn core code uses a couple of wait queues with
interruptible_sleep_on, which is racy and about to get
removed from the kernel. Fortunately, we know for each case
what we are waiting for, so they can all be converted to
the better wait_event_interruptible interface.
Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Karsten Keil <[email protected]>
Cc: [email protected]
---
drivers/isdn/i4l/isdn_common.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/isdn/i4l/isdn_common.c b/drivers/isdn/i4l/isdn_common.c
index 9bb12ba..130f216 100644
--- a/drivers/isdn/i4l/isdn_common.c
+++ b/drivers/isdn/i4l/isdn_common.c
@@ -777,7 +777,8 @@ isdn_readbchan(int di, int channel, u_char *buf, u_char *fp, int len, wait_queue
return 0;
if (skb_queue_empty(&dev->drv[di]->rpqueue[channel])) {
if (sleep)
- interruptible_sleep_on(sleep);
+ wait_event_interruptible(*sleep,
+ !skb_queue_empty(&dev->drv[di]->rpqueue[channel]));
else
return 0;
}
@@ -1072,7 +1073,8 @@ isdn_read(struct file *file, char __user *buf, size_t count, loff_t *off)
retval = -EAGAIN;
goto out;
}
- interruptible_sleep_on(&(dev->info_waitq));
+ wait_event_interruptible(dev->info_waitq,
+ file->private_data);
}
p = isdn_statstr();
file->private_data = NULL;
@@ -1128,7 +1130,8 @@ isdn_read(struct file *file, char __user *buf, size_t count, loff_t *off)
retval = -EAGAIN;
goto out;
}
- interruptible_sleep_on(&(dev->drv[drvidx]->st_waitq));
+ wait_event_interruptible(dev->drv[drvidx]->st_waitq,
+ dev->drv[drvidx]->stavail);
}
if (dev->drv[drvidx]->interface->readstat) {
if (count > dev->drv[drvidx]->stavail)
@@ -1188,8 +1191,8 @@ isdn_write(struct file *file, const char __user *buf, size_t count, loff_t *off)
goto out;
}
chidx = isdn_minor2chan(minor);
- while ((retval = isdn_writebuf_stub(drvidx, chidx, buf, count)) == 0)
- interruptible_sleep_on(&dev->drv[drvidx]->snd_waitq[chidx]);
+ wait_event_interruptible(dev->drv[drvidx]->snd_waitq[chidx],
+ (retval = isdn_writebuf_stub(drvidx, chidx, buf, count)));
goto out;
}
if (minor <= ISDN_MINOR_CTRLMAX) {
--
1.8.3.2
These two drivers use identical code for their procfs status
file handling, which contains a small race against status
data becoming available while reading the file.
This uses wait_event_interruptible instead to fix this
particular race and eventually get rid of all sleep_on
instances. There seems to be another race involving
multiple concurrent readers of the same procfs file, which
I don't try to fix here.
Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Karsten Keil <[email protected]>
Cc: [email protected]
---
drivers/isdn/divert/divert_procfs.c | 7 ++++---
drivers/isdn/hysdn/hysdn_proclog.c | 7 ++++---
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/isdn/divert/divert_procfs.c b/drivers/isdn/divert/divert_procfs.c
index fb4f1ba..1c5dc34 100644
--- a/drivers/isdn/divert/divert_procfs.c
+++ b/drivers/isdn/divert/divert_procfs.c
@@ -86,12 +86,13 @@ isdn_divert_read(struct file *file, char __user *buf, size_t count, loff_t *off)
struct divert_info *inf;
int len;
- if (!*((struct divert_info **) file->private_data)) {
+ if (!(inf = *((struct divert_info **) file->private_data))) {
if (file->f_flags & O_NONBLOCK)
return -EAGAIN;
- interruptible_sleep_on(&(rd_queue));
+ wait_event_interruptible(rd_queue, (inf =
+ *((struct divert_info **) file->private_data)));
}
- if (!(inf = *((struct divert_info **) file->private_data)))
+ if (!inf)
return (0);
inf->usage_cnt--; /* new usage count */
diff --git a/drivers/isdn/hysdn/hysdn_proclog.c b/drivers/isdn/hysdn/hysdn_proclog.c
index b61e8d5..7b5fd8f 100644
--- a/drivers/isdn/hysdn/hysdn_proclog.c
+++ b/drivers/isdn/hysdn/hysdn_proclog.c
@@ -175,14 +175,15 @@ hysdn_log_read(struct file *file, char __user *buf, size_t count, loff_t *off)
int len;
hysdn_card *card = PDE_DATA(file_inode(file));
- if (!*((struct log_data **) file->private_data)) {
+ if (!(inf = *((struct log_data **) file->private_data))) {
struct procdata *pd = card->proclog;
if (file->f_flags & O_NONBLOCK)
return (-EAGAIN);
- interruptible_sleep_on(&(pd->rd_queue));
+ wait_event_interruptible(pd->rd_queue, (inf =
+ *((struct log_data **) file->private_data)));
}
- if (!(inf = *((struct log_data **) file->private_data)))
+ if (!inf)
return (0);
inf->usage_cnt--; /* new usage count */
--
1.8.3.2
sleep_on and related functions are going away and should not be used
in this driver any more.
This removes the call to interruptible_sleep_on for a wait queue that
is never woken up, and replaces an interruptible_sleep_on_timeout
call with the equivalent wait_event_interruptible_timeout() to
avoid a small race.
Both call sites still look fishy and need more work.
Signed-off-by: Arnd Bergmann <arndb.de>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: [email protected]
Cc: Bill Pemberton <[email protected]>
---
drivers/staging/serqt_usb2/serqt_usb2.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/serqt_usb2/serqt_usb2.c b/drivers/staging/serqt_usb2/serqt_usb2.c
index 73fc3cc..e0c209e 100644
--- a/drivers/staging/serqt_usb2/serqt_usb2.c
+++ b/drivers/staging/serqt_usb2/serqt_usb2.c
@@ -970,17 +970,11 @@ static void qt_block_until_empty(struct tty_struct *tty,
{
int timeout = HZ / 10;
int wait = 30;
- int count;
-
- while (1) {
-
- count = qt_chars_in_buffer(tty);
-
- if (count <= 0)
- return;
-
- interruptible_sleep_on_timeout(&qt_port->wait, timeout);
+ /* returns if we get a signal, an error, or the buffer is empty */
+ while (wait_event_interruptible_timeout(qt_port->wait,
+ qt_chars_in_buffer(tty) <= 0,
+ timeout) == 0) {
wait--;
if (wait == 0) {
dev_dbg(&qt_port->port->dev, "%s - TIMEOUT", __func__);
@@ -1137,7 +1131,10 @@ static int qt_ioctl(struct tty_struct *tty,
if (cmd == TIOCMIWAIT) {
while (qt_port != NULL) {
+#if 0
+ /* this never wakes up */
interruptible_sleep_on(&qt_port->msr_wait);
+#endif
if (signal_pending(current))
return -ERESTARTSYS;
else {
--
1.8.3.2
We really want to kill off interruptible_sleep_on, which is defined
in a way that is always racy. There are four usb-serial drivers using
it to implement their tiocmiwait() functions, which is defined in
a way that always has a race when called from user space.
This patch changes the four drivers in the same way to use an open-coded
prepare_to_wait/finish_wait loop to get rid of the deprecated function
call, but it does not address the fundamental race.
This particular method of implementing it was chosen because it is
least invasive, a better but more invasive alternative would be
to use usb_serial_generic_tiocmiwait, which is something I did not
dare try without access to hardware.
Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Johan Hovold <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: [email protected]
---
drivers/usb/serial/ch341.c | 29 ++++++++++++++++--------
drivers/usb/serial/cypress_m8.c | 49 ++++++++++++++++++++++++++---------------
drivers/usb/serial/f81232.c | 29 ++++++++++++++++--------
drivers/usb/serial/pl2303.c | 29 ++++++++++++++++--------
4 files changed, 91 insertions(+), 45 deletions(-)
diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index c2a4171..f5e0eea 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -508,20 +508,22 @@ static int ch341_tiocmiwait(struct tty_struct *tty, unsigned long arg)
u8 status;
u8 changed;
u8 multi_change = 0;
+ DEFINE_WAIT(wait);
+ int ret;
spin_lock_irqsave(&priv->lock, flags);
prevstatus = priv->line_status;
priv->multi_status_change = 0;
spin_unlock_irqrestore(&priv->lock, flags);
+ ret = 0;
while (!multi_change) {
- interruptible_sleep_on(&port->port.delta_msr_wait);
- /* see if a signal did it */
- if (signal_pending(current))
- return -ERESTARTSYS;
+ prepare_to_wait(&port->port.delta_msr_wait, &wait, TASK_INTERRUPTIBLE);
- if (port->serial->disconnected)
- return -EIO;
+ if (port->serial->disconnected) {
+ ret = -EIO;
+ break;
+ }
spin_lock_irqsave(&priv->lock, flags);
status = priv->line_status;
@@ -534,12 +536,21 @@ static int ch341_tiocmiwait(struct tty_struct *tty, unsigned long arg)
((arg & TIOCM_DSR) && (changed & CH341_BIT_DSR)) ||
((arg & TIOCM_CD) && (changed & CH341_BIT_DCD)) ||
((arg & TIOCM_CTS) && (changed & CH341_BIT_CTS))) {
- return 0;
+ break;
}
+
+ schedule();
+
+ /* see if a signal did it */
+ if (signal_pending(current)) {
+ ret = -ERESTARTSYS;
+ break;
+ }
+
prevstatus = status;
}
-
- return 0;
+ finish_wait(&port->port.delta_msr_wait, &wait);
+ return ret;
}
static int ch341_tiocmget(struct tty_struct *tty)
diff --git a/drivers/usb/serial/cypress_m8.c b/drivers/usb/serial/cypress_m8.c
index 558605d..e05c9c6 100644
--- a/drivers/usb/serial/cypress_m8.c
+++ b/drivers/usb/serial/cypress_m8.c
@@ -869,38 +869,51 @@ static int cypress_tiocmiwait(struct tty_struct *tty, unsigned long arg)
{
struct usb_serial_port *port = tty->driver_data;
struct cypress_private *priv = usb_get_serial_port_data(port);
- char diff;
+ char changed;
+ DEFINE_WAIT(wait);
+ int ret;
- for (;;) {
- interruptible_sleep_on(&port->port.delta_msr_wait);
- /* see if a signal did it */
- if (signal_pending(current))
- return -ERESTARTSYS;
+ while (1) {
+ prepare_to_wait(&port->port.delta_msr_wait, &wait, TASK_INTERRUPTIBLE);
- if (port->serial->disconnected)
- return -EIO;
+ if (port->serial->disconnected) {
+ ret = -EIO;
+ break;
+ }
- diff = priv->diff_status;
- if (diff == 0)
- return -EIO; /* no change => error */
+ changed = priv->diff_status;
+ if (changed == 0) {
+ ret = -EIO; /* no change => error */
+ break;
+ }
/* consume all events */
priv->diff_status = 0;
/* return 0 if caller wanted to know about
these bits */
- if (((arg & TIOCM_RNG) && (diff & UART_RI)) ||
- ((arg & TIOCM_DSR) && (diff & UART_DSR)) ||
- ((arg & TIOCM_CD) && (diff & UART_CD)) ||
- ((arg & TIOCM_CTS) && (diff & UART_CTS)))
- return 0;
+ if (((arg & TIOCM_RNG) && (changed & UART_RI)) ||
+ ((arg & TIOCM_DSR) && (changed & UART_DSR)) ||
+ ((arg & TIOCM_CD) && (changed & UART_CD)) ||
+ ((arg & TIOCM_CTS) && (changed & UART_CTS))) {
+ ret = 0;
+ break;
+ }
+
+ schedule();
+
+ /* see if a signal did it */
+ if (signal_pending(current)) {
+ ret = -ERESTARTSYS;
+ break;
+ }
/* otherwise caller can't care less about what
* happened, and so we continue to wait for
* more events.
*/
}
-
- return 0;
+ finish_wait(&port->port.delta_msr_wait, &wait);
+ return ret;
}
static void cypress_set_termios(struct tty_struct *tty,
diff --git a/drivers/usb/serial/f81232.c b/drivers/usb/serial/f81232.c
index 639a18f..935f2e5 100644
--- a/drivers/usb/serial/f81232.c
+++ b/drivers/usb/serial/f81232.c
@@ -249,19 +249,20 @@ static int f81232_tiocmiwait(struct tty_struct *tty, unsigned long arg)
unsigned int prevstatus;
unsigned int status;
unsigned int changed;
+ DEFINE_WAIT(wait);
+ int ret;
spin_lock_irqsave(&priv->lock, flags);
prevstatus = priv->line_status;
spin_unlock_irqrestore(&priv->lock, flags);
while (1) {
- interruptible_sleep_on(&port->port.delta_msr_wait);
- /* see if a signal did it */
- if (signal_pending(current))
- return -ERESTARTSYS;
+ prepare_to_wait(&port->port.delta_msr_wait, &wait, TASK_INTERRUPTIBLE);
- if (port->serial->disconnected)
- return -EIO;
+ if (port->serial->disconnected) {
+ ret = -EIO;
+ break;
+ }
spin_lock_irqsave(&priv->lock, flags);
status = priv->line_status;
@@ -273,12 +274,22 @@ static int f81232_tiocmiwait(struct tty_struct *tty, unsigned long arg)
((arg & TIOCM_DSR) && (changed & UART_DSR)) ||
((arg & TIOCM_CD) && (changed & UART_DCD)) ||
((arg & TIOCM_CTS) && (changed & UART_CTS))) {
- return 0;
+ ret = 0;
+ break;
+ }
+
+ schedule();
+
+ /* see if a signal did it */
+ if (signal_pending(current)) {
+ ret = -ERESTARTSYS;
+ break;
}
+
prevstatus = status;
}
- /* NOTREACHED */
- return 0;
+ finish_wait(&port->port.delta_msr_wait, &wait);
+ return ret;
}
static int f81232_ioctl(struct tty_struct *tty,
diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
index 1e3318d..e8f30bc 100644
--- a/drivers/usb/serial/pl2303.c
+++ b/drivers/usb/serial/pl2303.c
@@ -594,19 +594,20 @@ static int pl2303_tiocmiwait(struct tty_struct *tty, unsigned long arg)
unsigned int prevstatus;
unsigned int status;
unsigned int changed;
+ DEFINE_WAIT(wait);
+ int ret;
spin_lock_irqsave(&priv->lock, flags);
prevstatus = priv->line_status;
spin_unlock_irqrestore(&priv->lock, flags);
while (1) {
- interruptible_sleep_on(&port->port.delta_msr_wait);
- /* see if a signal did it */
- if (signal_pending(current))
- return -ERESTARTSYS;
+ prepare_to_wait(&port->port.delta_msr_wait, &wait, TASK_INTERRUPTIBLE);
- if (port->serial->disconnected)
- return -EIO;
+ if (port->serial->disconnected) {
+ ret = -EIO;
+ break;
+ }
spin_lock_irqsave(&priv->lock, flags);
status = priv->line_status;
@@ -618,12 +619,22 @@ static int pl2303_tiocmiwait(struct tty_struct *tty, unsigned long arg)
((arg & TIOCM_DSR) && (changed & UART_DSR)) ||
((arg & TIOCM_CD) && (changed & UART_DCD)) ||
((arg & TIOCM_CTS) && (changed & UART_CTS))) {
- return 0;
+ ret = 0;
+ break;
+ }
+
+ schedule();
+
+ /* see if a signal did it */
+ if (signal_pending(current)) {
+ ret = -ERESTARTSYS;
+ break;
}
+
prevstatus = status;
}
- /* NOTREACHED */
- return 0;
+ finish_wait(&port->port.delta_msr_wait, &wait);
+ return ret;
}
static int pl2303_ioctl(struct tty_struct *tty,
--
1.8.3.2
sleep_on is known to be racy and going away because of this. All instances
of interruptible_sleep_on and interruptible_sleep_on_timeout in the midibuf
driver can trivially be replaced with wait_event_interruptible and
wait_event_interruptible_timeout.
Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Jaroslav Kysela <[email protected]>
Cc: Takashi Iwai <[email protected]>
Cc: [email protected]
---
sound/oss/midibuf.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/sound/oss/midibuf.c b/sound/oss/midibuf.c
index 8cdb2cf..79615cf 100644
--- a/sound/oss/midibuf.c
+++ b/sound/oss/midibuf.c
@@ -86,9 +86,8 @@ static void drain_midi_queue(int dev)
*/
if (midi_devs[dev]->buffer_status != NULL)
- while (!signal_pending(current) && midi_devs[dev]->buffer_status(dev))
- interruptible_sleep_on_timeout(&midi_sleeper[dev],
- HZ/10);
+ wait_event_interruptible_timeout(midi_sleeper[dev],
+ !midi_devs[dev]->buffer_status(dev), HZ/10);
}
static void midi_input_intr(int dev, unsigned char data)
@@ -233,8 +232,8 @@ void MIDIbuf_release(int dev, struct file *file)
* devices
*/
- while (!signal_pending(current) && DATA_AVAIL(midi_out_buf[dev]))
- interruptible_sleep_on(&midi_sleeper[dev]);
+ wait_event_interruptible(midi_sleeper[dev],
+ !DATA_AVAIL(midi_out_buf[dev]));
/*
* Sync
*/
@@ -282,8 +281,8 @@ int MIDIbuf_write(int dev, struct file *file, const char __user *buf, int count)
goto out;
}
- interruptible_sleep_on(&midi_sleeper[dev]);
- if (signal_pending(current))
+ if (wait_event_interruptible(midi_sleeper[dev],
+ SPACE_AVAIL(midi_out_buf[dev])))
{
c = -EINTR;
goto out;
@@ -325,8 +324,9 @@ int MIDIbuf_read(int dev, struct file *file, char __user *buf, int count)
c = -EAGAIN;
goto out;
}
- interruptible_sleep_on_timeout(&input_sleeper[dev],
- parms[dev].prech_timeout);
+ wait_event_interruptible_timeout(input_sleeper[dev],
+ DATA_AVAIL(midi_in_buf[dev]),
+ parms[dev].prech_timeout);
if (signal_pending(current))
c = -EINTR; /* The user is getting restless */
--
1.8.3.2
The four variants of the synclink driver use the same code in their
open() callback to wait for a port in process of being closed,
using interruptible_sleep_on, which is racy and going away soon.
Making things worse, these functions hold the BTM while doing so,
which means that if we ever enter this code path, we cannot actually
continue since the other thread that is in process of closing the
port can no longer get the BTM.
This addresses both issues by using wait_event_interruptible_tty()
instead.
Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Jiri Slaby <[email protected]>
---
drivers/char/pcmcia/synclink_cs.c | 4 ++--
drivers/tty/synclink.c | 4 ++--
drivers/tty/synclink_gt.c | 4 ++--
drivers/tty/synclinkmp.c | 4 ++--
4 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/char/pcmcia/synclink_cs.c b/drivers/char/pcmcia/synclink_cs.c
index d39cca6..8320abd 100644
--- a/drivers/char/pcmcia/synclink_cs.c
+++ b/drivers/char/pcmcia/synclink_cs.c
@@ -2511,8 +2511,8 @@ static int mgslpc_open(struct tty_struct *tty, struct file * filp)
/* If port is closing, signal caller to try again */
if (tty_hung_up_p(filp) || port->flags & ASYNC_CLOSING){
- if (port->flags & ASYNC_CLOSING)
- interruptible_sleep_on(&port->close_wait);
+ wait_event_interruptible_tty(tty, port->close_wait,
+ !(port->flags & ASYNC_CLOSING));
retval = ((port->flags & ASYNC_HUP_NOTIFY) ?
-EAGAIN : -ERESTARTSYS);
goto cleanup;
diff --git a/drivers/tty/synclink.c b/drivers/tty/synclink.c
index e1ce141..5ae14b4 100644
--- a/drivers/tty/synclink.c
+++ b/drivers/tty/synclink.c
@@ -3404,8 +3404,8 @@ static int mgsl_open(struct tty_struct *tty, struct file * filp)
/* If port is closing, signal caller to try again */
if (tty_hung_up_p(filp) || info->port.flags & ASYNC_CLOSING){
- if (info->port.flags & ASYNC_CLOSING)
- interruptible_sleep_on(&info->port.close_wait);
+ wait_event_interruptible_tty(tty, info->port.close_wait,
+ !(info->port.flags & ASYNC_CLOSING));
retval = ((info->port.flags & ASYNC_HUP_NOTIFY) ?
-EAGAIN : -ERESTARTSYS);
goto cleanup;
diff --git a/drivers/tty/synclink_gt.c b/drivers/tty/synclink_gt.c
index 1abf946..c359a91 100644
--- a/drivers/tty/synclink_gt.c
+++ b/drivers/tty/synclink_gt.c
@@ -674,8 +674,8 @@ static int open(struct tty_struct *tty, struct file *filp)
/* If port is closing, signal caller to try again */
if (tty_hung_up_p(filp) || info->port.flags & ASYNC_CLOSING){
- if (info->port.flags & ASYNC_CLOSING)
- interruptible_sleep_on(&info->port.close_wait);
+ wait_event_interruptible_tty(tty, info->port.close_wait,
+ !(info->port.flags & ASYNC_CLOSING));
retval = ((info->port.flags & ASYNC_HUP_NOTIFY) ?
-EAGAIN : -ERESTARTSYS);
goto cleanup;
diff --git a/drivers/tty/synclinkmp.c b/drivers/tty/synclinkmp.c
index dc6e969..144202e 100644
--- a/drivers/tty/synclinkmp.c
+++ b/drivers/tty/synclinkmp.c
@@ -754,8 +754,8 @@ static int open(struct tty_struct *tty, struct file *filp)
/* If port is closing, signal caller to try again */
if (tty_hung_up_p(filp) || info->port.flags & ASYNC_CLOSING){
- if (info->port.flags & ASYNC_CLOSING)
- interruptible_sleep_on(&info->port.close_wait);
+ wait_event_interruptible_tty(tty, info->port.close_wait,
+ !(info->port.flags & ASYNC_CLOSING));
retval = ((info->port.flags & ASYNC_HUP_NOTIFY) ?
-EAGAIN : -ERESTARTSYS);
goto cleanup;
--
1.8.3.2
interruptible_sleep_on is racy and going away. This replaces
one use in the radio-cadet driver with an open-coded
wait loop that lets us check the condition under the mutex
but sleep without it.
Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Hans Verkuil <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: [email protected]
---
drivers/media/radio/radio-cadet.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/media/radio/radio-cadet.c b/drivers/media/radio/radio-cadet.c
index 545c04c..67b5bbf 100644
--- a/drivers/media/radio/radio-cadet.c
+++ b/drivers/media/radio/radio-cadet.c
@@ -39,6 +39,7 @@
#include <linux/pnp.h>
#include <linux/sched.h>
#include <linux/io.h> /* outb, outb_p */
+#include <linux/wait.h>
#include <media/v4l2-device.h>
#include <media/v4l2-ioctl.h>
#include <media/v4l2-ctrls.h>
@@ -323,25 +324,32 @@ static ssize_t cadet_read(struct file *file, char __user *data, size_t count, lo
struct cadet *dev = video_drvdata(file);
unsigned char readbuf[RDS_BUFFER];
int i = 0;
+ DEFINE_WAIT(wait);
mutex_lock(&dev->lock);
if (dev->rdsstat == 0)
cadet_start_rds(dev);
- if (dev->rdsin == dev->rdsout) {
+ while (1) {
+ prepare_to_wait(&dev->read_queue, &wait, TASK_INTERRUPTIBLE);
+ if (dev->rdsin != dev->rdsout)
+ break;
+
if (file->f_flags & O_NONBLOCK) {
i = -EWOULDBLOCK;
goto unlock;
}
mutex_unlock(&dev->lock);
- interruptible_sleep_on(&dev->read_queue);
+ schedule();
mutex_lock(&dev->lock);
}
+
while (i < count && dev->rdsin != dev->rdsout)
readbuf[i++] = dev->rdsbuf[dev->rdsout++];
if (i && copy_to_user(data, readbuf, i))
i = -EFAULT;
unlock:
+ finish_wait(&dev->read_queue, &wait);
mutex_unlock(&dev->lock);
return i;
}
--
1.8.3.2
sleep_on is known broken and going away. The atari_scsi driver is one of
two remaining users in the falcon_get_lock() function, which is a rather
crazy piece of code. This does not attempt to fix the driver's locking
scheme in general, but at least prevents falcon_get_lock from going to
sleep when no other thread holds the same lock or tries to get it,
and we no longer schedule with irqs disabled.
Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: James E.J. Bottomley <[email protected]>
Cc: [email protected]
---
drivers/scsi/atari_scsi.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index a3e6c8a..b55a58a 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -90,6 +90,7 @@
#include <linux/init.h>
#include <linux/nvram.h>
#include <linux/bitops.h>
+#include <linux/wait.h>
#include <asm/setup.h>
#include <asm/atarihw.h>
@@ -549,8 +550,10 @@ static void falcon_get_lock(void)
local_irq_save(flags);
- while (!in_irq() && falcon_got_lock && stdma_others_waiting())
- sleep_on(&falcon_fairness_wait);
+ wait_event_cmd(falcon_fairness_wait,
+ !in_irq() && falcon_got_lock && stdma_others_waiting(),
+ local_irq_restore(flags),
+ local_irq_save(flags));
while (!falcon_got_lock) {
if (in_irq())
@@ -562,7 +565,10 @@ static void falcon_get_lock(void)
falcon_trying_lock = 0;
wake_up(&falcon_try_wait);
} else {
- sleep_on(&falcon_try_wait);
+ wait_event_cmd(falcon_try_wait,
+ !falcon_got_lock && !falcon_trying_lock,
+ local_irq_restore(flags),
+ local_irq_save(flags));
}
}
--
1.8.3.2
sleep_on() is inherently racy, and has been deprecated for a long time.
This fixes two instances in the atari floppy driver:
* fdc_wait/fdc_busy becomes an open-coded mutex. We cannot use the
regular mutex since it gets released in interrupt context. The
open-coded version using wait_event() and cmpxchg() is equivalent
to the existing code but does the checks atomically, and we can
now safely check the condition with irqs enabled.
* format_wait becomes a completion, which is the natural structure
here. The format ioctl waits for the background task to either
complete or abort.
This does not attempt to fix the preexisting bug of calling schedule
with local interrupts disabled.
Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Jens Axboe <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
---
drivers/block/ataflop.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
index 0e30c6e..96b629e 100644
--- a/drivers/block/ataflop.c
+++ b/drivers/block/ataflop.c
@@ -68,6 +68,8 @@
#include <linux/init.h>
#include <linux/blkdev.h>
#include <linux/mutex.h>
+#include <linux/completion.h>
+#include <linux/wait.h>
#include <asm/atafd.h>
#include <asm/atafdreg.h>
@@ -301,7 +303,7 @@ module_param_array(UserSteprate, int, NULL, 0);
/* Synchronization of FDC access. */
static volatile int fdc_busy = 0;
static DECLARE_WAIT_QUEUE_HEAD(fdc_wait);
-static DECLARE_WAIT_QUEUE_HEAD(format_wait);
+static DECLARE_COMPLETION(format_wait);
static unsigned long changed_floppies = 0xff, fake_change = 0;
#define CHECK_CHANGE_DELAY HZ/2
@@ -608,7 +610,7 @@ static void fd_error( void )
if (IsFormatting) {
IsFormatting = 0;
FormatError = 1;
- wake_up( &format_wait );
+ complete(&format_wait);
return;
}
@@ -650,9 +652,8 @@ static int do_format(int drive, int type, struct atari_format_descr *desc)
DPRINT(("do_format( dr=%d tr=%d he=%d offs=%d )\n",
drive, desc->track, desc->head, desc->sect_offset ));
+ wait_event(fdc_wait, cmpxchg(&fdc_busy, 0, 1) == 0);
local_irq_save(flags);
- while( fdc_busy ) sleep_on( &fdc_wait );
- fdc_busy = 1;
stdma_lock(floppy_irq, NULL);
atari_turnon_irq( IRQ_MFP_FDC ); /* should be already, just to be sure */
local_irq_restore(flags);
@@ -706,7 +707,7 @@ static int do_format(int drive, int type, struct atari_format_descr *desc)
ReqSide = desc->head;
do_fd_action( drive );
- sleep_on( &format_wait );
+ wait_for_completion(&format_wait);
redo_fd_request();
return( FormatError ? -EIO : 0 );
@@ -1229,7 +1230,7 @@ static void fd_writetrack_done( int status )
goto err_end;
}
- wake_up( &format_wait );
+ complete(&format_wait);
return;
err_end:
@@ -1497,8 +1498,7 @@ repeat:
void do_fd_request(struct request_queue * q)
{
DPRINT(("do_fd_request for pid %d\n",current->pid));
- while( fdc_busy ) sleep_on( &fdc_wait );
- fdc_busy = 1;
+ wait_event(fdc_wait, cmpxchg(&fdc_busy, 0, 1) == 0);
stdma_lock(floppy_irq, NULL);
atari_disable_irq( IRQ_MFP_FDC );
--
1.8.3.2
sleep_on and its variants are going away. The use of sleep_on() in
DAC960_V2_ExecuteUserCommand seems to be bogus because the command
by the time we get there, the command has completed already and
we just enter the timeout. Based on this interpretation, I concluded
that we can replace it with a simple msleep(1000) and rearrange the
code around it slightly.
The interruptible_sleep_on_timeout in DAC960_gam_ioctl seems equivalent
to the race-free version using wait_event_interruptible_timeout.
I left the driver to return -EINTR rather than -ERESTARTSYS to preserve
the timeout behavior.
Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Jens Axboe <[email protected]>
---
drivers/block/DAC960.c | 34 ++++++++++++++++------------------
1 file changed, 16 insertions(+), 18 deletions(-)
diff --git a/drivers/block/DAC960.c b/drivers/block/DAC960.c
index eb39501..125d845 100644
--- a/drivers/block/DAC960.c
+++ b/drivers/block/DAC960.c
@@ -6411,12 +6411,12 @@ static bool DAC960_V2_ExecuteUserCommand(DAC960_Controller_T *Controller,
.ScatterGatherSegments[0]
.SegmentByteCount =
CommandMailbox->ControllerInfo.DataTransferSize;
- DAC960_ExecuteCommand(Command);
- while (Controller->V2.NewControllerInformation->PhysicalScanActive)
- {
- DAC960_ExecuteCommand(Command);
- sleep_on_timeout(&Controller->CommandWaitQueue, HZ);
- }
+ while (1) {
+ DAC960_ExecuteCommand(Command);
+ if (!Controller->V2.NewControllerInformation->PhysicalScanActive)
+ break;
+ msleep(1000);
+ }
DAC960_UserCritical("Discovery Completed\n", Controller);
}
}
@@ -7035,18 +7035,16 @@ static long DAC960_gam_ioctl(struct file *file, unsigned int Request,
ErrorCode = -EFAULT;
break;
}
- while (Controller->V2.HealthStatusBuffer->StatusChangeCounter
- == HealthStatusBuffer.StatusChangeCounter &&
- Controller->V2.HealthStatusBuffer->NextEventSequenceNumber
- == HealthStatusBuffer.NextEventSequenceNumber)
- {
- interruptible_sleep_on_timeout(&Controller->HealthStatusWaitQueue,
- DAC960_MonitoringTimerInterval);
- if (signal_pending(current)) {
- ErrorCode = -EINTR;
- break;
- }
- }
+ ErrorCode = wait_event_interruptible_timeout(Controller->HealthStatusWaitQueue,
+ !(Controller->V2.HealthStatusBuffer->StatusChangeCounter
+ == HealthStatusBuffer.StatusChangeCounter &&
+ Controller->V2.HealthStatusBuffer->NextEventSequenceNumber
+ == HealthStatusBuffer.NextEventSequenceNumber),
+ DAC960_MonitoringTimerInterval);
+ if (ErrorCode == -ERESTARTSYS) {
+ ErrorCode = -EINTR;
+ break;
+ }
if (copy_to_user(GetHealthStatus.HealthStatusBuffer,
Controller->V2.HealthStatusBuffer,
sizeof(DAC960_V2_HealthStatusBuffer_T)))
--
1.8.3.2
interruptible_sleep_on is racy and going away. This replaces the one
caller in the swim3 driver with the equivalent race-free
wait_event_interruptible call. Since we're here already, this
also fixes the case where we get interrupted from atomic context,
which used to just spin in the loop.
Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Jens Axboe <[email protected]>
---
drivers/block/swim3.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/drivers/block/swim3.c b/drivers/block/swim3.c
index 20e061c..c74f7b5 100644
--- a/drivers/block/swim3.c
+++ b/drivers/block/swim3.c
@@ -30,6 +30,7 @@
#include <linux/mutex.h>
#include <linux/module.h>
#include <linux/spinlock.h>
+#include <linux/wait.h>
#include <asm/io.h>
#include <asm/dbdma.h>
#include <asm/prom.h>
@@ -840,14 +841,17 @@ static int grab_drive(struct floppy_state *fs, enum swim_state state,
spin_lock_irqsave(&swim3_lock, flags);
if (fs->state != idle && fs->state != available) {
++fs->wanted;
- while (fs->state != available) {
+ /* this will enable irqs in order to sleep */
+ if (!interruptible)
+ wait_event_lock_irq(fs->wait,
+ fs->state == available,
+ swim3_lock);
+ else if (wait_event_interruptible_lock_irq(fs->wait,
+ fs->state == available,
+ swim3_lock)) {
+ --fs->wanted;
spin_unlock_irqrestore(&swim3_lock, flags);
- if (interruptible && signal_pending(current)) {
- --fs->wanted;
- return -EINTR;
- }
- interruptible_sleep_on(&fs->wait);
- spin_lock_irqsave(&swim3_lock, flags);
+ return -EINTR;
}
--fs->wanted;
}
--
1.8.3.2
There is no reason to use sleep_on_timeout here, and we want to get
rid of that interface. Use the simpler msleep_interruptible instead.
Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Hans Verkuil <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: [email protected]
---
drivers/media/usb/usbvision/usbvision.h | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/media/usb/usbvision/usbvision.h b/drivers/media/usb/usbvision/usbvision.h
index 8a25876..eb6dc8a 100644
--- a/drivers/media/usb/usbvision/usbvision.h
+++ b/drivers/media/usb/usbvision/usbvision.h
@@ -205,10 +205,8 @@ enum {
/* Debugging aid */
#define USBVISION_SAY_AND_WAIT(what) { \
- wait_queue_head_t wq; \
- init_waitqueue_head(&wq); \
printk(KERN_INFO "Say: %s\n", what); \
- interruptible_sleep_on_timeout(&wq, HZ * 3); \
+ msleep_interruptible(3000); \
}
/*
--
1.8.3.2
Hello.
On 02-01-2014 16:07, Arnd Bergmann wrote:
> These two drivers use identical code for their procfs status
> file handling, which contains a small race against status
> data becoming available while reading the file.
> This uses wait_event_interruptible instead to fix this
> particular race and eventually get rid of all sleep_on
> instances. There seems to be another race involving
> multiple concurrent readers of the same procfs file, which
> I don't try to fix here.
> Signed-off-by: Arnd Bergmann <[email protected]>
> Cc: Karsten Keil <[email protected]>
> Cc: [email protected]
> ---
> drivers/isdn/divert/divert_procfs.c | 7 ++++---
> drivers/isdn/hysdn/hysdn_proclog.c | 7 ++++---
> 2 files changed, 8 insertions(+), 6 deletions(-)
> diff --git a/drivers/isdn/divert/divert_procfs.c b/drivers/isdn/divert/divert_procfs.c
> index fb4f1ba..1c5dc34 100644
> --- a/drivers/isdn/divert/divert_procfs.c
> +++ b/drivers/isdn/divert/divert_procfs.c
> @@ -86,12 +86,13 @@ isdn_divert_read(struct file *file, char __user *buf, size_t count, loff_t *off)
> struct divert_info *inf;
> int len;
>
> - if (!*((struct divert_info **) file->private_data)) {
> + if (!(inf = *((struct divert_info **) file->private_data))) {
checkpatch.pl shouldn't approve assignment inside *if*. Though you're
moving it from the existing code, it wouldn't hurt to fix it.
> if (file->f_flags & O_NONBLOCK)
> return -EAGAIN;
> - interruptible_sleep_on(&(rd_queue));
> + wait_event_interruptible(rd_queue, (inf =
> + *((struct divert_info **) file->private_data)));
Parens around assignment are hardly useful.
> }
> - if (!(inf = *((struct divert_info **) file->private_data)))
> + if (!inf)
> return (0);
>
> inf->usage_cnt--; /* new usage count */
> diff --git a/drivers/isdn/hysdn/hysdn_proclog.c b/drivers/isdn/hysdn/hysdn_proclog.c
> index b61e8d5..7b5fd8f 100644
> --- a/drivers/isdn/hysdn/hysdn_proclog.c
> +++ b/drivers/isdn/hysdn/hysdn_proclog.c
> @@ -175,14 +175,15 @@ hysdn_log_read(struct file *file, char __user *buf, size_t count, loff_t *off)
> int len;
> hysdn_card *card = PDE_DATA(file_inode(file));
>
> - if (!*((struct log_data **) file->private_data)) {
> + if (!(inf = *((struct log_data **) file->private_data))) {
Here too checkpatch.pk should complain...
> struct procdata *pd = card->proclog;
> if (file->f_flags & O_NONBLOCK)
> return (-EAGAIN);
>
> - interruptible_sleep_on(&(pd->rd_queue));
> + wait_event_interruptible(pd->rd_queue, (inf =
> + *((struct log_data **) file->private_data)));
And parens hardly needed.
WBR, Sergei
Acked-by: Robin Holt <[email protected]>
On Thu, Jan 2, 2014 at 6:07 AM, Arnd Bergmann <[email protected]> wrote:
> interruptible_sleep_on_timeout is deprecated and going away soon.
> The use in the sgi-xp driver leaves me puzzled, so I'd prefer not
> to touch it. This patch replaces it with an open-coded prepare_to_wait
> and finish_wait pair, which should be completely equivalent, so it
> doesn't fix an existing race, but lets us get away with removing
> the function so we can not get any new users.
>
> In order to remove the typical sleep_on race, one would have to
> replace the call with wait_event_interruptible_timeout and add
> a condition to wait for. The fact that there is a one-jiffy timeout
> suggests that we don't actually expect to get woken up properly
> and the caller just uses this as a short sleeping function
> if it doesn't wake up properly.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> Cc: Cliff Whickman <[email protected]>
> Cc: Robin Holt <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> ---
> drivers/misc/sgi-xp/xpc_channel.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/misc/sgi-xp/xpc_channel.c b/drivers/misc/sgi-xp/xpc_channel.c
> index 652593f..128d561 100644
> --- a/drivers/misc/sgi-xp/xpc_channel.c
> +++ b/drivers/misc/sgi-xp/xpc_channel.c
> @@ -828,6 +828,7 @@ enum xp_retval
> xpc_allocate_msg_wait(struct xpc_channel *ch)
> {
> enum xp_retval ret;
> + DEFINE_WAIT(wait);
>
> if (ch->flags & XPC_C_DISCONNECTING) {
> DBUG_ON(ch->reason == xpInterrupted);
> @@ -835,7 +836,9 @@ xpc_allocate_msg_wait(struct xpc_channel *ch)
> }
>
> atomic_inc(&ch->n_on_msg_allocate_wq);
> - ret = interruptible_sleep_on_timeout(&ch->msg_allocate_wq, 1);
> + prepare_to_wait(&ch->msg_allocate_wq, &wait, TASK_INTERRUPTIBLE);
> + ret = schedule_timeout(1);
> + finish_wait(&ch->msg_allocate_wq, &wait);
> atomic_dec(&ch->n_on_msg_allocate_wq);
>
> if (ch->flags & XPC_C_DISCONNECTING) {
> --
> 1.8.3.2
>
On Thursday 02 January 2014 19:01:15 Sergei Shtylyov wrote:
> > diff --git a/drivers/isdn/divert/divert_procfs.c b/drivers/isdn/divert/divert_procfs.c
> > index fb4f1ba..1c5dc34 100644
> > --- a/drivers/isdn/divert/divert_procfs.c
> > +++ b/drivers/isdn/divert/divert_procfs.c
> > @@ -86,12 +86,13 @@ isdn_divert_read(struct file *file, char __user *buf, size_t count, loff_t *off)
> > struct divert_info *inf;
> > int len;
> >
> > - if (!*((struct divert_info **) file->private_data)) {
> > + if (!(inf = *((struct divert_info **) file->private_data))) {
>
> checkpatch.pl shouldn't approve assignment inside *if*. Though you're
> moving it from the existing code, it wouldn't hurt to fix it.
I tried to touch as little as possible, and while I wouldn't use that
style myself, it is applied consistently in this driver, including the
wait_event line I'm adding, where I feel it actually makes sense.
> > if (file->f_flags & O_NONBLOCK)
> > return -EAGAIN;
> > - interruptible_sleep_on(&(rd_queue));
> > + wait_event_interruptible(rd_queue, (inf =
> > + *((struct divert_info **) file->private_data)));
>
> Parens around assignment are hardly useful.
We get a gcc warning without them:
drivers/isdn/divert/divert_procfs.c:95:14: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
*((struct divert_info **) file->private_data));
I can still change the first one (in both files) if you think it's important,
but I'd rather not spend too much energy at coding style changes.
Thanks for taking a look.
Arnd
Hi Arnd,
On Thu, Jan 02, 2014 at 01:07:39PM +0100, Arnd Bergmann wrote:
> We really want to kill off interruptible_sleep_on, which is defined
> in a way that is always racy. There are four usb-serial drivers using
> it to implement their tiocmiwait() functions, which is defined in
> a way that always has a race when called from user space.
>
> This patch changes the four drivers in the same way to use an open-coded
> prepare_to_wait/finish_wait loop to get rid of the deprecated function
> call, but it does not address the fundamental race.
>
> This particular method of implementing it was chosen because it is
> least invasive, a better but more invasive alternative would be
> to use usb_serial_generic_tiocmiwait, which is something I did not
> dare try without access to hardware.
I'd prefer to just fix the race once and for all. These four drivers
have been on my todo list since I converted the other usb-serial drivers
about a year ago. In fact, I posted a fix for f81232 last week, and
I've had a fix for pl2303 brewing as part of larger series for quite
some time.
I'll post a conversion series to linux-usb shortly and make sure to keep
you CC:ed on the sleep_on-killing patches.
Thanks,
Johan
> Signed-off-by: Arnd Bergmann <[email protected]>
> Cc: Johan Hovold <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: [email protected]
> ---
> drivers/usb/serial/ch341.c | 29 ++++++++++++++++--------
> drivers/usb/serial/cypress_m8.c | 49 ++++++++++++++++++++++++++---------------
> drivers/usb/serial/f81232.c | 29 ++++++++++++++++--------
> drivers/usb/serial/pl2303.c | 29 ++++++++++++++++--------
> 4 files changed, 91 insertions(+), 45 deletions(-)
Hello.
On 01/02/2014 07:48 PM, Arnd Bergmann wrote:
>>> diff --git a/drivers/isdn/divert/divert_procfs.c b/drivers/isdn/divert/divert_procfs.c
>>> index fb4f1ba..1c5dc34 100644
>>> --- a/drivers/isdn/divert/divert_procfs.c
>>> +++ b/drivers/isdn/divert/divert_procfs.c
>>> @@ -86,12 +86,13 @@ isdn_divert_read(struct file *file, char __user *buf, size_t count, loff_t *off)
>>> struct divert_info *inf;
>>> int len;
>>>
>>> - if (!*((struct divert_info **) file->private_data)) {
>>> + if (!(inf = *((struct divert_info **) file->private_data))) {
>> checkpatch.pl shouldn't approve assignment inside *if*. Though you're
>> moving it from the existing code, it wouldn't hurt to fix it.
> I tried to touch as little as possible, and while I wouldn't use that
> style myself, it is applied consistently in this driver, including the
> wait_event line I'm adding, where I feel it actually makes sense.
I wasn't feeling sure about that one. It seems to me now that it doesn't
make much sense -- we could do the assignment beforehand.
>>> if (file->f_flags & O_NONBLOCK)
>>> return -EAGAIN;
>>> - interruptible_sleep_on(&(rd_queue));
>>> + wait_event_interruptible(rd_queue, (inf =
>>> + *((struct divert_info **) file->private_data)));
>> Parens around assignment are hardly useful.
> We get a gcc warning without them:
> drivers/isdn/divert/divert_procfs.c:95:14: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
> *((struct divert_info **) file->private_data));
Ah...
> I can still change the first one (in both files) if you think it's important,
I always prefer checkpatch.pl-clean patches, though some people do argue
when they are just moving the badly styled code around.
> but I'd rather not spend too much energy at coding style changes.
Let's wait for the maintainer's opinion on this.
> Arnd
WBR, Sergei
On Thursday 02 January 2014, Johan Hovold wrote:
> I'd prefer to just fix the race once and for all. These four drivers
> have been on my todo list since I converted the other usb-serial drivers
> about a year ago. In fact, I posted a fix for f81232 last week, and
> I've had a fix for pl2303 brewing as part of larger series for quite
> some time.
>
> I'll post a conversion series to linux-usb shortly and make sure to keep
> you CC:ed on the sleep_on-killing patches.
Ah, excellent!
Much better to see the job done properly than me adding more hacks on top.
Arnd
On Thu, Jan 02, 2014 at 01:07:37PM +0100, Arnd Bergmann wrote:
> sleep_on and its variants are racy and going away. This replaces
> the two uses in the cris sync_serial drivers with the equivalent
> but race-free wait_event_interruptible.
Looks good, pulling it into the cris tree for 3.14.
/^JN - Jesper Nilsson
--
Jesper Nilsson -- [email protected]
At Thu, 2 Jan 2014 13:07:47 +0100,
Arnd Bergmann wrote:
>
> We want to remove all sleep_on variants from the kernel because they are
> racy. In case of the pinnacle driver, we can replace
> interruptible_sleep_on_timeout with wait_event_interruptible_timeout
> by changing the meaning of a few flags used in the driver so they
> are cleared at wakeup time, which is a somewhat more appropriate
> way to do the same, although probably still racy.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> Cc: Andrew Veliath <[email protected]>
> Cc: Jaroslav Kysela <[email protected]>
> Cc: Takashi Iwai <[email protected]>
> Cc: [email protected]
I applied all 5 patches (23, 25, 24, 26, and 27) for sound/oss/*,
some with trivial space fixes. Thanks!
Takashi
Hi Arnd,
On 01/02/2014 01:07 PM, Arnd Bergmann wrote:
> sleep_on and its variants are broken and going away soon. This changes
> the omap vout driver to use interruptible_sleep_on_timeout instead,
I assume you mean wait_event_interruptible_timeout here :-)
Reviewed-by: Hans Verkuil <[email protected]>
If there are no other comments, then I plan to merge this next week.
Regards,
Hans
> which fixes potential race where the dma is complete before we
> schedule.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: [email protected]
> ---
> drivers/media/platform/omap/omap_vout_vrfb.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/omap/omap_vout_vrfb.c b/drivers/media/platform/omap/omap_vout_vrfb.c
> index cf1c437..62e7e57 100644
> --- a/drivers/media/platform/omap/omap_vout_vrfb.c
> +++ b/drivers/media/platform/omap/omap_vout_vrfb.c
> @@ -270,7 +270,8 @@ int omap_vout_prepare_vrfb(struct omap_vout_device *vout,
> omap_dma_set_global_params(DMA_DEFAULT_ARB_RATE, 0x20, 0);
>
> omap_start_dma(tx->dma_ch);
> - interruptible_sleep_on_timeout(&tx->wait, VRFB_TX_TIMEOUT);
> + wait_event_interruptible_timeout(tx->wait, tx->tx_status == 1,
> + VRFB_TX_TIMEOUT);
>
> if (tx->tx_status == 0) {
> omap_stop_dma(tx->dma_ch);
>
On 01/02/2014 01:07 PM, Arnd Bergmann wrote:
> There is no reason to use sleep_on_timeout here, and we want to get
> rid of that interface. Use the simpler msleep_interruptible instead.
Since this define is unused anyway, lets just remove it completely.
I'll post a patch for this.
Regards,
Hans
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> Cc: Hans Verkuil <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: [email protected]
> ---
> drivers/media/usb/usbvision/usbvision.h | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/media/usb/usbvision/usbvision.h b/drivers/media/usb/usbvision/usbvision.h
> index 8a25876..eb6dc8a 100644
> --- a/drivers/media/usb/usbvision/usbvision.h
> +++ b/drivers/media/usb/usbvision/usbvision.h
> @@ -205,10 +205,8 @@ enum {
>
> /* Debugging aid */
> #define USBVISION_SAY_AND_WAIT(what) { \
> - wait_queue_head_t wq; \
> - init_waitqueue_head(&wq); \
> printk(KERN_INFO "Say: %s\n", what); \
> - interruptible_sleep_on_timeout(&wq, HZ * 3); \
> + msleep_interruptible(3000); \
> }
>
> /*
>
Hi Arnd!
On 01/02/2014 01:07 PM, Arnd Bergmann wrote:
> interruptible_sleep_on is racy and going away. This replaces
> one use in the radio-cadet driver with an open-coded
> wait loop that lets us check the condition under the mutex
> but sleep without it.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> Cc: Hans Verkuil <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: [email protected]
> ---
> drivers/media/radio/radio-cadet.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/radio/radio-cadet.c b/drivers/media/radio/radio-cadet.c
> index 545c04c..67b5bbf 100644
> --- a/drivers/media/radio/radio-cadet.c
> +++ b/drivers/media/radio/radio-cadet.c
> @@ -39,6 +39,7 @@
> #include <linux/pnp.h>
> #include <linux/sched.h>
> #include <linux/io.h> /* outb, outb_p */
> +#include <linux/wait.h>
> #include <media/v4l2-device.h>
> #include <media/v4l2-ioctl.h>
> #include <media/v4l2-ctrls.h>
> @@ -323,25 +324,32 @@ static ssize_t cadet_read(struct file *file, char __user *data, size_t count, lo
> struct cadet *dev = video_drvdata(file);
> unsigned char readbuf[RDS_BUFFER];
> int i = 0;
> + DEFINE_WAIT(wait);
>
> mutex_lock(&dev->lock);
> if (dev->rdsstat == 0)
> cadet_start_rds(dev);
> - if (dev->rdsin == dev->rdsout) {
> + while (1) {
> + prepare_to_wait(&dev->read_queue, &wait, TASK_INTERRUPTIBLE);
> + if (dev->rdsin != dev->rdsout)
> + break;
> +
> if (file->f_flags & O_NONBLOCK) {
> i = -EWOULDBLOCK;
> goto unlock;
> }
> mutex_unlock(&dev->lock);
> - interruptible_sleep_on(&dev->read_queue);
> + schedule();
> mutex_lock(&dev->lock);
> }
> +
This seems overly complicated. Isn't it enough to replace interruptible_sleep_on
by 'wait_event_interruptible(&dev->read_queue, dev->rdsin != dev->rdsout);'?
Or am I missing something subtle?
Regards,
Hans
> while (i < count && dev->rdsin != dev->rdsout)
> readbuf[i++] = dev->rdsbuf[dev->rdsout++];
>
> if (i && copy_to_user(data, readbuf, i))
> i = -EFAULT;
> unlock:
> + finish_wait(&dev->read_queue, &wait);
> mutex_unlock(&dev->lock);
> return i;
> }
>
On 01/02/2014 01:07 PM, Arnd Bergmann wrote:
> interruptible_sleep_on is racy and going away. In the arv driver that
> race has probably never caused problems since it would require a whole
> video frame to be captured before the read function has a chance to
> go to sleep, but using wait_event_interruptible lets us kill off the
> old interface. In order to do this, we have to slightly adapt the
> meaning of the ar->start_capture field to distinguish between not having
> started a frame and having completed it.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: [email protected]
> ---
> drivers/media/platform/arv.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/arv.c b/drivers/media/platform/arv.c
> index e346d32d..32f6d70 100644
> --- a/drivers/media/platform/arv.c
> +++ b/drivers/media/platform/arv.c
> @@ -307,11 +307,11 @@ static ssize_t ar_read(struct file *file, char *buf, size_t count, loff_t *ppos)
> /*
> * Okay, kick AR LSI to invoke an interrupt
> */
> - ar->start_capture = 0;
> + ar->start_capture = -1;
start_capture is defined as an unsigned. Can you make a new patch that changes
the type of start_capture to int?
Otherwise it looks fine.
Regards,
Hans
> ar_outl(arvcr1 | ARVCR1_HIEN, ARVCR1);
> local_irq_restore(flags);
> /* .... AR interrupts .... */
> - interruptible_sleep_on(&ar->wait);
> + wait_event_interruptible(ar->wait, ar->start_capture == 0);
> if (signal_pending(current)) {
> printk(KERN_ERR "arv: interrupted while get frame data.\n");
> ret = -EINTR;
>
On Friday 17 January 2014, Hans Verkuil wrote:
> > @@ -323,25 +324,32 @@ static ssize_t cadet_read(struct file *file, char __user *data, size_t count, lo
> > struct cadet *dev = video_drvdata(file);
> > unsigned char readbuf[RDS_BUFFER];
> > int i = 0;
> > + DEFINE_WAIT(wait);
> >
> > mutex_lock(&dev->lock);
> > if (dev->rdsstat == 0)
> > cadet_start_rds(dev);
> > - if (dev->rdsin == dev->rdsout) {
> > + while (1) {
> > + prepare_to_wait(&dev->read_queue, &wait, TASK_INTERRUPTIBLE);
> > + if (dev->rdsin != dev->rdsout)
> > + break;
> > +
> > if (file->f_flags & O_NONBLOCK) {
> > i = -EWOULDBLOCK;
> > goto unlock;
> > }
> > mutex_unlock(&dev->lock);
> > - interruptible_sleep_on(&dev->read_queue);
> > + schedule();
> > mutex_lock(&dev->lock);
> > }
> > +
>
> This seems overly complicated. Isn't it enough to replace interruptible_sleep_on
> by 'wait_event_interruptible(&dev->read_queue, dev->rdsin != dev->rdsout);'?
>
> Or am I missing something subtle?
The existing code sleeps with &dev->lock released because the cadet_handler()
function needs to grab (and release) the same lock before it can wake up
the reader thread.
Doing the simple wait_event_interruptible() would result in a deadlock here.
Arnd
On 01/17/2014 11:51 AM, Hans Verkuil wrote:
> On 01/02/2014 01:07 PM, Arnd Bergmann wrote:
>> interruptible_sleep_on is racy and going away. In the arv driver that
>> race has probably never caused problems since it would require a whole
>> video frame to be captured before the read function has a chance to
>> go to sleep, but using wait_event_interruptible lets us kill off the
>> old interface. In order to do this, we have to slightly adapt the
>> meaning of the ar->start_capture field to distinguish between not having
>> started a frame and having completed it.
>>
>> Signed-off-by: Arnd Bergmann <[email protected]>
>> Cc: Mauro Carvalho Chehab <[email protected]>
>> Cc: [email protected]
>> ---
>> drivers/media/platform/arv.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/arv.c b/drivers/media/platform/arv.c
>> index e346d32d..32f6d70 100644
>> --- a/drivers/media/platform/arv.c
>> +++ b/drivers/media/platform/arv.c
>> @@ -307,11 +307,11 @@ static ssize_t ar_read(struct file *file, char *buf, size_t count, loff_t *ppos)
>> /*
>> * Okay, kick AR LSI to invoke an interrupt
>> */
>> - ar->start_capture = 0;
>> + ar->start_capture = -1;
>
> start_capture is defined as an unsigned. Can you make a new patch that changes
> the type of start_capture to int?
>
> Otherwise it looks fine.
ping!
Regards,
Hans
Hi Arnd!
On 01/17/2014 03:28 PM, Arnd Bergmann wrote:
> On Friday 17 January 2014, Hans Verkuil wrote:
>>> @@ -323,25 +324,32 @@ static ssize_t cadet_read(struct file *file, char __user *data, size_t count, lo
>>> struct cadet *dev = video_drvdata(file);
>>> unsigned char readbuf[RDS_BUFFER];
>>> int i = 0;
>>> + DEFINE_WAIT(wait);
>>>
>>> mutex_lock(&dev->lock);
>>> if (dev->rdsstat == 0)
>>> cadet_start_rds(dev);
>>> - if (dev->rdsin == dev->rdsout) {
>>> + while (1) {
>>> + prepare_to_wait(&dev->read_queue, &wait, TASK_INTERRUPTIBLE);
>>> + if (dev->rdsin != dev->rdsout)
>>> + break;
>>> +
>>> if (file->f_flags & O_NONBLOCK) {
>>> i = -EWOULDBLOCK;
>>> goto unlock;
>>> }
>>> mutex_unlock(&dev->lock);
>>> - interruptible_sleep_on(&dev->read_queue);
>>> + schedule();
>>> mutex_lock(&dev->lock);
>>> }
>>> +
>>
>> This seems overly complicated. Isn't it enough to replace interruptible_sleep_on
>> by 'wait_event_interruptible(&dev->read_queue, dev->rdsin != dev->rdsout);'?
>>
>> Or am I missing something subtle?
>
> The existing code sleeps with &dev->lock released because the cadet_handler()
> function needs to grab (and release) the same lock before it can wake up
> the reader thread.
>
> Doing the simple wait_event_interruptible() would result in a deadlock here.
I don't see it. I propose this patch:
Signed-off-by: Hans Verkuil <[email protected]>
diff --git a/drivers/media/radio/radio-cadet.c b/drivers/media/radio/radio-cadet.c
index 545c04c..2f658c6 100644
--- a/drivers/media/radio/radio-cadet.c
+++ b/drivers/media/radio/radio-cadet.c
@@ -327,13 +327,15 @@ static ssize_t cadet_read(struct file *file, char __user *data, size_t count, lo
mutex_lock(&dev->lock);
if (dev->rdsstat == 0)
cadet_start_rds(dev);
- if (dev->rdsin == dev->rdsout) {
+ while (dev->rdsin == dev->rdsout) {
if (file->f_flags & O_NONBLOCK) {
i = -EWOULDBLOCK;
goto unlock;
}
mutex_unlock(&dev->lock);
- interruptible_sleep_on(&dev->read_queue);
+ if (wait_event_interruptible(&dev->read_queue,
+ dev->rdsin != dev->rdsout))
+ return -EINTR;
mutex_lock(&dev->lock);
}
while (i < count && dev->rdsin != dev->rdsout)
Tested with my radio-cadet card.
This looks good to me. If I am still missing something, let me know!
Regards,
Hans
On 02/07/2014 10:32 AM, Hans Verkuil wrote:
> Hi Arnd!
>
> On 01/17/2014 03:28 PM, Arnd Bergmann wrote:
>> On Friday 17 January 2014, Hans Verkuil wrote:
>>>> @@ -323,25 +324,32 @@ static ssize_t cadet_read(struct file *file, char __user *data, size_t count, lo
>>>> struct cadet *dev = video_drvdata(file);
>>>> unsigned char readbuf[RDS_BUFFER];
>>>> int i = 0;
>>>> + DEFINE_WAIT(wait);
>>>>
>>>> mutex_lock(&dev->lock);
>>>> if (dev->rdsstat == 0)
>>>> cadet_start_rds(dev);
>>>> - if (dev->rdsin == dev->rdsout) {
>>>> + while (1) {
>>>> + prepare_to_wait(&dev->read_queue, &wait, TASK_INTERRUPTIBLE);
>>>> + if (dev->rdsin != dev->rdsout)
>>>> + break;
>>>> +
>>>> if (file->f_flags & O_NONBLOCK) {
>>>> i = -EWOULDBLOCK;
>>>> goto unlock;
>>>> }
>>>> mutex_unlock(&dev->lock);
>>>> - interruptible_sleep_on(&dev->read_queue);
>>>> + schedule();
>>>> mutex_lock(&dev->lock);
>>>> }
>>>> +
>>>
>>> This seems overly complicated. Isn't it enough to replace interruptible_sleep_on
>>> by 'wait_event_interruptible(&dev->read_queue, dev->rdsin != dev->rdsout);'?
>>>
>>> Or am I missing something subtle?
>>
>> The existing code sleeps with &dev->lock released because the cadet_handler()
>> function needs to grab (and release) the same lock before it can wake up
>> the reader thread.
>>
>> Doing the simple wait_event_interruptible() would result in a deadlock here.
>
> I don't see it. I propose this patch:
>
> Signed-off-by: Hans Verkuil <[email protected]>
>
> diff --git a/drivers/media/radio/radio-cadet.c b/drivers/media/radio/radio-cadet.c
> index 545c04c..2f658c6 100644
> --- a/drivers/media/radio/radio-cadet.c
> +++ b/drivers/media/radio/radio-cadet.c
> @@ -327,13 +327,15 @@ static ssize_t cadet_read(struct file *file, char __user *data, size_t count, lo
> mutex_lock(&dev->lock);
> if (dev->rdsstat == 0)
> cadet_start_rds(dev);
> - if (dev->rdsin == dev->rdsout) {
> + while (dev->rdsin == dev->rdsout) {
> if (file->f_flags & O_NONBLOCK) {
> i = -EWOULDBLOCK;
> goto unlock;
> }
> mutex_unlock(&dev->lock);
> - interruptible_sleep_on(&dev->read_queue);
> + if (wait_event_interruptible(&dev->read_queue,
Oops, that's without an '&' before dev->read_queue. I forgot to update my
patch before posting, sorry about that.
Hans
> + dev->rdsin != dev->rdsout))
> + return -EINTR;
> mutex_lock(&dev->lock);
> }
> while (i < count && dev->rdsin != dev->rdsout)
>
> Tested with my radio-cadet card.
>
> This looks good to me. If I am still missing something, let me know!
>
> Regards,
>
> Hans
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Friday 07 February 2014 10:32:28 Hans Verkuil wrote:
> mutex_lock(&dev->lock);
> if (dev->rdsstat == 0)
> cadet_start_rds(dev);
> - if (dev->rdsin == dev->rdsout) {
> + while (dev->rdsin == dev->rdsout) {
> if (file->f_flags & O_NONBLOCK) {
> i = -EWOULDBLOCK;
> goto unlock;
> }
> mutex_unlock(&dev->lock);
> - interruptible_sleep_on(&dev->read_queue);
> + if (wait_event_interruptible(&dev->read_queue,
> + dev->rdsin != dev->rdsout))
> + return -EINTR;
> mutex_lock(&dev->lock);
> }
> while (i < count && dev->rdsin != dev->rdsout)
>
This will normally work, but now the mutex is no longer
protecting the shared access to the dev->rdsin and
dev->rdsout variables, which was evidently the intention
of the author of the original code.
AFAICT, the possible result is a similar race as before:
if once CPU changes dev->rdsin after the process in
cadet_read dropped the lock, the wakeup may get lost.
It's quite possible this race never happens in practice,
but the code is probably still wrong.
If you think we don't actually need the lock to check
"dev->rdsin != dev->rdsout", the code can be simplified
further, to
if ((dev->rdsin == dev->rdsout) && (file->f_flags & O_NONBLOCK)) {
return -EWOULDBLOCK;
i = wait_event_interruptible(&dev->read_queue, dev->rdsin != dev->rdsout);
if (i)
return i;
Arnd
On 02/07/2014 11:17 AM, Arnd Bergmann wrote:
> On Friday 07 February 2014 10:32:28 Hans Verkuil wrote:
>> mutex_lock(&dev->lock);
>> if (dev->rdsstat == 0)
>> cadet_start_rds(dev);
>> - if (dev->rdsin == dev->rdsout) {
>> + while (dev->rdsin == dev->rdsout) {
>> if (file->f_flags & O_NONBLOCK) {
>> i = -EWOULDBLOCK;
>> goto unlock;
>> }
>> mutex_unlock(&dev->lock);
>> - interruptible_sleep_on(&dev->read_queue);
>> + if (wait_event_interruptible(&dev->read_queue,
>> + dev->rdsin != dev->rdsout))
>> + return -EINTR;
>> mutex_lock(&dev->lock);
>> }
>> while (i < count && dev->rdsin != dev->rdsout)
>>
>
> This will normally work, but now the mutex is no longer
> protecting the shared access to the dev->rdsin and
> dev->rdsout variables, which was evidently the intention
> of the author of the original code.
>
> AFAICT, the possible result is a similar race as before:
> if once CPU changes dev->rdsin after the process in
> cadet_read dropped the lock, the wakeup may get lost.
>
> It's quite possible this race never happens in practice,
> but the code is probably still wrong.
>
> If you think we don't actually need the lock to check
> "dev->rdsin != dev->rdsout", the code can be simplified
> further, to
>
> if ((dev->rdsin == dev->rdsout) && (file->f_flags & O_NONBLOCK)) {
> return -EWOULDBLOCK;
> i = wait_event_interruptible(&dev->read_queue, dev->rdsin != dev->rdsout);
> if (i)
> return i;
>
> Arnd
>
OK, let's try again. This patch is getting bigger and bigger, but it is always
nice to know that your ISA card that almost no one else in the world has is really,
really working well. :-)
Regards,
Hans
Signed-off-by: Hans Verkuil <[email protected]>
diff --git a/drivers/media/radio/radio-cadet.c b/drivers/media/radio/radio-cadet.c
index 545c04c..d27e8b2 100644
--- a/drivers/media/radio/radio-cadet.c
+++ b/drivers/media/radio/radio-cadet.c
@@ -270,6 +270,16 @@ reset_rds:
outb(inb(dev->io + 1) & 0x7f, dev->io + 1);
}
+static bool cadet_has_rds_data(struct cadet *dev)
+{
+ bool result;
+
+ mutex_lock(&dev->lock);
+ result = dev->rdsin != dev->rdsout;
+ mutex_unlock(&dev->lock);
+ return result;
+}
+
static void cadet_handler(unsigned long data)
{
@@ -279,13 +289,12 @@ static void cadet_handler(unsigned long data)
if (mutex_trylock(&dev->lock)) {
outb(0x3, dev->io); /* Select RDS Decoder Control */
if ((inb(dev->io + 1) & 0x20) != 0)
- printk(KERN_CRIT "cadet: RDS fifo overflow\n");
+ pr_err("cadet: RDS fifo overflow\n");
outb(0x80, dev->io); /* Select RDS fifo */
+
while ((inb(dev->io) & 0x80) != 0) {
dev->rdsbuf[dev->rdsin] = inb(dev->io + 1);
- if (dev->rdsin + 1 == dev->rdsout)
- printk(KERN_WARNING "cadet: RDS buffer overflow\n");
- else
+ if (dev->rdsin + 1 != dev->rdsout)
dev->rdsin++;
}
mutex_unlock(&dev->lock);
@@ -294,7 +303,7 @@ static void cadet_handler(unsigned long data)
/*
* Service pending read
*/
- if (dev->rdsin != dev->rdsout)
+ if (cadet_has_rds_data(dev))
wake_up_interruptible(&dev->read_queue);
/*
@@ -327,22 +336,21 @@ static ssize_t cadet_read(struct file *file, char __user *data, size_t count, lo
mutex_lock(&dev->lock);
if (dev->rdsstat == 0)
cadet_start_rds(dev);
- if (dev->rdsin == dev->rdsout) {
- if (file->f_flags & O_NONBLOCK) {
- i = -EWOULDBLOCK;
- goto unlock;
- }
- mutex_unlock(&dev->lock);
- interruptible_sleep_on(&dev->read_queue);
- mutex_lock(&dev->lock);
- }
+ mutex_unlock(&dev->lock);
+
+ if (!cadet_has_rds_data(dev) && (file->f_flags & O_NONBLOCK))
+ return -EWOULDBLOCK;
+ i = wait_event_interruptible(dev->read_queue, cadet_has_rds_data(dev));
+ if (i)
+ return i;
+
+ mutex_lock(&dev->lock);
while (i < count && dev->rdsin != dev->rdsout)
readbuf[i++] = dev->rdsbuf[dev->rdsout++];
+ mutex_unlock(&dev->lock);
if (i && copy_to_user(data, readbuf, i))
- i = -EFAULT;
-unlock:
- mutex_unlock(&dev->lock);
+ return -EFAULT;
return i;
}
@@ -352,7 +360,7 @@ static int vidioc_querycap(struct file *file, void *priv,
{
strlcpy(v->driver, "ADS Cadet", sizeof(v->driver));
strlcpy(v->card, "ADS Cadet", sizeof(v->card));
- strlcpy(v->bus_info, "ISA", sizeof(v->bus_info));
+ strlcpy(v->bus_info, "ISA:radio-cadet", sizeof(v->bus_info));
v->device_caps = V4L2_CAP_TUNER | V4L2_CAP_RADIO |
V4L2_CAP_READWRITE | V4L2_CAP_RDS_CAPTURE;
v->capabilities = v->device_caps | V4L2_CAP_DEVICE_CAPS;
@@ -491,7 +499,7 @@ static unsigned int cadet_poll(struct file *file, struct poll_table_struct *wait
cadet_start_rds(dev);
mutex_unlock(&dev->lock);
}
- if (dev->rdsin != dev->rdsout)
+ if (cadet_has_rds_data(dev))
res |= POLLIN | POLLRDNORM;
return res;
}
On Friday 07 February 2014, Hans Verkuil wrote:
> OK, let's try again. This patch is getting bigger and bigger, but it is always
> nice to know that your ISA card that almost no one else in the world has is really,
> really working well. :-)
>
> Regards,
>
> Hans
>
> Signed-off-by: Hans Verkuil <[email protected]>
Looks good,
Acked-by: Arnd Bergmann <[email protected]>
On Friday 07 February 2014, Hans Verkuil wrote:
> On 01/17/2014 11:51 AM, Hans Verkuil wrote:
> >> diff --git a/drivers/media/platform/arv.c b/drivers/media/platform/arv.c
> >> index e346d32d..32f6d70 100644
> >> --- a/drivers/media/platform/arv.c
> >> +++ b/drivers/media/platform/arv.c
> >> @@ -307,11 +307,11 @@ static ssize_t ar_read(struct file *file, char *buf, size_t count, loff_t *ppos)
> >> /*
> >> * Okay, kick AR LSI to invoke an interrupt
> >> */
> >> - ar->start_capture = 0;
> >> + ar->start_capture = -1;
> >
> > start_capture is defined as an unsigned. Can you make a new patch that changes
> > the type of start_capture to int?
> >
> > Otherwise it looks fine.
Sorry for the delay. I've updated the patch now and will send it out today
with the other remaining ones.
Arnd
On Friday 17 January 2014, Hans Verkuil wrote:
> On 01/02/2014 01:07 PM, Arnd Bergmann wrote:
> > sleep_on and its variants are broken and going away soon. This changes
> > the omap vout driver to use interruptible_sleep_on_timeout instead,
>
> I assume you mean wait_event_interruptible_timeout here :-)
>
> Reviewed-by: Hans Verkuil <[email protected]>
>
> If there are no other comments, then I plan to merge this next week.
>
Hi Hans,
Not sure if you merged the media patches into a local tree, but I see
they are not in linux-next at the moment. I'll just re-send them,
but please let me know if I can drop them on my end, or better
make sure your tree is in linux-next if you have already picked them
up.
Arnd
On 02/26/14 10:03, Arnd Bergmann wrote:
> On Friday 17 January 2014, Hans Verkuil wrote:
>> On 01/02/2014 01:07 PM, Arnd Bergmann wrote:
>>> sleep_on and its variants are broken and going away soon. This changes
>>> the omap vout driver to use interruptible_sleep_on_timeout instead,
>>
>> I assume you mean wait_event_interruptible_timeout here :-)
>>
>> Reviewed-by: Hans Verkuil <[email protected]>
>>
>> If there are no other comments, then I plan to merge this next week.
>>
>
> Hi Hans,
>
> Not sure if you merged the media patches into a local tree, but I see
> they are not in linux-next at the moment. I'll just re-send them,
> but please let me know if I can drop them on my end, or better
> make sure your tree is in linux-next if you have already picked them
> up.
I've picked it up, but it has not yet been merged. Mauro has been
traveling so not much has been merged recently.
Regards,
Hans