sleep_on removal, second try

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(-)

Signature · 1 line

1.8.3.2

Arnd Bergmann

author

2014-02-26 11:06 UTC #1 lore raw

PATCH 1/16 ataflop: fix sleep_on races

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.

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(-)

drivers/block/ataflop.c +8 -8

index 0e30c6e..96b629e 100644
@@ -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 );

Signature · 1 line

1.8.3.2

Arnd Bergmann

author

2014-02-26 11:01 UTC #2 lore raw

PATCH 2/16 scsi: atari_scsi: fix sleep_on race

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.

Cc Michael Schmitz <[email protected]>

Cc Geert Uytterhoeven <[email protected]>

Cc James E.J. Bottomley <[email protected]>

---
 drivers/scsi/atari_scsi.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

drivers/scsi/atari_scsi.c +9 -3

index a3e6c8a..b33ce34 100644
@@ -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));
 		}
 	}

Signature · 1 line

1.8.3.2

Michael Schmitz

maintainer NAK

2014-02-27 07:58 UTC #3 lore raw

Re: [PATCH 02/16] scsi: atari_scsi: fix sleep_on race

Quoted 44 lines · Arnd Bergmann

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(-)
drivers/scsi/atari_scsi.c +5 -2
index a3e6c8a..b33ce34 100644
@@ -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;

drivers/scsi/atari_scsi.c static void falcon_get_lock(void) · line 574

Quoted 10 lines

drivers/scsi/atari_scsi.c +4 -1

 			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.

drivers/scsi/atari_scsi.c +0 -0

index a3e6c8a..cc1b013 100644
@@ -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

Arnd Bergmann

author

2014-02-27 20:44 UTC #4 lore raw

Re: [PATCH 02/16] scsi: atari_scsi: fix sleep_on race

Quoted 5 lines · Michael Schmitz

Quoted 1 line · Arnd Bergmann
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.

Quoted 3 lines

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.

Quoted 15 lines

drivers/scsi/atari_scsi.c +1 -0

index a3e6c8a..cc1b013 100644
@@ -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);

drivers/scsi/atari_scsi.c static void falcon_get_lock(void) · line 559

Quoted 10 lines

drivers/scsi/atari_scsi.c +4 -2

 
-       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.

drivers/scsi/atari_scsi.c static void falcon_get_lock(void) · line 572

Quoted 10 lines

drivers/scsi/atari_scsi.c +4 -1

@@ -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

Michael Schmitz

maintainer

2014-03-01 00:24 UTC #5 lore raw

Re: [PATCH 02/16] scsi: atari_scsi: fix sleep_on race

Hello Arnd,

Quoted 13 lines · Michael Schmitz

Quoted 7 lines
Quoted 2 lines · Arnd Bergmann
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.

Quoted 6 lines

Quoted 2 lines
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.

Quoted 24 lines

drivers/scsi/atari_scsi.c +5 -2

index a3e6c8a..cc1b013 100644
@@ -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));

Quoted 7 lines

drivers/scsi/atari_scsi.c static void falcon_get_lock(void) · line 559
Quoted 3 lines
drivers/scsi/atari_scsi.c +0 -0
        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

Quoted 16 lines

drivers/scsi/atari_scsi.c static void falcon_get_lock(void) · line 573

Quoted 12 lines

drivers/scsi/atari_scsi.c +4 -1

@@ -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.

Quoted 5 lines

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

Arnd Bergmann

author

2014-02-26 11:02 UTC #6 lore raw

PATCH 3/16 DAC960: remove sleep_on usage

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.

Cc Jens Axboe <[email protected]>

---
 drivers/block/DAC960.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

drivers/block/DAC960.c +16 ~~-18~~

index eb39501..125d845 100644
@@ -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)))

Signature · 1 line

1.8.3.2

Arnd Bergmann

author

2014-02-26 11:08 UTC #7 lore raw

PATCH 4/16 swim3: fix interruptible_sleep_on race

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.

Cc Jens Axboe <[email protected]>

---
 drivers/block/swim3.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

drivers/block/swim3.c +11 -7

index 20e061c..c74f7b5 100644
@@ -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;
 	}

Signature · 1 line

1.8.3.2

Arnd Bergmann

author

2014-02-26 11:02 UTC #8 lore raw

PATCH 5/16 omap_vout: avoid sleep_on race

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.

Reviewed-by Hans Verkuil <[email protected]>

Cc Mauro Carvalho Chehab <[email protected]>

---
 drivers/media/platform/omap/omap_vout_vrfb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/media/platform/omap/omap_vout_vrfb.c +2 -1

index cf1c437..62e7e57 100644
@@ -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);

Signature · 1 line

