On Mon, Aug 03, 2020 at 04:20:49PM +0530, madhuparnabhowmi...@gmail.com wrote: > From: Madhuparna Bhowmik <madhuparnabhowmi...@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 <madhuparnabhowmi...@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
