(added Cc: lkml to pull in some clues)
Daniel Drake wrote at linux1394-devel:
> Hi,
>
> I noticed that video1394 calls lock_kernel in it's file_operations. I
> thought about converting these into a per-instance mutex or something,
> but in the end I couldn't find a reason why this locking is needed. (The
> more important I/O system is protected by separate spinlocks)
>
> The BKL is only contended when operations such as ioctl() or release()
> are invoked. My knowledge lacks at this point, but I'm reasonably sure
> that some upper layer must ensure that these invokations are serialized,
> as opposed to leaving it up to the driver to handle nasty potential
> situations e.g. release() being called halfway through a read(). Can
> anyone clarify?
>
> Thanks,
> Daniel
I think you are right. Same with dv1394. Although we need to
double-check whether something needs replacement protection then.
> ------------------------------------------------------------------------
>
> Index: linux/drivers/ieee1394/video1394.c
> ===================================================================
> --- linux.orig/drivers/ieee1394/video1394.c
> +++ linux/drivers/ieee1394/video1394.c
> @@ -1161,9 +1161,7 @@ static int __video1394_ioctl(struct file
> static long video1394_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> {
> int err;
> - lock_kernel();
> err = __video1394_ioctl(file, cmd, arg);
> - unlock_kernel();
> return err;
> }
>
> @@ -1181,13 +1179,11 @@ static int video1394_mmap(struct file *f
> struct file_ctx *ctx = (struct file_ctx *)file->private_data;
> int res = -EINVAL;
>
> - lock_kernel();
> if (ctx->current_ctx == NULL) {
> PRINT(KERN_ERR, ctx->ohci->host->id,
> "Current iso context not set");
> } else
> res = dma_region_mmap(&ctx->current_ctx->dma, file, vma);
> - unlock_kernel();
>
> return res;
> }
> @@ -1200,7 +1196,6 @@ static unsigned int video1394_poll(struc
> struct dma_iso_ctx *d;
> int i;
>
> - lock_kernel();
> ctx = file->private_data;
> d = ctx->current_ctx;
> if (d == NULL) {
> @@ -1221,7 +1216,6 @@ static unsigned int video1394_poll(struc
> }
> spin_unlock_irqrestore(&d->lock, flags);
> done:
> - unlock_kernel();
>
> return mask;
> }
> @@ -1257,7 +1251,6 @@ static int video1394_release(struct inod
> struct list_head *lh, *next;
> u64 mask;
>
> - lock_kernel();
> list_for_each_safe(lh, next, &ctx->context_list) {
> struct dma_iso_ctx *d;
> d = list_entry(lh, struct dma_iso_ctx, link);
> @@ -1278,7 +1271,6 @@ static int video1394_release(struct inod
> kfree(ctx);
> file->private_data = NULL;
>
> - unlock_kernel();
> return 0;
> }
>
>
--
Stefan Richter
-=====-=-==- =-=- =--=-
http://arcgraph.de/sr/
-
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]