1.8.3.2

Arnd Bergmann

author

2014-02-26 11:02 UTC #9 lore raw

PATCH 6/16 usbvision: drop unused define USBVISION_SAY_AND_WAIT

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]>

Cc Mauro Carvalho Chehab <[email protected]>

---
 drivers/media/usb/usbvision/usbvision.h | 8 --------
 1 file changed, 8 deletions(-)

drivers/media/usb/usbvision/usbvision.h +0 -8

index 8a25876..a0c73cf 100644
@@ -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

Signature · 1 line

1.8.3.2

Arnd Bergmann

author

2014-02-26 11:02 UTC #10 lore raw

PATCH 7/16 radio-cadet: avoid interruptible_sleep_on race

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]>

Cc Mauro Carvalho Chehab <[email protected]>

---
 drivers/media/radio/radio-cadet.c | 46 +++++++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 19 deletions(-)

drivers/media/radio/radio-cadet.c +27 ~~-19~~

index 545c04c..d27e8b2 100644
@@ -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;
 }

Signature · 1 line

1.8.3.2

Arnd Bergmann

author

2014-02-26 11:02 UTC #11 lore raw

PATCH 8/16 arv: fix sleep_on race

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.

Cc Mauro Carvalho Chehab <[email protected]>

---
 drivers/media/platform/arv.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/media/platform/arv.c +3 -3

index e346d32d..e9410e4 100644
@@ -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;

Signature · 1 line

1.8.3.2

Arnd Bergmann

author

2014-02-26 11:06 UTC #12 lore raw

PATCH 9/16 parport: fix interruptible_sleep_on race

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.

Cc Andrew Morton <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

---
 drivers/parport/share.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/parport/share.c +2 -1

index 6a83ee1..3fa6624 100644
@@ -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;
 			}

Signature · 1 line

1.8.3.2

Arnd Bergmann

author

2014-02-26 11:01 UTC #13 lore raw

PATCH 10/16 atm: nicstar: remove interruptible_sleep_on_timeout

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.

Acked-by Chas Williams <[email protected]>

Cc David S. Miller <[email protected]>

---
 drivers/atm/nicstar.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

drivers/atm/nicstar.c +6 -7

index 9587e95..e1b17d57 100644
@@ -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) {

Signature · 1 line

1.8.3.2

David Miller

maintainer

2014-02-26 16:06 UTC #14 lore raw

Re: [PATCH 10/16] atm: nicstar: remove interruptible_sleep_on_timeout

From: Arnd Bergmann <[email protected]>
Date: Wed, 26 Feb 2014 12:01:50 +0100

drivers/atm/nicstar.c static int push_scqe(ns_dev * card, vc_map * vc, scq_info * scq, ns_scqe * tbd, · line 1744

Quoted 8 lines

drivers/atm/nicstar.c +3 -4

-		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.

Arnd Bergmann

author

2014-02-27 18:51 UTC #15 lore raw

PATCH v3 1/1 atm: nicstar: remove interruptible_sleep_on_timeout

Acked-by Chas Williams <[email protected]>

---

drivers/atm/nicstar.c +8 -8

index 9587e95..8cce153 100644
@@ -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) {

David Miller

maintainer Applied

2014-02-27 20:23 UTC #16 lore raw

Re: [PATCH v3] atm: nicstar: remove interruptible_sleep_on_timeout

From: Arnd Bergmann <[email protected]>
Date: Thu, 27 Feb 2014 19:51:54 +0100

Quoted 8 lines

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.

Arnd Bergmann

author

2014-02-26 11:01 UTC #17 lore raw

PATCH 11/16 atm: firestream: fix interruptible_sleep_on race

interruptible_sleep_on is racy and going away. This replaces the one use in the firestream driver with the appropriate wait_event_interruptible variant.

Acked-by Chas Williams <[email protected]>

---
 drivers/atm/firestream.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/atm/firestream.c +2 -2

index b41c948..f43e1c1 100644
@@ -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;

Signature · 1 line

1.8.3.2

David Miller

maintainer Applied

2014-02-26 21:08 UTC #18 lore raw

Re: [PATCH 11/16] atm: firestream: fix interruptible_sleep_on race

From: Arnd Bergmann <[email protected]>
Date: Wed, 26 Feb 2014 12:01:51 +0100

Quoted 6 lines

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.

Arnd Bergmann

author

2014-02-26 11:01 UTC #19 lore raw

PATCH 12/16 isdn: pcbit: fix interruptible_sleep_on race

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.

Cc Karsten Keil <[email protected]>

---
 drivers/isdn/pcbit/drv.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/isdn/pcbit/drv.c +3 -3

index 1eaf622..f02cc50 100644
@@ -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));

