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 <[email protected]> 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 <[email protected]>
--
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 `-
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
