[PATCH RFC] PPC-BRIQ_PANEL: Remove BKL and replace with atomic variable.

[PATCH RFC] PPC-BRIQ_PANEL: Remove BKL and replace with atomic variable.

[John Kacur]

>From b64c7d0f11eab96cb253b23c7264c999746116c0 Mon Sep 17 00:00:00 2001
From: John Kacur <jkacur@redhat.com>
Date: Sun, 18 Oct 2009 21:29:21 +0200
Subject: [PATCH] PPC-BRIQ_PANEL: Remove BKL and replace with atomic variable.

There are no locks here except the bkl in briq_panel_open. It's only
purpose is to ensure single access. Remove the bkl and ensure single access
by making vfd_is_open an atomic_variable.

Signed-off-by: John Kacur <jkacur@redhat.com>
---
 drivers/char/briq_panel.c |   25 ++++++++-----------------
 1 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/drivers/char/briq_panel.c b/drivers/char/briq_panel.c
index d8cff90..5396df4 100644
--- a/drivers/char/briq_panel.c
+++ b/drivers/char/briq_panel.c
@@ -6,7 +6,6 @@
 
 #include <linux/module.h>
 
-#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/errno.h>
 #include <linux/tty.h>
@@ -25,6 +24,7 @@
 #include <asm/uaccess.h>
 #include <asm/io.h>
 #include <asm/prom.h>
+#include <asm/atomic.h>
 
 #define		BRIQ_PANEL_MINOR	156
 #define		BRIQ_PANEL_VFD_IOPORT	0x0390
@@ -32,7 +32,7 @@
 #define		BRIQ_PANEL_VER		"1.1 (04/20/2002)"
 #define		BRIQ_PANEL_MSG0		"Loading Linux"
 
-static int		vfd_is_open;
+static atomic_t vfd_is_open = ATOMIC_INIT(0);
 static unsigned char	vfd[40];
 static int		vfd_cursor;
 static unsigned char	ledpb, led;
@@ -68,35 +68,26 @@ static void set_led(char state)
 
 static int briq_panel_open(struct inode *ino, struct file *filep)
 {
-	lock_kernel();
-	/* enforce single access, vfd_is_open is protected by BKL */
-	if (vfd_is_open) {
-		unlock_kernel();
+	/* enforce single access */
+	if (atomic_cmpxchg(vfd_is_open, 0, 1))
 		return -EBUSY;
-	}
-	vfd_is_open = 1;
-
-	unlock_kernel();
 	return 0;
 }
 
 static int briq_panel_release(struct inode *ino, struct file *filep)
 {
-	if (!vfd_is_open)
+	if (!atomic_cmpxchg(vfd_is_open, 1, 0))
 		return -ENODEV;
-
-	vfd_is_open = 0;
-
 	return 0;
 }
 
-static ssize_t briq_panel_read(struct file *file, char __user *buf, size_t count,
-			 loff_t *ppos)
+static ssize_t briq_panel_read(struct file *file, char __user *buf,
+		size_t count, loff_t *ppos)
 {
 	unsigned short c;
 	unsigned char cp;
 
-	if (!vfd_is_open)
+	if (!atomic_read(vfd_is_open))
 		return -ENODEV;
 
 	c = (inb(BRIQ_PANEL_LED_IOPORT) & 0x000c) | (ledpb & 0x0003);
-- 
1.6.0.6

Re: [PATCH RFC] PPC-BRIQ_PANEL: Remove BKL and replace with atomic variable.

[Thomas Gleixner]

B1;2005;0cOn Sun, 18 Oct 2009, John Kacur wrote:

> >From b64c7d0f11eab96cb253b23c7264c999746116c0 Mon Sep 17 00:00:00 2001
> From: John Kacur <jkacur@redhat.com>
> Date: Sun, 18 Oct 2009 21:29:21 +0200
> Subject: [PATCH] PPC-BRIQ_PANEL: Remove BKL and replace with atomic variable.
> 
> There are no locks here except the bkl in briq_panel_open. It's only
> purpose is to ensure single access. Remove the bkl and ensure single access
> by making vfd_is_open an atomic_variable.

And again, can you please look more carefully at the init
vs. read/write functions ?

The BKL is not only protecting the single user variable it's also
serializing write against the access to the display in init.

Thanks,

	tglx

Re: [PATCH RFC] PPC-BRIQ_PANEL: Remove BKL and replace with atomic variable.

[Frederic Weisbecker]

On Mon, Oct 19, 2009 at 07:04:02AM +0200, Thomas Gleixner wrote:
> B1;2005;0cOn Sun, 18 Oct 2009, John Kacur wrote:
> 
> > >From b64c7d0f11eab96cb253b23c7264c999746116c0 Mon Sep 17 00:00:00 2001
> > From: John Kacur <jkacur@redhat.com>
> > Date: Sun, 18 Oct 2009 21:29:21 +0200
> > Subject: [PATCH] PPC-BRIQ_PANEL: Remove BKL and replace with atomic variable.
> > 
> > There are no locks here except the bkl in briq_panel_open. It's only
> > purpose is to ensure single access. Remove the bkl and ensure single access
> > by making vfd_is_open an atomic_variable.
> 
> And again, can you please look more carefully at the init
> vs. read/write functions ?
> 
> The BKL is not only protecting the single user variable it's also
> serializing write against the access to the display in init.
> 
> Thanks,
> 
> 	tglx


That could be solved by statically initializing vfd_is_open to -1
and then set it to 0 once briq_panel_init has finished initializing
the device.

Another thing, I really don't see the point in this check in
briq_panel_read() and briq_panel_write():

	if (!vfd_is_open)
		return -ENODEV;


You can't read/write if vfd_is_open hasn't been set to 1 (open set)
and you're not racing against the release callback since it is called
after the file is closed.

I guess this check can disappear from read/write callbacks.

Re: [PATCH RFC] PPC-BRIQ_PANEL: Remove BKL and replace with atomic variable.

[John Kacur]

On Tue, 20 Oct 2009, Frederic Weisbecker wrote:

> On Mon, Oct 19, 2009 at 07:04:02AM +0200, Thomas Gleixner wrote:
> > B1;2005;0cOn Sun, 18 Oct 2009, John Kacur wrote:
> > 
> > > >From b64c7d0f11eab96cb253b23c7264c999746116c0 Mon Sep 17 00:00:00 2001
> > > From: John Kacur <jkacur@redhat.com>
> > > Date: Sun, 18 Oct 2009 21:29:21 +0200
> > > Subject: [PATCH] PPC-BRIQ_PANEL: Remove BKL and replace with atomic variable.
> > > 
> > > There are no locks here except the bkl in briq_panel_open. It's only
> > > purpose is to ensure single access. Remove the bkl and ensure single access
> > > by making vfd_is_open an atomic_variable.
> > 
> > And again, can you please look more carefully at the init
> > vs. read/write functions ?
> > 
> > The BKL is not only protecting the single user variable it's also
> > serializing write against the access to the display in init.
> > 
> > Thanks,
> > 
> > 	tglx
> 
> 
> That could be solved by statically initializing vfd_is_open to -1
> and then set it to 0 once briq_panel_init has finished initializing
> the device.

Well, I did think of a similar scheme, but in another chat or email, 
(don't remember which) Thomas told me that any such synchronization is a 
hack, the only correct thing to do is fix the init.
His point being, either the device is available to functions like read or 
write or it isn't. However, that sync technique seems so simple here, I 
may not give up on it.
 >