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

[Andrew Morton]

On Sat, 14 Apr 2007 07:09:07 +0200
Matthias Kaehlcke <matthias.kaehlcke@xxxxxxxxx> 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.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/