linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drivers: char: applicom.c: Add lock for protecting DeviceErrorCount
@ 2020-08-03 10:50 madhuparnabhowmik10
  2020-08-03 11:53 ` Greg KH
  0 siblings, 1 reply; 3+ messages in thread
From: madhuparnabhowmik10 @ 2020-08-03 10:50 UTC (permalink / raw)
  To: arnd, gregkh; +Cc: linux-kernel, andrianov, ldv-project, Madhuparna Bhowmik

From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>

The variable DeviceErrorCount is used to keep track of the number of
errors in read, write and interrupt routines, however it was not
protected by proper locking.
Therefore, this patch adds a spinlock: error_lock to protect the
variable.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
---
 drivers/char/applicom.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/char/applicom.c b/drivers/char/applicom.c
index 14b2d8034c51..6df7450b8b99 100644
--- a/drivers/char/applicom.c
+++ b/drivers/char/applicom.c
@@ -106,6 +106,7 @@ static DECLARE_WAIT_QUEUE_HEAD(FlagSleepRec);
 static unsigned int WriteErrorCount;	/* number of write error      */
 static unsigned int ReadErrorCount;	/* number of read error       */
 static unsigned int DeviceErrorCount;	/* number of device error     */
+DEFINE_SPINLOCK(error_lock);		/* lock to protect error count variables */
 
 static ssize_t ac_read (struct file *, char __user *, size_t, loff_t *);
 static ssize_t ac_write (struct file *, const char __user *, size_t, loff_t *);
@@ -428,7 +429,9 @@ static ssize_t ac_write(struct file *file, const char __user *buf, size_t count,
 		spin_unlock_irqrestore(&apbs[IndexCard].mutex, flags);
 		printk(KERN_WARNING "APPLICOM driver write error board %d, DataFromPcReady = %d\n",
 		       IndexCard,(int)readb(apbs[IndexCard].RamIO + DATA_FROM_PC_READY));
+		spin_lock_irqsave(&error_lock, flags);
 		DeviceErrorCount++;
+		spin_unlock_irqrestore(&error_lock, flags);
 		return -EIO;
 	}
 	
@@ -593,7 +596,9 @@ static ssize_t ac_read (struct file *filp, char __user *buf, size_t count, loff_
 				
 				printk(KERN_WARNING "APPLICOM driver read error board %d, DataToPcReady = %d\n",
 				       i,(int)readb(apbs[i].RamIO + DATA_TO_PC_READY));
+				spin_lock_irqsave(&error_lock, flags);
 				DeviceErrorCount++;
+				spin_unlock_irqrestore(&error_lock, flags);
 				return -EIO;
 			}
 			
@@ -653,7 +658,9 @@ static irqreturn_t ac_interrupt(int vec, void *dev_instance)
 			if (readb(apbs[i].RamIO + DATA_TO_PC_READY) > 2) {
 				printk(KERN_WARNING "APPLICOM driver interrupt err board %d, DataToPcReady = %d\n",
 				       i+1,(int)readb(apbs[i].RamIO + DATA_TO_PC_READY));
+				spin_lock(&error_lock);
 				DeviceErrorCount++;
+				spin_unlock(&error_lock);
 			}
 
 			if((readb(apbs[i].RamIO + DATA_FROM_PC_READY) > 2) && 
@@ -661,7 +668,9 @@ static irqreturn_t ac_interrupt(int vec, void *dev_instance)
 				
 				printk(KERN_WARNING "APPLICOM driver interrupt err board %d, DataFromPcReady = %d\n",
 				       i+1,(int)readb(apbs[i].RamIO + DATA_FROM_PC_READY));
+				spin_lock(&error_lock);
 				DeviceErrorCount++;
+				spin_unlock(&error_lock);
 			}
 
 			if (readb(apbs[i].RamIO + DATA_TO_PC_READY) == 2) {	/* mailbox sent by the card ?   */
@@ -699,6 +708,7 @@ static long ac_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
      
 {				/* @ ADG ou ATO selon le cas */
 	int i;
+	unsigned long flags;
 	unsigned char IndexCard;
 	void __iomem *pmem;
 	int ret = 0;
@@ -819,12 +829,14 @@ static long ac_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 			else
 				printk("\n");
 		}
+		spin_lock_irqsave(&error_lock, flags);
 		if (DeviceErrorCount != 0)
 			printk(KERN_INFO "DeviceErrorCount ........... %d\n", DeviceErrorCount);
 		if (ReadErrorCount != 0)
 			printk(KERN_INFO "ReadErrorCount ............. %d\n", ReadErrorCount);
 		if (WriteErrorCount != 0)
 			printk(KERN_INFO "WriteErrorCount ............ %d\n", WriteErrorCount);
+		spin_unlock_irqrestore(&error_lock, flags);
 		if (waitqueue_active(&FlagSleepRec))
 			printk(KERN_INFO "Process in read pending\n");
 		for (i = 0; i < MAX_BOARD; i++) {
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] drivers: char: applicom.c: Add lock for protecting DeviceErrorCount
  2020-08-03 10:50 [PATCH] drivers: char: applicom.c: Add lock for protecting DeviceErrorCount madhuparnabhowmik10
@ 2020-08-03 11:53 ` Greg KH
  2020-08-03 22:50   ` Madhuparna Bhowmik
  0 siblings, 1 reply; 3+ messages in thread
From: Greg KH @ 2020-08-03 11:53 UTC (permalink / raw)
  To: madhuparnabhowmik10; +Cc: arnd, linux-kernel, andrianov, ldv-project

On Mon, Aug 03, 2020 at 04:20:49PM +0530, madhuparnabhowmik10@gmail.com wrote:
> From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
> 
> The variable DeviceErrorCount is used to keep track of the number of
> errors in read, write and interrupt routines, however it was not
> protected by proper locking.
> Therefore, this patch adds a spinlock: error_lock to protect the
> variable.
> 
> Found by Linux Driver Verification project (linuxtesting.org).
> 
> Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
> ---
>  drivers/char/applicom.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/char/applicom.c b/drivers/char/applicom.c
> index 14b2d8034c51..6df7450b8b99 100644
> --- a/drivers/char/applicom.c
> +++ b/drivers/char/applicom.c
> @@ -106,6 +106,7 @@ static DECLARE_WAIT_QUEUE_HEAD(FlagSleepRec);
>  static unsigned int WriteErrorCount;	/* number of write error      */
>  static unsigned int ReadErrorCount;	/* number of read error       */
>  static unsigned int DeviceErrorCount;	/* number of device error     */
> +DEFINE_SPINLOCK(error_lock);		/* lock to protect error count variables */

That's a horrible global name, shouldn't it be static?

>  
>  static ssize_t ac_read (struct file *, char __user *, size_t, loff_t *);
>  static ssize_t ac_write (struct file *, const char __user *, size_t, loff_t *);
> @@ -428,7 +429,9 @@ static ssize_t ac_write(struct file *file, const char __user *buf, size_t count,
>  		spin_unlock_irqrestore(&apbs[IndexCard].mutex, flags);
>  		printk(KERN_WARNING "APPLICOM driver write error board %d, DataFromPcReady = %d\n",
>  		       IndexCard,(int)readb(apbs[IndexCard].RamIO + DATA_FROM_PC_READY));
> +		spin_lock_irqsave(&error_lock, flags);

Why all of these irqsave?

>  		DeviceErrorCount++;

Does this really matter?  Who cares if we drop one of these, or any
other of these debugging-only values?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH] drivers: char: applicom.c: Add lock for protecting DeviceErrorCount
  2020-08-03 11:53 ` Greg KH
