Fix the implementation of a single-open policy for both
devices (lcd and keypad) by using atomic_t instead of plain ints.
Signed-off-by: Mariusz Gorski <[email protected]>
---
drivers/staging/panel/panel.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index 98556ce..d30ccb5 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -404,8 +404,11 @@ static unsigned char lcd_bits[LCD_PORTS][LCD_BITS][BIT_STATES];
#endif /* DEFAULT_PROFILE == 0 */
/* global variables */
-static int keypad_open_cnt; /* #times opened */
-static int lcd_open_cnt; /* #times opened */
+
+/* Device single-open policy control */
+static atomic_t lcd_available = ATOMIC_INIT(1);
+static atomic_t keypad_available = ATOMIC_INIT(1);
+
static struct pardevice *pprt;
static int lcd_initialized;
@@ -1347,7 +1350,7 @@ static ssize_t lcd_write(struct file *file,
static int lcd_open(struct inode *inode, struct file *file)
{
- if (lcd_open_cnt)
+ if (!atomic_dec_and_test(&lcd_available))
return -EBUSY; /* open only once at a time */
if (file->f_mode & FMODE_READ) /* device is write-only */
@@ -1357,13 +1360,12 @@ static int lcd_open(struct inode *inode, struct file *file)
lcd_clear_display();
lcd_must_clear = 0;
}
- lcd_open_cnt++;
return nonseekable_open(inode, file);
}
static int lcd_release(struct inode *inode, struct file *file)
{
- lcd_open_cnt--;
+ atomic_inc(&lcd_available);
return 0;
}
@@ -1627,20 +1629,19 @@ static ssize_t keypad_read(struct file *file,
static int keypad_open(struct inode *inode, struct file *file)
{
- if (keypad_open_cnt)
+ if (!atomic_dec_and_test(&keypad_available))
return -EBUSY; /* open only once at a time */
if (file->f_mode & FMODE_WRITE) /* device is read-only */
return -EPERM;
keypad_buflen = 0; /* flush the buffer on opening */
- keypad_open_cnt++;
return 0;
}
static int keypad_release(struct inode *inode, struct file *file)
{
- keypad_open_cnt--;
+ atomic_inc(&keypad_available);
return 0;
}
@@ -1663,7 +1664,7 @@ static void keypad_send_key(const char *string, int max_len)
return;
/* send the key to the device only if a process is attached to it. */
- if (keypad_open_cnt > 0) {
+ if (!atomic_read(&keypad_available)) {
while (max_len-- && keypad_buflen < KEYPAD_BUFFER && *string) {
keypad_buffer[(keypad_start + keypad_buflen++) %
KEYPAD_BUFFER] = *string++;
--
2.1.3
On Tue, Nov 04, 2014 at 10:47:19PM +0100, Mariusz Gorski wrote:
> Fix the implementation of a single-open policy for both
> devices (lcd and keypad) by using atomic_t instead of plain ints.
>
This seems like it might be a real life bug that you have experienced?
The changelog should tell the user visible effects of the bug. If you
have some dmesg output that would be helpful as well.
regards,
dan carpenter
On Wed, Nov 05, 2014 at 01:19:10PM +0300, Dan Carpenter wrote:
> On Tue, Nov 04, 2014 at 10:47:19PM +0100, Mariusz Gorski wrote:
> > Fix the implementation of a single-open policy for both
> > devices (lcd and keypad) by using atomic_t instead of plain ints.
> >
>
> This seems like it might be a real life bug that you have experienced?
No, I don't think it might really happen in real life. I found it just
by reading the code. A similar solution is used in Chapter 6 of the LDD3
book, so I thought it might be a good idea to fix is here.
> The changelog should tell the user visible effects of the bug. If you
> have some dmesg output that would be helpful as well.
Nope, I can't really reproduce it ATM.
> regards,
> dan carpenter
>
Cheers,
Mariusz
On Wed, Nov 05, 2014 at 12:51:22PM +0100, Mariusz Gorski wrote:
> On Wed, Nov 05, 2014 at 01:19:10PM +0300, Dan Carpenter wrote:
> > On Tue, Nov 04, 2014 at 10:47:19PM +0100, Mariusz Gorski wrote:
> > > Fix the implementation of a single-open policy for both
> > > devices (lcd and keypad) by using atomic_t instead of plain ints.
> > >
> >
> > This seems like it might be a real life bug that you have experienced?
>
> No, I don't think it might really happen in real life. I found it just
> by reading the code. A similar solution is used in Chapter 6 of the LDD3
> book, so I thought it might be a good idea to fix is here.
BTW, it should be kept in mind that I first wrote this driver on 2.0 or
2.2 and it used to run on an i386. So it's extremely likely that a lot of
locking was missing by then and that until it gets discovered by code
review as Mariusz did, issues may still be present. Note that I'm currently
using this driver on production systems where this issue cannot happen
since a few scripts are allowed to send data to the LCD (which might most
always be the case by design given that nobody wants to build a system
where many scripts send unreadable crap at the same time on the display).
Thanks Mariusz for fixing this.
Willy