From: Madhuparna Bhowmik <[email protected]>
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 <[email protected]>
---
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
On Mon, Aug 03, 2020 at 04:20:49PM +0530, [email protected] wrote:
> From: Madhuparna Bhowmik <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
On Mon, Aug 03, 2020 at 01:53:28PM +0200, Greg KH wrote:
> On Mon, Aug 03, 2020 at 04:20:49PM +0530, [email protected] wrote:
> > From: Madhuparna Bhowmik <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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