tty: the briq panel isn't a tty, make it use its own locking

PATCH In review Alan Cox 4 participants 6 messages Feb 23, 2012 – Feb 29, 2012 Char and misc drivers

lore

Share & tools

lore

Alan Cox

author

2012-02-23 13:25 UTC #0 lore raw

PATCH 1/1 tty: the briq panel isn't a tty, make it use its own locking

From: Alan Cox <[email protected]>

The driver appears to be terminall broken, if nobody is maintaining it then perhaps it should go into staging and the out-tray.

(Fixed bug noted by Jiri)

Signed-off-by Alan Cox <[email protected]>

---

 drivers/char/briq_panel.c |   26 +++++++-------------------
 1 files changed, 7 insertions(+), 19 deletions(-)

drivers/char/briq_panel.c +7 ~~-19~~

index 095ab90..3335d07 100644
@@ -20,6 +20,7 @@
 #include <linux/mm.h>
 #include <linux/init.h>
 
+#include <asm/bitops.h>
 #include <asm/uaccess.h>
 #include <asm/io.h>
 #include <asm/prom.h>

@@ -30,7 +31,7 @@
 #define		BRIQ_PANEL_VER		"1.1 (04/20/2002)"
 #define		BRIQ_PANEL_MSG0		"Loading Linux"
 
-static int		vfd_is_open;
+static unsigned long	vfd_is_open;
 static unsigned char	vfd[40];
 static int		vfd_cursor;
 static unsigned char	ledpb, led;

@@ -66,37 +67,27 @@ static void set_led(char state)
 
 static int briq_panel_open(struct inode *ino, struct file *filep)
 {
-	tty_lock();
-	/* enforce single access, vfd_is_open is protected by BKL */
-	if (vfd_is_open) {
-		tty_unlock();
+	/* enforce single open */
+	if (test_and_set_bit(0, &vfd_is_open))
 		return -EBUSY;
 	}
-	vfd_is_open = 1;
-
-	tty_unlock();
 	return 0;
 }
 
 static int briq_panel_release(struct inode *ino, struct file *filep)
 {
-	if (!vfd_is_open)
-		return -ENODEV;
-
-	vfd_is_open = 0;
-
+	clear_bit(0, &vfd_is_open);
 	return 0;
 }
 
+/* Note that single open doesn't mean single threaded read or write so all
+   this code is unsafe */
 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)
-		return -ENODEV;
-
 	c = (inb(BRIQ_PANEL_LED_IOPORT) & 0x000c) | (ledpb & 0x0003);
 	set_led(' ');
 	/* upper button released */

