Re: [PATCH] fixed spinlock use in hysdn_log_close()

[Matthias Kaehlcke <matthias.kaehlcke@gmail.com>]

El Fri, Apr 20, 2007 at 05:41:46PM -0700 Andrew Morton ha dit:

> On Sat, 14 Apr 2007 07:09:07 +0200
> Matthias Kaehlcke <matthias.kaehlcke@gmail.com> wrote:
> 
> > fixed incorrect spinlock use in hysdn_log_close(). the function
> > declared a spinlock on the stack and used it to 'protect' a shared
> > driver structure. the patch removes the declaration of hysdn_lock and 
> > uses card->hysdn_lock instead.
> > 
> 
> Interesting.
> 
> > 
> > ---
> > diff --git a/drivers/isdn/hysdn/hysdn_proclog.c b/drivers/isdn/hysdn/hysdn_proclog.c
> > index f7e83a8..32f0b75 100644
> > --- a/drivers/isdn/hysdn/hysdn_proclog.c
> > +++ b/drivers/isdn/hysdn/hysdn_proclog.c
> > @@ -299,7 +299,6 @@ hysdn_log_close(struct inode *ino, struct file *filep)
> >  	hysdn_card *card;
> >  	int retval = 0;
> >  	unsigned long flags;
> > -	spinlock_t hysdn_lock = SPIN_LOCK_UNLOCKED;
> >  
> >  	lock_kernel();
> >  	if ((filep->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_WRITE) {
> > @@ -309,7 +308,7 @@ hysdn_log_close(struct inode *ino, struct file *filep)
> >  		/* read access -> log/debug read, mark one further file as closed */
> >  
> >  		pd = NULL;
> > -		spin_lock_irqsave(&hysdn_lock, flags);
> > +		spin_lock_irqsave(&card->hysdn_lock, flags);
> 
> I guess it won't hurt - are you actually able to test this code?
> 
> afaict most of the data in there is locked with lock_kernel(), if it's
> locked at all.
> 
> If you had some runtime problem and this patch fixed it then fine.  If
> however you're not able to test this code then perhaps the safest option is
> to simply remove that locking altogether, which is pretty much a
> runtime-equivalent change.

i'm sorry, but i can't test it, i don't have the device. initially the
patch was a kernel janitorial with the idea to replace the assignation
to SPIN_LOCK_UNLOCKED by DEFINE_SPINLOCK. when i sent the first patch
to the kj list someone pointed me to the spinlock on stack. 

i also wondered about the use of the spinlock after aquiring the big
kernel lock, i agree that it's probable the safest to remove the
spinlockaltogether. below you find a patch that does this.

thanks for your comments

--

remove spinlock hysdn_lock (declared on the stack) from HYSDN
driver. the code region is already protected by the big kernel lock.

Signed-off-by: Matthias Kaehlcke <matthias.kaehlcke@gmail.com>

--

diff --git a/drivers/isdn/hysdn/hysdn_proclog.c b/drivers/isdn/hysdn/hysdn_proclog.c
index f7e83a8..ab18965 100644
--- a/drivers/isdn/hysdn/hysdn_proclog.c
+++ b/drivers/isdn/hysdn/hysdn_proclog.c
@@ -298,8 +298,6 @@ hysdn_log_close(struct inode *ino, struct file *filep)
 	struct procdata *pd;
 	hysdn_card *card;
 	int retval = 0;
-	unsigned long flags;
-	spinlock_t hysdn_lock = SPIN_LOCK_UNLOCKED;
 
 	lock_kernel();
 	if ((filep->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_WRITE) {
@@ -309,7 +307,6 @@ hysdn_log_close(struct inode *ino, struct file *filep)
 		/* read access -> log/debug read, mark one further file as closed */
 
 		pd = NULL;
-		spin_lock_irqsave(&hysdn_lock, flags);
 		inf = *((struct log_data **) filep->private_data);	/* get first log entry */
 		if (inf)
 			pd = (struct procdata *) inf->proc_ctrl;	/* still entries there */
@@ -332,7 +329,6 @@ hysdn_log_close(struct inode *ino, struct file *filep)
 			inf->usage_cnt--;	/* decrement usage count for buffers */
 			inf = inf->next;
 		}
-		spin_unlock_irqrestore(&hysdn_lock, flags);
 
 		if (pd)
 			if (pd->if_used <= 0)	/* delete buffers if last file closed */


-- 
Matthias Kaehlcke
Linux Application Developer
Barcelona

               If sharing a thing in no way diminishes it,
               it is not rightly owned if it is not shared
                                                                 .''`.
    using free software / Debian GNU/Linux | http://debian.org  : :'  :
                                                                `. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4                  `-