[PATCH, RFC 00/30] sleep_on removal

[PATCH, RFC 00/30] sleep_on removal

[Arnd Bergmann]

The functions sleep_on, sleep_on_timeout, interruptible_sleep_on
and interruptible_sleep_on_timeout have been deprecated for as
long as I can remember, and a number of people have contributed
patches in the past to remove them from various drivers.

This has recently popped up again and I decided to spend some
of my time on tracking down the last users and kill it for good.
The work is somewhat related to the BKL removal I did a couple
of years ago, since most of these drivers were using the BKL
in a way that actually made the sleep_on function safe to call.
As the BKL was converted into mutexes, the semantics changed
slightly and typically opened up a race between checking
a condition and going to sleep.

I don't have any of the hardware that I'm sending the patches
for, so it would be great if someone could test them before
they get applied. Otherwise please review very carefully.

I'm definitely happy for any patches to go into maintainer
trees right away. Obviously the final patch cannot go in
until everything else gets merged first and I suspect there
will be a series of patches for maintainerless drivers that
will go along with it.

	Arnd

Arnd Bergmann (30):
  ataflop: fix sleep_on races
  scsi: atari_scsi: fix sleep_on race
  DAC960: remove sleep_on usage
  swim3: fix interruptible_sleep_on race
  [media] omap_vout: avoid sleep_on race
  [media] usbvision: remove bogus sleep_on_timeout
  [media] radio-cadet: avoid interruptible_sleep_on race
  [media] arv: fix sleep_on race
  staging: serqt_usb2: don't use sleep_on
  staging: gdm72xx: fix interruptible_sleep_on race
  staging: panel: fix interruptible_sleep_on race
  parport: fix interruptible_sleep_on race
  cris: sync_serial: remove interruptible_sleep_on
  tty/amiserial: avoid interruptible_sleep_on
  usbserial: stop using interruptible_sleep_on
  tty: synclink: avoid sleep_on race
  atm: nicstar: remove interruptible_sleep_on_timeout
  atm: firestream: fix interruptible_sleep_on race
  isdn: pcbit: fix interruptible_sleep_on race
  isdn: hisax/elsa: fix sleep_on race in elsa FSM
  isdn: divert, hysdn: fix interruptible_sleep_on race
  isdn: fix multiple sleep_on races
  oss: msnd_pinnacle: avoid interruptible_sleep_on_timeout
  oss: midibuf: fix sleep_on races
  oss: vwsnd: avoid interruptible_sleep_on
  oss: dmasound: kill SLEEP() macro to avoid race
  oss: remove last sleep_on users
  sgi-xp: open-code interruptible_sleep_on_timeout
  char: nwbutton: open-code interruptible_sleep_on
  sched: remove sleep_on() and friends

 Documentation/DocBook/kernel-hacking.tmpl    | 10 ------
 arch/cris/arch-v10/drivers/sync_serial.c     |  4 ++-
 arch/cris/arch-v32/drivers/sync_serial.c     |  4 ++-
 drivers/atm/firestream.c                     |  4 +--
 drivers/atm/nicstar.c                        | 13 ++++----
 drivers/block/DAC960.c                       | 34 +++++++++----------
 drivers/block/ataflop.c                      | 16 ++++-----
 drivers/block/swim3.c                        | 18 ++++++----
 drivers/char/nwbutton.c                      |  5 ++-
 drivers/char/pcmcia/synclink_cs.c            |  4 +--
 drivers/isdn/divert/divert_procfs.c          |  7 ++--
 drivers/isdn/hisax/elsa.c                    |  9 +++--
 drivers/isdn/hisax/elsa_ser.c                |  3 +-
 drivers/isdn/hysdn/hysdn_proclog.c           |  7 ++--
 drivers/isdn/i4l/isdn_common.c               | 13 +++++---
 drivers/isdn/pcbit/drv.c                     |  6 ++--
 drivers/media/platform/arv.c                 |  4 +--
 drivers/media/platform/omap/omap_vout_vrfb.c |  3 +-
 drivers/media/radio/radio-cadet.c            | 12 +++++--
 drivers/media/usb/usbvision/usbvision.h      |  4 +--
 drivers/misc/sgi-xp/xpc_channel.c            |  5 ++-
 drivers/parport/share.c                      |  3 +-
 drivers/scsi/atari_scsi.c                    | 12 +++++--
 drivers/staging/gdm72xx/gdm_usb.c            |  5 +--
 drivers/staging/panel/panel.c                |  4 +--
 drivers/staging/serqt_usb2/serqt_usb2.c      | 17 ++++------
 drivers/tty/amiserial.c                      | 26 ++++++++++-----
 drivers/tty/synclink.c                       |  4 +--
 drivers/tty/synclink_gt.c                    |  4 +--
 drivers/tty/synclinkmp.c                     |  4 +--
 drivers/usb/serial/ch341.c                   | 29 +++++++++++-----
 drivers/usb/serial/cypress_m8.c              | 49 ++++++++++++++++++----------
 drivers/usb/serial/f81232.c                  | 29 +++++++++++-----
 drivers/usb/serial/pl2303.c                  | 29 +++++++++++-----
 include/linux/wait.h                         | 11 -------
 kernel/sched/core.c                          | 46 --------------------------
 sound/oss/dmabuf.c                           | 14 ++++----
 sound/oss/dmasound/dmasound.h                |  1 -
 sound/oss/dmasound/dmasound_core.c           | 28 +++++++++++-----
 sound/oss/midibuf.c                          | 18 +++++-----
 sound/oss/msnd_pinnacle.c                    | 31 ++++++++++--------
 sound/oss/sequencer.c                        | 16 ++++-----
 sound/oss/sleep.h                            | 18 ++++++++++
 sound/oss/swarm_cs4297a.c                    | 14 ++++----
 sound/oss/vwsnd.c                            | 14 +++++---
 45 files changed, 334 insertions(+), 277 deletions(-)
 create mode 100644 sound/oss/sleep.h

