[PATCH 1/3] staging: dgnc: use a real mutex instead of masquerading a semaphore as a mutex

[PATCH 1/3] staging: dgnc: use a real mutex instead of masquerading a semaphore as a mutex

[Giedrius Statkevicius]

A mutex should be used here (and it's name even says that) so remove the hiding
of a semaphore behind a #define and use a real mutex that the kernel provides.

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
---
 drivers/staging/dgnc/dgnc_tty.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
index ce4187f..0b9bed4 100644
--- a/drivers/staging/dgnc/dgnc_tty.c
+++ b/drivers/staging/dgnc/dgnc_tty.c
@@ -42,16 +42,12 @@
 #include "dgnc_sysfs.h"
 #include "dgnc_utils.h"
 
-#define init_MUTEX(sem)	 sema_init(sem, 1)
-#define DECLARE_MUTEX(name)     \
-	struct semaphore name = __SEMAPHORE_INITIALIZER(name, 1)
-
 /*
  * internal variables
  */
 static struct dgnc_board	*dgnc_BoardsByMajor[256];
 static unsigned char		*dgnc_TmpWriteBuf;
-static DECLARE_MUTEX(dgnc_TmpWriteSem);
+static DEFINE_MUTEX(dgnc_TmpWriteSem);
 
 /*
  * Default transparent print information.
@@ -1797,7 +1793,7 @@ static int dgnc_tty_write(struct tty_struct *tty,
 		 * the board.
 		 */
 		/* we're allowed to block if it's from_user */
-		if (down_interruptible(&dgnc_TmpWriteSem))
+		if (mutex_lock_interruptible(&dgnc_TmpWriteSem))
 			return -EINTR;
 
 		/*
@@ -1807,7 +1803,7 @@ static int dgnc_tty_write(struct tty_struct *tty,
 		count -= copy_from_user(dgnc_TmpWriteBuf, (const unsigned char __user *) buf, count);
 
 		if (!count) {
-			up(&dgnc_TmpWriteSem);
+			mutex_unlock(&dgnc_TmpWriteSem);
 			return -EFAULT;
 		}
 
@@ -1855,7 +1851,7 @@ static int dgnc_tty_write(struct tty_struct *tty,
 
 	if (from_user) {
 		spin_unlock_irqrestore(&ch->ch_lock, flags);
-		up(&dgnc_TmpWriteSem);
+		mutex_unlock(&dgnc_TmpWriteSem);
 	} else {
 		spin_unlock_irqrestore(&ch->ch_lock, flags);
 	}
-- 
2.3.5

[PATCH 1/3] staging: dgnc: use a real mutex instead of masquerading a semaphore as a mutex

[Giedrius Statkevicius]

A mutex should be used here (and it's name even says that) so remove the hiding
of a semaphore behind a #define and use a real mutex that the kernel provides.

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
---
 drivers/staging/dgnc/dgnc_tty.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
index ce4187f..0b9bed4 100644
--- a/drivers/staging/dgnc/dgnc_tty.c
+++ b/drivers/staging/dgnc/dgnc_tty.c
@@ -42,16 +42,12 @@
 #include "dgnc_sysfs.h"
 #include "dgnc_utils.h"
 
-#define init_MUTEX(sem)	 sema_init(sem, 1)
-#define DECLARE_MUTEX(name)     \
-	struct semaphore name = __SEMAPHORE_INITIALIZER(name, 1)
-
 /*
  * internal variables
  */
 static struct dgnc_board	*dgnc_BoardsByMajor[256];
 static unsigned char		*dgnc_TmpWriteBuf;
-static DECLARE_MUTEX(dgnc_TmpWriteSem);
+static DEFINE_MUTEX(dgnc_TmpWriteSem);
 
 /*
  * Default transparent print information.
@@ -1797,7 +1793,7 @@ static int dgnc_tty_write(struct tty_struct *tty,
 		 * the board.
 		 */
 		/* we're allowed to block if it's from_user */
-		if (down_interruptible(&dgnc_TmpWriteSem))
+		if (mutex_lock_interruptible(&dgnc_TmpWriteSem))
 			return -EINTR;
 
 		/*
@@ -1807,7 +1803,7 @@ static int dgnc_tty_write(struct tty_struct *tty,
 		count -= copy_from_user(dgnc_TmpWriteBuf, (const unsigned char __user *) buf, count);
 
 		if (!count) {
-			up(&dgnc_TmpWriteSem);
+			mutex_unlock(&dgnc_TmpWriteSem);
 			return -EFAULT;
 		}
 
@@ -1855,7 +1851,7 @@ static int dgnc_tty_write(struct tty_struct *tty,
 
 	if (from_user) {
 		spin_unlock_irqrestore(&ch->ch_lock, flags);
-		up(&dgnc_TmpWriteSem);
+		mutex_unlock(&dgnc_TmpWriteSem);
 	} else {
 		spin_unlock_irqrestore(&ch->ch_lock, flags);
 	}
-- 
2.3.5

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/3] staging: dgnc: remove redundant check

[Giedrius Statkevicius]

count doesn't change between the last check a few lines before so remove this
redundant check

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
---
 drivers/staging/dgnc/dgnc_tty.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
index 0b9bed4..651a925 100644
--- a/drivers/staging/dgnc/dgnc_tty.c
+++ b/drivers/staging/dgnc/dgnc_tty.c
@@ -1775,12 +1775,6 @@ static int dgnc_tty_write(struct tty_struct *tty,
 		ch->ch_flags &= ~CH_PRON;
 	}
 
-	/*
-	 * If there is nothing left to copy, or I can't handle any more data, leave.
-	 */
-	if (count <= 0)
-		goto exit_retry;
-
 	if (from_user) {
 
 		count = min(count, WRITEBUFLEN);
-- 
2.3.5

[PATCH 2/3] staging: dgnc: remove redundant check

[Giedrius Statkevicius]

count doesn't change between the last check a few lines before so remove this
redundant check

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
---
 drivers/staging/dgnc/dgnc_tty.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
index 0b9bed4..651a925 100644
--- a/drivers/staging/dgnc/dgnc_tty.c
+++ b/drivers/staging/dgnc/dgnc_tty.c
@@ -1775,12 +1775,6 @@ static int dgnc_tty_write(struct tty_struct *tty,
 		ch->ch_flags &= ~CH_PRON;
 	}
 
-	/*
-	 * If there is nothing left to copy, or I can't handle any more data, leave.
-	 */
-	if (count <= 0)
-		goto exit_retry;
-
 	if (from_user) {
 
 		count = min(count, WRITEBUFLEN);
-- 
2.3.5

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 3/3] staging: dgnc: improve the coding style in unlocking part of dgnc_tty_write()

[Giedrius Statkevicius]

do

if (blah) foo();
bar();

instead of

if (blah) {
	foo();
	bar();
} else {
	bar();
}

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
---
 drivers/staging/dgnc/dgnc_tty.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
index 651a925..c59a784 100644
--- a/drivers/staging/dgnc/dgnc_tty.c
+++ b/drivers/staging/dgnc/dgnc_tty.c
@@ -1843,12 +1843,10 @@ static int dgnc_tty_write(struct tty_struct *tty,
 		ch->ch_cpstime += (HZ * count) / ch->ch_digi.digi_maxcps;
 	}
 
-	if (from_user) {
-		spin_unlock_irqrestore(&ch->ch_lock, flags);
+	if (from_user)
 		mutex_unlock(&dgnc_TmpWriteSem);
-	} else {
-		spin_unlock_irqrestore(&ch->ch_lock, flags);
-	}
+
+	spin_unlock_irqrestore(&ch->ch_lock, flags);
 
 	if (count) {
 		/*
-- 
2.3.5

Re: [PATCH 3/3] staging: dgnc: improve the coding style in unlocking part of dgnc_tty_write()

[Dan Carpenter]

This patch changes the lock ordering (behavior change) and it's not
described in the changelog.  Please figure out which way is the correct
ordering and resend.

regards,
dan carpenter

Re: [PATCH 3/3] staging: dgnc: improve the coding style in unlocking part of dgnc_tty_write()

[Dan Carpenter]

On Tue, Apr 07, 2015 at 11:17:48AM +0300, Dan Carpenter wrote:
> This patch changes the lock ordering (behavior change) and it's not
> described in the changelog.  Please figure out which way is the correct
> ordering and resend.

Actually the original ordering was obviously correct.  You can't take
a mutex if you are holding a spinlock.  So it always has to be:

mutex_lock();
spin_lock();

spin_unlock();
mutext_unlock();

regards,
dan carpenter

Re: [PATCH 3/3] staging: dgnc: improve the coding style in unlocking part of dgnc_tty_write()

[Dan Carpenter]

On Tue, Apr 07, 2015 at 11:19:53AM +0300, Dan Carpenter wrote:
> On Tue, Apr 07, 2015 at 11:17:48AM +0300, Dan Carpenter wrote:
> > This patch changes the lock ordering (behavior change) and it's not
> > described in the changelog.  Please figure out which way is the correct
> > ordering and resend.
> 
> Actually the original ordering was obviously correct.  You can't take
> a mutex if you are holding a spinlock.  So it always has to be:
> 
> mutex_lock();
> spin_lock();
> 
> spin_unlock();
> mutext_unlock();
> 

Oh, hm...  You could take a mutex with trylock I suppose.  That would be
safe.

Anyway, I just saw that you sent a v2 patch.

When you send a v2 patch, then you *must* send a reply to the original
thread.  Greg has thousands and thousands of messages in his inbox and
he applies patches in chronological order.  So he will apply this one
because it has not responses then get to the v2 patch and try to apply
that one as well which will fail.

regards,
dan carpenter