@ 2020-08-03 22:50   ` Madhuparna Bhowmik
  0 siblings, 0 replies; 3+ messages in thread
From: Madhuparna Bhowmik @ 2020-08-03 22:50 UTC (permalink / raw)
  To: Greg KH; +Cc: madhuparnabhowmik10, arnd, linux-kernel, andrianov, ldv-project

On Mon, Aug 03, 2020 at 01:53:28PM +0200, Greg KH wrote:
> On Mon, Aug 03, 2020 at 04:20:49PM +0530, madhuparnabhowmik10@gmail.com wrote:
> > From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
> > 
> > The variable DeviceErrorCount is used to keep track of the number of
> > errors in read, write and interrupt routines, however it was not
> > protected by proper locking.
> > Therefore, this patch adds a spinlock: error_lock to protect the
> > variable.
> > 
> > Found by Linux Driver Verification project (linuxtesting.org).
> > 
> > Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
> > ---
> >  drivers/char/applicom.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/char/applicom.c b/drivers/char/applicom.c
> > index 14b2d8034c51..6df7450b8b99 100644
> > --- a/drivers/char/applicom.c
> > +++ b/drivers/char/applicom.c
> > @@ -106,6 +106,7 @@ static DECLARE_WAIT_QUEUE_HEAD(FlagSleepRec);
> >  static unsigned int WriteErrorCount;	/* number of write error      */
> >  static unsigned int ReadErrorCount;	/* number of read error       */
> >  static unsigned int DeviceErrorCount;	/* number of device error     */
> > +DEFINE_SPINLOCK(error_lock);		/* lock to protect error count variables */
> 
> That's a horrible global name, shouldn't it be static?
> 
> >  
> >  static ssize_t ac_read (struct file *, char __user *, size_t, loff_t *);
> >  static ssize_t ac_write (struct file *, const char __user *, size_t, loff_t *);
> > @@ -428,7 +429,9 @@ static ssize_t ac_write(struct file *file, const char __user *buf, size_t count,
> >  		spin_unlock_irqrestore(&apbs[IndexCard].mutex, flags);
> >  		printk(KERN_WARNING "APPLICOM driver write error board %d, DataFromPcReady = %d\n",
> >  		       IndexCard,(int)readb(apbs[IndexCard].RamIO + DATA_FROM_PC_READY));
> > +		spin_lock_irqsave(&error_lock, flags);
> 
> Why all of these irqsave?
> 
> >  		DeviceErrorCount++;
> 
> Does this really matter?  Who cares if we drop one of these, or any
> other of these debugging-only values?
>
Yes, since the error count variables are just for debugging, it is not
really necessary to have an exact count, but I just thought it would
be nice if it is precise, rest it is upto you if these changes are
required or not. Let me know, if required then I can think of a better name
for the lock.

Also, one other thing that I noticed in this code is that some of the
variables (WriteErrorCount and ReadErrorCount) are just initialized
and never incremented after that. So, if you could confirm that they
were supposed to be used in ac_write and ac_read instead of
using DeviceErrorCount, or otherwise, then I can make this change as
well.

Thanks,
Madhuparna

> thanks,
> 
> greg k-h

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-08-03 22:50 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-03 10:50 [PATCH] drivers: char: applicom.c: Add lock for protecting DeviceErrorCount madhuparnabhowmik10
2020-08-03 11:53 ` Greg KH
2020-08-03 22:50   ` Madhuparna Bhowmik

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).