-- 
1.8.3.2

Cc: akpm@osdl.org
Cc: chas@cmf.nrl.navy.mil
Cc: davem@davemloft.net
Cc: geert@linux-m68k.org
Cc: gregkh@linuxfoundation.org
Cc: hverkuil@xs4all.nl
Cc: mingo@kernel.org
Cc: JBottomley@parallels.com
Cc: perex@perex.cz
Cc: axboe@kernel.dk
Cc: jslaby@suse.cz
Cc: isdn@linux-pingi.de
Cc: m.chehab@samsung.com
Cc: peterz@infradead.org
Cc: robinmholt@gmail.com
Cc: tiwai@suse.de
Cc: alsa-devel@alsa-project.org
Cc: devel@driverdev.osuosl.org
Cc: linux-atm-general@lists.sourceforge.net
Cc: linux-cris-kernel@axis.com
Cc: linux-media@vger.kernel.org
Cc: linux-scsi@vger.kernel.org
Cc: linux-usb@vger.kernel.org
Cc: netdev@vger.kernel.org

[PATCH, RFC 05/30] [media] omap_vout: avoid sleep_on race

[Arnd Bergmann]

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 <arnd@arndb.de>
Cc: Mauro Carvalho Chehab <m.chehab@samsung.com>
Cc: linux-media@vger.kernel.org
---
 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

[PATCH, RFC 06/30] [media] usbvision: remove bogus sleep_on_timeout

[Arnd Bergmann]

There is no reason to use sleep_on_timeout here, and we want to get
rid of that interface. Use the simpler msleep_interruptible instead.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: Hans Verkuil <hverkuil@xs4all.nl>
Cc: Mauro Carvalho Chehab <m.chehab@samsung.com>
Cc: linux-media@vger.kernel.org
---
 drivers/media/usb/usbvision/usbvision.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/media/usb/usbvision/usbvision.h b/drivers/media/usb/usbvision/usbvision.h
index 8a25876..eb6dc8a 100644
--- a/drivers/media/usb/usbvision/usbvision.h
+++ b/drivers/media/usb/usbvision/usbvision.h
@@ -205,10 +205,8 @@ enum {
 
 /* Debugging aid */
 #define USBVISION_SAY_AND_WAIT(what) { \
-	wait_queue_head_t wq; \
-	init_waitqueue_head(&wq); \
 	printk(KERN_INFO "Say: %s\n", what); \
-	interruptible_sleep_on_timeout(&wq, HZ * 3); \
+	msleep_interruptible(3000); \
 }
 
 /*
-- 
1.8.3.2

[PATCH, RFC 07/30] [media] radio-cadet: avoid interruptible_sleep_on race

[Arnd Bergmann]

interruptible_sleep_on is racy and going away. This replaces
one use in the radio-cadet driver with an open-coded
wait loop that lets us check the condition under the mutex
but sleep without it.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: Hans Verkuil <hverkuil@xs4all.nl>
Cc: Mauro Carvalho Chehab <m.chehab@samsung.com>
Cc: linux-media@vger.kernel.org
---
 drivers/media/radio/radio-cadet.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/media/radio/radio-cadet.c b/drivers/media/radio/radio-cadet.c
index 545c04c..67b5bbf 100644
--- a/drivers/media/radio/radio-cadet.c
+++ b/drivers/media/radio/radio-cadet.c
@@ -39,6 +39,7 @@
 #include <linux/pnp.h>
 #include <linux/sched.h>
 #include <linux/io.h>		/* outb, outb_p			*/
