It's been a while since the first submission of these patches,
but a lot of them have made it into linux-next already, so here
is the stuff that is not merged yet, hopefully addressing all
the comments.
Geert and Michael: the I was expecting the ataflop and atari_scsi
patches to be merged already, based on earlier discussion.
Can you apply them to the linux-m68k tree, or do you prefer
them to go through the scsi and block maintainers?
Jens: I did not get any comments for the DAC960 and swim3 patches,
I assume they are good to go in. Please merge.
Hans and Mauro: As I commented on the old thread, I thought the
four media patches were on their way. I have addressed the one
comment that I missed earlier now, and used Hans' version for
the two patches he changed. Please merge or let me know the status
if you have already put them in some tree, but not yet into linux-next
Greg or Andrew: The parport subsystem is orphaned unfortunately,
can one of you pick up that patch?
Davem: The two ATM patches got acks, but I did not hear back from
Karsten regarding the ISDN patches. Can you pick up all six, or
should we wait for comments about the ISDN patches?
Arnd
Cc: Andrew Morton <[email protected]>
Cc: David S. Miller <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: "James E.J. Bottomley" <[email protected]>
Cc: Jens Axboe <[email protected]>
Cc: Karsten Keil <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Michael Schmitz <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Arnd Bergmann (16):
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: drop unused define USBVISION_SAY_AND_WAIT
[media] radio-cadet: avoid interruptible_sleep_on race
[media] arv: fix sleep_on race
parport: fix interruptible_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
sched: remove sleep_on() and friends
Documentation/DocBook/kernel-hacking.tmpl | 10 ------
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/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 | 6 ++--
drivers/media/platform/omap/omap_vout_vrfb.c | 3 +-
drivers/media/radio/radio-cadet.c | 46 ++++++++++++++++------------
drivers/media/usb/usbvision/usbvision.h | 8 -----
drivers/parport/share.c | 3 +-
drivers/scsi/atari_scsi.c | 12 ++++++--
include/linux/wait.h | 11 -------
kernel/sched/core.c | 46 ----------------------------
20 files changed, 113 insertions(+), 162 deletions(-)
--
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 use
in the firestream driver with the appropriate wait_event_interruptible
variant.
Signed-off-by: Arnd Bergmann <[email protected]>
Acked-by: 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
interruptible_sleep_on is racy and going away. This replaces
one use in the radio-cadet driver with an wait_event_interruptible
call that lets us check the condition under the mutex
but sleep without it.
The first version of this patch was done by Arnd, but Hans
came up with a nicer solution.
Signed-off-by: Hans Verkuil <[email protected]>
Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: [email protected]
---
drivers/media/radio/radio-cadet.c | 46 +++++++++++++++++++++++----------------
1 file changed, 27 insertions(+), 19 deletions(-)
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;
}
--
1.8.3.2
Do not apply yet, we first have to wait for the other patches
in the series to get merged. Comments appreciated.
8<----
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 559044c..e7d9d9e 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 b46131e..25377f4 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2852,52 +2852,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
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
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]>
Reviewed-by: Hans Verkuil <[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
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]>
Acked-by: Chas Williams <[email protected]>
Cc: David S. Miller <[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 9587e95..e1b17d57 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
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
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
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
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: Michael Schmitz <[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..b33ce34 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
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]>
Cc: Greg Kroah-Hartman <[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
This define uses the deprecated interruptible_sleep_on_timeout
function. Since this define is unused anyway we just remove it.
Signed-off-by: Hans Verkuil <[email protected]>
Signed-off-by: Arnd Bergmann <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: [email protected]
---
drivers/media/usb/usbvision/usbvision.h | 8 --------
1 file changed, 8 deletions(-)
diff --git a/drivers/media/usb/usbvision/usbvision.h b/drivers/media/usb/usbvision/usbvision.h
index 8a25876..a0c73cf 100644
--- a/drivers/media/usb/usbvision/usbvision.h
+++ b/drivers/media/usb/usbvision/usbvision.h
@@ -203,14 +203,6 @@ enum {
mr = LIMIT_RGB(mm_r); \
}
-/* 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); \
-}
-
/*
* This macro checks if usbvision is still operational. The 'usbvision'
* pointer must be valid, usbvision->dev must be valid, we are not
--
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]>
Cc: Michael Schmitz <[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
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
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 | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/media/platform/arv.c b/drivers/media/platform/arv.c
index e346d32d..e9410e4 100644
--- a/drivers/media/platform/arv.c
+++ b/drivers/media/platform/arv.c
@@ -109,7 +109,7 @@ extern struct cpuinfo_m32r boot_cpu_data;
struct ar {
struct v4l2_device v4l2_dev;
struct video_device vdev;
- unsigned int start_capture; /* duaring capture in INT. mode. */
+ int start_capture; /* duaring capture in INT. mode. */
#if USE_INT
unsigned char *line_buff; /* DMA line buffer */
#endif
@@ -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
From: Arnd Bergmann <[email protected]>
Date: Wed, 26 Feb 2014 12:01:50 +0100
> - 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);
>
When a function call spans multiple lines, please place arguments for
those function calls starting at the first column after the openning
parenthesis.
Thank you.
On Wed, Feb 26 2014, Arnd Bergmann wrote:
> It's been a while since the first submission of these patches,
> but a lot of them have made it into linux-next already, so here
> is the stuff that is not merged yet, hopefully addressing all
> the comments.
>
> Geert and Michael: the I was expecting the ataflop and atari_scsi
> patches to be merged already, based on earlier discussion.
> Can you apply them to the linux-m68k tree, or do you prefer
> them to go through the scsi and block maintainers?
>
> Jens: I did not get any comments for the DAC960 and swim3 patches,
> I assume they are good to go in. Please merge.
Picked up 1, 3, 4 of the patches. Thanks Arnd.
--
Jens Axboe
From: Arnd Bergmann <[email protected]>
Date: Wed, 26 Feb 2014 12:01:51 +0100
> 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]>
> Acked-by: Chas Williams <[email protected]>
Applied.
From: Arnd Bergmann <[email protected]>
Date: Wed, 26 Feb 2014 12:01:52 +0100
> 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]>
Applied.
From: Arnd Bergmann <[email protected]>
Date: Wed, 26 Feb 2014 12:01:53 +0100
> 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]>
Applied.
From: Arnd Bergmann <[email protected]>
Date: Wed, 26 Feb 2014 12:01:54 +0100
> 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]>
Applied.
From: Arnd Bergmann <[email protected]>
Date: Wed, 26 Feb 2014 12:01:55 +0100
> 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]>
Applied.
Arnd,
> It's been a while since the first submission of these patches,
> but a lot of them have made it into linux-next already, so here
> is the stuff that is not merged yet, hopefully addressing all
> the comments.
>
> Geert and Michael: the I was expecting the ataflop and atari_scsi
> patches to be merged already, based on earlier discussion.
> Can you apply them to the linux-m68k tree, or do you prefer
> them to go through the scsi and block maintainers?
Not sure what we decided to do - I'd prefer to double-check the latest
ones first, but I'd be OK with these to go via m68k.
Maybe Geert waits for acks from linux-scsi and linux-block? (The rest
of my patches to Atari SCSI still awaits comment there.)
Geert?
Regards,
Michael
> Jens: I did not get any comments for the DAC960 and swim3 patches,
> I assume they are good to go in. Please merge.
>
> Hans and Mauro: As I commented on the old thread, I thought the
> four media patches were on their way. I have addressed the one
> comment that I missed earlier now, and used Hans' version for
> the two patches he changed. Please merge or let me know the status
> if you have already put them in some tree, but not yet into linux-next
>
> Greg or Andrew: The parport subsystem is orphaned unfortunately,
> can one of you pick up that patch?
>
> Davem: The two ATM patches got acks, but I did not hear back from
> Karsten regarding the ISDN patches. Can you pick up all six, or
> should we wait for comments about the ISDN patches?
>
> Arnd
>
> Cc: Andrew Morton <[email protected]>
> Cc: David S. Miller <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: "James E.J. Bottomley" <[email protected]>
> Cc: Jens Axboe <[email protected]>
> Cc: Karsten Keil <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: Michael Schmitz <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
>
> Arnd Bergmann (16):
> 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: drop unused define USBVISION_SAY_AND_WAIT
> [media] radio-cadet: avoid interruptible_sleep_on race
> [media] arv: fix sleep_on race
> parport: fix interruptible_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
> sched: remove sleep_on() and friends
>
> Documentation/DocBook/kernel-hacking.tmpl | 10 ------
> 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/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 | 6 ++--
> drivers/media/platform/omap/omap_vout_vrfb.c | 3 +-
> drivers/media/radio/radio-cadet.c | 46
> ++++++++++++++++------------
> drivers/media/usb/usbvision/usbvision.h | 8 -----
> drivers/parport/share.c | 3 +-
> drivers/scsi/atari_scsi.c | 12 ++++++--
> include/linux/wait.h | 11 -------
> kernel/sched/core.c | 46
> ----------------------------
> 20 files changed, 113 insertions(+), 162 deletions(-)
>
> --
> 1.8.3.2
Arnd Bergmann wrote:
> 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: Michael Schmitz <[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..b33ce34 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));
> }
> }
>
>
Nack - the completion condition in the first hunk has its logic
reversed. Try this instead (while() loops while condition true, do {}
until () loops while condition false, no?)
I'm 99% confident I had tested your current version of the patch before
and found it still attempts to schedule while in interrupt. I can retest
if you prefer, but that'll have to wait a few days.
diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index a3e6c8a..cc1b013 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));
}
}
Cheers,
Michael
Hi Michael, Arnd,
On Thu, Feb 27, 2014 at 7:37 AM, Michael Schmitz
<[email protected]> wrote:
>> It's been a while since the first submission of these patches,
>> but a lot of them have made it into linux-next already, so here
>> is the stuff that is not merged yet, hopefully addressing all
>> the comments.
>>
>> Geert and Michael: the I was expecting the ataflop and atari_scsi
>> patches to be merged already, based on earlier discussion.
>> Can you apply them to the linux-m68k tree, or do you prefer
>> them to go through the scsi and block maintainers?
>
> Not sure what we decided to do - I'd prefer to double-check the latest ones
> first, but I'd be OK with these to go via m68k.
>
> Maybe Geert waits for acks from linux-scsi and linux-block? (The rest of my
> patches to Atari SCSI still awaits comment there.)
I was waiting for a final confirmation. I was under the impression some rework
was needed, and seeing Michael's NAK confirms that.
I'd be glad to take them through the m68k tree (for 3.15), once they have
received testing and Michael's ACK. Or the block resp. SCSI maintainers can
take them if they prefer, which apparently already happened for 01/16.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
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]>
Acked-by: Chas Williams <[email protected]>
---
diff --git a/drivers/atm/nicstar.c b/drivers/atm/nicstar.c
index 9587e95..8cce153 100644
--- a/drivers/atm/nicstar.c
+++ b/drivers/atm/nicstar.c
@@ -1739,10 +1739,10 @@ 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 +1789,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,
- 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) {
From: Arnd Bergmann <[email protected]>
Date: Thu, 27 Feb 2014 19:51:54 +0100
> 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]>
> Acked-by: Chas Williams <[email protected]>
Applied, thanks.
On Thursday 27 February 2014, Michael Schmitz wrote:
> Arnd Bergmann wrote:
> >
> Nack - the completion condition in the first hunk has its logic
> reversed. Try this instead (while() loops while condition true, do {}
> until () loops while condition false, no?)
Sorry about messing it up again. I though I had fixed it up the
way you commented when you said it worked.
> I'm 99% confident I had tested your current version of the patch before
> and found it still attempts to schedule while in interrupt. I can retest
> if you prefer, but that'll have to wait a few days.
I definitely trust you to have the right version, since you did the
testing.
> diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
> index a3e6c8a..cc1b013 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())
Yes, by inspection your version looks correct and mine looks wrong.
I had figured this out before, just sent the wrong version.
> @@ -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));
> }
I did correct this part compared to my first patch, but forgot
to change the other hunk.
Can you send your version of the patch to Geert for inclusion?
That way I don't have the danger of missing another negation.
This code is clearly too weird to rely on inspection alone and
we know that your version was working when you last tested it.
Arnd
Am 26.02.2014 12:01, schrieb Arnd Bergmann:
> It's been a while since the first submission of these patches,
> but a lot of them have made it into linux-next already, so here
> is the stuff that is not merged yet, hopefully addressing all
> the comments.
>
> Geert and Michael: the I was expecting the ataflop and atari_scsi
> patches to be merged already, based on earlier discussion.
> Can you apply them to the linux-m68k tree, or do you prefer
> them to go through the scsi and block maintainers?
>
> Jens: I did not get any comments for the DAC960 and swim3 patches,
> I assume they are good to go in. Please merge.
>
> Hans and Mauro: As I commented on the old thread, I thought the
> four media patches were on their way. I have addressed the one
> comment that I missed earlier now, and used Hans' version for
> the two patches he changed. Please merge or let me know the status
> if you have already put them in some tree, but not yet into linux-next
>
> Greg or Andrew: The parport subsystem is orphaned unfortunately,
> can one of you pick up that patch?
>
> Davem: The two ATM patches got acks, but I did not hear back from
> Karsten regarding the ISDN patches. Can you pick up all six, or
> should we wait for comments about the ISDN patches?
>
Ack on the ISDN stuff (12,13,14,15)
Hello Arnd,
> On Thursday 27 February 2014, Michael Schmitz wrote:
>
>> Arnd Bergmann wrote:
>>
>>>
>>>
>> Nack - the completion condition in the first hunk has its logic
>> reversed. Try this instead (while() loops while condition true, do {}
>> until () loops while condition false, no?)
>>
>
> Sorry about messing it up again. I though I had fixed it up the
> way you commented when you said it worked.
>
>
>> I'm 99% confident I had tested your current version of the patch before
>> and found it still attempts to schedule while in interrupt. I can retest
>> if you prefer, but that'll have to wait a few days.
>>
>
> I definitely trust you to have the right version, since you did the
> testing.
>
I'm glad I double checked, since there's one other error left in my
correction to your patch below:
The in_irq() condition is not sufficient, we need in_interrupt() there.
This has somehow slipped into a related patch sent to linux-scsi, so
I'll have to refactor the lot. Bugger.
I'll resend the correct version via Geert.
>
>> diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
>> index a3e6c8a..cc1b013 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())
>>
>
> Yes, by inspection your version looks correct and mine looks wrong.
> I had figured this out before, just sent the wrong version.
>
These things happen if you bother fixing other people's weird code :-)
And as I mentioned above, I missed another detail myself
>
>> @@ -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));
>> }
>>
>
> I did correct this part compared to my first patch, but forgot
> to change the other hunk.
>
> Can you send your version of the patch to Geert for inclusion?
> That way I don't have the danger of missing another negation.
> This code is clearly too weird to rely on inspection alone and
> we know that your version was working when you last tested it.
>
Will do - I'll CC: you in so you can ACK the patch if Geert needs
convincing.
Cheers,
Michael