Signature · 1 line

1.8.3.2

David Miller

maintainer Applied

2014-02-26 21:08 UTC #20 lore raw

Re: [PATCH 12/16] isdn: pcbit: fix interruptible_sleep_on race

From: Arnd Bergmann <[email protected]>
Date: Wed, 26 Feb 2014 12:01:52 +0100

Quoted 8 lines

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.

Arnd Bergmann

author

2014-02-26 11:01 UTC #21 lore raw

PATCH 13/16 isdn: hisax/elsa: fix sleep_on race in elsa FSM

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.

Cc Karsten Keil <[email protected]>

---
 drivers/isdn/hisax/elsa.c     | 9 ++++++---
 drivers/isdn/hisax/elsa_ser.c | 3 ++-
 2 files changed, 8 insertions(+), 4 deletions(-)

drivers/isdn/hisax/elsa.c +6 -3

index 2be1c8a..d8ef64d 100644
@@ -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);

drivers/isdn/hisax/elsa_ser.c +2 -1

index 3f84dd8..a2a358c 100644
@@ -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);

Signature · 1 line

1.8.3.2

David Miller

maintainer Applied

2014-02-26 21:08 UTC #22 lore raw

Re: [PATCH 13/16] isdn: hisax/elsa: fix sleep_on race in elsa FSM

From: Arnd Bergmann <[email protected]>
Date: Wed, 26 Feb 2014 12:01:53 +0100

Quoted 6 lines

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.

Arnd Bergmann

author

2014-02-26 11:01 UTC #23 lore raw

PATCH 14/16 isdn: divert, hysdn: fix interruptible_sleep_on race

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.

Cc Karsten Keil <[email protected]>

---
 drivers/isdn/divert/divert_procfs.c | 7 ++++---
 drivers/isdn/hysdn/hysdn_proclog.c  | 7 ++++---
 2 files changed, 8 insertions(+), 6 deletions(-)

drivers/isdn/divert/divert_procfs.c +4 -3

index fb4f1ba..1c5dc34 100644
@@ -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 */

drivers/isdn/hysdn/hysdn_proclog.c +4 -3

index b61e8d5..7b5fd8f 100644
@@ -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 */

Signature · 1 line

1.8.3.2

David Miller

maintainer Applied

2014-02-26 21:08 UTC #24 lore raw

Re: [PATCH 14/16] isdn: divert, hysdn: fix interruptible_sleep_on race

From: Arnd Bergmann <[email protected]>
Date: Wed, 26 Feb 2014 12:01:54 +0100

Quoted 11 lines

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.

Arnd Bergmann

author

2014-02-26 11:01 UTC #25 lore raw

PATCH 15/16 isdn: fix multiple sleep_on races

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.

Cc Karsten Keil <[email protected]>

---
 drivers/isdn/i4l/isdn_common.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

drivers/isdn/i4l/isdn_common.c +8 -5

index 9bb12ba..130f216 100644
@@ -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) {

Signature · 1 line

1.8.3.2

David Miller

maintainer Applied

2014-02-26 21:08 UTC #26 lore raw

Re: [PATCH 15/16] isdn: fix multiple sleep_on races

From: Arnd Bergmann <[email protected]>
Date: Wed, 26 Feb 2014 12:01:55 +0100

Quoted 7 lines

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 Bergmann

author

2014-02-26 11:03 UTC #27 lore raw

PATCH 16/16 NOTYET: sched: remove sleep_on() and friends

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.

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(-)

Documentation/DocBook/kernel-hacking.tmpl +0 ~~-10~~

index d0758b24..bd9015d 100644
@@ -850,16 +850,6 @@ printk(KERN_INFO "my ip: %pI4\n", &amp;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>

include/linux/wait.h +0 ~~-11~~

index 559044c..e7d9d9e 100644
@@ -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
  */

kernel/sched/core.c +0 ~~-46~~

52 lines not on this page · raw message

Signature · 1 line

1.8.3.2

Jens Axboe

maintainer Applied

2014-02-26 17:36 UTC #28 lore raw

Quoted 5 lines · 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.

Quoted 7 lines

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.

Signature · 1 line

Jens Axboe

Michael Schmitz

2014-02-27 06:37 UTC #29 lore raw

Arnd,

Quoted 9 lines

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

Quoted 78 lines

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(-)
Signature · 1 line
1.8.3.2

Geert Uytterhoeven

2014-02-27 11:55 UTC #30 lore raw

Hi Michael, Arnd,

Quoted 10 lines · Michael Schmitz

Quoted 9 lines
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?

Quoted 5 lines

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

Karsten Keil

2014-02-28 08:53 UTC #31 lore raw