+#include <linux/wait.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-ioctl.h>
 #include <media/v4l2-ctrls.h>
@@ -323,25 +324,32 @@ static ssize_t cadet_read(struct file *file, char __user *data, size_t count, lo
 	struct cadet *dev = video_drvdata(file);
 	unsigned char readbuf[RDS_BUFFER];
 	int i = 0;
+	DEFINE_WAIT(wait);
 
 	mutex_lock(&dev->lock);
 	if (dev->rdsstat == 0)
 		cadet_start_rds(dev);
-	if (dev->rdsin == dev->rdsout) {
+	while (1) {
+		prepare_to_wait(&dev->read_queue, &wait, TASK_INTERRUPTIBLE);
+		if (dev->rdsin != dev->rdsout)
+			break;
+
 		if (file->f_flags & O_NONBLOCK) {
 			i = -EWOULDBLOCK;
 			goto unlock;
 		}
 		mutex_unlock(&dev->lock);
-		interruptible_sleep_on(&dev->read_queue);
+		schedule();
 		mutex_lock(&dev->lock);
 	}
+
 	while (i < count && dev->rdsin != dev->rdsout)
 		readbuf[i++] = dev->rdsbuf[dev->rdsout++];
 
 	if (i && copy_to_user(data, readbuf, i))
 		i = -EFAULT;
 unlock:
+	finish_wait(&dev->read_queue, &wait);
 	mutex_unlock(&dev->lock);
 	return i;
 }
-- 
1.8.3.2

[PATCH, RFC 08/30] [media] arv: fix sleep_on race

[Arnd Bergmann]

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 <arnd@arndb.de>
Cc: Mauro Carvalho Chehab <m.chehab@samsung.com>
Cc: linux-media@vger.kernel.org
---
 drivers/media/platform/arv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/arv.c b/drivers/media/platform/arv.c
index e346d32d..32f6d70 100644
--- a/drivers/media/platform/arv.c
+++ b/drivers/media/platform/arv.c
@@ -307,11 +307,11 @@ static ssize_t ar_read(struct file *file, char *buf, size_t count, loff_t *ppos)
 	/*
 	 * Okay, kick AR LSI to invoke an interrupt
 	 */
-	ar->start_capture = 0;
+	ar->start_capture = -1;
 	ar_outl(arvcr1 | ARVCR1_HIEN, ARVCR1);
 	local_irq_restore(flags);
 	/* .... AR interrupts .... */
-	interruptible_sleep_on(&ar->wait);
+	wait_event_interruptible(ar->wait, ar->start_capture == 0);
 	if (signal_pending(current)) {
 		printk(KERN_ERR "arv: interrupted while get frame data.\n");
 		ret = -EINTR;
-- 
1.8.3.2

Re: [PATCH, RFC 05/30] [media] omap_vout: avoid sleep_on race

[Hans Verkuil]

Hi Arnd,

On 01/02/2014 01:07 PM, Arnd Bergmann wrote:
> sleep_on and its variants are broken and going away soon. This changes
> the omap vout driver to use interruptible_sleep_on_timeout instead,

I assume you mean wait_event_interruptible_timeout here :-)

Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>

If there are no other comments, then I plan to merge this next week.

Regards,

	Hans

> which fixes potential race where the dma is complete before we
> schedule.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Cc: Mauro Carvalho Chehab <m.chehab@samsung.com>
> Cc: linux-media@vger.kernel.org
> ---
>  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);
> 

Re: [PATCH, RFC 06/30] [media] usbvision: remove bogus sleep_on_timeout

[Hans Verkuil]

On 01/02/2014 01:07 PM, Arnd Bergmann wrote:
> There is no reason to use sleep_on_timeout here, and we want to get
> rid of that interface. Use the simpler msleep_interruptible instead.

Since this define is unused anyway, lets just remove it completely.

I'll post a patch for this.

Regards,

	Hans

> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Cc: Hans Verkuil <hverkuil@xs4all.nl>
> Cc: Mauro Carvalho Chehab <m.chehab@samsung.com>
> Cc: linux-media@vger.kernel.org
> ---
>  drivers/media/usb/usbvision/usbvision.h | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/media/usb/usbvision/usbvision.h b/drivers/media/usb/usbvision/usbvision.h
> index 8a25876..eb6dc8a 100644
> --- a/drivers/media/usb/usbvision/usbvision.h
> +++ b/drivers/media/usb/usbvision/usbvision.h
> @@ -205,10 +205,8 @@ enum {
>  
>  /* Debugging aid */
>  #define USBVISION_SAY_AND_WAIT(what) { \
> -	wait_queue_head_t wq; \
> -	init_waitqueue_head(&wq); \
>  	printk(KERN_INFO "Say: %s\n", what); \
> -	interruptible_sleep_on_timeout(&wq, HZ * 3); \
> +	msleep_interruptible(3000); \
>  }
>  
>  /*
> 

Re: [PATCH, RFC 07/30] [media] radio-cadet: avoid interruptible_sleep_on race

[Hans Verkuil]

Hi Arnd!

On 01/02/2014 01:07 PM, Arnd Bergmann wrote:
> interruptible_sleep_on is racy and going away. This replaces
> one use in the radio-cadet driver with an open-coded
> wait loop that lets us check the condition under the mutex
> but sleep without it.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Cc: Hans Verkuil <hverkuil@xs4all.nl>
> Cc: Mauro Carvalho Chehab <m.chehab@samsung.com>
> Cc: linux-media@vger.kernel.org
> ---
>  drivers/media/radio/radio-cadet.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/radio/radio-cadet.c b/drivers/media/radio/radio-cadet.c
> index 545c04c..67b5bbf 100644
> --- a/drivers/media/radio/radio-cadet.c
> +++ b/drivers/media/radio/radio-cadet.c
> @@ -39,6 +39,7 @@
>  #include <linux/pnp.h>
>  #include <linux/sched.h>
>  #include <linux/io.h>		/* outb, outb_p			*/
> +#include <linux/wait.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-ioctl.h>
>  #include <media/v4l2-ctrls.h>
> @@ -323,25 +324,32 @@ static ssize_t cadet_read(struct file *file, char __user *data, size_t count, lo
>  	struct cadet *dev = video_drvdata(file);
>  	unsigned char readbuf[RDS_BUFFER];
>  	int i = 0;
> +	DEFINE_WAIT(wait);
>  
>  	mutex_lock(&dev->lock);
>  	if (dev->rdsstat == 0)
>  		cadet_start_rds(dev);
> -	if (dev->rdsin == dev->rdsout) {
> +	while (1) {
> +		prepare_to_wait(&dev->read_queue, &wait, TASK_INTERRUPTIBLE);
> +		if (dev->rdsin != dev->rdsout)
> +			break;
> +
>  		if (file->f_flags & O_NONBLOCK) {
>  			i = -EWOULDBLOCK;
>  			goto unlock;
>  		}
>  		mutex_unlock(&dev->lock);
> -		interruptible_sleep_on(&dev->read_queue);
> +		schedule();
>  		mutex_lock(&dev->lock);
>  	}
> +

This seems overly complicated. Isn't it enough to replace interruptible_sleep_on
by 'wait_event_interruptible(&dev->read_queue, dev->rdsin != dev->rdsout);'?

Or am I missing something subtle?

Regards,

	Hans

>  	while (i < count && dev->rdsin != dev->rdsout)
>  		readbuf[i++] = dev->rdsbuf[dev->rdsout++];
>  
>  	if (i && copy_to_user(data, readbuf, i))
>  		i = -EFAULT;
>  unlock:
> +	finish_wait(&dev->read_queue, &wait);
>  	mutex_unlock(&dev->lock);
>  	return i;
>  }
> 

Re: [PATCH, RFC 08/30] [media] arv: fix sleep_on race

[Hans Verkuil]

On 01/02/2014 01:07 PM, Arnd Bergmann wrote:
> interruptible_sleep_on is racy and going away. In the arv driver that
> race has probably never caused problems since it would require a whole
> video frame to be captured before the read function has a chance to
> go to sleep, but using wait_event_interruptible lets us kill off the
> old interface. In order to do this, we have to slightly adapt the
> meaning of the ar->start_capture field to distinguish between not having
> started a frame and having completed it.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Cc: Mauro Carvalho Chehab <m.chehab@samsung.com>
> Cc: linux-media@vger.kernel.org
> ---
>  drivers/media/platform/arv.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/arv.c b/drivers/media/platform/arv.c
> index e346d32d..32f6d70 100644
> --- a/drivers/media/platform/arv.c
> +++ b/drivers/media/platform/arv.c
> @@ -307,11 +307,11 @@ static ssize_t ar_read(struct file *file, char *buf, size_t count, loff_t *ppos)
>  	/*
>  	 * Okay, kick AR LSI to invoke an interrupt
>  	 */
> -	ar->start_capture = 0;
> +	ar->start_capture = -1;

start_capture is defined as an unsigned. Can you make a new patch that changes
the type of start_capture to int?

Otherwise it looks fine.

Regards,

	Hans

>  	ar_outl(arvcr1 | ARVCR1_HIEN, ARVCR1);
>  	local_irq_restore(flags);
>  	/* .... AR interrupts .... */
> -	interruptible_sleep_on(&ar->wait);
> +	wait_event_interruptible(ar->wait, ar->start_capture == 0);
>  	if (signal_pending(current)) {
>  		printk(KERN_ERR "arv: interrupted while get frame data.\n");
>  		ret = -EINTR;
> 

Re: [PATCH, RFC 07/30] [media] radio-cadet: avoid interruptible_sleep_on race

[Arnd Bergmann]

On Friday 17 January 2014, Hans Verkuil wrote:
> > @@ -323,25 +324,32 @@ static ssize_t cadet_read(struct file *file, char __user *data, size_t count, lo
> >       struct cadet *dev = video_drvdata(file);
> >       unsigned char readbuf[RDS_BUFFER];
> >       int i = 0;
> > +     DEFINE_WAIT(wait);
> >  
> >       mutex_lock(&dev->lock);
> >       if (dev->rdsstat == 0)
> >               cadet_start_rds(dev);
> > -     if (dev->rdsin == dev->rdsout) {
> > +     while (1) {
> > +             prepare_to_wait(&dev->read_queue, &wait, TASK_INTERRUPTIBLE);
> > +             if (dev->rdsin != dev->rdsout)
> > +                     break;
> > +
> >               if (file->f_flags & O_NONBLOCK) {
> >                       i = -EWOULDBLOCK;
> >                       goto unlock;
> >               }
> >               mutex_unlock(&dev->lock);
> > -             interruptible_sleep_on(&dev->read_queue);
> > +             schedule();
> >               mutex_lock(&dev->lock);
> >       }
> > +
> 
> This seems overly complicated. Isn't it enough to replace interruptible_sleep_on
> by 'wait_event_interruptible(&dev->read_queue, dev->rdsin != dev->rdsout);'?
> 
> Or am I missing something subtle?

The existing code sleeps with &dev->lock released because the cadet_handler()
function needs to grab (and release) the same lock before it can wake up
the reader thread.

Doing the simple wait_event_interruptible() would result in a deadlock here.

	Arnd

Re: [PATCH, RFC 08/30] [media] arv: fix sleep_on race

[Hans Verkuil]

On 01/17/2014 11:51 AM, Hans Verkuil wrote:
> On 01/02/2014 01:07 PM, Arnd Bergmann wrote:
>> interruptible_sleep_on is racy and going away. In the arv driver that
>> race has probably never caused problems since it would require a whole
>> video frame to be captured before the read function has a chance to
>> go to sleep, but using wait_event_interruptible lets us kill off the
>> old interface. In order to do this, we have to slightly adapt the
>> meaning of the ar->start_capture field to distinguish between not having
>> started a frame and having completed it.
>>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> Cc: Mauro Carvalho Chehab <m.chehab@samsung.com>
>> Cc: linux-media@vger.kernel.org
>> ---
>>  drivers/media/platform/arv.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/arv.c b/drivers/media/platform/arv.c
>> index e346d32d..32f6d70 100644
>> --- a/drivers/media/platform/arv.c
>> +++ b/drivers/media/platform/arv.c
>> @@ -307,11 +307,11 @@ static ssize_t ar_read(struct file *file, char *buf, size_t count, loff_t *ppos)
>>  	/*
>>  	 * Okay, kick AR LSI to invoke an interrupt
>>  	 */
>> -	ar->start_capture = 0;
>> +	ar->start_capture = -1;
> 
> start_capture is defined as an unsigned. Can you make a new patch that changes
> the type of start_capture to int?
> 
> Otherwise it looks fine.

ping!

Regards,

	Hans

Re: [PATCH, RFC 07/30] [media] radio-cadet: avoid interruptible_sleep_on race

[Hans Verkuil]

Hi Arnd!

On 01/17/2014 03:28 PM, Arnd Bergmann wrote:
> On Friday 17 January 2014, Hans Verkuil wrote:
>>> @@ -323,25 +324,32 @@ static ssize_t cadet_read(struct file *file, char __user *data, size_t count, lo
>>>       struct cadet *dev = video_drvdata(file);
>>>       unsigned char readbuf[RDS_BUFFER];
>>>       int i = 0;
>>> +     DEFINE_WAIT(wait);
>>>  
>>>       mutex_lock(&dev->lock);
>>>       if (dev->rdsstat == 0)
>>>               cadet_start_rds(dev);
>>> -     if (dev->rdsin == dev->rdsout) {
>>> +     while (1) {
>>> +             prepare_to_wait(&dev->read_queue, &wait, TASK_INTERRUPTIBLE);
>>> +             if (dev->rdsin != dev->rdsout)
>>> +                     break;
>>> +
>>>               if (file->f_flags & O_NONBLOCK) {
>>>                       i = -EWOULDBLOCK;
>>>                       goto unlock;
>>>               }
>>>               mutex_unlock(&dev->lock);
>>> -             interruptible_sleep_on(&dev->read_queue);
>>> +             schedule();
>>>               mutex_lock(&dev->lock);
>>>       }
>>> +
>>
>> This seems overly complicated. Isn't it enough to replace interruptible_sleep_on
>> by 'wait_event_interruptible(&dev->read_queue, dev->rdsin != dev->rdsout);'?
>>
>> Or am I missing something subtle?
> 
> The existing code sleeps with &dev->lock released because the cadet_handler()
> function needs to grab (and release) the same lock before it can wake up
> the reader thread.
> 
> Doing the simple wait_event_interruptible() would result in a deadlock here.

I don't see it. I propose this patch:

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

diff --git a/drivers/media/radio/radio-cadet.c b/drivers/media/radio/radio-cadet.c
index 545c04c..2f658c6 100644
--- a/drivers/media/radio/radio-cadet.c
+++ b/drivers/media/radio/radio-cadet.c
@@ -327,13 +327,15 @@ static ssize_t cadet_read(struct file *file, char __user *data, size_t count, lo
 	mutex_lock(&dev->lock);
 	if (dev->rdsstat == 0)
 		cadet_start_rds(dev);
-	if (dev->rdsin == dev->rdsout) {
+	while (dev->rdsin == dev->rdsout) {
 		if (file->f_flags & O_NONBLOCK) {
 			i = -EWOULDBLOCK;
 			goto unlock;
 		}
 		mutex_unlock(&dev->lock);
-		interruptible_sleep_on(&dev->read_queue);
+		if (wait_event_interruptible(&dev->read_queue,
+					     dev->rdsin != dev->rdsout))
+			return -EINTR;
 		mutex_lock(&dev->lock);
 	}
 	while (i < count && dev->rdsin != dev->rdsout)

Tested with my radio-cadet card.

This looks good to me. If I am still missing something, let me know!

Regards,

	Hans

Re: [PATCH, RFC 07/30] [media] radio-cadet: avoid interruptible_sleep_on race

[Hans Verkuil]

On 02/07/2014 10:32 AM, Hans Verkuil wrote:
> Hi Arnd!
> 
> On 01/17/2014 03:28 PM, Arnd Bergmann wrote:
>> On Friday 17 January 2014, Hans Verkuil wrote:
>>>> @@ -323,25 +324,32 @@ static ssize_t cadet_read(struct file *file, char __user *data, size_t count, lo
>>>>       struct cadet *dev = video_drvdata(file);
>>>>       unsigned char readbuf[RDS_BUFFER];
>>>>       int i = 0;
>>>> +     DEFINE_WAIT(wait);
>>>>  
>>>>       mutex_lock(&dev->lock);
>>>>       if (dev->rdsstat == 0)
>>>>               cadet_start_rds(dev);
>>>> -     if (dev->rdsin == dev->rdsout) {
>>>> +     while (1) {
>>>> +             prepare_to_wait(&dev->read_queue, &wait, TASK_INTERRUPTIBLE);
>>>> +             if (dev->rdsin != dev->rdsout)
>>>> +                     break;
>>>> +
>>>>               if (file->f_flags & O_NONBLOCK) {
>>>>                       i = -EWOULDBLOCK;
>>>>                       goto unlock;
>>>>               }
>>>>               mutex_unlock(&dev->lock);
>>>> -             interruptible_sleep_on(&dev->read_queue);
>>>> +             schedule();
>>>>               mutex_lock(&dev->lock);
>>>>       }
>>>> +
>>>
>>> This seems overly complicated. Isn't it enough to replace interruptible_sleep_on
>>> by 'wait_event_interruptible(&dev->read_queue, dev->rdsin != dev->rdsout);'?
>>>
>>> Or am I missing something subtle?
>>
>> The existing code sleeps with &dev->lock released because the cadet_handler()
>> function needs to grab (and release) the same lock before it can wake up
>> the reader thread.
>>
>> Doing the simple wait_event_interruptible() would result in a deadlock here.
> 
> I don't see it. I propose this patch:
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> 
> diff --git a/drivers/media/radio/radio-cadet.c b/drivers/media/radio/radio-cadet.c
> index 545c04c..2f658c6 100644
> --- a/drivers/media/radio/radio-cadet.c
> +++ b/drivers/media/radio/radio-cadet.c
> @@ -327,13 +327,15 @@ static ssize_t cadet_read(struct file *file, char __user *data, size_t count, lo
>  	mutex_lock(&dev->lock);
>  	if (dev->rdsstat == 0)
>  		cadet_start_rds(dev);
> -	if (dev->rdsin == dev->rdsout) {
> +	while (dev->rdsin == dev->rdsout) {
>  		if (file->f_flags & O_NONBLOCK) {
>  			i = -EWOULDBLOCK;
>  			goto unlock;
>  		}
>  		mutex_unlock(&dev->lock);
> -		interruptible_sleep_on(&dev->read_queue);
> +		if (wait_event_interruptible(&dev->read_queue,

Oops, that's without an '&' before dev->read_queue. I forgot to update my
patch before posting, sorry about that.

	Hans

> +					     dev->rdsin != dev->rdsout))
> +			return -EINTR;
>  		mutex_lock(&dev->lock);
>  	}
>  	while (i < count && dev->rdsin != dev->rdsout)
> 
> Tested with my radio-cadet card.
> 
> This looks good to me. If I am still missing something, let me know!
> 
> Regards,
> 
> 	Hans
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH, RFC 07/30] [media] radio-cadet: avoid interruptible_sleep_on race

[Arnd Bergmann]

On Friday 07 February 2014 10:32:28 Hans Verkuil wrote:
>         mutex_lock(&dev->lock);
>         if (dev->rdsstat == 0)
>                 cadet_start_rds(dev);
> -       if (dev->rdsin == dev->rdsout) {
> +       while (dev->rdsin == dev->rdsout) {
>                 if (file->f_flags & O_NONBLOCK) {
>                         i = -EWOULDBLOCK;
>                         goto unlock;
>                 }
>                 mutex_unlock(&dev->lock);
> -               interruptible_sleep_on(&dev->read_queue);
> +               if (wait_event_interruptible(&dev->read_queue,
> +                                            dev->rdsin != dev->rdsout))
> +                       return -EINTR;
>                 mutex_lock(&dev->lock);
>         }
>         while (i < count && dev->rdsin != dev->rdsout)
> 

This will normally work, but now the mutex is no longer
protecting the shared access to the dev->rdsin and
dev->rdsout variables, which was evidently the intention
of the author of the original code.

AFAICT, the possible result is a similar race as before:
if once CPU changes dev->rdsin after the process in
cadet_read dropped the lock, the wakeup may get lost.

It's quite possible this race never happens in practice,
but the code is probably still wrong.

If you think we don't actually need the lock to check
"dev->rdsin != dev->rdsout", the code can be simplified
further, to

	if ((dev->rdsin == dev->rdsout) && (file->f_flags & O_NONBLOCK)) {
	        return -EWOULDBLOCK;
	i = wait_event_interruptible(&dev->read_queue, dev->rdsin != dev->rdsout);
	if (i)
		return i;
	
	Arnd

Re: [PATCH, RFC 07/30] [media] radio-cadet: avoid interruptible_sleep_on race

[Hans Verkuil]

On 02/07/2014 11:17 AM, Arnd Bergmann wrote:
> On Friday 07 February 2014 10:32:28 Hans Verkuil wrote:
>>         mutex_lock(&dev->lock);
>>         if (dev->rdsstat == 0)
>>                 cadet_start_rds(dev);
>> -       if (dev->rdsin == dev->rdsout) {
>> +       while (dev->rdsin == dev->rdsout) {
>>                 if (file->f_flags & O_NONBLOCK) {
>>                         i = -EWOULDBLOCK;
>>                         goto unlock;
>>                 }
>>                 mutex_unlock(&dev->lock);
>> -               interruptible_sleep_on(&dev->read_queue);
>> +               if (wait_event_interruptible(&dev->read_queue,
>> +                                            dev->rdsin != dev->rdsout))
>> +                       return -EINTR;
>>                 mutex_lock(&dev->lock);
>>         }
>>         while (i < count && dev->rdsin != dev->rdsout)
>>
> 
> This will normally work, but now the mutex is no longer
> protecting the shared access to the dev->rdsin and
> dev->rdsout variables, which was evidently the intention
> of the author of the original code.
> 
> AFAICT, the possible result is a similar race as before:
> if once CPU changes dev->rdsin after the process in
> cadet_read dropped the lock, the wakeup may get lost.
> 
> It's quite possible this race never happens in practice,
> but the code is probably still wrong.
> 
> If you think we don't actually need the lock to check
> "dev->rdsin != dev->rdsout", the code can be simplified
> further, to
> 
> 	if ((dev->rdsin == dev->rdsout) && (file->f_flags & O_NONBLOCK)) {
> 	        return -EWOULDBLOCK;
> 	i = wait_event_interruptible(&dev->read_queue, dev->rdsin != dev->rdsout);
> 	if (i)
> 		return i;
> 	
> 	Arnd
> 

OK, let's try again. This patch is getting bigger and bigger, but it is always
nice to know that your ISA card that almost no one else in the world has is really,
really working well. :-)

Regards,

	Hans

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

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;
 }

Re: [PATCH, RFC 07/30] [media] radio-cadet: avoid interruptible_sleep_on race

[Arnd Bergmann]

On Friday 07 February 2014, Hans Verkuil wrote:
> OK, let's try again. This patch is getting bigger and bigger, but it is always
> nice to know that your ISA card that almost no one else in the world has is really,
> really working well. :-)
> 
> Regards,
> 
>         Hans
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

Looks good,

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH, RFC 08/30] [media] arv: fix sleep_on race

[Arnd Bergmann]

On Friday 07 February 2014, Hans Verkuil wrote:
> On 01/17/2014 11:51 AM, Hans Verkuil wrote:

> >> diff --git a/drivers/media/platform/arv.c b/drivers/media/platform/arv.c
> >> index e346d32d..32f6d70 100644
> >> --- a/drivers/media/platform/arv.c
> >> +++ b/drivers/media/platform/arv.c
> >> @@ -307,11 +307,11 @@ static ssize_t ar_read(struct file *file, char *buf, size_t count, loff_t *ppos)
> >>  	/*
> >>  	 * Okay, kick AR LSI to invoke an interrupt
> >>  	 */
> >> -	ar->start_capture = 0;
> >> +	ar->start_capture = -1;
> > 
> > start_capture is defined as an unsigned. Can you make a new patch that changes
> > the type of start_capture to int?
> > 
> > Otherwise it looks fine.


Sorry for the delay. I've updated the patch now and will send it out today
with the other remaining ones.

	Arnd

Re: [PATCH, RFC 05/30] [media] omap_vout: avoid sleep_on race

[Arnd Bergmann]

On Friday 17 January 2014, Hans Verkuil wrote:
> On 01/02/2014 01:07 PM, Arnd Bergmann wrote:
> > sleep_on and its variants are broken and going away soon. This changes
> > the omap vout driver to use interruptible_sleep_on_timeout instead,
> 
> I assume you mean wait_event_interruptible_timeout here :-)
> 
> Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>
> 
> If there are no other comments, then I plan to merge this next week.
> 

Hi Hans,

Not sure if you merged the media patches into a local tree, but I see
they are not in linux-next at the moment. I'll just re-send them,
but please let me know if I can drop them on my end, or better
make sure your tree is in linux-next if you have already picked them
up.

	Arnd

Re: [PATCH, RFC 05/30] [media] omap_vout: avoid sleep_on race

[Hans Verkuil]

On 02/26/14 10:03, Arnd Bergmann wrote:
> On Friday 17 January 2014, Hans Verkuil wrote:
>> On 01/02/2014 01:07 PM, Arnd Bergmann wrote:
>>> sleep_on and its variants are broken and going away soon. This changes
>>> the omap vout driver to use interruptible_sleep_on_timeout instead,
>>
>> I assume you mean wait_event_interruptible_timeout here :-)
>>
>> Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> If there are no other comments, then I plan to merge this next week.
>>
> 
> Hi Hans,
> 
> Not sure if you merged the media patches into a local tree, but I see
> they are not in linux-next at the moment. I'll just re-send them,
> but please let me know if I can drop them on my end, or better
> make sure your tree is in linux-next if you have already picked them
> up.

I've picked it up, but it has not yet been merged. Mauro has been
traveling so not much has been merged recently.

Regards,

	Hans