[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 17/30] atm: nicstar: remove interruptible_sleep_on_timeout

[Arnd Bergmann]

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 <arnd@arndb.de>
Cc: David S. Miller <davem@davemloft.net>
Cc: Chas Williams <chas@cmf.nrl.navy.mil>
Cc: linux-atm-general@lists.sourceforge.net
Cc: netdev@vger.kernel.org
---
 drivers/atm/nicstar.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/atm/nicstar.c b/drivers/atm/nicstar.c
index 5aca5f4..3c079eb 100644
--- a/drivers/atm/nicstar.c
+++ b/drivers/atm/nicstar.c
@@ -1739,10 +1739,9 @@ static int push_scqe(ns_dev * card, vc_map * vc, scq_info * scq, ns_scqe * tbd,
 		}
 
 		scq->full = 1;
-		spin_unlock_irqrestore(&scq->lock, flags);
-		interruptible_sleep_on_timeout(&scq->scqfull_waitq,
-					       SCQFULL_TIMEOUT);
-		spin_lock_irqsave(&scq->lock, flags);
+		wait_event_interruptible_lock_irq_timeout(scq->scqfull_waitq,
+				scq->tail != scq->next, scq->lock,
+				SCQFULL_TIMEOUT);
 
 		if (scq->full) {
 			spin_unlock_irqrestore(&scq->lock, flags);
@@ -1789,10 +1788,10 @@ static int push_scqe(ns_dev * card, vc_map * vc, scq_info * scq, ns_scqe * tbd,
 			scq->full = 1;
 			if (has_run++)
 				break;
-			spin_unlock_irqrestore(&scq->lock, flags);
-			interruptible_sleep_on_timeout(&scq->scqfull_waitq,
+			wait_event_interruptible_lock_irq_timeout(scq->scqfull_waitq,
+						       scq->tail != scq->next,
+						       scq->lock,
 						       SCQFULL_TIMEOUT);
-			spin_lock_irqsave(&scq->lock, flags);
 		}
 
 		if (!scq->full) {
-- 
1.8.3.2

[PATCH, RFC 18/30] atm: firestream: fix interruptible_sleep_on race

[Arnd Bergmann]

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 <arnd@arndb.de>
Cc: Chas Williams <chas@cmf.nrl.navy.mil>
Cc: linux-atm-general@lists.sourceforge.net
Cc: netdev@vger.kernel.org
---
 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

[PATCH, RFC 19/30] isdn: pcbit: fix interruptible_sleep_on race

[Arnd Bergmann]

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 <arnd@arndb.de>
Cc: Karsten Keil <isdn@linux-pingi.de>
Cc: netdev@vger.kernel.org
---
 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

[PATCH, RFC 20/30] isdn: hisax/elsa: fix sleep_on race in elsa FSM

[Arnd Bergmann]

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 <arnd@arndb.de>
Cc: Karsten Keil <isdn@linux-pingi.de>
Cc: netdev@vger.kernel.org
---
 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

[PATCH, RFC 21/30] isdn: divert, hysdn: fix interruptible_sleep_on race

[Arnd Bergmann]

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 <arnd@arndb.de>
Cc: Karsten Keil <isdn@linux-pingi.de>
Cc: netdev@vger.kernel.org
---
 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

[PATCH, RFC 22/30] isdn: fix multiple sleep_on races

[Arnd Bergmann]

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 <arnd@arndb.de>
Cc: Karsten Keil <isdn@linux-pingi.de>
Cc: netdev@vger.kernel.org
---
 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

Re: [PATCH, RFC 21/30] isdn: divert, hysdn: fix interruptible_sleep_on race

[Sergei Shtylyov]

Hello.

On 02-01-2014 16:07, Arnd Bergmann wrote:

> These two drivers use identical code for their procfs status
> file handling, which contains a small race against status
> data becoming available while reading the file.

> This uses wait_event_interruptible instead to fix this
> particular race and eventually get rid of all sleep_on
> instances. There seems to be another race involving
> multiple concurrent readers of the same procfs file, which
> I don't try to fix here.

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Cc: Karsten Keil <isdn@linux-pingi.de>
> Cc: netdev@vger.kernel.org
> ---
>   drivers/isdn/divert/divert_procfs.c | 7 ++++---
>   drivers/isdn/hysdn/hysdn_proclog.c  | 7 ++++---
>   2 files changed, 8 insertions(+), 6 deletions(-)

> diff --git a/drivers/isdn/divert/divert_procfs.c b/drivers/isdn/divert/divert_procfs.c
> index fb4f1ba..1c5dc34 100644
> --- a/drivers/isdn/divert/divert_procfs.c
> +++ b/drivers/isdn/divert/divert_procfs.c
> @@ -86,12 +86,13 @@ isdn_divert_read(struct file *file, char __user *buf, size_t count, loff_t *off)
>   	struct divert_info *inf;
>   	int len;
>
> -	if (!*((struct divert_info **) file->private_data)) {
> +	if (!(inf = *((struct divert_info **) file->private_data))) {

    checkpatch.pl shouldn't approve assignment inside *if*. Though you're 
moving it from the existing code, it wouldn't hurt to fix it.

>   		if (file->f_flags & O_NONBLOCK)
>   			return -EAGAIN;
> -		interruptible_sleep_on(&(rd_queue));
> +		wait_event_interruptible(rd_queue, (inf =
> +			*((struct divert_info **) file->private_data)));

    Parens around assignment are hardly useful.

>   	}
> -	if (!(inf = *((struct divert_info **) file->private_data)))
> +	if (!inf)
>   		return (0);
>
>   	inf->usage_cnt--;	/* new usage count */
> diff --git a/drivers/isdn/hysdn/hysdn_proclog.c b/drivers/isdn/hysdn/hysdn_proclog.c
> index b61e8d5..7b5fd8f 100644
> --- a/drivers/isdn/hysdn/hysdn_proclog.c
> +++ b/drivers/isdn/hysdn/hysdn_proclog.c
> @@ -175,14 +175,15 @@ hysdn_log_read(struct file *file, char __user *buf, size_t count, loff_t *off)
>   	int len;
>   	hysdn_card *card = PDE_DATA(file_inode(file));
>
> -	if (!*((struct log_data **) file->private_data)) {
> +	if (!(inf = *((struct log_data **) file->private_data))) {

    Here too checkpatch.pk should complain...

>   		struct procdata *pd = card->proclog;
>   		if (file->f_flags & O_NONBLOCK)
>   			return (-EAGAIN);
>
> -		interruptible_sleep_on(&(pd->rd_queue));
> +		wait_event_interruptible(pd->rd_queue, (inf =
> +				*((struct log_data **) file->private_data)));

    And parens hardly needed.

WBR, Sergei

Re: [PATCH, RFC 21/30] isdn: divert, hysdn: fix interruptible_sleep_on race

[Arnd Bergmann]

On Thursday 02 January 2014 19:01:15 Sergei Shtylyov wrote:
> > diff --git a/drivers/isdn/divert/divert_procfs.c b/drivers/isdn/divert/divert_procfs.c
> > index fb4f1ba..1c5dc34 100644
> > --- a/drivers/isdn/divert/divert_procfs.c
> > +++ b/drivers/isdn/divert/divert_procfs.c
> > @@ -86,12 +86,13 @@ isdn_divert_read(struct file *file, char __user *buf, size_t count, loff_t *off)
> >       struct divert_info *inf;
> >       int len;
> >
> > -     if (!*((struct divert_info **) file->private_data)) {
> > +     if (!(inf = *((struct divert_info **) file->private_data))) {
> 
>     checkpatch.pl shouldn't approve assignment inside *if*. Though you're 
> moving it from the existing code, it wouldn't hurt to fix it.

I tried to touch as little as possible, and while I wouldn't use that
style myself, it is applied consistently in this driver, including the
wait_event line I'm adding, where I feel it actually makes sense.

> >               if (file->f_flags & O_NONBLOCK)
> >                       return -EAGAIN;
> > -             interruptible_sleep_on(&(rd_queue));
> > +             wait_event_interruptible(rd_queue, (inf =
> > +                     *((struct divert_info **) file->private_data)));
> 
>     Parens around assignment are hardly useful.

We get a gcc warning without them:

drivers/isdn/divert/divert_procfs.c:95:14: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
    *((struct divert_info **) file->private_data));


I can still change the first one (in both files) if you think it's important,
but I'd rather not spend too much energy at coding style changes.

Thanks for taking a look.

	Arnd

Re: [PATCH, RFC 21/30] isdn: divert, hysdn: fix interruptible_sleep_on race

[Sergei Shtylyov]

Hello.

On 01/02/2014 07:48 PM, Arnd Bergmann wrote:

>>> diff --git a/drivers/isdn/divert/divert_procfs.c b/drivers/isdn/divert/divert_procfs.c
>>> index fb4f1ba..1c5dc34 100644
>>> --- a/drivers/isdn/divert/divert_procfs.c
>>> +++ b/drivers/isdn/divert/divert_procfs.c
>>> @@ -86,12 +86,13 @@ isdn_divert_read(struct file *file, char __user *buf, size_t count, loff_t *off)
>>>        struct divert_info *inf;
>>>        int len;
>>>
>>> -     if (!*((struct divert_info **) file->private_data)) {
>>> +     if (!(inf = *((struct divert_info **) file->private_data))) {

>>      checkpatch.pl shouldn't approve assignment inside *if*. Though you're
>> moving it from the existing code, it wouldn't hurt to fix it.

> I tried to touch as little as possible, and while I wouldn't use that
> style myself, it is applied consistently in this driver, including the
> wait_event line I'm adding, where I feel it actually makes sense.

    I wasn't feeling sure about that one. It seems to me now that it doesn't 
make much sense -- we could do the assignment beforehand.

>>>                if (file->f_flags & O_NONBLOCK)
>>>                        return -EAGAIN;
>>> -             interruptible_sleep_on(&(rd_queue));
>>> +             wait_event_interruptible(rd_queue, (inf =
>>> +                     *((struct divert_info **) file->private_data)));

>>      Parens around assignment are hardly useful.

> We get a gcc warning without them:

> drivers/isdn/divert/divert_procfs.c:95:14: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
>      *((struct divert_info **) file->private_data));

    Ah...

> I can still change the first one (in both files) if you think it's important,

    I always prefer checkpatch.pl-clean patches, though some people do argue 
when they are just moving the badly styled code around.

> but I'd rather not spend too much energy at coding style changes.

    Let's wait for the maintainer's opinion on this.

> 	Arnd

WBR, Sergei