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

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

[Alan Cox]

From: Alan Cox <alan@linux.intel.com>

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 <alan@linux.intel.com>
---

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


diff --git a/drivers/char/briq_panel.c b/drivers/char/briq_panel.c
index 095ab90..3335d07 100644
--- a/drivers/char/briq_panel.c
+++ b/drivers/char/briq_panel.c
@@ -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)

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

[Benjamin Herrenschmidt]

On Thu, 2012-02-23 at 13:39 +0000, Alan Cox wrote:
> From: Alan Cox <alan@linux.intel.com>
> 
> 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.

> (Fixed bug noted by Jiri)
> 
> Signed-off-by: Alan Cox <alan@linux.intel.com>
> ---
> 
>  drivers/char/briq_panel.c |   26 +++++++-------------------
>  1 files changed, 7 insertions(+), 19 deletions(-)
> 
> 
> diff --git a/drivers/char/briq_panel.c b/drivers/char/briq_panel.c
> index 095ab90..3335d07 100644
> --- a/drivers/char/briq_panel.c
> +++ b/drivers/char/briq_panel.c
> @@ -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)

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

[Alan Cox]

On Fri, 24 Feb 2012 07:42:27 +1100
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> On Thu, 2012-02-23 at 13:39 +0000, Alan Cox wrote:
> > From: Alan Cox <alan@linux.intel.com>
> > 
> > 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

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

[Josh Boyer]

On Thu, Feb 23, 2012 at 5:56 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Fri, 24 Feb 2012 07:42:27 +1100
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>
>> On Thu, 2012-02-23 at 13:39 +0000, Alan Cox wrote:
>> > From: Alan Cox <alan@linux.intel.com>
>> >
>> > 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

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

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

[Benjamin Herrenschmidt]

On Thu, 2012-02-23 at 22:56 +0000, Alan Cox wrote:
> > 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.

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

[Greg KH]

On Fri, Feb 24, 2012 at 12:37:17PM +1100, Benjamin Herrenschmidt wrote:
> On Thu, 2012-02-23 at 22:56 +0000, Alan Cox wrote:
> > > 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

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

[Alan Cox]

From: Alan Cox <alan@linux.intel.com>

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

Signed-off-by: Alan Cox <alan@linux.intel.com>
---

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


diff --git a/drivers/char/briq_panel.c b/drivers/char/briq_panel.c
index 8abfd26..1c323f4 100644
--- a/drivers/char/briq_panel.c
+++ b/drivers/char/briq_panel.c
@@ -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,28 @@ 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) {
+	/* enforce single open */
+	if (test_and_set_bit(0, &vfd_is_open)) {
 		tty_unlock();
 		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 +129,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)

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

[Jiri Slaby]

On 02/23/2012 01:34 PM, Alan Cox wrote:
> --- a/drivers/char/briq_panel.c
> +++ b/drivers/char/briq_panel.c
...
> @@ -66,37 +67,28 @@ 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) {
> +	/* enforce single open */
> +	if (test_and_set_bit(0, &vfd_is_open)) {
>  		tty_unlock();

This one should be removed too.

>  		return -EBUSY;
>  	}
> -	vfd_is_open = 1;
> -
> -	tty_unlock();
>  	return 0;
>  }

thanks,
-- 
js
suse labs