Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
Date:   Tue, 4 Aug 2020 04:20:10 +0530
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     madhuparnabhowmik10@gmail.com, arnd@arndb.de,
        linux-kernel@vger.kernel.org, andrianov@ispras.ru,
        ldv-project@linuxtesting.org
Subject: Re: [PATCH] drivers: char: applicom.c: Add lock for protecting
 DeviceErrorCount
Message-ID: <20200803225010.GA8670@madhuparna-HP-Notebook>
References: <20200803105049.11089-1-madhuparnabhowmik10@gmail.com>
 <20200803115328.GA955269@kroah.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200803115328.GA955269@kroah.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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