@@ -137,9 +128,6 @@ static ssize_t briq_panel_write(struct file *file, const char __user *buf, size_
 	size_t indx = len;
 	int i, esc = 0;
 
-	if (!vfd_is_open)
-		return -EBUSY;
-
 	for (;;) {
 		char c;
 		if (!indx)

Benjamin Herrenschmidt

2012-02-23 20:42 UTC #1 lore raw

Quoted 4 lines · Alan Cox

From: Alan Cox <[email protected]>
The driver appears to be terminall broken, if nobody is maintaining it then perhaps it should go into staging and the out-tray.

Hrm, I don't think anybody maintains it but I do have a BriQ somewhere in storage, so I suppose I could pick it up, not that I need another crap driver to deal with right now though...

Cheers,
Ben.

Quoted 83 lines

(Fixed bug noted by Jiri)

Signed-off-by Alan Cox <[email protected]>

---

 drivers/char/briq_panel.c |   26 +++++++-------------------
 1 files changed, 7 insertions(+), 19 deletions(-)

drivers/char/briq_panel.c +7 ~~-19~~

index 095ab90..3335d07 100644
@@ -20,6 +20,7 @@
 #include <linux/mm.h>
 #include <linux/init.h>
 
+#include <asm/bitops.h>
 #include <asm/uaccess.h>
 #include <asm/io.h>
 #include <asm/prom.h>

@@ -30,7 +31,7 @@
 #define		BRIQ_PANEL_VER		"1.1 (04/20/2002)"
 #define		BRIQ_PANEL_MSG0		"Loading Linux"
 
-static int		vfd_is_open;
+static unsigned long	vfd_is_open;
 static unsigned char	vfd[40];
 static int		vfd_cursor;
 static unsigned char	ledpb, led;

@@ -66,37 +67,27 @@ static void set_led(char state)
 
 static int briq_panel_open(struct inode *ino, struct file *filep)
 {
-	tty_lock();
-	/* enforce single access, vfd_is_open is protected by BKL */
-	if (vfd_is_open) {
-		tty_unlock();
+	/* enforce single open */
+	if (test_and_set_bit(0, &vfd_is_open))
 		return -EBUSY;
 	}
-	vfd_is_open = 1;
-
-	tty_unlock();
 	return 0;
 }
 
 static int briq_panel_release(struct inode *ino, struct file *filep)
 {
-	if (!vfd_is_open)
-		return -ENODEV;
-
-	vfd_is_open = 0;
-
+	clear_bit(0, &vfd_is_open);
 	return 0;
 }
 
+/* Note that single open doesn't mean single threaded read or write so all
+   this code is unsafe */
 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)
-		return -ENODEV;
-
 	c = (inb(BRIQ_PANEL_LED_IOPORT) & 0x000c) | (ledpb & 0x0003);
 	set_led(' ');
 	/* upper button released */

@@ -137,9 +128,6 @@ static ssize_t briq_panel_write(struct file *file, const char __user *buf, size_
 	size_t indx = len;
 	int i, esc = 0;
 
-	if (!vfd_is_open)
-		return -EBUSY;
-
 	for (;;) {
 		char c;
 		if (!indx)

Alan Cox

author

2012-02-23 22:54 UTC #2 lore raw

Quoted 9 lines · On Fri, 24 Feb 2012 07:42:27 +1100 Benjamin Herrenschmidt…

Quoted 4 lines · Alan Cox
From: Alan Cox <[email protected]>
The driver appears to be terminall broken, if nobody is maintaining it then perhaps it should go into staging and the out-tray.
Hrm, I don't think anybody maintains it but I do have a BriQ somewhere in storage, so I suppose I could pick it up, not that I need another crap driver to deal with right now though...

In which case can we just stuff it into the tree, or take the briq driver out into staging as this is the last blocker on tackling the tty_lock entirely in the tty layer

Alan

Josh Boyer

2012-02-24 01:08 UTC #3 lore raw

Quoted 9 lines · Alan Cox

Quoted 6 lines · On Fri, 24 Feb 2012 07:42:27 +1100 Benjamin Herrenschmidt…
Quoted 4 lines · Alan Cox
From: Alan Cox <[email protected]>
The driver appears to be terminall broken, if nobody is maintaining it then perhaps it should go into staging and the out-tray.

Quoted 7 lines

Quoted 3 lines
Hrm, I don't think anybody maintains it but I do have a BriQ somewhere in storage, so I suppose I could pick it up, not that I need another crap driver to deal with right now though...
In which case can we just stuff it into the tree, or take the briq driver out into staging as this is the last blocker on tackling the tty_lock entirely in the tty layer

I'd vote for out to staging. Don't rely on BenH to make sane decisions when it comes to deprecating stuff nobody uses anymore. I mean... he's only just now dropping iSeries support.

josh

Benjamin Herrenschmidt

2012-02-24 01:37 UTC #4 lore raw

In reply to Alan Cox

Quoted 7 lines · Alan Cox

Quoted 3 lines
Hrm, I don't think anybody maintains it but I do have a BriQ somewhere in storage, so I suppose I could pick it up, not that I need another crap driver to deal with right now though...
In which case can we just stuff it into the tree, or take the briq driver out into staging as this is the last blocker on tackling the tty_lock entirely in the tty layer

Just remove the driver, I don't think anybody cares.

Cheers,
Ben.

Greg KH

maintainer

2012-02-29 20:21 UTC #5 lore raw

Quoted 10 lines · Benjamin Herrenschmidt

Quoted 7 lines · Alan Cox
Quoted 3 lines
Hrm, I don't think anybody maintains it but I do have a BriQ somewhere in storage, so I suppose I could pick it up, not that I need another crap driver to deal with right now though...
In which case can we just stuff it into the tree, or take the briq driver out into staging as this is the last blocker on tackling the tty_lock entirely in the tty layer
Just remove the driver, I don't think anybody cares.

Wonderful, I'll go do that right now :)

greg k-h